From: Sage Weil <sage@redhat.com>
Date: Wed, 8 Nov 2017 04:05:10 +0000 (-0600)
Subject: os/bluestore: fix SharedBlob unregistration
X-Git-Tag: v13.0.1~245^2
X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=refs%2Fpull%2F18805%2Fhead;p=ceph.git

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 7a86d8ea8343..62f5ccb13881 100644
--- 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 474e52ac26cf..298cd0ec9432 100644
--- 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();