os/bluestore: fix SharedBlobSet deregister race

In commit 8c8944b2c45ca9dc5b8fd4db1590e1d24206c0b3 we fixed one narrow race
and introduced a new (probably less narrow) one:

A: put shared blob foo, nref = 0, start removing self from set
B: open_shared_blob -> lookup gets nullptr (nref==null), creates a new shared blob
  B: takes lock, sets sb_map[sbid] = newblob
A: gets lock, erases sb_map[sbid]
B: open_shared_blob -> lookup gets null, creates another new shared blob

Fix by only removing the sb_map entry for the nref=0 sb if it still points
to us.  If it doesn't, that means some new blob has shown up in its place.

Fixes: http://tracker.ceph.com/issues/24319
Signed-off-by: Sage Weil <sage@redhat.com>

diff --git a/src/os/bluestore/BlueStore.cc b/src/os/bluestore/BlueStore.cc
index 12edfbde5b81d474b5df1cd1c5d4dceee4f379ec..cabd7d220fb6df65d0f71d226a6376980fe37f33 100644 (file)
--- a/src/os/bluestore/BlueStore.cc
+++ b/src/os/bluestore/BlueStore.cc
@@ -1672,7 +1672,7 @@ void BlueStore::SharedBlob::put()
                             << " removing self from set " << get_parent()
                             << dendl;
     if (get_parent()) {
-      get_parent()->remove_last(this);
+      get_parent()->remove(this);
     }
     delete this;
   }

diff --git a/src/os/bluestore/BlueStore.h b/src/os/bluestore/BlueStore.h
index 84693b081a770373c2667c10b98935e3489de6fe..78d89ee6d6faa7b6a370bfb6ce5bbbd53ea4820f 100644 (file)
--- a/src/os/bluestore/BlueStore.h
+++ b/src/os/bluestore/BlueStore.h
@@ -450,17 +450,15 @@ public:
       sb->coll = coll;
     }
 
-    void remove_last(SharedBlob *sb) {
-      std::lock_guard<std::mutex> l(lock);
-      assert(sb->nref == 0);
-      assert(sb->get_parent() == this);
-      sb_map.erase(sb->get_sbid());
-    }
-
     void remove(SharedBlob *sb) {
       std::lock_guard<std::mutex> l(lock);
       assert(sb->get_parent() == this);
-      sb_map.erase(sb->get_sbid());
+      // only remove if it still points to us
+      auto p = sb_map.find(sb->get_sbid());
+      if (p != sb_map.end() &&
+         p->second == sb) {
+       sb_map.erase(p);
+      }
     }
 
     bool empty() {