From 8afd93e4289048da582732918a678af8c6660d69 Mon Sep 17 00:00:00 2001 From: Liang Zhen Date: Thu, 5 Sep 2024 00:53:39 +0800 Subject: [PATCH] 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 Signed-off-by: Jeff Olivier Co-authored-by: Jeff Olivier --- src/common/lru.c | 54 ++++++++++++++++---------------------- src/container/srv_target.c | 4 ++- src/include/daos/lru.h | 38 +++++++-------------------- 3 files changed, 35 insertions(+), 61 deletions(-) diff --git a/src/common/lru.c b/src/common/lru.c index bb270500ab7..de86d367e0e 100644 --- a/src/common/lru.c +++ b/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; + } } diff --git a/src/container/srv_target.c b/src/container/srv_target.c index b5abcc2d759..f3ef47c8447 100644 --- a/src/container/srv_target.c +++ b/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", diff --git a/src/include/daos/lru.h b/src/include/daos/lru.h index 03b1eb90e4c..40bee5c492b 100644 --- a/src/include/daos/lru.h +++ b/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