crimson/os/seastore: construct TransactionManager classes after device mount

To construct TransactionManager after all the devices are discoverred.

Also, it makes the following cleanups possible:
* Cleanup SeaStore and TransactionManager factory methods.
* Decouple TransactionManager from SegmentManagerGroup.
* Drop the unnecessary tm_make_config_t.
* Drop the unnecessary add_device() methods.

Signed-off-by: Yingxin Cheng <yingxin.cheng@intel.com>

src/crimson/os/seastore/extent_placement_manager.h

@@ -250,10 +250,6 @@ public:
       return crimson::do_for_each(md_writers_by_gen, [](auto &writer) {
         return writer->close();
       });
-    }).safe_then([this] {
-      devices_by_id.clear();
-      devices_by_id.resize(DEVICE_ID_MAX, nullptr);
-      primary_device = nullptr;
     });
   }
 

src/crimson/os/seastore/journal/circular_bounded_journal.h

@@ -277,9 +277,7 @@ public:
   rbm_abs_addr get_journal_end() const {
     return get_start_addr() + header.size + get_block_size(); // journal size + header length
   }
-  void add_device(RBMDevice* dev) {
-    device = dev;
-  }
+
 private:
   cbj_header_t header;
   RBMDevice* device;

src/crimson/os/seastore/seastore.cc

@@ -117,32 +117,17 @@ SeaStore::SeaStore(
   const std::string& root,
   MDStoreRef mdstore,
   DeviceRef dev,
-  TransactionManagerRef tm,
-  CollectionManagerRef cm,
-  OnodeManagerRef om)
+  bool is_test)
   : root(root),
     mdstore(std::move(mdstore)),
     device(std::move(dev)),
-    transaction_manager(std::move(tm)),
-    collection_manager(std::move(cm)),
-    onode_manager(std::move(om)),
     max_object_size(
-      get_conf<uint64_t>("seastore_default_max_object_size"))
+      get_conf<uint64_t>("seastore_default_max_object_size")),
+    is_test(is_test)
 {
   register_metrics();
 }
 
-SeaStore::SeaStore(
-  const std::string& root,
-  DeviceRef dev,
-  TransactionManagerRef tm,
-  CollectionManagerRef cm,
-  OnodeManagerRef om)
-  : SeaStore(
-    root,
-    std::make_unique<FileMDStore>(root),
-    std::move(dev), std::move(tm), std::move(cm), std::move(om)) {}
-
 SeaStore::~SeaStore() = default;
 
 void SeaStore::register_metrics()
