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

* 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 <liang.zhen@intel.com>
Signed-off-by: Jeff Olivier <jeffolivier@google.com>
Co-authored-by: Jeff Olivier <jeffolivier@google.com>

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