From: Matt Benjamin <mbenjamin@redhat.com>
Date: Wed, 26 Nov 2025 14:00:03 +0000 (-0500)
Subject: posixdriver:  properly destruct BucketCacheEntry objects
X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=217285fa65fd8cc94bef9279feb8e1bf57235297;p=ceph.git

posixdriver:  properly destruct BucketCacheEntry objects

* avoids leak of database handles during eviction

Also adds missing return-ref in invalidate_entry--this would
leak a cache entry.

With this change, we can now tolerate indefinite s3-test runs
wit rgw_posix_cache_max_buckets=100.

Signed-off-by: Matt Benjamin <mbenjamin@redhat.com>
---

diff --git a/src/rgw/driver/posix/bucket_cache.h b/src/rgw/driver/posix/bucket_cache.h
index 47059790827..e662427521b 100644
--- a/src/rgw/driver/posix/bucket_cache.h
+++ b/src/rgw/driver/posix/bucket_cache.h
@@ -67,13 +67,23 @@ struct BucketCacheEntry : public cohort::lru::Object
 
 public:
   BucketCacheEntry(BucketCache<D, B>* bc, const std::string& name, uint64_t hk)
-    : bc(bc), name(name), hk(hk), flags(FLAG_NONE) {}
+    : bc(bc), name(name), env{nullptr}, hk(hk), flags(FLAG_NONE) {}
 
   void set_env(std::shared_ptr<LMDBSafe::MDBEnv>& _env, LMDBSafe::MDBDbi& _dbi) {
     env = _env;
     dbi = _dbi;
   }
 
+  virtual ~BucketCacheEntry()
+  {
+    /* XXX depends on safe_link -- but I think on balance, built-in safe_link
+     * is preferable to a custom mechanism with likely the same cost */
+    if (name_hook.is_linked()) {
+      bc->cache.remove(hk, this, bucket_avl_cache::FLAG_NONE);
+    }
+    mdb_dbi_close(*env, dbi); // return db handle
+  }
+
   inline bool deleted() const {
     return flags & FLAG_DELETED;
   }
@@ -152,14 +162,12 @@ public:
 
 	//std::cout << fmt::format("reclaim {}!", name) << std::endl;
 	bc->un->remove_watch(name);
-#if 1
-	// depends on safe_link
+
+	// XXX depends on safe_link
 	if (! name_hook.is_linked()) {
 	  // this should not happen!
 	  abort();
 	}
-#endif
-	bc->cache.remove(hk, this, bucket_avl_cache::FLAG_NONE);
 
 	/* discard lmdb data associated with this bucket */
 	auto txn = env->getRWTransaction();
@@ -646,6 +654,7 @@ public:
       b->flags &= ~BucketCacheEntry<D, B>::FLAG_FILLED;
 
       ulk.unlock();
+      lru.unref(b, cohort::lru::FLAG_NONE);
     } /* b */
 
     return 0;
diff --git a/src/rgw/driver/posix/rgw_sal_posix.cc b/src/rgw/driver/posix/rgw_sal_posix.cc
index 4888fd30d34..edd98baa992 100644
--- a/src/rgw/driver/posix/rgw_sal_posix.cc
+++ b/src/rgw/driver/posix/rgw_sal_posix.cc
@@ -2916,6 +2916,8 @@ int POSIXObject::delete_object(const DoutPrefixProvider* dpp,
 
   cls_rgw_obj_key key;
   get_key().get_index_key(&key);
+
+  /* XXXX we should get bucket cache once, ne? hint:  operate functor */
   driver->get_bucket_cache()->remove_entry(dpp, b->get_name(), key);
 
   if (!key.instance.empty() && !ent->exists()) {