os/bluestore: fix race between SharedBlobSet::lookup and SharedBlob::put

A                             B
SharedBlobSet::lookup()
  takes lock
  nref is not 0
                              SharedBlob::put()
                                --nref
returns SharedBlobRef,
  ++nref
                                takes cache lock
                                SharedBlobSet::remove
                                  takes lock
                                  removes
                                deletes SharedBlob

-> A ends up with a ref to deleted SharedBlob

Fix by verifying that nref is still zero in SharedBlobSet::remove(),
while we are holding the SharedBlobSet::lock.  The lock ensures that we
have increased the ref for the lookup before entering remove, so we can
verify that nref is still zero before removing it.  If not, we have
raced, and put() bails out and does nothing.

Fixes: http://tracker.ceph.com/issues/36526
Signed-off-by: Sage Weil <sage@redhat.com>

diff --git a/src/os/bluestore/BlueStore.cc b/src/os/bluestore/BlueStore.cc
index 2be713c06d8261db7a3633aa99f55d98ca817b19..4e603d8f15933ed554584c5880d28ab10d110699 100644 (file)
--- a/src/os/bluestore/BlueStore.cc
+++ b/src/os/bluestore/BlueStore.cc
@@ -1622,8 +1622,10 @@ void BlueStore::SharedBlob::put()
       if (coll_snap != coll) {
        goto again;
       }
-      coll_snap->shared_blob_set.remove(this);
-
+      if (!coll_snap->shared_blob_set.remove(this, true)) {
+       // race with lookup
+       return;
+      }
       bc._clear(coll_snap->cache);
       coll_snap->cache->rm_blob();
     }

diff --git a/src/os/bluestore/BlueStore.h b/src/os/bluestore/BlueStore.h
index c45ac8651f1ddf4827a3f766d4c710fc0f8ef46d..0051e10658b9ca20379475e9b55d182d74d36590 100644 (file)
--- a/src/os/bluestore/BlueStore.h
+++ b/src/os/bluestore/BlueStore.h
@@ -461,15 +461,19 @@ public:
       sb->coll = coll;
     }
 
-    void remove(SharedBlob *sb) {
+    bool remove(SharedBlob *sb, bool verify_nref_is_zero=false) {
       std::lock_guard l(lock);
       ceph_assert(sb->get_parent() == this);
+      if (verify_nref_is_zero && sb->nref != 0) {
+       return false;
+      }
       // 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);
       }
+      return true;
     }
 
     bool empty() {