os/bluestore: fix _split_collections race with osr_reap

The SharedBlobSet may not yet be empty at split time because previous
transactions may not have been reaped, leaving some Blobs still alive
even after the cache refs are cleared.  Drop them explicitly here (they
will go away shortly).

Signed-off-by: Sage Weil <sage@redhat.com>

diff --git a/src/os/bluestore/BlueStore.cc b/src/os/bluestore/BlueStore.cc
index 2212774c118647f4dcc60baa2f63046777b4709d..0791b6a917739749c1b57fa774a7a46f7993846f 100644 (file)
--- a/src/os/bluestore/BlueStore.cc
+++ b/src/os/bluestore/BlueStore.cc
@@ -1143,6 +1143,12 @@ void BlueStore::OnodeSpace::clear()
   onode_map.clear();
 }
 
+bool BlueStore::OnodeSpace::empty()
+{
+  std::lock_guard<std::recursive_mutex> l(cache->lock);
+  return onode_map.empty();
+}
+
 void BlueStore::OnodeSpace::rename(OnodeRef& oldo,
                                     const ghobject_t& old_oid,
                                     const ghobject_t& new_oid,
@@ -8922,17 +8928,24 @@ int BlueStore::_split_collection(TransContext *txc,
 {
   dout(15) << __func__ << " " << c->cid << " to " << d->cid << " "
           << " bits " << bits << dendl;
-  int r;
   RWLock::WLocker l(c->lock);
   RWLock::WLocker l2(d->lock);
 
-  // blow away the caches.  FIXME.
+  // blow away src cache
   c->onode_map.clear();
-  d->onode_map.clear();
 
-  assert(c->shared_blob_set.empty());
+  // We have an awkward race here: previous pipelinex transactions may
+  // still reference blobs and their shared_blobs.  They will be flushed
+  // shortly by _osr_reap_done, but it's awkward to block for that (and
+  // a waste of time).  Instead, explicitly remove them from the shared blob
+  // map.
+  c->shared_blob_set.violently_clear();
+
+  // the destination should be empty.
+  assert(d->onode_map.empty());
   assert(d->shared_blob_set.empty());
 
+  // adjust bits
   c->cnode.bits = bits;
   assert(d->cnode.bits == bits);
   r = 0;
@@ -8943,7 +8956,7 @@ int BlueStore::_split_collection(TransContext *txc,
 
   dout(10) << __func__ << " " << c->cid << " to " << d->cid << " "
           << " bits " << bits << " = " << r << dendl;
-  return r;
+  return 0;
 }
 
 

diff --git a/src/os/bluestore/BlueStore.h b/src/os/bluestore/BlueStore.h
index ea2ce8d685b6cc77670902b09efa722a4cca9145..53ea198a0c5753e018d03c9373376401299408f3 100644 (file)
--- a/src/os/bluestore/BlueStore.h
+++ b/src/os/bluestore/BlueStore.h
@@ -393,6 +393,14 @@ public:
       std::lock_guard<std::mutex> l(lock);
       return sb_map.empty();
     }
+
+    void violently_clear() {
+      std::lock_guard<std::mutex> l(lock);
+      for (auto& p : sb_map) {
+       p.second->parent_set = nullptr;
+      }
+      sb_map.clear();
+    }
   };
 
   /// in-memory blob metadata and associated cached buffers (if any)
@@ -1004,6 +1012,7 @@ public:
                const ghobject_t& new_oid,
                const string& new_okey);
     void clear();
+    bool empty();
 
     /// return true if f true for any item
     bool map_any(std::function<bool(OnodeRef)> f);

diff --git a/src/test/objectstore/store_test.cc b/src/test/objectstore/store_test.cc
index d615355fc3defd1175fa89f3a8c8d0e8930eec72..0bf4b1585f0205ee753eaf159b1005820ad3d08e 100644 (file)
--- a/src/test/objectstore/store_test.cc
+++ b/src/test/objectstore/store_test.cc
@@ -5107,7 +5107,8 @@ TEST_P(StoreTest, XattrTest) {
 void colsplittest(
   ObjectStore *store,
   unsigned num_objects,
-  unsigned common_suffix_size
+  unsigned common_suffix_size,
+  bool clones
   ) {
   ObjectStore::Sequencer osr("test");
   coll_t cid(spg_t(pg_t(0,52),shard_id_t::NO_SHARD));
@@ -5119,17 +5120,30 @@ void colsplittest(
     r = apply_transaction(store, &osr, std::move(t));
     ASSERT_EQ(r, 0);
   }
+  bufferlist small;
+  small.append("small");
   {
     ObjectStore::Transaction t;
-    for (uint32_t i = 0; i < 2*num_objects; ++i) {
+    for (uint32_t i = 0; i < (2 - (int)clones)*num_objects; ++i) {
       stringstream objname;
       objname << "obj" << i;
-      t.touch(cid, ghobject_t(hobject_t(
-         objname.str(),
-         "",
-         CEPH_NOSNAP,
-         i<<common_suffix_size,
-         52, "")));
+      ghobject_t a(hobject_t(
+                    objname.str(),
+                    "",
+                    CEPH_NOSNAP,
+                    i<<common_suffix_size,
+                    52, ""));
+      t.write(cid, a, 0, small.length(), small);
+      if (clones) {
+       objname << "-clone";
+       ghobject_t b(hobject_t(
+                      objname.str(),
+                      "",
+                      CEPH_NOSNAP,
+                      i<<common_suffix_size,
+                      52, ""));
+       t.clone(cid, a, b);
+      }
     }
     r = apply_transaction(store, &osr, std::move(t));
     ASSERT_EQ(r, 0);
@@ -5174,10 +5188,16 @@ void colsplittest(
 }
 
 TEST_P(StoreTest, ColSplitTest1) {
-  colsplittest(store.get(), 10000, 11);
+  colsplittest(store.get(), 10000, 11, false);
+}
+TEST_P(StoreTest, ColSplitTest1Clones) {
+  colsplittest(store.get(), 10000, 11, true);
 }
 TEST_P(StoreTest, ColSplitTest2) {
-  colsplittest(store.get(), 100, 7);
+  colsplittest(store.get(), 100, 7, false);
+}
+TEST_P(StoreTest, ColSplitTest2Clones) {
+  colsplittest(store.get(), 100, 7, true);
 }
 
 #if 0