@@ -185,10 +170,8 @@ seastar::future<> SeaStore::stop()
 
 SeaStore::mount_ertr::future<> SeaStore::mount()
 {
-  secondaries.clear();
   return device->mount(
   ).safe_then([this] {
-    transaction_manager->add_device(device.get(), true);
     auto sec_devices = device->get_secondary_devices();
     return crimson::do_for_each(sec_devices, [this](auto& device_entry) {
       device_id_t id = device_entry.first;
@@ -202,12 +185,12 @@ SeaStore::mount_ertr::future<> SeaStore::mount()
         ).safe_then([this, sec_dev=std::move(sec_dev), magic]() mutable {
           boost::ignore_unused(magic);  // avoid clang warning;
           assert(sec_dev->get_magic() == magic);
-          transaction_manager->add_device(sec_dev.get(), false);
           secondaries.emplace_back(std::move(sec_dev));
         });
       });
     });
   }).safe_then([this] {
+    init_managers();
     return transaction_manager->mount();
   }).handle_error(
     crimson::ct_error::assert_all{
@@ -218,8 +201,13 @@ SeaStore::mount_ertr::future<> SeaStore::mount()
 
 seastar::future<> SeaStore::umount()
 {
-  return transaction_manager->close(
-  ).safe_then([this] {
+  return [this] {
+    if (transaction_manager) {
+      return transaction_manager->close();
+    } else {
+      return TransactionManager::close_ertr::now();
+    }
+  }().safe_then([this] {
     return crimson::do_for_each(
       secondaries,
       [](auto& sec_dev) -> SegmentManager::close_ertr::future<>
@@ -228,6 +216,11 @@ seastar::future<> SeaStore::umount()
     });
   }).safe_then([this] {
     return device->close();
+  }).safe_then([this] {
+    secondaries.clear();
+    transaction_manager.reset();
+    collection_manager.reset();
+    onode_manager.reset();
   }).handle_error(
     crimson::ct_error::assert_all{
       "Invalid error in SeaStore::umount"
@@ -328,23 +321,16 @@ SeaStore::mkfs_ertr::future<> SeaStore::mkfs(uuid_d new_osd_fsid)
               sds}
           );
         }).safe_then([this] {
-          return crimson::do_for_each(secondaries, [this](auto& sec_dev) {
-            return sec_dev->mount().safe_then([this, &sec_dev] {
-              transaction_manager->add_device(sec_dev.get(), false);
-              return seastar::now();
-            });
+          return crimson::do_for_each(secondaries, [](auto& sec_dev) {
+            return sec_dev->mount();
           });
         });
       }).safe_then([this] {
         return device->mount();
       }).safe_then([this] {
-        transaction_manager->add_device(device.get(), true);
+        init_managers();
         return transaction_manager->mkfs();
       }).safe_then([this] {
-        for (auto& sec_dev : secondaries) {
-          transaction_manager->add_device(sec_dev.get(), false);
-        }
-        transaction_manager->add_device(device.get(), true);
         return transaction_manager->mount();
       }).safe_then([this] {
         return repeat_eagain([this] {
@@ -1872,6 +1858,23 @@ uuid_d SeaStore::get_fsid() const
   return device->get_meta().seastore_id;
 }
 
+void SeaStore::init_managers()
+{
+  ceph_assert(!transaction_manager);
+  ceph_assert(!collection_manager);
+  ceph_assert(!onode_manager);
+  std::vector<Device*> sec_devices;
+  for (auto &dev : secondaries) {
+    sec_devices.emplace_back(dev.get());
+  }
+  transaction_manager = make_transaction_manager(
+      device.get(), sec_devices, is_test);
+  collection_manager = std::make_unique<collection_manager::FlatCollectionManager>(
+      *transaction_manager);
+  onode_manager = std::make_unique<crimson::os::seastore::onode::FLTreeOnodeManager>(
+      *transaction_manager);
+}
+
 seastar::future<std::unique_ptr<SeaStore>> make_seastore(
   const std::string &device,
   const ConfigValues &config)
@@ -1880,19 +1883,28 @@ seastar::future<std::unique_ptr<SeaStore>> make_seastore(
     device
   ).then([&device](DeviceRef device_obj) {
 #ifndef NDEBUG
-    auto tm = make_transaction_manager(
-        tm_make_config_t::get_test_segmented_journal());
+    bool is_test = true;
 #else
-    auto tm = make_transaction_manager(tm_make_config_t::get_default());
+    bool is_test = false;
 #endif
-    auto cm = std::make_unique<collection_manager::FlatCollectionManager>(*tm);
+    auto mdstore = std::make_unique<FileMDStore>(device);
     return std::make_unique<SeaStore>(
       device,
+      std::move(mdstore),
       std::move(device_obj),
-      std::move(tm),
-      std::move(cm),
-      std::make_unique<crimson::os::seastore::onode::FLTreeOnodeManager>(*tm));
+      is_test);
   });
 }
 
+std::unique_ptr<SeaStore> make_test_seastore(
+  DeviceRef device,
+  SeaStore::MDStoreRef mdstore)
+{
+  return std::make_unique<SeaStore>(
+    "",
+    std::move(mdstore),
+    std::move(device),
+    true);
+}
+
 }

src/crimson/os/seastore/seastore.h

@@ -82,15 +82,7 @@ public:
     const std::string& root,
     MDStoreRef mdstore,
     DeviceRef device,
-    TransactionManagerRef tm,
-    CollectionManagerRef cm,
-    OnodeManagerRef om);
-  SeaStore(
-    const std::string& root,
-    DeviceRef device,
-    TransactionManagerRef tm,
-    CollectionManagerRef cm,
-    OnodeManagerRef om);
+    bool is_test);
   ~SeaStore();
     
   seastar::future<> stop() final;
@@ -331,14 +323,18 @@ private:
     const std::optional<std::string> &_start,
     OMapManager::omap_list_config_t config);
 
+  void init_managers();
+
   std::string root;
   MDStoreRef mdstore;
   DeviceRef device;
+  const uint32_t max_object_size = 0;
+  bool is_test;
+
   std::vector<DeviceRef> secondaries;
   TransactionManagerRef transaction_manager;
   CollectionManagerRef collection_manager;
   OnodeManagerRef onode_manager;
-  const uint32_t max_object_size = 0;
 
   using tm_iertr = TransactionManager::base_iertr;
   using tm_ret = tm_iertr::future<>;
@@ -447,4 +443,8 @@ private:
 seastar::future<std::unique_ptr<SeaStore>> make_seastore(
   const std::string &device,
   const ConfigValues &config);
