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 <sjust@redhat.com>

diff --git a/src/crimson/os/seastore/lba_manager.h b/src/crimson/os/seastore/lba_manager.h
index a44f48fe96e2e4cf9726383a2f4072b2e6a4427e..d9c68ef0c392d6672a7f3748fcc261f46eff9c1d 100644 (file)
--- 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<unsigned>;
+  using ref_ret = ref_ertr::future<ref_update_result_t>;
 
   /**
    * 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 998ab256097f7ddacee79d0d148ef5534e104d3c..c90e41f058ab5c247af8bc9c72bd1c143bcff8bb 100644 (file)
--- 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<lba_map_val_t>();
-      } else {
-       return std::optional<lba_map_val_t>(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 4a01d7fe3a79daa6f0ed232a06f59ae9020d2e87..720f3ae4e79ec5a44e341154f6f5170bfbefe250 100644 (file)
--- 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<std::optional<lba_map_val_t>>;
+  using update_mapping_ret = ref_ertr::future<lba_map_val_t>;
   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 68c7d23a672440506bfdcf65cf11bb1b6706081a..55860544d41968f9ac6fc29459eaa1c0439bc7b9 100644 (file)
--- 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>>;
+    lba_map_val_t>;
   using mutate_func_t = std::function<
-    std::optional<lba_map_val_t>(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 9f138b0c1ce074bdfbf661ae621cbfddcfdcc5f8..b31dfcf762dc3816d99aeec1d0b219327b5ffc66 100644 (file)
--- 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 65f9377088ce74c28bb431d5f634574764c477ad..05859a3626a3093029b5683c6a319b77f6cda116 100644 (file)
--- 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 011d3d3b3842b8c3d77e4910165347c99a237268..c9c2eb0f594c1870560de9c99e180f35b76ee8f8 100644 (file)
--- 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<unsigned>;
 
   /// 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 f7aa4584603f50666d7308b8bd1a4ba98c5c9d28..4381ef557504b8bdc6ca267426cfdb6ba4ba11e4 100644 (file)
--- 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);
   }