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 <k.chai@proxmox.com>

diff --git a/src/rgw/driver/posix/bucket_cache.h b/src/rgw/driver/posix/bucket_cache.h
index 721c4e50abba125d9e199ada3f25419dfd3463aa..91ca76c50060c1f1d022b56f0e51f3b3ac091a3e 100644 (file)
--- a/src/rgw/driver/posix/bucket_cache.h
+++ b/src/rgw/driver/posix/bucket_cache.h
@@ -17,6 +17,7 @@
 #include <boost/intrusive/avl_set.hpp>
 #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<D, B>::FLAG_LOCK | BucketCache<D, B>::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<D, B>::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<D, B>::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<D, B>::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<D, B>::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<D, B>::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<D, B>::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();