+
+std::unique_ptr<SeaStore> make_test_seastore(
+  DeviceRef device,
+  SeaStore::MDStoreRef mdstore);
 }

src/crimson/os/seastore/transaction_manager.cc

@@ -8,6 +8,7 @@
 #include "crimson/os/seastore/transaction_manager.h"
 #include "crimson/os/seastore/journal.h"
 #include "crimson/os/seastore/lba_manager/btree/lba_btree_node.h"
+#include "crimson/os/seastore/random_block_manager/rbm_device.h"
 
 /*
  * TransactionManager logs
@@ -34,8 +35,7 @@ TransactionManager::TransactionManager(
     lba_manager(std::move(_lba_manager)),
     journal(std::move(_journal)),
     epm(std::move(epm)),
-    backref_manager(std::move(backref_manager)),
-    sm_group(*async_cleaner->get_segment_manager_group())
+    backref_manager(std::move(backref_manager))
 {
   async_cleaner->set_extent_callback(this);
   journal->set_write_pipeline(&write_pipeline);
@@ -170,9 +170,8 @@ TransactionManager::close_ertr::future<> TransactionManager::close() {
     return journal->close();
   }).safe_then([this] {
     return epm->close();
-  }).safe_then([FNAME, this] {
+  }).safe_then([FNAME] {
     INFO("completed");
-    sm_group.reset();
     return seastar::now();
   });
 }
@@ -621,7 +620,10 @@ TransactionManager::get_extents_if_live_ret TransactionManager::get_extents_if_l
 
 TransactionManager::~TransactionManager() {}
 
-TransactionManagerRef make_transaction_manager(tm_make_config_t config)
+TransactionManagerRef make_transaction_manager(
+    Device *primary_device,
+    const std::vector<Device*> &secondary_devices,
+    bool is_test)
 {
   LOG_PREFIX(make_transaction_manager);
   auto epm = std::make_unique<ExtentPlacementManager>();
@@ -630,9 +632,19 @@ TransactionManagerRef make_transaction_manager(tm_make_config_t config)
   auto sms = std::make_unique<SegmentManagerGroup>();
   auto backref_manager = create_backref_manager(*cache);
 
+  epm->add_device(primary_device, true);
+  if (primary_device->get_device_type() == device_type_t::SEGMENTED) {
+    sms->add_segment_manager(static_cast<SegmentManager*>(primary_device));
+  }
+  for (auto &p_dev : secondary_devices) {
+    epm->add_device(p_dev, false);
+    ceph_assert(p_dev->get_device_type() == device_type_t::SEGMENTED);
+    sms->add_segment_manager(static_cast<SegmentManager*>(p_dev));
+  }
+
   bool cleaner_is_detailed;
   AsyncCleaner::config_t cleaner_config;
-  if (config.is_test) {
+  if (is_test) {
     cleaner_is_detailed = true;
     cleaner_config = AsyncCleaner::config_t::get_test();
   } else {
@@ -645,15 +657,18 @@ TransactionManagerRef make_transaction_manager(tm_make_config_t config)
     *backref_manager,
     cleaner_is_detailed);
 
+  auto p_device_type = primary_device->get_device_type();
   JournalRef journal;
-  if (config.j_type == journal_type_t::SEGMENT_JOURNAL) {
+  if (p_device_type == device_type_t::SEGMENTED) {
     journal = journal::make_segmented(*async_cleaner);
   } else {
+    ceph_assert(p_device_type == device_type_t::RANDOM_BLOCK);
     journal = journal::make_circularbounded(
-      nullptr, "");
+      static_cast<random_block_device::RBMDevice*>(primary_device),
+      "");
     async_cleaner->set_disable_trim(true);
-    ERROR("disabling journal trimming since support for CircularBoundedJournal\
-	  hasn't been added yet");
+    ERROR("disabling journal trimming since support for CircularBoundedJournal "
+          "hasn't been added yet");
   }
   epm->init_ool_writers(
       *async_cleaner,

src/crimson/os/seastore/transaction_manager.h

@@ -29,48 +29,10 @@
 #include "crimson/os/seastore/journal.h"
 #include "crimson/os/seastore/extent_placement_manager.h"
 #include "crimson/os/seastore/device.h"
-#include "crimson/os/seastore/segment_manager_group.h"
 
 namespace crimson::os::seastore {
 class Journal;
 
-struct tm_make_config_t {
-  bool is_test;
-  journal_type_t j_type;
-
-  static tm_make_config_t get_default() {
-    return tm_make_config_t {
-      false,
-      journal_type_t::SEGMENT_JOURNAL
-    };
-  }
-  static tm_make_config_t get_test_segmented_journal() {
-    LOG_PREFIX(get_test_segmented_journal);
-    SUBWARN(seastore_tm, "test mode enabled!");
-    return tm_make_config_t {
-      true,
-      journal_type_t::SEGMENT_JOURNAL
-    };
-  }
-  static tm_make_config_t get_test_cb_journal() {
-    LOG_PREFIX(get_test_cb_journal);
-    SUBWARN(seastore_tm, "test mode enabled!");
-    return tm_make_config_t {
-      true,
-      journal_type_t::CIRCULARBOUNDED_JOURNAL
-    };
-  }
-
-  tm_make_config_t(const tm_make_config_t &) = default;
-  tm_make_config_t &operator=(const tm_make_config_t &) = default;
-private:
-  tm_make_config_t(
-    bool is_test,
-    journal_type_t j_type)
-    : is_test(is_test), j_type(j_type)
-  {}
-};
-
 template <typename F>
 auto repeat_eagain(F &&f) {
   return seastar::do_with(
@@ -625,19 +587,6 @@ public:
     return async_cleaner->stat();
   }
 
-  void add_device(Device* dev, bool is_primary) {
-    LOG_PREFIX(TransactionManager::add_device);
-    SUBDEBUG(seastore_tm, "adding device {}, is_primary={}",
-             dev->get_device_id(), is_primary);
-    epm->add_device(dev, is_primary);
-
-    if (dev->get_device_type() == device_type_t::SEGMENTED) {
-      auto sm = dynamic_cast<SegmentManager*>(dev);
-      ceph_assert(sm != nullptr);
-      sm_group.add_segment_manager(sm);
-    }
-  }
-
   ~TransactionManager();
 
 private:
@@ -649,7 +598,6 @@ private:
   JournalRef journal;
   ExtentPlacementManagerRef epm;
   BackrefManagerRef backref_manager;
-  SegmentManagerGroup &sm_group;
 
   WritePipeline write_pipeline;
 
@@ -680,5 +628,8 @@ public:
 };
 using TransactionManagerRef = std::unique_ptr<TransactionManager>;
 
-TransactionManagerRef make_transaction_manager(tm_make_config_t config);
+TransactionManagerRef make_transaction_manager(
+    Device *primary_device,
+    const std::vector<Device*> &secondary_devices,
+    bool is_test);
 }

src/crimson/tools/store_nbd/tm_driver.cc

@@ -131,13 +131,12 @@ seastar::future<bufferlist> TMDriver::read(
 
 void TMDriver::init()
 {
+  std::vector<Device*> sec_devices;
 #ifndef NDEBUG
-  tm = make_transaction_manager(
-      tm_make_config_t::get_test_segmented_journal());
+  tm = make_transaction_manager(device.get(), sec_devices, true);
 #else
-  tm = make_transaction_manager(tm_make_config_t::get_default());
+  tm = make_transaction_manager(device.get(), sec_devices, false);
 #endif
-  tm->add_device(device.get(), true);
 }
 
 void TMDriver::clear()

src/test/crimson/seastore/onode_tree/test_fltree_onode_manager.cc

@@ -84,7 +84,6 @@ struct fltree_onode_manager_test_t
   virtual FuturizedStore::mkfs_ertr::future<> _mkfs() final {
     return TMTestState::_mkfs(
     ).safe_then([this] {
-      tm->add_device(segment_manager.get(), true);
       return tm->mount(
       ).safe_then([this] {
 	return repeat_eagain([this] {

src/test/crimson/seastore/test_transaction_manager.cc

@@ -75,9 +75,9 @@ struct transaction_manager_test_t :
   seastar::future<> set_up_fut() final {
     std::string j_type = GetParam();
     if (j_type == "segmented") {
-      return tm_setup(tm_make_config_t::get_test_segmented_journal());
+      return tm_setup(journal_type_t::SEGMENT_JOURNAL);
     } else if (j_type == "circularbounded") {
-      return tm_setup(tm_make_config_t::get_test_cb_journal());
+      return tm_setup(journal_type_t::CIRCULARBOUNDED_JOURNAL);
     } else {
       ceph_assert(0 == "no support");
     }

src/test/crimson/seastore/transaction_manager_test_state.h

@@ -27,7 +27,7 @@ protected:
   segment_manager::EphemeralSegmentManagerRef segment_manager;
   std::list<segment_manager::EphemeralSegmentManagerRef> secondary_segment_managers;
   std::unique_ptr<random_block_device::RBMDevice> rb_device;
-  tm_make_config_t tm_config = tm_make_config_t::get_test_segmented_journal();
+  journal_type_t journal_type;
 
   EphemeralTestState(std::size_t num_segment_managers) {
     assert(num_segment_managers > 0);
@@ -67,15 +67,16 @@ protected:
   }
 
   seastar::future<> tm_setup(
-    tm_make_config_t config = tm_make_config_t::get_test_segmented_journal()) {
+      journal_type_t type = journal_type_t::SEGMENT_JOURNAL) {
     LOG_PREFIX(EphemeralTestState::tm_setup);
     SUBINFO(test, "begin with {} devices ...", get_num_devices());
-    tm_config = config;
+    journal_type = type;
+    // FIXME: should not initialize segment_manager with circularbounded-journal
     segment_manager = segment_manager::create_test_ephemeral();
     for (auto &sec_sm : secondary_segment_managers) {
       sec_sm = segment_manager::create_test_ephemeral();
     }
-    if (tm_config.j_type == journal_type_t::CIRCULARBOUNDED_JOURNAL) {
+    if (journal_type == journal_type_t::CIRCULARBOUNDED_JOURNAL) {
       auto config =
 	journal::CircularBoundedJournal::mkfs_config_t::get_default();
       rb_device.reset(new random_block_device::TestMemory(config.total_size));
@@ -140,19 +141,6 @@ protected:
   }
 };
 
-auto get_seastore(SeaStore::MDStoreRef mdstore, SegmentManagerRef sm) {
-  auto tm = make_transaction_manager(tm_make_config_t::get_test_segmented_journal());
-  auto cm = std::make_unique<collection_manager::FlatCollectionManager>(*tm);
-  return std::make_unique<SeaStore>(
-    "",
-    std::move(mdstore),
-    std::move(sm),
-    std::move(tm),
-    std::move(cm),
-    std::make_unique<crimson::os::seastore::onode::FLTreeOnodeManager>(*tm));
-}
-
-
 class TMTestState : public EphemeralTestState {
 protected:
   TransactionManagerRef tm;
@@ -166,17 +154,17 @@ protected:
   TMTestState(std::size_t num_devices) : EphemeralTestState(num_devices) {}
 
   virtual void _init() override {
-    tm = make_transaction_manager(tm_config);
-    tm->add_device(segment_manager.get(), true);
-    if (tm_config.j_type == journal_type_t::CIRCULARBOUNDED_JOURNAL) {
-      tm->add_device(rb_device.get(), false);
-      static_cast<journal::CircularBoundedJournal*>(tm->get_journal())->
-	add_device(rb_device.get());
+    std::vector<Device*> sec_devices;
+    for (auto &sec_sm : secondary_segment_managers) {
+      sec_devices.emplace_back(sec_sm.get());
     }
-    if (get_num_devices() > 1) {
-      for (auto &sec_sm : secondary_segment_managers) {
-        tm->add_device(sec_sm.get(), false);
-      }
+    if (journal_type == journal_type_t::CIRCULARBOUNDED_JOURNAL) {
+      // FIXME: should not initialize segment_manager with circularbounded-journal
+      // FIXME: no secondary device in the single device test
+      sec_devices.emplace_back(segment_manager.get());
+      tm = make_transaction_manager(rb_device.get(), sec_devices, true);
+    } else {
+      tm = make_transaction_manager(segment_manager.get(), sec_devices, true);
     }
     async_cleaner = tm->get_async_cleaner();
     lba_manager = tm->get_lba_manager();
@@ -211,7 +199,7 @@ protected:
   }
 
   virtual FuturizedStore::mkfs_ertr::future<> _mkfs() {
-    if (tm_config.j_type == journal_type_t::SEGMENT_JOURNAL) {
+    if (journal_type == journal_type_t::SEGMENT_JOURNAL) {
       return tm->mkfs(
       ).handle_error(
 	crimson::ct_error::assert_all{"Error in mkfs"}
@@ -365,9 +353,9 @@ protected:
   SeaStoreTestState() : EphemeralTestState(1) {}
 
   virtual void _init() final {
-    seastore = get_seastore(
-      std::make_unique<TestMDStoreState::Store>(mdstore_state.get_mdstore()),
-      std::make_unique<TestSegmentManagerWrapper>(*segment_manager));
+    seastore = make_test_seastore(
+      std::make_unique<TestSegmentManagerWrapper>(*segment_manager),
+      std::make_unique<TestMDStoreState::Store>(mdstore_state.get_mdstore()));
   }
 
   virtual void _destroy() final {