os/bluestore: fix races on SharedBlob::coll in ~SharedBlob.

Example scenario:
  ```
  A: BlueStore::Collection::split_cache(src, dest)
  A: std::lock(src->cache->lock, dest->cache->lock)

  B: SharedBlob::~SharedBlob
  B: waits coll->cache->lock // coll == src

  A: sb->coll := dest
  A: unlocks both src and dest's cache mutexes

  C: // any locked operation on dest's cache
  C: acquires dest->cache->lock
  C: begins the op

  B: // with the src's cache mutex acquired
  B: BufferSpace::_clear(coll->cache) // coll == dest
  B: // oops, B operates on cache already locked by C
  ```

Fixes: http://tracker.ceph.com/issues/24859
Signed-off-by: Radoslaw Zarzynski <rzarzyns@redhat.com>

diff --git a/src/os/bluestore/BlueStore.cc b/src/os/bluestore/BlueStore.cc
index c73749ded22d6b6b97c74e9a0cd5736d7ecba841..1a662bc6411fcc35de5b8999482425a0279d09e4 100644 (file)
--- a/src/os/bluestore/BlueStore.cc
+++ b/src/os/bluestore/BlueStore.cc
@@ -1598,11 +1598,6 @@ BlueStore::SharedBlob::SharedBlob(uint64_t i, Collection *_coll)
 
 BlueStore::SharedBlob::~SharedBlob()
 {
-  if (get_cache()) {   // the dummy instances have a nullptr
-    std::lock_guard<std::recursive_mutex> l(get_cache()->lock);
-    bc._clear(get_cache());
-    get_cache()->rm_blob();
-  }
   if (loaded && persistent) {
     delete persistent; 
   }
@@ -1614,8 +1609,17 @@ void BlueStore::SharedBlob::put()
     ldout(coll->store->cct, 20) << __func__ << " " << this
                             << " removing self from set " << get_parent()
                             << dendl;
-    if (get_parent()) {
-      get_parent()->remove(this);
+  again:
+    auto coll_snap = coll;
+    if (coll_snap) {
+      std::lock_guard<std::recursive_mutex> l(coll_snap->cache->lock);
+      if (coll_snap != coll) {
+       goto again;
+      }
+      coll_snap->shared_blob_set.remove(this);
+
+      bc._clear(coll_snap->cache);
+      coll_snap->cache->rm_blob();
     }
     delete this;
   }