From: Kefu Chai Date: Sat, 30 May 2026 07:49:14 +0000 (+0800) Subject: rgw/posix: fix refcount leaks in BucketCache X-Git-Tag: v21.0.1~64^2~1 X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=6e3c66b701015cefa20aeb96cf75d546e246daac;p=ceph.git rgw/posix: fix refcount leaks in BucketCache get_bucket(FLAG_LOCK) increments the refcount via lru.ref(), but three paths returned without the paired lru.unref(): the "do nothing" early return and the INVALIDATE branch in notify(), and unconditionally in invalidate_bucket(). Entries hitting these paths accumulated inflated refcounts that the LRU could never reclaim, leaking during ~BucketCache() → cache.drain(). Replace the manual lru.unref() calls in notify(), add_entry(), remove_entry(), invalidate_bucket(), and list_bucket() with a scope_guard declared before unique_lock. Since the guard outlives ulk, it fires after the mutex is released on all paths, including exceptions from getRWTransaction() or txn->commit() (e.g. MDB_MAP_FULL, EIO) that the manual calls never reached. list_bucket() also had a bare b->mtx.unlock() after fill(); replace it with unique_lock{..., std::adopt_lock} so a throw from fill() releases the mutex too. Signed-off-by: Kefu Chai --- diff --git a/src/rgw/driver/posix/bucket_cache.h b/src/rgw/driver/posix/bucket_cache.h index 721c4e50abb..91ca76c5006 100644 --- a/src/rgw/driver/posix/bucket_cache.h +++ b/src/rgw/driver/posix/bucket_cache.h @@ -17,6 +17,7 @@ #include #include "include/function2.hpp" #include "common/cohort_lru.h" +#include "include/scope_guard.h" #include "lmdb-safe.hh" #include "zpp_bits.h" #include "notify.h" @@ -286,8 +287,10 @@ public: * Drain the AVL cache and unref each entry to trigger deletion. * * Entries should have refcount=1 (sentinel state) after normal use - * because list_bucket() calls lru.unref() on all code paths. When - * we call unref here, the refcount goes to 0 and the entry is deleted. + * because every public method that calls get_bucket() uses a scope_guard + * to pair lru.unref() with the get_bucket() ref on all paths (including + * exceptions). When we call unref here, the refcount goes to 0 and the + * entry is deleted. * * The drain() method safely removes all entries * from the AVL cache partitions and calls the supplied lambda on each, @@ -426,12 +429,14 @@ public: BucketCache::FLAG_LOCK | BucketCache::FLAG_CREATE); auto [b /* BucketCacheEntry */, flags] = gbr; if (b /* XXX again, can this fail? */) { + auto unref_guard = make_scope_guard([this, b]{ lru.unref(b, cohort::lru::FLAG_NONE); }); + unique_lock ulk{b->mtx, std::adopt_lock}; if (! (b->flags & BucketCacheEntry::FLAG_FILLED)) { /* bulk load into lmdb cache */ rc = fill(dpp, b, sal_bucket, FLAG_NONE, y); } /* display them */ - b->mtx.unlock(); + ulk.unlock(); /*! LOCKED */ auto txn = b->env->getROTransaction(); @@ -467,7 +472,6 @@ public: auto rc = cursor.lower_bound(k, key, data); if (rc == MDB_NOTFOUND) { /* no key sorts after k/marker, so there is nothing to do */ - lru.unref(b, cohort::lru::FLAG_NONE); return 0; } proc_result(); @@ -476,7 +480,6 @@ public: auto rc = cursor.get(key, data, MDB_FIRST); if (rc == MDB_NOTFOUND) { /* no initial key */ - lru.unref(b, cohort::lru::FLAG_NONE); return 0; } if (rc == MDB_SUCCESS) { @@ -485,12 +488,10 @@ public: } while(cursor.get(key, data, MDB_NEXT) == MDB_SUCCESS) { if (!again) { - lru.unref(b, cohort::lru::FLAG_NONE); return 0; } proc_result(); } - lru.unref(b, cohort::lru::FLAG_NONE); } /* b */ return 0; @@ -505,6 +506,7 @@ public: GetBucketResult gbr = get_bucket(nullptr, bname, BucketCache::FLAG_LOCK); auto [b /* BucketCacheEntry */, flags] = gbr; if (b) { + auto unref_guard = make_scope_guard([this, b]{ lru.unref(b, cohort::lru::FLAG_NONE); }); unique_lock ulk{b->mtx, std::adopt_lock}; if ((b->name != bname) || (opaque && (b != opaque)) || @@ -560,6 +562,7 @@ public: mdb_drop(*txn, b->dbi, 0); txn->commit(); b->flags &= ~BucketCacheEntry::FLAG_FILLED; + ulk.unlock(); return 0; /* don't process any more events in this batch */ } break; @@ -569,7 +572,6 @@ public: } } /* all events */ txn->commit(); - lru.unref(b, cohort::lru::FLAG_NONE); } /* b */ return rc; } /* notify */ @@ -580,6 +582,7 @@ public: GetBucketResult gbr = get_bucket(dpp, bname, BucketCache::FLAG_LOCK); auto [b /* BucketCacheEntry */, flags] = gbr; if (b) { + auto unref_guard = make_scope_guard([this, b]{ lru.unref(b, cohort::lru::FLAG_NONE); }); unique_lock ulk{b->mtx, std::adopt_lock}; ulk.unlock(); @@ -602,7 +605,6 @@ public: txn->put(b->dbi, concat_k, ser_data); txn->commit(); - lru.unref(b, cohort::lru::FLAG_NONE); } /* b */ return 0; @@ -614,6 +616,7 @@ public: GetBucketResult gbr = get_bucket(dpp, bname, BucketCache::FLAG_LOCK); auto [b /* BucketCacheEntry */, flags] = gbr; if (b) { + auto unref_guard = make_scope_guard([this, b]{ lru.unref(b, cohort::lru::FLAG_NONE); }); unique_lock ulk{b->mtx, std::adopt_lock}; ulk.unlock(); @@ -621,8 +624,6 @@ public: auto concat_k = concat_key(key); txn->del(b->dbi, concat_k); txn->commit(); - - lru.unref(b, cohort::lru::FLAG_NONE); } /* b */ return 0; @@ -634,6 +635,7 @@ public: GetBucketResult gbr = get_bucket(dpp, bname, BucketCache::FLAG_LOCK); auto [b /* BucketCacheEntry */, flags] = gbr; if (b) { + auto unref_guard = make_scope_guard([this, b]{ lru.unref(b, cohort::lru::FLAG_NONE); }); unique_lock ulk{b->mtx, std::adopt_lock}; auto txn = b->env->getRWTransaction();