os/bluestore: reuse zombie OpSequencers by collection id

We can get a sequence that deletes and then recreates a collection where
the transaction removing the collection is delayed (due to pending IO on
its sequencer) but colleciton create is not (new sequencer).

Avoid any such reordering by recycling the old collection's sequencer if
the zombie_osr has not been reaped yet.

Fixes: http://tracker.ceph.com/issues/24550
Signed-off-by: Sage Weil <sage@redhat.com>
(cherry picked from commit 199a110c9e2a42be64bd7fd873b6b172ce1347a8)

diff --git a/src/os/bluestore/BlueStore.cc b/src/os/bluestore/BlueStore.cc
index ce560e7fa3619f906b17519e67a7c1962ed97025..9dea4e235a7852521014d6f3b6310581904955b0 100644 (file)
--- a/src/os/bluestore/BlueStore.cc
+++ b/src/os/bluestore/BlueStore.cc
@@ -3214,16 +3214,28 @@ void BlueStore::DeferredBatch::_audit(CephContext *cct)
 #undef dout_prefix
 #define dout_prefix *_dout << "bluestore(" << store->path << ").collection(" << cid << " " << this << ") "
 
-BlueStore::Collection::Collection(BlueStore *ns, Cache *c, coll_t cid)
+BlueStore::Collection::Collection(BlueStore *store_, Cache *c, coll_t cid)
   : CollectionImpl(cid),
-    store(ns),
-    osr(new OpSequencer(store)),
+    store(store_),
     cache(c),
     lock("BlueStore::Collection::lock", true, false),
     exists(true),
     onode_map(c)
 {
-  osr->shard = cid.hash_to_shard(ns->m_finisher_num);
+  {
+    std::lock_guard<std::mutex> l(store->zombie_osr_lock);
+    auto p = store->zombie_osr_set.find(cid);
+    if (p == store->zombie_osr_set.end()) {
+      osr = new OpSequencer(store, cid);
+      osr->shard = cid.hash_to_shard(store->m_finisher_num);
+    } else {
+      osr = p->second;
+      store->zombie_osr_set.erase(p);
+      ldout(store->cct, 10) << "resurrecting zombie osr " << osr << dendl;
+      osr->zombie = false;
+      assert(osr->shard == cid.hash_to_shard(store->m_finisher_num));
+    }
+  }
 }
 
 bool BlueStore::Collection::flush_commit(Context *c)
@@ -8704,12 +8716,11 @@ void BlueStore::_txc_finish(TransContext *txc)
 
   if (empty && osr->zombie) {
     std::lock_guard<std::mutex> l(zombie_osr_lock);
-    auto p = zombie_osr_set.find(osr);
-    if (p != zombie_osr_set.end()) {
+    if (zombie_osr_set.erase(osr->cid)) {
       dout(10) << __func__ << " reaping empty zombie osr " << osr << dendl;
-      zombie_osr_set.erase(p);
     } else {
-      dout(10) << __func__ << " empty zombie osr " << osr << " already reaped" << dendl;
+      dout(10) << __func__ << " empty zombie osr " << osr << " already reaped"
+              << dendl;
     }
   }
   logger->set(l_bluestore_fragmentation,
@@ -8746,9 +8757,10 @@ out:
 void BlueStore::_osr_register_zombie(OpSequencer *osr)
 {
   std::lock_guard<std::mutex> l(zombie_osr_lock);
-  dout(10) << __func__ << " " << osr << dendl;
+  dout(10) << __func__ << " " << osr << " " << osr->cid << dendl;
   osr->zombie = true;
-  zombie_osr_set.insert(osr);
+  auto i = zombie_osr_set.emplace(osr->cid, osr);
+  assert(i.second); // this should be a new insertion
 }
 
 void BlueStore::_osr_drain_preceding(TransContext *txc)
@@ -8790,8 +8802,8 @@ void BlueStore::_osr_drain_all()
   {
     std::lock_guard<std::mutex> l(zombie_osr_lock);
     for (auto& i : zombie_osr_set) {
-      s.insert(i);
-      zombies.insert(i);
+      s.insert(i.second);
+      zombies.insert(i.second);
     }
   }
   dout(20) << __func__ << " osr_set " << s << dendl;
@@ -8819,14 +8831,17 @@ void BlueStore::_osr_drain_all()
   {
     std::lock_guard<std::mutex> l(zombie_osr_lock);
     for (auto& osr : zombies) {
-      auto p = zombie_osr_set.find(osr);
-      if (p != zombie_osr_set.end()) {
+      if (zombie_osr_set.erase(osr->cid)) {
        dout(10) << __func__ << " reaping empty zombie osr " << osr << dendl;
-       zombie_osr_set.erase(p);
+       assert(osr->q.empty());
+      } else if (osr->zombie) {
+       dout(10) << __func__ << " empty zombie osr " << osr
+                << " already reaped" << dendl;
+       assert(osr->q.empty());
       } else {
-       dout(10) << __func__ << " empty zombie osr " << osr << " already reaped" << dendl;
+       dout(10) << __func__ << " empty zombie osr " << osr
+                << " resurrected" << dendl;
       }
-      assert(osr->q.empty());
     }
   }
 

diff --git a/src/os/bluestore/BlueStore.h b/src/os/bluestore/BlueStore.h
index 4f31146909795d054a9259eb9887f2d47f54ddc4..9a9b56d1df7f447d3f500533c12f3db7771f217d 100644 (file)
--- a/src/os/bluestore/BlueStore.h
+++ b/src/os/bluestore/BlueStore.h
@@ -1678,6 +1678,7 @@ public:
     DeferredBatch *deferred_pending = nullptr;
 
     BlueStore *store;
+    coll_t cid;
 
     size_t shard;
 
@@ -1691,9 +1692,9 @@ public:
 
     std::atomic_bool zombie = {false};    ///< in zombie_osr set (collection going away)
 
-    OpSequencer(BlueStore *store)
+    OpSequencer(BlueStore *store, const coll_t& c)
       : RefCountedObject(store->cct, 0),
-       store(store) {
+       store(store), cid(c) {
     }
     ~OpSequencer() {
       assert(q.empty());
@@ -1830,7 +1831,7 @@ private:
   vector<Cache*> cache_shards;
 
   std::mutex zombie_osr_lock;              ///< protect zombie_osr_set
-  std::set<OpSequencerRef> zombie_osr_set; ///< set of OpSequencers for deleted collections
+  std::map<coll_t,OpSequencerRef> zombie_osr_set; ///< set of OpSequencers for deleted collections
 
   std::atomic<uint64_t> nid_last = {0};
   std::atomic<uint64_t> nid_max = {0};