From 5f70785fac1f4e6bc38eea439415037fe46382e7 Mon Sep 17 00:00:00 2001 From: Samuel Just Date: Wed, 5 Aug 2020 21:30:34 -0700 Subject: [PATCH 1/5] test/crimson/seastore: add fake paddr for better test debugging Signed-off-by: Samuel Just --- src/crimson/os/seastore/seastore_types.cc | 2 ++ src/crimson/os/seastore/seastore_types.h | 7 +++++++ src/test/crimson/seastore/test_btree_lba_manager.cc | 13 +++++++++---- 3 files changed, 18 insertions(+), 4 deletions(-) diff --git a/src/crimson/os/seastore/seastore_types.cc b/src/crimson/os/seastore/seastore_types.cc index b2ce3a76696..40c54455fd0 100644 --- a/src/crimson/os/seastore/seastore_types.cc +++ b/src/crimson/os/seastore/seastore_types.cc @@ -13,6 +13,8 @@ std::ostream &segment_to_stream(std::ostream &out, const segment_id_t &t) return out << "BLOCK_REL_SEG"; else if (t == RECORD_REL_SEG_ID) return out << "RECORD_REL_SEG"; + else if (t == FAKE_SEG_ID) + return out << "FAKE_SEG"; else return out << t; } diff --git a/src/crimson/os/seastore/seastore_types.h b/src/crimson/os/seastore/seastore_types.h index 90b753695d6..70f41ab20a5 100644 --- a/src/crimson/os/seastore/seastore_types.h +++ b/src/crimson/os/seastore/seastore_types.h @@ -28,6 +28,10 @@ constexpr segment_id_t RECORD_REL_SEG_ID = constexpr segment_id_t BLOCK_REL_SEG_ID = std::numeric_limits::max() - 3; +// for tests which generate fake paddrs +constexpr segment_id_t FAKE_SEG_ID = + std::numeric_limits::max() - 4; + std::ostream &segment_to_stream(std::ostream &, const segment_id_t &t); // Offset within a segment on disk, see SegmentManager @@ -151,6 +155,9 @@ constexpr paddr_t make_record_relative_paddr(segment_off_t off) { constexpr paddr_t make_block_relative_paddr(segment_off_t off) { return paddr_t{BLOCK_REL_SEG_ID, off}; } +constexpr paddr_t make_fake_paddr(segment_off_t off) { + return paddr_t{FAKE_SEG_ID, off}; +} struct paddr_le_t { ceph_le32 segment = init_le32(NULL_SEG_ID); diff --git a/src/test/crimson/seastore/test_btree_lba_manager.cc b/src/test/crimson/seastore/test_btree_lba_manager.cc index 264606975b1..f7aa4584603 100644 --- a/src/test/crimson/seastore/test_btree_lba_manager.cc +++ b/src/test/crimson/seastore/test_btree_lba_manager.cc @@ -147,6 +147,12 @@ struct btree_lba_manager_test : ); } + segment_off_t next_off = 0; + paddr_t get_paddr() { + next_off += block_size; + return make_fake_paddr(next_off); + } + auto alloc_mapping( test_transaction_t &t, laddr_t hint, @@ -258,13 +264,12 @@ TEST_F(btree_lba_manager_test, basic) { run_async([this] { laddr_t laddr = 0x12345678 * block_size; - paddr_t paddr = { 1, static_cast(block_size * 10) }; { // write initial mapping auto t = create_transaction(); check_mappings(t); // check in progress transaction sees mapping check_mappings(); // check concurrent does not - auto ret = alloc_mapping(t, laddr, block_size, paddr); + auto ret = alloc_mapping(t, laddr, block_size, get_paddr()); submit_test_transaction(std::move(t)); } check_mappings(); // check new transaction post commit sees it @@ -278,7 +283,7 @@ TEST_F(btree_lba_manager_test, force_split) auto t = create_transaction(); logger().debug("opened transaction"); for (unsigned j = 0; j < 5; ++j) { - auto ret = alloc_mapping(t, 0, block_size, P_ADDR_MIN); + auto ret = alloc_mapping(t, 0, block_size, get_paddr()); if ((i % 10 == 0) && (j == 3)) { check_mappings(t); check_mappings(); @@ -298,7 +303,7 @@ TEST_F(btree_lba_manager_test, force_split_merge) auto t = create_transaction(); logger().debug("opened transaction"); for (unsigned j = 0; j < 5; ++j) { - auto ret = alloc_mapping(t, 0, block_size, P_ADDR_MIN); + auto ret = alloc_mapping(t, 0, block_size, get_paddr()); // just to speed things up a bit if ((i % 100 == 0) && (j == 3)) { check_mappings(t); From 4943d1464404d8b16ea41530cea6a8f23b14f499 Mon Sep 17 00:00:00 2001 From: Samuel Just Date: Wed, 5 Aug 2020 17:00:07 -0700 Subject: [PATCH 2/5] crimson/os/seastore/cached_extent: add print_detail for logical extents This way we can always see laddr in debug output. Signed-off-by: Samuel Just --- src/crimson/os/seastore/cached_extent.cc | 11 +++++++++++ src/crimson/os/seastore/cached_extent.h | 5 +++++ 2 files changed, 16 insertions(+) diff --git a/src/crimson/os/seastore/cached_extent.cc b/src/crimson/os/seastore/cached_extent.cc index cbe730871e6..7019b9fb802 100644 --- a/src/crimson/os/seastore/cached_extent.cc +++ b/src/crimson/os/seastore/cached_extent.cc @@ -65,6 +65,17 @@ CachedExtent::~CachedExtent() } } +std::ostream &LogicalCachedExtent::print_detail(std::ostream &out) const +{ + out << ", laddr=" << laddr; + if (pin) { + out << ", pin=" << *pin; + } else { + out << ", pin=empty"; + } + return print_detail_l(out); +} + std::ostream &operator<<(std::ostream &out, const LBAPin &rhs) { return out << "LBAPin(" << rhs.get_laddr() << "~" << rhs.get_length() diff --git a/src/crimson/os/seastore/cached_extent.h b/src/crimson/os/seastore/cached_extent.h index 6de468ffb94..6ae87f83cbb 100644 --- a/src/crimson/os/seastore/cached_extent.h +++ b/src/crimson/os/seastore/cached_extent.h @@ -566,8 +566,13 @@ public: bool is_logical() const final { return true; } + + std::ostream &print_detail(std::ostream &out) const final; protected: virtual void apply_delta(const ceph::bufferlist &bl) = 0; + virtual std::ostream &print_detail_l(std::ostream &out) const { + return out; + } private: laddr_t laddr = L_ADDR_NULL; From 8cd2dfaa772743715787e3ad1b7185b9a2ca641c Mon Sep 17 00:00:00 2001 From: Samuel Just Date: Wed, 5 Aug 2020 17:03:31 -0700 Subject: [PATCH 3/5] crimson/os/seastore/cache: don't mark invalid extents clean In the event that an extent is created and removed in the same transaction (invalid extent in fresh_block_list), update block specific metadata but don't add to cache and especially do not mark clean. LBAManager::complete_commit implementations are meant to use skip based on state in the same way. Signed-off-by: Samuel Just --- src/crimson/os/seastore/cache.cc | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/crimson/os/seastore/cache.cc b/src/crimson/os/seastore/cache.cc index d77ffde8864..2e7855651ab 100644 --- a/src/crimson/os/seastore/cache.cc +++ b/src/crimson/os/seastore/cache.cc @@ -187,10 +187,16 @@ void Cache::complete_commit( for (auto &i: t.fresh_block_list) { i->set_paddr(cur); cur.offset += i->get_length(); - i->state = CachedExtent::extent_state_t::CLEAN; i->last_committed_crc = i->get_crc32c(); - logger().debug("complete_commit: fresh {}", *i); i->on_initial_write(); + + if (!i->is_valid()) { + logger().debug("complete_commit: invalid {}", *i); + continue; + } + + i->state = CachedExtent::extent_state_t::CLEAN; + logger().debug("complete_commit: fresh {}", *i); add_extent(i); } From 3af6617673ce539a933afbf3c04b70b841bcced1 Mon Sep 17 00:00:00 2001 From: Samuel Just Date: Wed, 5 Aug 2020 21:35:48 -0700 Subject: [PATCH 4/5] crimson/os/seastore/lba_manager: clarify ref count operation return Previously, we returned a refcount from inc_ref and dec_ref. Now, return the paddr as well for future code accounting for released extents. In addition, replumb btree_lba_manager to return an enoent error if the mapping does not exist, and the resulting refcount, paddr otherwise with a refcount of 0 indicating that the mapping has been removed. Signed-off-by: Samuel Just --- src/crimson/os/seastore/lba_manager.h | 7 ++++++- .../seastore/lba_manager/btree/btree_lba_manager.cc | 11 ++--------- .../seastore/lba_manager/btree/btree_lba_manager.h | 2 +- .../os/seastore/lba_manager/btree/lba_btree_node.h | 4 ++-- .../lba_manager/btree/lba_btree_node_impl.cc | 9 +++------ src/crimson/os/seastore/transaction_manager.cc | 13 +++++++++---- src/crimson/os/seastore/transaction_manager.h | 3 ++- src/test/crimson/seastore/test_btree_lba_manager.cc | 4 ++-- 8 files changed, 27 insertions(+), 26 deletions(-) diff --git a/src/crimson/os/seastore/lba_manager.h b/src/crimson/os/seastore/lba_manager.h index a44f48fe96e..d9c68ef0c39 100644 --- a/src/crimson/os/seastore/lba_manager.h +++ b/src/crimson/os/seastore/lba_manager.h @@ -88,10 +88,15 @@ public: Transaction &t, laddr_t off, extent_len_t len, paddr_t addr) = 0; + + struct ref_update_result_t { + unsigned refcount = 0; + paddr_t addr; + }; using ref_ertr = crimson::errorator< crimson::ct_error::enoent, crimson::ct_error::input_output_error>; - using ref_ret = ref_ertr::future; + using ref_ret = ref_ertr::future; /** * Decrements ref count on extent diff --git a/src/crimson/os/seastore/lba_manager/btree/btree_lba_manager.cc b/src/crimson/os/seastore/lba_manager/btree/btree_lba_manager.cc index 998ab256097..c90e41f058a 100644 --- a/src/crimson/os/seastore/lba_manager/btree/btree_lba_manager.cc +++ b/src/crimson/os/seastore/lba_manager/btree/btree_lba_manager.cc @@ -356,16 +356,9 @@ BtreeLBAManager::update_refcount_ret BtreeLBAManager::update_refcount( lba_map_val_t out = in; ceph_assert((int)out.refcount + delta >= 0); out.refcount += delta; - if (out.refcount == 0) { - return std::optional(); - } else { - return std::optional(out); - } + return out; }).safe_then([](auto result) { - if (!result) - return 0u; - else - return result->refcount; + return ref_update_result_t{result.refcount, result.paddr}; }); } diff --git a/src/crimson/os/seastore/lba_manager/btree/btree_lba_manager.h b/src/crimson/os/seastore/lba_manager/btree/btree_lba_manager.h index 4a01d7fe3a7..720f3ae4e79 100644 --- a/src/crimson/os/seastore/lba_manager/btree/btree_lba_manager.h +++ b/src/crimson/os/seastore/lba_manager/btree/btree_lba_manager.h @@ -145,7 +145,7 @@ private: * Updates mapping, removes if f returns nullopt */ using update_mapping_ertr = ref_ertr; - using update_mapping_ret = ref_ertr::future>; + using update_mapping_ret = ref_ertr::future; using update_func_t = LBANode::mutate_func_t; update_mapping_ret update_mapping( Transaction &t, diff --git a/src/crimson/os/seastore/lba_manager/btree/lba_btree_node.h b/src/crimson/os/seastore/lba_manager/btree/lba_btree_node.h index 68c7d23a672..55860544d41 100644 --- a/src/crimson/os/seastore/lba_manager/btree/lba_btree_node.h +++ b/src/crimson/os/seastore/lba_manager/btree/lba_btree_node.h @@ -129,9 +129,9 @@ struct LBANode : CachedExtent { crimson::ct_error::input_output_error >; using mutate_mapping_ret = mutate_mapping_ertr::future< - std::optional>; + lba_map_val_t>; using mutate_func_t = std::function< - std::optional(const lba_map_val_t &v) + lba_map_val_t(const lba_map_val_t &v) >; virtual mutate_mapping_ret mutate_mapping( op_context_t c, diff --git a/src/crimson/os/seastore/lba_manager/btree/lba_btree_node_impl.cc b/src/crimson/os/seastore/lba_manager/btree/lba_btree_node_impl.cc index 9f138b0c1ce..b31dfcf762d 100644 --- a/src/crimson/os/seastore/lba_manager/btree/lba_btree_node_impl.cc +++ b/src/crimson/os/seastore/lba_manager/btree/lba_btree_node_impl.cc @@ -404,15 +404,12 @@ LBALeafNode::mutate_mapping_ret LBALeafNode::mutate_mapping( ceph_assert(!at_min_capacity()); auto mutation_pt = find(laddr); if (mutation_pt == end()) { - ceph_assert(0 == "should be impossible"); - return mutate_mapping_ret( - mutate_mapping_ertr::ready_future_marker{}, - std::nullopt); + return crimson::ct_error::enoent::make(); } auto mutated = f(mutation_pt.get_val()); - if (mutated) { - journal_update(mutation_pt, *mutated, maybe_get_delta_buffer()); + if (mutated.refcount > 0) { + journal_update(mutation_pt, mutated, maybe_get_delta_buffer()); return mutate_mapping_ret( mutate_mapping_ertr::ready_future_marker{}, mutated); diff --git a/src/crimson/os/seastore/transaction_manager.cc b/src/crimson/os/seastore/transaction_manager.cc index 65f9377088c..05859a3626a 100644 --- a/src/crimson/os/seastore/transaction_manager.cc +++ b/src/crimson/os/seastore/transaction_manager.cc @@ -95,8 +95,9 @@ TransactionManager::ref_ret TransactionManager::inc_ref( Transaction &t, LogicalCachedExtentRef &ref) { - return lba_manager.incref_extent(t, ref->get_laddr() - ).handle_error( + return lba_manager.incref_extent(t, ref->get_laddr()).safe_then([](auto r) { + return r.refcount; + }).handle_error( ref_ertr::pass_further{}, ct_error::all_same_way([](auto e) { ceph_assert(0 == "unhandled error, TODO"); @@ -107,7 +108,9 @@ TransactionManager::ref_ret TransactionManager::inc_ref( Transaction &t, laddr_t offset) { - return lba_manager.incref_extent(t, offset); + return lba_manager.incref_extent(t, offset).safe_then([](auto result) { + return result.refcount; + }); } TransactionManager::ref_ret TransactionManager::dec_ref( @@ -127,7 +130,9 @@ TransactionManager::ref_ret TransactionManager::dec_ref( laddr_t offset) { // TODO: need to retire the extent (only) if it's live, will need cache call - return lba_manager.decref_extent(t, offset); + return lba_manager.decref_extent(t, offset).safe_then([](auto ret) { + return ret.refcount; + }); } TransactionManager::submit_transaction_ertr::future<> diff --git a/src/crimson/os/seastore/transaction_manager.h b/src/crimson/os/seastore/transaction_manager.h index 011d3d3b384..c9c2eb0f594 100644 --- a/src/crimson/os/seastore/transaction_manager.h +++ b/src/crimson/os/seastore/transaction_manager.h @@ -143,8 +143,9 @@ public: return ret; } + using ref_ertr = LBAManager::ref_ertr; - using ref_ret = LBAManager::ref_ret; + using ref_ret = ref_ertr::future; /// Add refcount for ref ref_ret inc_ref( diff --git a/src/test/crimson/seastore/test_btree_lba_manager.cc b/src/test/crimson/seastore/test_btree_lba_manager.cc index f7aa4584603..4381ef55750 100644 --- a/src/test/crimson/seastore/test_btree_lba_manager.cc +++ b/src/test/crimson/seastore/test_btree_lba_manager.cc @@ -214,7 +214,7 @@ struct btree_lba_manager_test : auto refcnt = lba_manager->decref_extent( *t.t, - target->first).unsafe_get0(); + target->first).unsafe_get0().refcount; EXPECT_EQ(refcnt, target->second.refcount); if (target->second.refcount == 0) { t.mappings.erase(target); @@ -228,7 +228,7 @@ struct btree_lba_manager_test : target->second.refcount++; auto refcnt = lba_manager->incref_extent( *t.t, - target->first).unsafe_get0(); + target->first).unsafe_get0().refcount; EXPECT_EQ(refcnt, target->second.refcount); } From bb3a7f35152f6f21e5df53349112ee806246f26a Mon Sep 17 00:00:00 2001 From: Samuel Just Date: Tue, 4 Aug 2020 19:50:45 -0700 Subject: [PATCH 5/5] crimson/os/seastore/transaction_manager: complete dec_ref Previously, dec_ref didn't handle actually retiring the extent from the cache. dec_ref will now reach into the cache and mark the extent retired if it exists either in the cache or in the current transaction. Signed-off-by: Samuel Just --- src/crimson/os/seastore/cache.cc | 18 +++++++++++ src/crimson/os/seastore/cache.h | 7 ++++ .../os/seastore/transaction_manager.cc | 29 +++++++++++------ .../seastore/test_transaction_manager.cc | 32 +++++++++++++++++++ 4 files changed, 77 insertions(+), 9 deletions(-) diff --git a/src/crimson/os/seastore/cache.cc b/src/crimson/os/seastore/cache.cc index 2e7855651ab..7b130c71216 100644 --- a/src/crimson/os/seastore/cache.cc +++ b/src/crimson/os/seastore/cache.cc @@ -28,6 +28,24 @@ Cache::~Cache() ceph_assert(extents.empty()); } +Cache::retire_extent_ret Cache::retire_extent_if_cached( + Transaction &t, paddr_t addr) +{ + if (auto ext = t.write_set.find_offset(addr); ext != t.write_set.end()) { + t.add_to_retired_set(CachedExtentRef(&*ext)); + return retire_extent_ertr::now(); + } else if (auto iter = extents.find_offset(addr); + iter != extents.end()) { + auto ret = CachedExtentRef(&*iter); + return ret->wait_io().then([&t, ret=std::move(ret)]() mutable { + t.add_to_retired_set(ret); + return retire_extent_ertr::now(); + }); + } else { + return retire_extent_ertr::now(); + } +} + void Cache::add_extent(CachedExtentRef ref) { assert(ref->is_valid()); diff --git a/src/crimson/os/seastore/cache.h b/src/crimson/os/seastore/cache.h index d8859b3e7fb..a31e044e1d6 100644 --- a/src/crimson/os/seastore/cache.h +++ b/src/crimson/os/seastore/cache.h @@ -90,6 +90,13 @@ public: t.add_to_retired_set(ref); } + /// Declare paddr retired in t, noop if not cached + using retire_extent_ertr = crimson::errorator< + crimson::ct_error::input_output_error>; + using retire_extent_ret = retire_extent_ertr::future<>; + retire_extent_ret retire_extent_if_cached( + Transaction &t, paddr_t addr); + /** * get_root * diff --git a/src/crimson/os/seastore/transaction_manager.cc b/src/crimson/os/seastore/transaction_manager.cc index 05859a3626a..82a4d3de327 100644 --- a/src/crimson/os/seastore/transaction_manager.cc +++ b/src/crimson/os/seastore/transaction_manager.cc @@ -117,21 +117,32 @@ TransactionManager::ref_ret TransactionManager::dec_ref( Transaction &t, LogicalCachedExtentRef &ref) { - return dec_ref(t, ref->get_laddr() - ).handle_error( - ref_ertr::pass_further{}, - ct_error::all_same_way([](auto e) { - ceph_assert(0 == "unhandled error, TODO"); - })); + return lba_manager.decref_extent(t, ref->get_laddr() + ).safe_then([this, &t, ref](auto ret) { + if (ret.refcount == 0) { + cache.retire_extent(t, ref); + } + return ret.refcount; + }); } TransactionManager::ref_ret TransactionManager::dec_ref( Transaction &t, laddr_t offset) { - // TODO: need to retire the extent (only) if it's live, will need cache call - return lba_manager.decref_extent(t, offset).safe_then([](auto ret) { - return ret.refcount; + return lba_manager.decref_extent(t, offset + ).safe_then([this, &t](auto result) -> ref_ret { + if (result.refcount == 0) { + return cache.retire_extent_if_cached(t, result.addr).safe_then([] { + return ref_ret( + ref_ertr::ready_future_marker{}, + 0); + }); + } else { + return ref_ret( + ref_ertr::ready_future_marker{}, + result.refcount); + } }); } diff --git a/src/test/crimson/seastore/test_transaction_manager.cc b/src/test/crimson/seastore/test_transaction_manager.cc index d086c4ca348..076d7ad2dce 100644 --- a/src/test/crimson/seastore/test_transaction_manager.cc +++ b/src/test/crimson/seastore/test_transaction_manager.cc @@ -278,6 +278,38 @@ TEST_F(transaction_manager_test_t, mutate) }); } +TEST_F(transaction_manager_test_t, create_remove_same_transaction) +{ + constexpr laddr_t SIZE = 4096; + run_async([this] { + constexpr laddr_t ADDR = 0xFF * SIZE; + { + auto t = create_transaction(); + auto extent = alloc_extent( + t, + ADDR, + SIZE, + 'a'); + ASSERT_EQ(ADDR, extent->get_laddr()); + check_mappings(t); + dec_ref(t, ADDR); + check_mappings(t); + + extent = alloc_extent( + t, + ADDR, + SIZE, + 'a'); + + submit_transaction(std::move(t)); + check_mappings(); + } + replay(); + check_mappings(); + }); +} + + TEST_F(transaction_manager_test_t, inc_dec_ref) { constexpr laddr_t SIZE = 4096;