Merge pull request ceph#58766 from athanatos/sjust/wip-66294-collection-race

crimson: access coll_map under alien tp with a lock

Reviewed-by: Yingxin Cheng <[email protected]>
Reviewed-by: Matan Breizman <[email protected]>

Author: Matan-B

src/crimson/os/alienstore/alien_store.cc

@@ -128,8 +128,12 @@ seastar::future<> AlienStore::stop()
     return seastar::now();
   }
   return tp->submit([this] {
-    for (auto [cid, ch]: coll_map) {
-      static_cast<AlienCollection*>(ch.get())->collection.reset();
+    {
+      std::lock_guard l(coll_map_lock);
+      for (auto [cid, ch]: coll_map) {
+	static_cast<AlienCollection*>(ch.get())->collection.reset();
+      }
+      coll_map.clear();
     }
     store.reset();
     cct.reset();
@@ -233,48 +237,22 @@ seastar::future<CollectionRef> AlienStore::create_new_collection(const coll_t& c
   logger().debug("{}", __func__);
   assert(tp);
   return tp->submit([this, cid] {
-    return store->create_new_collection(cid);
-  }).then([this, cid] (ObjectStore::CollectionHandle c) {
-    CollectionRef ch;
-    auto cp = coll_map.find(c->cid);
-    if (cp == coll_map.end()) {
-      ch = new AlienCollection(c);
-      coll_map[c->cid] = ch;
-    } else {
-      ch = cp->second;
-      auto ach = static_cast<AlienCollection*>(ch.get());
-      if (ach->collection != c) {
-        ach->collection = c;
-      }
-    }
-    return seastar::make_ready_future<CollectionRef>(ch);
+    ObjectStore::CollectionHandle c = store->create_new_collection(cid);
+    return get_alien_coll_ref(std::move(c));
   });
-
 }
 
 seastar::future<CollectionRef> AlienStore::open_collection(const coll_t& cid)
 {
   logger().debug("{}", __func__);
   assert(tp);
   return tp->submit([this, cid] {
-    return store->open_collection(cid);
-  }).then([this] (ObjectStore::CollectionHandle c) {
+    ObjectStore::CollectionHandle c = store->open_collection(cid);
     if (!c) {
-      return seastar::make_ready_future<CollectionRef>();
-    }
-    CollectionRef ch;
-    auto cp = coll_map.find(c->cid);
-    if (cp == coll_map.end()){
-      ch = new AlienCollection(c);
-      coll_map[c->cid] = ch;
+      return CollectionRef{};
     } else {
-      ch = cp->second;
-      auto ach = static_cast<AlienCollection*>(ch.get());
-      if (ach->collection != c){
-        ach->collection = c;
-      }
+      return get_alien_coll_ref(std::move(c));
     }
-    return seastar::make_ready_future<CollectionRef>(ch);
   });
 }
 
@@ -657,4 +635,21 @@ AlienStore::read_errorator::future<std::map<uint64_t, uint64_t>> AlienStore::fie
   });
 }
 
+CollectionRef AlienStore::get_alien_coll_ref(ObjectStore::CollectionHandle c) {
+  std::lock_guard l(coll_map_lock);
+  CollectionRef ch;
+  auto cp = coll_map.find(c->cid);
+  if (cp == coll_map.end()) {
+    ch = new AlienCollection(c);
+    coll_map[c->cid] = ch;
+  } else {
+    ch = cp->second;
+    auto ach = static_cast<AlienCollection*>(ch.get());
+    if (ach->collection != c) {
+      ach->collection = c;
+    }
+  }
+  return ch;
+}
+
 }

src/crimson/os/alienstore/alien_store.h

@@ -131,6 +131,29 @@ class AlienStore final : public FuturizedStore,
   std::unique_ptr<ObjectStore> store;
   std::unique_ptr<CephContext> cct;
   mutable seastar::gate op_gate;
+
+  /**
+   * coll_map
+   *
+   * Contains a reference to every CollectionRef returned to the upper layer.
+   * It's important that ObjectStore::CollectionHandle instances (in particular,
+   * those from BlueStore) not be released from seastar reactor threads.
+   * Keeping a reference here and breaking the
+   * CollectionRef->ObjectStore::CollectionHandle links in AlienStore::stop()
+   * ensures that all CollectionHandle's are released in the alien thread pool.
+   *
+   * Long term, we probably want to drop this map.  To do that two things need
+   * to happen:
+   * 1. ~AlienCollection() needs to submit the ObjectStore::CollectionHandle
+   *    instance to the alien thread pool to be released.
+   * 2. OSD shutdown needs to *guarantee* that all outstanding CollectionRefs
+   *    are released before unmounting and stopping the store.
+   *
+   * coll_map is accessed exclusively from alien threadpool threads under the
+   * coll_map_lock.
+   */
+  std::mutex coll_map_lock;
   std::unordered_map<coll_t, CollectionRef> coll_map;
+  CollectionRef get_alien_coll_ref(ObjectStore::CollectionHandle c);
 };
 }