From 1084a7dd4ad726bda64cb474ea4ad9bed6fb6c32 Mon Sep 17 00:00:00 2001 From: Samuel Just Date: Fri, 26 Mar 2021 23:10:08 -0700 Subject: [PATCH] crimson/os/seastore/transaction_manager: replace read_extents, expose pins read_extents in all except one case was used to read a known single extent -- replace those users with read_extent. store-nbd uses read_extents as intended, but other users will need to be able to deal with zero mappings. Signed-off-by: Samuel Just --- .../flat_collection_manager.cc | 6 +- .../btree/omap_btree_node_impl.cc | 12 +- .../node_extent_manager/seastore.h | 6 +- src/crimson/os/seastore/transaction_manager.h | 129 +++++++++++------- src/crimson/tools/store-nbd.cc | 40 +++++- .../seastore/test_transaction_manager.cc | 6 +- 6 files changed, 126 insertions(+), 73 deletions(-) diff --git a/src/crimson/os/seastore/collection_manager/flat_collection_manager.cc b/src/crimson/os/seastore/collection_manager/flat_collection_manager.cc index 08cf84ffa3358..40c6648778878 100644 --- a/src/crimson/os/seastore/collection_manager/flat_collection_manager.cc +++ b/src/crimson/os/seastore/collection_manager/flat_collection_manager.cc @@ -47,13 +47,11 @@ FlatCollectionManager::get_coll_root(const coll_root_t &coll_root, Transaction & logger().debug("FlatCollectionManager: {}", __func__); assert(coll_root.get_location() != L_ADDR_NULL); auto cc = get_coll_context(t); - return cc.tm.read_extents( + return cc.tm.read_extent( cc.t, coll_root.get_location(), coll_root.get_size() - ).safe_then([](auto&& extents) { - assert(extents.size() == 1); - [[maybe_unused]] auto [laddr, e] = extents.front(); + ).safe_then([](auto&& e) { return get_root_ertr::make_ready_future(std::move(e)); }); } diff --git a/src/crimson/os/seastore/omap_manager/btree/omap_btree_node_impl.cc b/src/crimson/os/seastore/omap_manager/btree/omap_btree_node_impl.cc index 59d8050a21241..c325dc4845f97 100644 --- a/src/crimson/os/seastore/omap_manager/btree/omap_btree_node_impl.cc +++ b/src/crimson/os/seastore/omap_manager/btree/omap_btree_node_impl.cc @@ -603,25 +603,21 @@ omap_load_extent(omap_context_t oc, laddr_t laddr, depth_t depth) { ceph_assert(depth > 0); if (depth > 1) { - return oc.tm.read_extents(oc.t, laddr, OMAP_BLOCK_SIZE + return oc.tm.read_extent(oc.t, laddr, OMAP_BLOCK_SIZE ).handle_error( omap_load_extent_ertr::pass_further{}, crimson::ct_error::assert_all{ "Invalid error in omap_load_extent" } ).safe_then( - [](auto&& extents) { - assert(extents.size() == 1); - [[maybe_unused]] auto [laddr, e] = extents.front(); + [](auto&& e) { return seastar::make_ready_future(std::move(e)); }); } else { - return oc.tm.read_extents(oc.t, laddr, OMAP_BLOCK_SIZE + return oc.tm.read_extent(oc.t, laddr, OMAP_BLOCK_SIZE ).handle_error( omap_load_extent_ertr::pass_further{}, crimson::ct_error::assert_all{ "Invalid error in omap_load_extent" } ).safe_then( - [](auto&& extents) { - assert(extents.size() == 1); - [[maybe_unused]] auto [laddr, e] = extents.front(); + [](auto&& e) { return seastar::make_ready_future(std::move(e)); }); } diff --git a/src/crimson/os/seastore/onode_manager/staged-fltree/node_extent_manager/seastore.h b/src/crimson/os/seastore/onode_manager/staged-fltree/node_extent_manager/seastore.h index f80b99fabe008..4b584372d1d52 100644 --- a/src/crimson/os/seastore/onode_manager/staged-fltree/node_extent_manager/seastore.h +++ b/src/crimson/os/seastore/onode_manager/staged-fltree/node_extent_manager/seastore.h @@ -75,10 +75,8 @@ class SeastoreNodeExtentManager final: public NodeExtentManager { tm_future read_extent( Transaction& t, laddr_t addr, extent_len_t len) override { logger().debug("OTree::Seastore: reading {}B at {:#x} ...", len, addr); - return tm.read_extents(t, addr, len - ).safe_then([addr, len](auto&& extents) { - assert(extents.size() == 1); - [[maybe_unused]] auto [laddr, e] = extents.front(); + return tm.read_extent(t, addr, len + ).safe_then([addr, len](auto&& e) { logger().trace("OTree::Seastore: read {}B at {:#x}", e->get_length(), e->get_laddr()); assert(e->get_laddr() == addr); diff --git a/src/crimson/os/seastore/transaction_manager.h b/src/crimson/os/seastore/transaction_manager.h index 93a24c8109f57..cc62376116265 100644 --- a/src/crimson/os/seastore/transaction_manager.h +++ b/src/crimson/os/seastore/transaction_manager.h @@ -96,64 +96,91 @@ public: } /** - * Read extents corresponding to specified lba range + * get_pins + * + * Get logical pins overlapping offset~length */ - using read_extent_ertr = LBAManager::get_mapping_ertr::extend_ertr< + using get_pins_ertr = LBAManager::get_mapping_ertr; + using get_pins_ret = get_pins_ertr::future; + get_pins_ret get_pins( + Transaction &t, + laddr_t offset, + extent_len_t length) { + return lba_manager->get_mapping( + t, offset, length); + } + + /** + * pin_to_extent + * + * Get extent mapped at pin. + */ + using pin_to_extent_ertr = get_pins_ertr::extend_ertr< SegmentManager::read_ertr>; template - using read_extent_ret = read_extent_ertr::future>; + using pin_to_extent_ret = pin_to_extent_ertr::future< + TCachedExtentRef>; template - read_extent_ret read_extents( + pin_to_extent_ret pin_to_extent( + Transaction &t, + LBAPinRef pin) { + using ret = pin_to_extent_ret; + crimson::get_logger(ceph_subsys_filestore).debug( + "pin_to_extent: getting extent {}", + *pin); + return cache->get_extent( + t, + pin->get_paddr(), + pin->get_length() + ).safe_then([this, pin=std::move(pin)](auto ref) mutable -> ret { + if (!ref->has_pin()) { + if (pin->has_been_invalidated() || ref->has_been_invalidated()) { + return crimson::ct_error::eagain::make(); + } else { + ref->set_pin(std::move(pin)); + lba_manager->add_pin(ref->get_pin()); + } + } + crimson::get_logger(ceph_subsys_filestore).debug( + "pin_to_extent: got extent {}", + *ref); + return pin_to_extent_ret( + pin_to_extent_ertr::ready_future_marker{}, + std::move(ref)); + }); + } + + /** + * read_extent + * + * Read extent of type T at offset~length + */ + using read_extent_ertr = get_pins_ertr::extend_ertr< + SegmentManager::read_ertr>; + template + using read_extent_ret = read_extent_ertr::future< + TCachedExtentRef>; + template + read_extent_ret read_extent( Transaction &t, laddr_t offset, - extent_len_t length) - { - std::unique_ptr> ret = - std::make_unique>(); - auto &ret_ref = *ret; - std::unique_ptr pin_list = - std::make_unique(); - auto &pin_list_ref = *pin_list; - return lba_manager->get_mapping( + extent_len_t length) { + return get_pins( t, offset, length - ).safe_then([this, &t, &pin_list_ref, &ret_ref](auto pins) { - crimson::get_logger(ceph_subsys_filestore).debug( - "read_extents: mappings {}", - pins); - pins.swap(pin_list_ref); - return crimson::do_for_each( - pin_list_ref.begin(), - pin_list_ref.end(), - [this, &t, &ret_ref](auto &pin) { - crimson::get_logger(ceph_subsys_filestore).debug( - "read_extents: get_extent {}~{}", - pin->get_paddr(), - pin->get_length()); - return cache->get_extent( - t, - pin->get_paddr(), - pin->get_length() - ).safe_then([this, &pin, &ret_ref](auto ref) mutable - -> read_extent_ertr::future<> { - if (!ref->has_pin()) { - if (pin->has_been_invalidated() || ref->has_been_invalidated()) { - return crimson::ct_error::eagain::make(); - } else { - ref->set_pin(std::move(pin)); - lba_manager->add_pin(ref->get_pin()); - } - } - ret_ref.push_back(std::make_pair(ref->get_laddr(), ref)); - crimson::get_logger(ceph_subsys_filestore).debug( - "read_extents: got extent {}", - *ref); - return read_extent_ertr::now(); - }); - }); - }).safe_then([ret=std::move(ret), pin_list=std::move(pin_list)]() mutable { - return read_extent_ret( - read_extent_ertr::ready_future_marker{}, - std::move(*ret)); + ).safe_then([this, &t, offset, length](auto pins) { + if (pins.size() != 1) { + auto &logger = crimson::get_logger(ceph_subsys_filestore); + logger.error( + "TransactionManager::read_extent offset {} len {} got {} extents:", + offset, + length, + pins.size()); + for (auto &i: pins) { + logger.error("\t{}", *i); + } + ceph_assert(0 == "Should be impossible"); + } + return pin_to_extent(t, std::move(pins.front())); }); } diff --git a/src/crimson/tools/store-nbd.cc b/src/crimson/tools/store-nbd.cc index 3aa3d7968b9d2..38c4a49c495e0 100644 --- a/src/crimson/tools/store-nbd.cc +++ b/src/crimson/tools/store-nbd.cc @@ -556,6 +556,44 @@ public: ); } + auto read_extents( + Transaction &t, + laddr_t offset, + extent_len_t length) { + return seastar::do_with( + lba_pin_list_t(), + lextent_list_t(), + [this, &t, offset, length](auto &pins, auto &ret) { + return tm->get_pins( + t, offset, length + ).safe_then([this, &t, &pins, &ret](auto _pins) { + _pins.swap(pins); + logger().debug("read_extents: mappings {}", pins); + return crimson::do_for_each( + pins.begin(), + pins.end(), + [this, &t, &ret](auto &&pin) { + logger().debug( + "read_extents: get_extent {}~{}", + pin->get_paddr(), + pin->get_length()); + return tm->pin_to_extent( + t, + std::move(pin) + ).safe_then([this, &ret](auto ref) mutable { + ret.push_back(std::make_pair(ref->get_laddr(), ref)); + logger().debug( + "read_extents: got extent {}", + *ref); + return seastar::now(); + }); + }).safe_then([&ret] { + return std::move(ret); + }); + }); + }); + } + seastar::future read( off_t offset, size_t size) final { @@ -568,7 +606,7 @@ public: return seastar::do_with( tm->create_transaction(), [=, &blret](auto &t) { - return tm->read_extents(*t, offset, size + return read_extents(*t, offset, size ).safe_then([=, &blret](auto ext_list) mutable { size_t cur = offset; for (auto &i: ext_list) { diff --git a/src/test/crimson/seastore/test_transaction_manager.cc b/src/test/crimson/seastore/test_transaction_manager.cc index 745efae1ef697..93a8b222a3fd2 100644 --- a/src/test/crimson/seastore/test_transaction_manager.cc +++ b/src/test/crimson/seastore/test_transaction_manager.cc @@ -424,13 +424,9 @@ struct transaction_manager_test_t : ceph_assert(test_mappings.contains(addr, t.mapping_delta)); ceph_assert(test_mappings.get(addr, t.mapping_delta).desc.len == len); - auto ret_list = tm->read_extents( + auto ext = tm->read_extent( *t.t, addr, len ).unsafe_get0(); - EXPECT_EQ(ret_list.size(), 1); - auto &ext = ret_list.begin()->second; - auto &laddr = ret_list.begin()->first; - EXPECT_EQ(addr, laddr); EXPECT_EQ(addr, ext->get_laddr()); return ext; } -- 2.39.5