DAOS-15863 container: fix a race for container cache (#15038)

* DAOS-15863 container: fix a race for container cache

while destroying a container, cont_child_destroy_one() releases
its own refcount before waiting, if another ULT releases its
refcount, which is the last one, wakes up the waiting ULT and frees
it ds_cont_child straightaway, because no one else has refcount.

When the waiting ULT is waken up, it will try to change the already
freed ds_cont_child.

This patch changes the LRU eviction logic and fixes this race.


Signed-off-by: Liang Zhen <[email protected]>
Signed-off-by: Jeff Olivier <[email protected]>
Co-authored-by: Jeff Olivier <[email protected]>

src/common/lru.c

@@ -36,7 +36,10 @@ lru_hop_rec_decref(struct d_hash_table *htable, d_list_t *link)
 
 	D_ASSERT(llink->ll_ref > 0);
 	llink->ll_ref--;
-	if (llink->ll_ref == 1 && llink->ll_ops->lop_wakeup)
+
+	/* eviction waiter is the last one holds refcount */
+	if (llink->ll_wait_evict &&
+	    llink->ll_ops->lop_wakeup && daos_lru_is_last_user(llink))
 		llink->ll_ops->lop_wakeup(llink);
 
 	/* Delete from hash only if no more references */
@@ -215,15 +218,6 @@ daos_lru_ref_hold(struct daos_lru_cache *lcache, void *key,
 	if (link != NULL) {
 		llink = link2llink(link);
 		D_ASSERT(llink->ll_evicted == 0);
-		if (llink->ll_evicting) {
-			/**
-			 * Avoid calling `lru_hop_rec_decref()` at this point
-			 * to prevent `wakeup()` from being invoked twice.
-			 */
-			D_ASSERT(llink->ll_ref > 1);
-			llink->ll_ref--;
-			D_GOTO(out, rc = -DER_SHUTDOWN);
-		}
 		/* remove busy item from LRU */
 		if (!d_list_empty(&llink->ll_qlink))
 			d_list_del_init(&llink->ll_qlink);
@@ -257,24 +251,17 @@ daos_lru_ref_hold(struct daos_lru_cache *lcache, void *key,
 	return rc;
 }
 
-static void
-lru_ref_release_internal(struct daos_lru_cache *lcache, struct daos_llink *llink, bool wait)
+void
+daos_lru_ref_release(struct daos_lru_cache *lcache, struct daos_llink *llink)
 {
 	D_ASSERT(lcache != NULL && llink != NULL && llink->ll_ref > 1);
 	D_ASSERT(d_list_empty(&llink->ll_qlink));
 
 	lru_hop_rec_decref(&lcache->dlc_htable, &llink->ll_link);
 
-	if (wait && llink->ll_ref > 1) {
-		D_ASSERT(llink->ll_evicting == 0);
-		llink->ll_evicting = 1;
-		lcache->dlc_ops->lop_wait(llink);
-		llink->ll_evicting = 0;
-		llink->ll_evicted = 1;
-	}
-
 	if (llink->ll_ref == 1) { /* the last refcount */
-		if (lcache->dlc_csize == 0)
+		/* zero-sized cache always evicts unused item */
+		if (lcache->dlc_csize == 0 && !llink->ll_evicted)
 			llink->ll_evicted = 1;
 
 		if (llink->ll_evicted) {
@@ -297,15 +284,20 @@ lru_ref_release_internal(struct daos_lru_cache *lcache, struct daos_llink *llink
 }
 
 void
-daos_lru_ref_release(struct daos_lru_cache *lcache, struct daos_llink *llink)
-{
-	lru_ref_release_internal(lcache, llink, false);
-}
-
-void
-daos_lru_ref_wait_evict(struct daos_lru_cache *lcache, struct daos_llink *llink)
+daos_lru_ref_evict_wait(struct daos_lru_cache *lcache, struct daos_llink *llink)
 {
-	D_ASSERT(lcache->dlc_ops->lop_wait);
-
-	lru_ref_release_internal(lcache, llink, true);
+	if (!llink->ll_evicted)
+		daos_lru_ref_evict(lcache, llink);
+
+	if (lcache->dlc_ops->lop_wait && !daos_lru_is_last_user(llink)) {
+		/* Wait until I'm the last one.
+		 * XXX: the implementation can only support one waiter for now, if there
+		 * is a secondary ULT calls this function on the same item, it will hit
+		 * the assertion.
+		 */
+		D_ASSERT(!llink->ll_wait_evict);
+		llink->ll_wait_evict = 1;
+		lcache->dlc_ops->lop_wait(llink);
+		llink->ll_wait_evict = 0;
+	}
 }

src/container/srv_target.c

@@ -1261,7 +1261,9 @@ cont_child_destroy_one(void *vin)
 			D_GOTO(out_pool, rc = -DER_BUSY);
 		} /* else: resync should have completed, try again */
 
-		daos_lru_ref_wait_evict(tls->dt_cont_cache, &cont->sc_list);
+		/* nobody should see it again after eviction */
+		daos_lru_ref_evict_wait(tls->dt_cont_cache, &cont->sc_list);
+		daos_lru_ref_release(tls->dt_cont_cache, &cont->sc_list);
 	}
 
 	D_DEBUG(DB_MD, DF_CONT": destroying vos container\n",

src/include/daos/lru.h

@@ -37,8 +37,8 @@ struct daos_llink {
 	d_list_t		 ll_link;	/**< LRU hash link */
 	d_list_t		 ll_qlink;	/**< Temp link for traverse */
 	uint32_t		 ll_ref;	/**< refcount for this ref */
-	uint32_t		 ll_evicted:1,	/**< has been evicted */
-				 ll_evicting:1; /**< been evicting */
+	uint32_t		 ll_evicted:1;	/**< has been evicted */
+	uint32_t		 ll_wait_evict:1; /**< wait for completion of eviction */
 	struct daos_llink_ops	*ll_ops;	/**< ops to maintain refs */
 };
 
@@ -121,26 +121,7 @@ void
 daos_lru_ref_release(struct daos_lru_cache *lcache, struct daos_llink *llink);
 
 /**
- * Evicts the LRU link from the DAOS LRU cache after waiting
- * for all references to be released.
- *
- * \param[in] lcache		DAOS LRU cache
- * \param[in] llink		DAOS LRU link to be evicted
- *
- */
-void
-daos_lru_ref_wait_evict(struct daos_lru_cache *lcache, struct daos_llink *llink);
-
-/**
- * Flush old items from LRU.
- *
- * \param[in] lcache		DAOS LRU cache
- */
-void
-daos_lru_ref_flush(struct daos_lru_cache *lcache);
-
-/**
- * Evict the item from LRU after releasing the last refcount on it.
+ * Evict the item from LRU before releasing the refcount on it.
  *
  * \param[in] lcache		DAOS LRU cache
  * \param[in] llink		DAOS LRU item to be evicted
@@ -153,15 +134,14 @@ daos_lru_ref_evict(struct daos_lru_cache *lcache, struct daos_llink *llink)
 }
 
 /**
- * Check if a LRU element has been evicted or not
+ * Evict the item from LRU before releasing the refcount on it, wait until
+ * the caller is the last one holds refcount.
  *
- * \param[in] llink		DAOS LRU item to check
+ * \param[in] lcache		DAOS LRU cache
+ * \param[in] llink		DAOS LRU item to be evicted
  */
-static inline bool
-daos_lru_ref_evicted(struct daos_llink *llink)
-{
-	return llink->ll_evicted;
-}
+void
+daos_lru_ref_evict_wait(struct daos_lru_cache *lcache, struct daos_llink *llink);
 
 /**
  * Increase a usage reference to the LRU element