From acad2c7da54c9fe1e084db7f6a4bf87d143c4106 Mon Sep 17 00:00:00 2001 From: Matthew DeVore Date: Fri, 6 Nov 2020 16:02:42 -0800 Subject: [PATCH] mdcache: do not remove sentinel ref in cih module Currently, the sentinel reference of an mdcache entry can be removed in _state_del_locked (see last line in that function) as well as in cih_remove_checked. After the last ref is removed, mdcache_lru_unref calls mdcache_lru_clean which calls mdc_clean_entry which calls cih_remove_checked, which attempts to 'put' the reference again, which finally leads to the -1 ref count. Other calls to cih_remove checked have been amended with a put ref, except one; mdc_up_try_release also tries to remove the sentinel ref twice: once through cih_remove_latched and again explicitly right before returning. This can also cause the refcnt to become negative, so its use of cih_remove_latched need not be amended with another put ref. --- .../FSAL_MDCACHE/mdcache_hash.h | 17 ++++------------- .../FSAL_MDCACHE/mdcache_helpers.c | 3 ++- .../Stackable_FSALs/FSAL_MDCACHE/mdcache_lru.c | 18 +++++++++--------- 3 files changed, 15 insertions(+), 23 deletions(-) diff --git a/src/FSAL/Stackable_FSALs/FSAL_MDCACHE/mdcache_hash.h b/src/FSAL/Stackable_FSALs/FSAL_MDCACHE/mdcache_hash.h index e9c1cd0d7..e4281813d 100644 --- a/src/FSAL/Stackable_FSALs/FSAL_MDCACHE/mdcache_hash.h +++ b/src/FSAL/Stackable_FSALs/FSAL_MDCACHE/mdcache_hash.h @@ -375,57 +375,48 @@ cih_set_latched(mdcache_entry_t *entry, cih_latch_t *latch, } /** * @brief Remove cache entry with existence check. * * Remove cache entry with existence check. The entry is assumed to * be hashed. * * @param entry [in] Entry to be removed. * - * @return (void) + * @return true if the entry was present and got removed */ static inline bool cih_remove_checked(mdcache_entry_t *entry) { struct avltree_node *node; cih_partition_t *cp = cih_partition_of_scalar(&cih_fhcache, entry->fh_hk.key.hk); - bool unref = false; - bool freed = false; + bool removed = false; PTHREAD_RWLOCK_wrlock(&cp->lock); node = cih_fhcache_inline_lookup(&cp->t, &entry->fh_hk.node_k); if (entry->fh_hk.inavl && node) { LogFullDebug(COMPONENT_CACHE_INODE, "Unhashing entry %p", entry); #ifdef USE_LTTNG tracepoint(mdcache, mdc_lru_remove, __func__, __LINE__, &entry->obj_handle, entry->lru.refcnt); #endif avltree_remove(node, &cp->t); cp->cache[cih_cache_offsetof(&cih_fhcache, entry->fh_hk.key.hk)] = NULL; entry->fh_hk.inavl = false; - /* return sentinel ref */ - unref = true; + removed = true; } PTHREAD_RWLOCK_unlock(&cp->lock); - if (unref) { - /* We can't unref with the lock held, in case this is the last - * ref. That will recurse into here, and try to take the lock - * again. */ - freed = mdcache_lru_unref(entry); - } - - return freed; + return removed; } /** * @brief Remove cache entry protected by latch * * Remove cache entry. * * @note Must NOT be called with qlane lock held. * * @param entry [in] Entry to be removed.0 diff --git a/src/FSAL/Stackable_FSALs/FSAL_MDCACHE/mdcache_helpers.c b/src/FSAL/Stackable_FSALs/FSAL_MDCACHE/mdcache_helpers.c index 55c21c817..4af3ca4a8 100644 --- a/src/FSAL/Stackable_FSALs/FSAL_MDCACHE/mdcache_helpers.c +++ b/src/FSAL/Stackable_FSALs/FSAL_MDCACHE/mdcache_helpers.c @@ -3453,21 +3453,22 @@ _mdcache_kill_entry(mdcache_entry_t *entry, if (isDebug(COMPONENT_CACHE_INODE)) { DisplayLogComponentLevel(COMPONENT_CACHE_INODE, file, line, function, NIV_DEBUG, "Kill %s entry %p obj_handle %p", object_file_type_to_str( entry->obj_handle.type), entry, &entry->obj_handle); } - freed = cih_remove_checked(entry); /* !reachable, drop sentinel ref */ + /* !reachable, drop sentinel ref */ + freed = cih_remove_checked(entry) && mdcache_lru_unref(entry); #ifdef USE_LTTNG tracepoint(mdcache, mdc_kill_entry, function, line, &entry->obj_handle, entry->lru.refcnt, freed); #endif if (!freed) { /* queue for cleanup */ mdcache_lru_cleanup_push(entry); } diff --git a/src/FSAL/Stackable_FSALs/FSAL_MDCACHE/mdcache_lru.c b/src/FSAL/Stackable_FSALs/FSAL_MDCACHE/mdcache_lru.c index 0344161b7..ac51651f7 100644 --- a/src/FSAL/Stackable_FSALs/FSAL_MDCACHE/mdcache_lru.c +++ b/src/FSAL/Stackable_FSALs/FSAL_MDCACHE/mdcache_lru.c @@ -746,29 +746,27 @@ lru_reap_impl(enum lru_q_id qid) struct lru_q *q = lru_queue_of(entry); #ifdef USE_LTTNG tracepoint(mdcache, mdc_lru_reap, __func__, __LINE__, &entry->obj_handle, entry->lru.refcnt); #endif LRU_DQ_SAFE(lru, q); entry->lru.qid = LRU_ENTRY_NONE; QUNLOCK(qlane); - cih_remove_latched(entry, &latch, - CIH_REMOVE_UNLOCK); - /* Note, we're not releasing our ref here. - * cih_remove_latched() called - * mdcache_lru_unref(), which released the - * sentinal ref, leaving just the one ref we - * took earlier. Returning this as is leaves it - * with a ref of 1 (ie, just the sentinal ref) - * */ + if (unlikely(!cih_remove_latched(entry, &latch, + CIH_REMOVE_UNLOCK))) { + abort(); + } + /* Remove sentinel ref, leaving only the one ref we + * took earlier. */ + mdcache_lru_unref(entry); goto out; } cih_hash_release(&latch); QUNLOCK(qlane); /* return the ref we took above--unref deals * correctly with reclaim case */ mdcache_lru_unref(entry); } /* foreach lane */ /* ! reclaimable */ @@ -1085,20 +1083,22 @@ mdcache_lru_cleanup_try_push(mdcache_entry_t *entry) * in this callpath is the one owned by * mdcache_unexport(). When it unref's, that may free * this object; otherwise, it will be freed when the * last op is done, as it's unreachable. */ /* It's safe to drop the attr lock here, as we have the * latch, so no one can look up the entry */ PTHREAD_RWLOCK_unlock(&entry->attr_lock); QUNLOCK(qlane); cih_remove_latched(entry, &latch, CIH_REMOVE_NONE); + /* Drop sentinel ref. */ + mdcache_lru_unref(entry); } else { PTHREAD_RWLOCK_unlock(&entry->attr_lock); QUNLOCK(qlane); } cih_hash_release(&latch); } /** * @brief Function that executes in the lru thread to process one lane -- 2.29.2.222.g5d2a92d10f8-goog