From 3af6617673ce539a933afbf3c04b70b841bcced1 Mon Sep 17 00:00:00 2001 From: Samuel Just Date: Wed, 5 Aug 2020 21:35:48 -0700 Subject: [PATCH] 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 a44f48fe96e2e..d9c68ef0c392d 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 998ab256097f7..c90e41f058ab5 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 4a01d7fe3a79d..720f3ae4e79ec 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 68c7d23a67244..55860544d4196 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 9f138b0c1ce07..b31dfcf762dc3 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 65f9377088ce7..05859a3626a30 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 011d3d3b3842b..c9c2eb0f594c1 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 f7aa4584603f5..4381ef557504b 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); } -- 2.39.5