From 6ac0621ebf9472930992650a7de2ab93da1dab52 Mon Sep 17 00:00:00 2001 From: Yingxin Cheng Date: Tue, 18 Mar 2025 12:52:18 +0800 Subject: [PATCH] crimson/os/seastore: drop unused inc/decref_extent and dec_ref with tests Signed-off-by: Yingxin Cheng --- src/crimson/os/seastore/lba_manager.h | 13 +--- .../lba_manager/btree/btree_lba_manager.h | 11 +--- .../os/seastore/transaction_manager.cc | 36 +--------- src/crimson/os/seastore/transaction_manager.h | 12 ---- .../seastore/test_btree_lba_manager.cc | 31 +-------- .../seastore/test_transaction_manager.cc | 66 ++----------------- 6 files changed, 10 insertions(+), 159 deletions(-) diff --git a/src/crimson/os/seastore/lba_manager.h b/src/crimson/os/seastore/lba_manager.h index 2d347e504c9..26a59f82e92 100644 --- a/src/crimson/os/seastore/lba_manager.h +++ b/src/crimson/os/seastore/lba_manager.h @@ -120,20 +120,11 @@ public: using ref_ret = ref_iertr::future; /** - * Decrements ref count on extent + * Removes a mapping and deal with indirection * * @return returns resulting refcount */ - virtual ref_ret decref_extent( - Transaction &t, - laddr_t addr) = 0; - - /** - * Increments ref count on extent - * - * @return returns resulting refcount - */ - virtual ref_ret incref_extent( + virtual ref_ret remove_mapping( Transaction &t, laddr_t addr) = 0; 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 50bf2ca5e0c..cdc62a5de33 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 @@ -430,7 +430,7 @@ public: }); } - ref_ret decref_extent( + ref_ret remove_mapping( Transaction &t, laddr_t addr) final { return update_refcount(t, addr, -1, true @@ -439,15 +439,6 @@ public: }); } - ref_ret incref_extent( - Transaction &t, - laddr_t addr) final { - return update_refcount(t, addr, 1, false - ).si_then([](auto res) { - return std::move(res.ref_update_res); - }); - } - remap_ret remap_mappings( Transaction &t, LBAMappingRef orig_mapping, diff --git a/src/crimson/os/seastore/transaction_manager.cc b/src/crimson/os/seastore/transaction_manager.cc index e8f4801b42e..45c5610658b 100644 --- a/src/crimson/os/seastore/transaction_manager.cc +++ b/src/crimson/os/seastore/transaction_manager.cc @@ -198,45 +198,13 @@ TransactionManager::close() { }); } -#ifdef UNIT_TESTS_BUILT -TransactionManager::ref_ret TransactionManager::inc_ref( - Transaction &t, - LogicalChildNodeRef &ref) -{ - LOG_PREFIX(TransactionManager::inc_ref); - TRACET("{}", t, *ref); - return lba_manager->incref_extent(t, ref->get_laddr() - ).si_then([FNAME, ref, &t](auto result) { - DEBUGT("extent refcount is incremented to {} -- {}", - t, result.refcount, *ref); - return result.refcount; - }).handle_error_interruptible( - ref_iertr::pass_further{}, - ct_error::assert_all{"unhandled error, TODO"}); -} - -TransactionManager::ref_ret TransactionManager::inc_ref( - Transaction &t, - laddr_t offset) -{ - LOG_PREFIX(TransactionManager::inc_ref); - TRACET("{}", t, offset); - return lba_manager->incref_extent(t, offset - ).si_then([FNAME, offset, &t](auto result) { - DEBUGT("extent refcount is incremented to {} -- {}~0x{:x}, {}", - t, result.refcount, offset, result.length, result.addr); - return result.refcount; - }); -} -#endif - TransactionManager::ref_ret TransactionManager::remove( Transaction &t, LogicalChildNodeRef &ref) { LOG_PREFIX(TransactionManager::remove); DEBUGT("{} ...", t, *ref); - return lba_manager->decref_extent(t, ref->get_laddr() + return lba_manager->remove_mapping(t, ref->get_laddr() ).si_then([this, FNAME, &t, ref](auto result) { if (result.refcount == 0) { cache->retire_extent(t, ref); @@ -253,7 +221,7 @@ TransactionManager::ref_ret TransactionManager::remove( { LOG_PREFIX(TransactionManager::remove); DEBUGT("{} ...", t, offset); - return lba_manager->decref_extent(t, offset + return lba_manager->remove_mapping(t, offset ).si_then([this, FNAME, offset, &t](auto result) -> ref_ret { auto fut = ref_iertr::now(); if (result.refcount == 0) { diff --git a/src/crimson/os/seastore/transaction_manager.h b/src/crimson/os/seastore/transaction_manager.h index fc364b9a2fb..fc7137dfe51 100644 --- a/src/crimson/os/seastore/transaction_manager.h +++ b/src/crimson/os/seastore/transaction_manager.h @@ -339,18 +339,6 @@ public: using ref_iertr = LBAManager::ref_iertr; using ref_ret = ref_iertr::future; -#ifdef UNIT_TESTS_BUILT - /// Add refcount for ref - ref_ret inc_ref( - Transaction &t, - LogicalChildNodeRef &ref); - - /// Add refcount for offset - ref_ret inc_ref( - Transaction &t, - laddr_t offset); -#endif - /** * remove * diff --git a/src/test/crimson/seastore/test_btree_lba_manager.cc b/src/test/crimson/seastore/test_btree_lba_manager.cc index 2ec8b7b0162..68293262e4e 100644 --- a/src/test/crimson/seastore/test_btree_lba_manager.cc +++ b/src/test/crimson/seastore/test_btree_lba_manager.cc @@ -594,7 +594,7 @@ struct btree_lba_manager_test : btree_test_base { (void) with_trans_intr( *t.t, [=, this](auto &t) { - return lba_manager->decref_extent( + return lba_manager->remove_mapping( t, target->first ).si_then([this, &t, target](auto result) { @@ -611,27 +611,6 @@ struct btree_lba_manager_test : btree_test_base { } } - auto incref_mapping( - test_transaction_t &t, - laddr_t addr) { - return incref_mapping(t, t.mappings.find(addr)); - } - - void incref_mapping( - test_transaction_t &t, - test_lba_mapping_t::iterator target) { - ceph_assert(target->second.refcount > 0); - target->second.refcount++; - auto refcnt = with_trans_intr( - *t.t, - [=, this](auto &t) { - return lba_manager->incref_extent( - t, - target->first); - }).unsafe_get().refcount; - EXPECT_EQ(refcnt, target->second.refcount); - } - std::vector get_mapped_addresses() { std::vector addresses; addresses.reserve(test_lba_mappings.size()); @@ -754,10 +733,6 @@ TEST_F(btree_lba_manager_test, force_split_merge) check_mappings(t); check_mappings(); } - for (auto &ret : rets) { - incref_mapping(t, ret->get_key()); - decref_mapping(t, ret->get_key()); - } } logger().debug("submitting transaction"); submit_test_transaction(std::move(t)); @@ -770,8 +745,6 @@ TEST_F(btree_lba_manager_test, force_split_merge) auto t = create_transaction(); for (unsigned i = 0; i != addresses.size(); ++i) { if (i % 2 == 0) { - incref_mapping(t, addresses[i]); - decref_mapping(t, addresses[i]); decref_mapping(t, addresses[i]); } logger().debug("submitting transaction"); @@ -790,8 +763,6 @@ TEST_F(btree_lba_manager_test, force_split_merge) auto addresses = get_mapped_addresses(); auto t = create_transaction(); for (unsigned i = 0; i != addresses.size(); ++i) { - incref_mapping(t, addresses[i]); - decref_mapping(t, addresses[i]); decref_mapping(t, addresses[i]); } check_mappings(t); diff --git a/src/test/crimson/seastore/test_transaction_manager.cc b/src/test/crimson/seastore/test_transaction_manager.cc index 6e0fe65c345..09cec738900 100644 --- a/src/test/crimson/seastore/test_transaction_manager.cc +++ b/src/test/crimson/seastore/test_transaction_manager.cc @@ -687,18 +687,7 @@ struct transaction_manager_test_t : return pin; } - void inc_ref(test_transaction_t &t, laddr_t offset) { - ceph_assert(test_mappings.contains(offset, t.mapping_delta)); - ceph_assert(test_mappings.get(offset, t.mapping_delta).refcount > 0); - - auto refcnt = with_trans_intr(*(t.t), [&](auto& trans) { - return tm->inc_ref(trans, offset); - }).unsafe_get(); - auto check_refcnt = test_mappings.inc_ref(offset, t.mapping_delta); - EXPECT_EQ(refcnt, check_refcnt); - } - - void dec_ref(test_transaction_t &t, laddr_t offset) { + void remove(test_transaction_t &t, laddr_t offset) { ceph_assert(test_mappings.contains(offset, t.mapping_delta)); ceph_assert(test_mappings.get(offset, t.mapping_delta).refcount > 0); @@ -1964,7 +1953,7 @@ TEST_P(tm_single_device_test_t, create_remove_same_transaction) 'a'); ASSERT_EQ(ADDR, extent->get_laddr()); check_mappings(t); - dec_ref(t, ADDR); + remove(t, ADDR); check_mappings(t); extent = alloc_extent( @@ -2000,7 +1989,7 @@ TEST_P(tm_single_device_test_t, split_merge_read_same_transaction) { auto t = create_transaction(); for (unsigned i = 0; i < 240; ++i) { - dec_ref( + remove( t, get_laddr_hint(i * SIZE)); } @@ -2011,53 +2000,6 @@ TEST_P(tm_single_device_test_t, split_merge_read_same_transaction) }); } -TEST_P(tm_single_device_test_t, inc_dec_ref) -{ - constexpr size_t SIZE = 4096; - run_async([this] { - laddr_t ADDR = get_laddr_hint(0xFF * SIZE); - { - auto t = create_transaction(); - auto extent = alloc_extent( - t, - ADDR, - SIZE, - 'a'); - ASSERT_EQ(ADDR, extent->get_laddr()); - check_mappings(t); - check(); - submit_transaction(std::move(t)); - check(); - } - replay(); - { - auto t = create_transaction(); - inc_ref(t, ADDR); - check_mappings(t); - check(); - submit_transaction(std::move(t)); - check(); - } - { - auto t = create_transaction(); - dec_ref(t, ADDR); - check_mappings(t); - check(); - submit_transaction(std::move(t)); - check(); - } - replay(); - { - auto t = create_transaction(); - dec_ref(t, ADDR); - check_mappings(t); - check(); - submit_transaction(std::move(t)); - check(); - } - }); -} - TEST_P(tm_single_device_test_t, cause_lba_split) { constexpr size_t SIZE = 4096; @@ -2108,7 +2050,7 @@ TEST_P(tm_single_device_test_t, random_writes) get_laddr_hint(TOTAL + (k * PADDING_SIZE)), PADDING_SIZE); for (auto &padding : paddings) { - dec_ref(t, padding->get_laddr()); + remove(t, padding->get_laddr()); } } submit_transaction(std::move(t)); -- 2.39.5