From: Samuel Just Date: Tue, 26 Jan 2021 00:21:21 +0000 (-0800) Subject: crimson/os/seastore: clarify reference ownership for TransactionManager X-Git-Tag: v17.1.0~2805^2~4 X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=668ee4593f32dc6fdeacb610117a3de49243169f;p=ceph.git crimson/os/seastore: clarify reference ownership for TransactionManager Signed-off-by: Samuel Just --- diff --git a/src/crimson/os/seastore/cache.h b/src/crimson/os/seastore/cache.h index cb34561fdae5..d441940cca26 100644 --- a/src/crimson/os/seastore/cache.h +++ b/src/crimson/os/seastore/cache.h @@ -518,5 +518,6 @@ private: /// Replace prev with next void replace_extent(CachedExtentRef next, CachedExtentRef prev); }; +using CacheRef = std::unique_ptr; } diff --git a/src/crimson/os/seastore/journal.h b/src/crimson/os/seastore/journal.h index 20b6cb92f6fa..8def2be3a4a2 100644 --- a/src/crimson/os/seastore/journal.h +++ b/src/crimson/os/seastore/journal.h @@ -408,6 +408,7 @@ private: ); }; +using JournalRef = std::unique_ptr; } WRITE_CLASS_DENC_BOUNDED(crimson::os::seastore::segment_header_t) diff --git a/src/crimson/os/seastore/seastore.cc b/src/crimson/os/seastore/seastore.cc index d1c60c49c042..f9221294cc00 100644 --- a/src/crimson/os/seastore/seastore.cc +++ b/src/crimson/os/seastore/seastore.cc @@ -232,7 +232,7 @@ seastar::future<> SeaStore::do_transaction( }).safe_then([this, &ctx] { return onode_manager->write_dirty(*ctx.transaction, ctx.onodes); }).safe_then([this, &ctx] { - return transaction_manager.submit_transaction(std::move(ctx.transaction)); + return transaction_manager->submit_transaction(std::move(ctx.transaction)); }).safe_then([&ctx]() { for (auto i : { ctx.ext_transaction.get_on_applied(), diff --git a/src/crimson/os/seastore/seastore.h b/src/crimson/os/seastore/seastore.h index 25ccdd899c7c..3bdc7bdb2290 100644 --- a/src/crimson/os/seastore/seastore.h +++ b/src/crimson/os/seastore/seastore.h @@ -18,6 +18,7 @@ #include "crimson/os/futurized_store.h" #include "crimson/os/seastore/transaction.h" #include "crimson/os/seastore/onode_manager.h" +#include "crimson/os/seastore/collection_manager.h" namespace crimson::os::seastore { @@ -31,8 +32,13 @@ class SeaStore final : public FuturizedStore { public: - SeaStore(TransactionManager &tm) : - transaction_manager(tm) {} + SeaStore( + TransactionManagerRef tm, + CollectionManagerRef cm, + OnodeManagerRef om + ) : transaction_manager(std::move(tm)), + collection_manager(std::move(cm)), + onode_manager(std::move(om)) {} ~SeaStore(); @@ -160,8 +166,9 @@ private: }); } - TransactionManager &transaction_manager; - std::unique_ptr onode_manager; + TransactionManagerRef transaction_manager; + CollectionManagerRef collection_manager; + OnodeManagerRef onode_manager; using tm_ertr = TransactionManager::base_ertr; using tm_ret = tm_ertr::future<>; diff --git a/src/crimson/os/seastore/segment_cleaner.h b/src/crimson/os/seastore/segment_cleaner.h index c04dd3362eed..e6297e57e2c0 100644 --- a/src/crimson/os/seastore/segment_cleaner.h +++ b/src/crimson/os/seastore/segment_cleaner.h @@ -685,5 +685,6 @@ private: segments[segment].state = Segment::segment_state_t::OPEN; } }; +using SegmentCleanerRef = std::unique_ptr; } diff --git a/src/crimson/os/seastore/transaction_manager.cc b/src/crimson/os/seastore/transaction_manager.cc index ae23b4ba0df5..35d686c0a337 100644 --- a/src/crimson/os/seastore/transaction_manager.cc +++ b/src/crimson/os/seastore/transaction_manager.cc @@ -19,33 +19,34 @@ namespace { namespace crimson::os::seastore { TransactionManager::TransactionManager( - SegmentManager &segment_manager, - SegmentCleaner &segment_cleaner, - Journal &journal, - Cache &cache, - LBAManager &lba_manager) - : segment_manager(segment_manager), - segment_cleaner(segment_cleaner), - cache(cache), - lba_manager(lba_manager), - journal(journal) + SegmentManager &_segment_manager, + SegmentCleanerRef _segment_cleaner, + JournalRef _journal, + CacheRef _cache, + LBAManagerRef _lba_manager) + : segment_manager(_segment_manager), + segment_cleaner(std::move(_segment_cleaner)), + cache(std::move(_cache)), + lba_manager(std::move(_lba_manager)), + journal(std::move(_journal)) { - journal.set_write_pipeline(&write_pipeline); + segment_cleaner->set_extent_callback(this); + journal->set_write_pipeline(&write_pipeline); } TransactionManager::mkfs_ertr::future<> TransactionManager::mkfs() { - return journal.open_for_write().safe_then([this](auto addr) { + return journal->open_for_write().safe_then([this](auto addr) { logger().debug("TransactionManager::mkfs: about to do_with"); - segment_cleaner.set_journal_head(addr); + segment_cleaner->set_journal_head(addr); return seastar::do_with( create_transaction(), [this](auto &transaction) { - logger().debug("TransactionManager::mkfs: about to cache.mkfs"); - cache.init(); - return cache.mkfs(*transaction + logger().debug("TransactionManager::mkfs: about to cache->mkfs"); + cache->init(); + return cache->mkfs(*transaction ).safe_then([this, &transaction] { - return lba_manager.mkfs(*transaction); + return lba_manager->mkfs(*transaction); }).safe_then([this, &transaction] { logger().debug("TransactionManager::mkfs: about to submit_transaction"); return submit_transaction(std::move(transaction)).handle_error( @@ -58,34 +59,34 @@ TransactionManager::mkfs_ertr::future<> TransactionManager::mkfs() }); }); }).safe_then([this] { - return journal.close(); + return journal->close(); }); } TransactionManager::mount_ertr::future<> TransactionManager::mount() { - cache.init(); - return journal.replay([this](auto seq, auto paddr, const auto &e) { - return cache.replay_delta(seq, paddr, e); + cache->init(); + return journal->replay([this](auto seq, auto paddr, const auto &e) { + return cache->replay_delta(seq, paddr, e); }).safe_then([this] { - return journal.open_for_write(); + return journal->open_for_write(); }).safe_then([this](auto addr) { - segment_cleaner.set_journal_head(addr); + segment_cleaner->set_journal_head(addr); return seastar::do_with( make_weak_transaction(), [this](auto &t) { - return cache.init_cached_extents(*t, [this](auto &t, auto &e) { - return lba_manager.init_cached_extent(t, e); + return cache->init_cached_extents(*t, [this](auto &t, auto &e) { + return lba_manager->init_cached_extent(t, e); }).safe_then([this, &t] { - assert(segment_cleaner.debug_check_space( - *segment_cleaner.get_empty_space_tracker())); - return lba_manager.scan_mapped_space( + assert(segment_cleaner->debug_check_space( + *segment_cleaner->get_empty_space_tracker())); + return lba_manager->scan_mapped_space( *t, [this](paddr_t addr, extent_len_t len) { logger().debug("TransactionManager::mount: marking {}~{} used", addr, len); - segment_cleaner.mark_space_used( + segment_cleaner->mark_space_used( addr, len , /* init_scan = */ true); @@ -93,7 +94,7 @@ TransactionManager::mount_ertr::future<> TransactionManager::mount() }); }); }).safe_then([this] { - segment_cleaner.complete_init(); + segment_cleaner->complete_init(); }).handle_error( mount_ertr::pass_further{}, crimson::ct_error::all_same_way([] { @@ -103,9 +104,9 @@ TransactionManager::mount_ertr::future<> TransactionManager::mount() } TransactionManager::close_ertr::future<> TransactionManager::close() { - return cache.close( + return cache->close( ).safe_then([this] { - return journal.close(); + return journal->close(); }); } @@ -113,7 +114,7 @@ TransactionManager::ref_ret TransactionManager::inc_ref( Transaction &t, LogicalCachedExtentRef &ref) { - return lba_manager.incref_extent(t, ref->get_laddr()).safe_then([](auto r) { + return lba_manager->incref_extent(t, ref->get_laddr()).safe_then([](auto r) { return r.refcount; }).handle_error( ref_ertr::pass_further{}, @@ -126,7 +127,7 @@ TransactionManager::ref_ret TransactionManager::inc_ref( Transaction &t, laddr_t offset) { - return lba_manager.incref_extent(t, offset).safe_then([](auto result) { + return lba_manager->incref_extent(t, offset).safe_then([](auto result) { return result.refcount; }); } @@ -135,13 +136,13 @@ TransactionManager::ref_ret TransactionManager::dec_ref( Transaction &t, LogicalCachedExtentRef &ref) { - return lba_manager.decref_extent(t, ref->get_laddr() + return lba_manager->decref_extent(t, ref->get_laddr() ).safe_then([this, &t, ref](auto ret) { if (ret.refcount == 0) { logger().debug( "TransactionManager::dec_ref: extent {} refcount 0", *ref); - cache.retire_extent(t, ref); + cache->retire_extent(t, ref); } return ret.refcount; }); @@ -151,13 +152,13 @@ TransactionManager::ref_ret TransactionManager::dec_ref( Transaction &t, laddr_t offset) { - return lba_manager.decref_extent(t, offset + return lba_manager->decref_extent(t, offset ).safe_then([this, offset, &t](auto result) -> ref_ret { if (result.refcount == 0) { logger().debug( "TransactionManager::dec_ref: offset {} refcount 0", offset); - return cache.retire_extent_if_cached(t, result.addr).safe_then([] { + return cache->retire_extent_if_cached(t, result.addr).safe_then([] { return ref_ret( ref_ertr::ready_future_marker{}, 0); @@ -195,24 +196,24 @@ TransactionManager::submit_transaction( auto &tref = *t; return tref.handle.enter(write_pipeline.prepare ).then([this, &tref]() mutable { - return segment_cleaner.do_immediate_work(tref); + return segment_cleaner->do_immediate_work(tref); }).safe_then([this, &tref]() mutable -> submit_transaction_ertr::future<> { logger().debug("TransactionManager::submit_transaction after do_immediate"); - auto record = cache.try_construct_record(tref); + auto record = cache->try_construct_record(tref); if (!record) { return crimson::ct_error::eagain::make(); } - return journal.submit_record(std::move(*record), tref.handle + return journal->submit_record(std::move(*record), tref.handle ).safe_then([this, &tref](auto p) mutable { auto [addr, journal_seq] = p; - segment_cleaner.set_journal_head(journal_seq); - cache.complete_commit(tref, addr, journal_seq, &segment_cleaner); - lba_manager.complete_transaction(tref); + segment_cleaner->set_journal_head(journal_seq); + cache->complete_commit(tref, addr, journal_seq, segment_cleaner.get()); + lba_manager->complete_transaction(tref); auto to_release = tref.get_segment_to_release(); if (to_release != NULL_SEG_ID) { - segment_cleaner.mark_segment_released(to_release); + segment_cleaner->mark_segment_released(to_release); return segment_manager.release(to_release); } else { return SegmentManager::release_ertr::now(); @@ -232,7 +233,7 @@ TransactionManager::submit_transaction( TransactionManager::get_next_dirty_extents_ret TransactionManager::get_next_dirty_extents(journal_seq_t seq) { - return cache.get_next_dirty_extents(seq); + return cache->get_next_dirty_extents(seq); } TransactionManager::rewrite_extent_ret TransactionManager::rewrite_extent( @@ -240,7 +241,7 @@ TransactionManager::rewrite_extent_ret TransactionManager::rewrite_extent( CachedExtentRef extent) { { - auto updated = cache.update_extent_from_transaction(t, extent); + auto updated = cache->update_extent_from_transaction(t, extent); if (!updated) { logger().debug( "{}: {} is already retired, skipping", @@ -256,10 +257,10 @@ TransactionManager::rewrite_extent_ret TransactionManager::rewrite_extent( "{}: marking root {} for rewrite", __func__, *extent); - cache.duplicate_for_write(t, extent); + cache->duplicate_for_write(t, extent); return rewrite_extent_ertr::now(); } - return lba_manager.rewrite_extent(t, extent); + return lba_manager->rewrite_extent(t, extent); } TransactionManager::get_extent_if_live_ret TransactionManager::get_extent_if_live( @@ -270,7 +271,7 @@ TransactionManager::get_extent_if_live_ret TransactionManager::get_extent_if_liv segment_off_t len) { CachedExtentRef ret; - auto status = cache.get_extent_if_cached(t, addr, &ret); + auto status = cache->get_extent_if_cached(t, addr, &ret); if (status != Transaction::get_extent_ret::ABSENT) { return get_extent_if_live_ret( get_extent_if_live_ertr::ready_future_marker{}, @@ -278,7 +279,7 @@ TransactionManager::get_extent_if_live_ret TransactionManager::get_extent_if_liv } if (is_logical_type(type)) { - return lba_manager.get_mapping( + return lba_manager->get_mapping( t, laddr, len).safe_then([=, &t](lba_pin_list_t pins) { @@ -294,7 +295,7 @@ TransactionManager::get_extent_if_live_ret TransactionManager::get_extent_if_liv ceph_assert(pin->get_laddr() == laddr); ceph_assert(pin->get_length() == (extent_len_t)len); if (pin->get_paddr() == addr) { - return cache.get_extent_by_type( + return cache->get_extent_by_type( t, type, addr, @@ -308,7 +309,7 @@ TransactionManager::get_extent_if_live_ret TransactionManager::get_extent_if_liv return crimson::ct_error::eagain::make(); } else { lref->set_pin(std::move(pin)); - lba_manager.add_pin(lref->get_pin()); + lba_manager->add_pin(lref->get_pin()); } } return get_extent_if_live_ret( @@ -325,7 +326,7 @@ TransactionManager::get_extent_if_live_ret TransactionManager::get_extent_if_liv logger().debug( "TransactionManager::get_extent_if_live: non-logical extent {}", addr); - return lba_manager.get_physical_extent_if_live( + return lba_manager->get_physical_extent_if_live( t, type, addr, diff --git a/src/crimson/os/seastore/transaction_manager.h b/src/crimson/os/seastore/transaction_manager.h index 6e616a804377..4317e4b83985 100644 --- a/src/crimson/os/seastore/transaction_manager.h +++ b/src/crimson/os/seastore/transaction_manager.h @@ -62,10 +62,10 @@ public: TransactionManager( SegmentManager &segment_manager, - SegmentCleaner &segment_cleaner, - Journal &journal, - Cache &cache, - LBAManager &lba_manager); + SegmentCleanerRef segment_cleaner, + JournalRef journal, + CacheRef cache, + LBAManagerRef lba_manager); /// Writes initial metadata to disk using mkfs_ertr = crimson::errorator< @@ -114,7 +114,7 @@ public: std::unique_ptr pin_list = std::make_unique(); auto &pin_list_ref = *pin_list; - return lba_manager.get_mapping( + return lba_manager->get_mapping( t, offset, length ).safe_then([this, &t, &pin_list_ref, &ret_ref](auto pins) { crimson::get_logger(ceph_subsys_filestore).debug( @@ -129,7 +129,7 @@ public: "read_extents: get_extent {}~{}", pin->get_paddr(), pin->get_length()); - return cache.get_extent( + return cache->get_extent( t, pin->get_paddr(), pin->get_length() @@ -140,7 +140,7 @@ public: return crimson::ct_error::eagain::make(); } else { ref->set_pin(std::move(pin)); - lba_manager.add_pin(ref->get_pin()); + lba_manager->add_pin(ref->get_pin()); } } ret_ref.push_back(std::make_pair(ref->get_laddr(), ref)); @@ -160,7 +160,7 @@ public: /// Obtain mutable copy of extent LogicalCachedExtentRef get_mutable_extent(Transaction &t, LogicalCachedExtentRef ref) { auto &logger = crimson::get_logger(ceph_subsys_filestore); - auto ret = cache.duplicate_for_write( + auto ret = cache->duplicate_for_write( t, ref)->cast(); if (!ret->has_pin()) { @@ -225,10 +225,10 @@ public: Transaction &t, laddr_t hint, extent_len_t len) { - auto ext = cache.alloc_new_extent( + auto ext = cache->alloc_new_extent( t, len); - return lba_manager.alloc_extent( + return lba_manager->alloc_extent( t, hint, len, @@ -308,7 +308,7 @@ public: scan_extents_ret scan_extents( scan_extents_cursor &cursor, extent_len_t bytes_to_read) final { - return journal.scan_extents(cursor, bytes_to_read); + return journal->scan_extents(cursor, bytes_to_read); } using release_segment_ret = @@ -326,7 +326,7 @@ public: using read_onode_root_ertr = base_ertr; using read_onode_root_ret = read_onode_root_ertr::future; read_onode_root_ret read_onode_root(Transaction &t) { - return cache.get_root(t).safe_then([](auto croot) { + return cache->get_root(t).safe_then([](auto croot) { return croot->get_root().onode_root; }); } @@ -337,8 +337,8 @@ public: * Write onode-tree root logical address, must be called after read. */ void write_onode_root(Transaction &t, laddr_t addr) { - auto croot = cache.get_root_fast(t); - croot = cache.duplicate_for_write(t, croot)->cast(); + auto croot = cache->get_root_fast(t); + croot = cache->duplicate_for_write(t, croot)->cast(); croot->get_root().onode_root = addr; } @@ -377,12 +377,22 @@ private: friend class Transaction; SegmentManager &segment_manager; - SegmentCleaner &segment_cleaner; - Cache &cache; - LBAManager &lba_manager; - Journal &journal; + SegmentCleanerRef segment_cleaner; + CacheRef cache; + LBAManagerRef lba_manager; + JournalRef journal; WritePipeline write_pipeline; + +public: + // Testing interfaces + auto get_segment_cleaner() { + return segment_cleaner.get(); + } + + auto get_lba_manager() { + return lba_manager.get(); + } }; using TransactionManagerRef = std::unique_ptr; diff --git a/src/crimson/tools/store-nbd.cc b/src/crimson/tools/store-nbd.cc index 22b9e987d61f..619f2abd89c6 100644 --- a/src/crimson/tools/store-nbd.cc +++ b/src/crimson/tools/store-nbd.cc @@ -510,10 +510,6 @@ seastar::future<> NBDHandler::run() class TMDriver final : public BlockDriver { const config_t config; std::unique_ptr segment_manager; - std::unique_ptr segment_cleaner; - std::unique_ptr journal; - std::unique_ptr cache; - LBAManagerRef lba_manager; std::unique_ptr tm; public: @@ -599,25 +595,26 @@ public: } void init() { - segment_cleaner = std::make_unique( + auto segment_cleaner = std::make_unique( SegmentCleaner::config_t::default_from_segment_manager( *segment_manager), true); - journal = std::make_unique(*segment_manager); - cache = std::make_unique(*segment_manager); - lba_manager = lba_manager::create_lba_manager(*segment_manager, *cache); - tm = std::make_unique( - *segment_manager, *segment_cleaner, *journal, *cache, *lba_manager); + auto journal = std::make_unique(*segment_manager); + auto cache = std::make_unique(*segment_manager); + auto lba_manager = lba_manager::create_lba_manager(*segment_manager, *cache); + journal->set_segment_provider(&*segment_cleaner); - segment_cleaner->set_extent_callback(&*tm); + + tm = std::make_unique( + *segment_manager, + std::move(segment_cleaner), + std::move(journal), + std::move(cache), + std::move(lba_manager)); } void clear() { tm.reset(); - lba_manager.reset(); - cache.reset(); - journal.reset(); - segment_cleaner.reset(); } size_t get_size() const final { diff --git a/src/test/crimson/seastore/transaction_manager_test_state.h b/src/test/crimson/seastore/transaction_manager_test_state.h index d62d84a49567..ed422329669d 100644 --- a/src/test/crimson/seastore/transaction_manager_test_state.h +++ b/src/test/crimson/seastore/transaction_manager_test_state.h @@ -20,9 +20,7 @@ protected: std::unique_ptr segment_manager; EphemeralTestState() - : segment_manager(segment_manager::create_test_ephemeral()) { - // init(); derived class must call - } + : segment_manager(segment_manager::create_test_ephemeral()) {} virtual void _init() = 0; void init() { @@ -47,6 +45,7 @@ protected: } seastar::future<> tm_setup() { + init(); return segment_manager->init( ).safe_then([this] { return _mkfs(); @@ -66,56 +65,63 @@ protected: } }; +auto get_transaction_manager( + SegmentManager &segment_manager +) { + auto segment_cleaner = std::make_unique( + SegmentCleaner::config_t::default_from_segment_manager( + segment_manager), + true); + auto journal = std::make_unique(segment_manager); + auto cache = std::make_unique(segment_manager); + auto lba_manager = lba_manager::create_lba_manager(segment_manager, *cache); + + journal->set_segment_provider(&*segment_cleaner); + + auto ret = std::make_unique( + segment_manager, + std::move(segment_cleaner), + std::move(journal), + std::move(cache), + std::move(lba_manager)); + return ret; +} + class TMTestState : public EphemeralTestState { protected: - std::unique_ptr segment_cleaner; - std::unique_ptr journal; - std::unique_ptr cache; - LBAManagerRef lba_manager; std::unique_ptr tm; + LBAManager *lba_manager; + SegmentCleaner *segment_cleaner; - TMTestState() : EphemeralTestState() { - init(); - } + TMTestState() : EphemeralTestState() {} - void _init() final { - segment_cleaner = std::make_unique( - SegmentCleaner::config_t::default_from_segment_manager( - *segment_manager), - true); - journal = std::make_unique(*segment_manager); - cache = std::make_unique(*segment_manager); - lba_manager = lba_manager::create_lba_manager(*segment_manager, *cache); - tm = std::make_unique( - *segment_manager, *segment_cleaner, *journal, *cache, *lba_manager); - - journal->set_segment_provider(&*segment_cleaner); - segment_cleaner->set_extent_callback(&*tm); + virtual void _init() { + tm = get_transaction_manager(*segment_manager); + segment_cleaner = tm->get_segment_cleaner(); + lba_manager = tm->get_lba_manager(); } - void _destroy() final { + virtual void _destroy() { + segment_cleaner = nullptr; + lba_manager = nullptr; tm.reset(); - lba_manager.reset(); - cache.reset(); - journal.reset(); - segment_cleaner.reset(); } - seastar::future<> _teardown() final { + virtual seastar::future<> _teardown() { return tm->close( ).handle_error( crimson::ct_error::assert_all{"Error in teardown"} ); } - seastar::future<> _mount() final { + virtual seastar::future<> _mount() { return tm->mount( ).handle_error( crimson::ct_error::assert_all{"Error in mount"} ); } - seastar::future<> _mkfs() final { + virtual seastar::future<> _mkfs() { return tm->mkfs( ).handle_error( crimson::ct_error::assert_all{"Error in teardown"}