crimson/os/seastore: drop unused inc/decref_extent and dec_ref with tests

cyx1231st · cyx1231st · commit 6ac0621ebf94 · 2025-03-21T11:19:19.000+08:00
Signed-off-by: Yingxin Cheng &lt;yingxin.cheng@intel.com&gt;
diff --git a/src/crimson/os/seastore/lba_manager.h b/src/crimson/os/seastore/lba_manager.h
@@ -120,20 +120,11 @@ class LBAManager {
   using ref_ret = ref_iertr::future<ref_update_result_t>;
 
   /**
-   * 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
@@ -430,7 +430,7 @@ class BtreeLBAManager : public LBAManager {
     });
   }
 
-  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 @@ class BtreeLBAManager : public LBAManager {
     });
   }
 
-  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
@@ -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
@@ -339,18 +339,6 @@ class TransactionManager : public ExtentCallbackInterface {
   using ref_iertr = LBAManager::ref_iertr;
   using ref_ret = ref_iertr::future<extent_ref_count_t>;
 
-#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
@@ -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<laddr_t> get_mapped_addresses() {
     std::vector<laddr_t> 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
@@ -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));
