rgw_file: alternate fix deadlock on lru eviction

This change is an alternate fix for two problems found and fixed
by Yao Zongyou <yaozongyou@vip.qq.com>.

The deadlock can be avoided just by not taking it in the recycle
case, which invariantly holds the lock.

The invalidation of the insert iterator by the recyle-path unlink
we'd like to handle as a condition in order to preserve the cached
insertion point optimization we get in the common case.  (The
original behavior was, indeed, incorrect.)

Based on feedback from Yao, removed the RGWFileHandle dtor version
of the unlink check, which I think happened twice.

Signed-off-by: Matt Benjamin <mbenjamin@redhat.com>
(cherry picked from commit 3cf0880f86b8f7911139c4e3d672cf47420c8f49)

diff --git a/src/common/cohort_lru.h b/src/common/cohort_lru.h
index 6c8264a5efd0c21ba071736fa872027e8b4dbfca..99316b9267968017671840bb0a27c2963af8f665 100644 (file)
--- a/src/common/cohort_lru.h
+++ b/src/common/cohort_lru.h
@@ -32,6 +32,7 @@ namespace cohort {
     /* public flag values */
     constexpr uint32_t FLAG_NONE = 0x0000;
     constexpr uint32_t FLAG_INITIAL = 0x0001;
+    constexpr uint32_t FLAG_RECYCLE = 0x0002;
 
     enum class Edge : std::uint8_t
     {
@@ -232,12 +233,14 @@ namespace cohort {
          delete tdo;
       } /* unref */
 
-      Object* insert(ObjectFactory* fac, Edge edge, uint32_t flags) {
+      Object* insert(ObjectFactory* fac, Edge edge, uint32_t& flags) {
        /* use supplied functor to re-use an evicted object, or
         * allocate a new one of the descendant type */
        Object* o = evict_block();
-       if (o)
+       if (o) {
          fac->recycle(o); /* recycle existing object */
+         flags |= FLAG_RECYCLE;
+       }
        else
          o = fac->alloc(); /* get a new one */
 

diff --git a/src/rgw/rgw_file.cc b/src/rgw/rgw_file.cc
index 84f8e961e4b691a65685928fd98a1e0867a486b2..6a5f39542f21e3045c9ac5eae521f35cbe3a83ce 100644 (file)
--- a/src/rgw/rgw_file.cc
+++ b/src/rgw/rgw_file.cc
@@ -951,10 +951,6 @@ namespace rgw {
   }
 
   RGWFileHandle::~RGWFileHandle() {
-    /* in the non-delete case, handle may still be in handle table */
-    if (fh_hook.is_linked()) {
-      fs->fh_cache.remove(fh.fh_hk.object, this, FHCache::FLAG_LOCK);
-    }
     /* cond-unref parent */
     if (parent && (! parent->is_mount())) {
       /* safe because if parent->unref causes its deletion,
@@ -1002,9 +998,11 @@ namespace rgw {
     lsubdout(fs->get_context(), rgw, 17)
       << __func__ << " " << *this
       << dendl;
-    /* remove if still in fh_cache */
+    /* in the non-delete case, handle may still be in handle table */
     if (fh_hook.is_linked()) {
-      fs->fh_cache.remove(fh.fh_hk.object, this, FHCache::FLAG_LOCK);
+      /* in this case, we are being called from a context which holds
+       * the partition lock */
+      fs->fh_cache.remove(fh.fh_hk.object, this, FHCache::FLAG_NONE);
     }
     return true;
   } /* RGWFileHandle::reclaim */

diff --git a/src/rgw/rgw_file.h b/src/rgw/rgw_file.h
index 2f8b8bad140cffa2df3327d82e592d1ea85be5bb..9c82694de76c04ff7f52fddcd2df7884b3358e19 100644 (file)
--- a/src/rgw/rgw_file.h
+++ b/src/rgw/rgw_file.h
@@ -1059,16 +1059,25 @@ namespace rgw {
        /* make or re-use handle */
        RGWFileHandle::Factory prototype(this, parent, fhk,
                                         obj_name, CREATE_FLAGS(flags));
+       uint32_t iflags{cohort::lru::FLAG_INITIAL};
        fh = static_cast<RGWFileHandle*>(
          fh_lru.insert(&prototype,
                        cohort::lru::Edge::MRU,
-                       cohort::lru::FLAG_INITIAL));
+                       iflags));
        if (fh) {
          /* lock fh (LATCHED) */
          if (flags & RGWFileHandle::FLAG_LOCK)
            fh->mtx.lock();
-         /* inserts, releasing latch */
-         fh_cache.insert_latched(fh, lat, RGWFileHandle::FHCache::FLAG_UNLOCK);
+         if (likely(! (iflags & cohort::lru::FLAG_RECYCLE))) {
+           /* inserts at cached insert iterator, releasing latch */
+           fh_cache.insert_latched(
+             fh, lat, RGWFileHandle::FHCache::FLAG_UNLOCK);
+         } else {
+           /* recycle step invalidates Latch */
+           fh_cache.insert(
+             fhk.fh_hk.object, fh, RGWFileHandle::FHCache::FLAG_NONE);
+           lat.lock->unlock(); /* !LATCHED */
+         }
          get<1>(fhr) |= RGWFileHandle::FLAG_CREATE;
          /* ref parent (non-initial ref cannot fail on valid object) */
          if (! parent->is_mount()) {