os/bluestore: fix SharedBlob unregistration

We use the SharedBlobSet remove() in three cases:

- from SharedBlob::put(), we try to remove ourselves from the set, but
  have to deal with a racing lookup, so the removal is conditional on
  nref still being 0.
- from split_cache(), we move the SharedBlob to another collection
- from make_blob_unshared(), we remove the entry when we clear the sbid.

The problem is that the condtiional remove() (for the first case) was being
used for all three cases, and in the second two cases nref is always != 0,
so it doesn't actually happen.  This can lead to a crash during cache
shutdown.

Fix by making two variants: remove() that is unconditional, and
try_remove() that is conditional.

Set the sb->coll pointer after because remove() asserts the parent matches
where we are unregistering.

Fixes: http://tracker.ceph.com/issues/22039
Signed-off-by: Sage Weil <sage@redhat.com>
(cherry picked from commit 78a465b90b1c5187cfec9f25eb58b3bf617ca39b)

diff --git a/src/os/bluestore/BlueStore.cc b/src/os/bluestore/BlueStore.cc
index 47eb571b426ca2680eeadaa129b091439019e818..4f5666e5da27e7ad2033f52546137ea77fe6a4e4 100644 (file)
--- 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()) {
-      if (get_parent()->remove(this)) {
+      if (get_parent()->try_remove(this)) {
        delete this;
       } else {
        ldout(coll->store->cct, 20)
@@ -3317,13 +3317,13 @@ void BlueStore::Collection::split_cache(
          continue;
        }
        ldout(store->cct, 20) << __func__ << "  moving " << *sb << dendl;
-       sb->coll = dest;
        if (sb->get_sbid()) {
          ldout(store->cct, 20) << __func__
                                << "   moving registration " << *sb << dendl;
          shared_blob_set.remove(sb);
          dest->shared_blob_set.add(dest, sb);
        }
+       sb->coll = dest;
        if (dest->cache != cache) {
          for (auto& i : sb->bc.buffer_map) {
            if (!i.second->is_writing()) {

diff --git a/src/os/bluestore/BlueStore.h b/src/os/bluestore/BlueStore.h
index df42ffa8c44da0916572b302edb23efe41817996..cff739105ef9971f004875a1d2609a1ca8879680 100644 (file)
--- a/src/os/bluestore/BlueStore.h
+++ b/src/os/bluestore/BlueStore.h
@@ -442,7 +442,7 @@ public:
       sb->coll = coll;
     }
 
-    bool remove(SharedBlob *sb) {
+    bool try_remove(SharedBlob *sb) {
       std::lock_guard<std::mutex> l(lock);
       if (sb->nref == 0) {
        assert(sb->get_parent() == this);
@@ -452,6 +452,12 @@ public:
       return false;
     }
 
+    void remove(SharedBlob *sb) {
+      std::lock_guard<std::mutex> l(lock);
+      assert(sb->get_parent() == this);
+      sb_map.erase(sb->get_sbid());
+    }
+
     bool empty() {
       std::lock_guard<std::mutex> l(lock);
       return sb_map.empty();