From: Sage Weil <sage@redhat.com>
Date: Sun, 10 Jun 2018 20:57:31 +0000 (-0500)
Subject: os/bluestore: fix SharedBlobSet deregister race
X-Git-Tag: v12.2.6~14^2~5
X-Git-Url: http://git.apps.os.sepia.ceph.com/?a=commitdiff_plain;h=81d32633d96f8995df0ed084d5db3da1dbc47780;p=ceph.git

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>
(cherry picked from commit 345ea53bd30426d4e6159d0866497d5a8bf6f327)
---

diff --git a/src/os/bluestore/BlueStore.cc b/src/os/bluestore/BlueStore.cc
index d000804b74115..804011d9fd696 100644
--- a/src/os/bluestore/BlueStore.cc
+++ b/src/os/bluestore/BlueStore.cc
@@ -1667,7 +1667,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 29a531b62bd07..387c22373243b 100644
--- a/src/os/bluestore/BlueStore.h
+++ b/src/os/bluestore/BlueStore.h
@@ -444,17 +444,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() {