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 47059790827d9b3e5702508017d7420b8c58655c..e662427521b164b7b8bdd36cc78d9ab7cbf003ce 100644 (file)
--- 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 4888fd30d34170f464577f91c1f6349740254484..edd98baa992523a33fabe1b1c2938d5797fe7118 100644 (file)
--- 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()) {