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>

diff --git a/src/os/bluestore/BlueStore.cc b/src/os/bluestore/BlueStore.cc
index 7a86d8ea83434e8f76fabfdc9b45a2f17d89a455..62f5ccb13881422883fbd1fa6777241f1da51505 100644 (file)
--- a/src/os/bluestore/BlueStore.cc
+++ b/src/os/bluestore/BlueStore.cc
@@ -1670,7 +1670,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)
@@ -3323,13 +3323,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 474e52ac26cffba6d1787405f7298e628bec28d9..298cd0ec94325cac19ebeaf5a1be222fd17fc6d2 100644 (file)
--- a/src/os/bluestore/BlueStore.h
+++ b/src/os/bluestore/BlueStore.h
@@ -443,7 +443,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);
@@ -453,6 +453,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();