Merge pull request ceph#54036 from xxhdx1985126/wip-seastore-clone-remap-ut

crimson/os/seastore: add unittests for transaction manager's clone/remap/read_pin and fix bugs

Reviewed-by: Yingxin Cheng <[email protected]>
Reviewed-by: Myoungwon Oh <[email protected]>

Author: cyx1231st

src/crimson/os/seastore/btree/btree_range_pin.cc

@@ -22,6 +22,16 @@ BtreeNodeMapping<key_t, val_t>::get_logical_extent(
   return v;
 }
 
+template <typename key_t, typename val_t>
+bool BtreeNodeMapping<key_t, val_t>::is_stable() const
+{
+  assert(parent);
+  assert(parent->is_valid());
+  assert(pos != std::numeric_limits<uint16_t>::max());
+  auto &p = (FixedKVNode<key_t>&)*parent;
+  return p.is_child_stable(pos);
+}
+
 template class BtreeNodeMapping<laddr_t, paddr_t>;
 template class BtreeNodeMapping<paddr_t, laddr_t>;
 } // namespace crimson::os::seastore

src/crimson/os/seastore/btree/btree_range_pin.h

@@ -153,10 +153,6 @@ class BtreeNodeMapping : public PhysicalNodeMapping<key_t, val_t> {
     return parent;
   }
 
-  void set_parent(CachedExtentRef ext) {
-    parent = ext;
-  }
-
   uint16_t get_pos() const final {
     return pos;
   }
@@ -199,6 +195,7 @@ class BtreeNodeMapping : public PhysicalNodeMapping<key_t, val_t> {
   }
 
   get_child_ret_t<LogicalCachedExtent> get_logical_extent(Transaction&) final;
+  bool is_stable() const final;
 };
 
 }

src/crimson/os/seastore/btree/fixed_kv_btree.h

@@ -505,7 +505,9 @@ class FixedKVBtree {
           i->get_val().maybe_relative_to(node->get_paddr()),
           &child_node);
       } else {
-        assert(i->get_val().pladdr.is_paddr());
+        if (i->get_val().pladdr.is_laddr()) {
+          continue;
+        }
         ret = c.trans.get_extent(
           i->get_val().pladdr.get_paddr().maybe_relative_to(node->get_paddr()),
           &child_node);

src/crimson/os/seastore/btree/fixed_kv_node.h

@@ -157,7 +157,7 @@ struct FixedKVNode : ChildableCachedExtent {
       (get_node_size() - offset - 1) * sizeof(ChildableCachedExtent*));
   }
 
-  FixedKVNode& get_stable_for_key(node_key_t key) {
+  FixedKVNode& get_stable_for_key(node_key_t key) const {
     ceph_assert(is_pending());
     if (is_mutation_pending()) {
       return (FixedKVNode&)*get_prior_instance();
@@ -229,6 +229,8 @@ struct FixedKVNode : ChildableCachedExtent {
   virtual get_child_ret_t<LogicalCachedExtent>
   get_logical_child(op_context_t<node_key_t> c, uint16_t pos) = 0;
 
+  virtual bool is_child_stable(uint16_t pos) const = 0;
+
   template <typename T, typename iter_t>
   get_child_ret_t<T> get_child(op_context_t<node_key_t> c, iter_t iter) {
     auto pos = iter.get_offset();
@@ -592,6 +594,11 @@ struct FixedKVInternalNode
     return get_child_ret_t<LogicalCachedExtent>(child_pos_t(nullptr, 0));
   }
 
+  bool is_child_stable(uint16_t pos) const final {
+    ceph_abort("impossible");
+    return false;
+  }
+
   bool validate_stable_children() final {
     LOG_PREFIX(FixedKVInternalNode::validate_stable_children);
     if (this->children.empty()) {
@@ -984,6 +991,35 @@ struct FixedKVLeafNode
     }
   }
 
+  // children are considered stable if any of the following case is true:
+  // 1. Not in memory
+  // 2. being stable
+  // 3. being mutation pending and under-io
+  bool is_child_stable(uint16_t pos) const final {
+    auto child = this->children[pos];
+    if (is_valid_child_ptr(child)) {
+      ceph_assert(child->is_logical());
+      return child->is_stable() ||
+	(child->is_mutation_pending() &&
+	 child->is_pending_io());
+    } else if (this->is_pending()) {
+      auto key = this->iter_idx(pos).get_key();
+      auto &sparent = this->get_stable_for_key(key);
+      auto spos = sparent.child_pos_for_key(key);
+      auto child = sparent.children[spos];
+      if (is_valid_child_ptr(child)) {
+	ceph_assert(child->is_logical());
+	return child->is_stable() ||
+	  (child->is_mutation_pending() &&
+	   child->is_pending_io());
+      } else {
+	return true;
+      }
+    } else {
+      return true;
+    }
+  }
+
   bool validate_stable_children() override {
     return true;
   }

src/crimson/os/seastore/cached_extent.cc

@@ -158,8 +158,15 @@ parent_tracker_t::~parent_tracker_t() {
 
 std::ostream &operator<<(std::ostream &out, const LBAMapping &rhs)
 {
-  return out << "LBAMapping(" << rhs.get_key() << "~" << rhs.get_length()
-	     << "->" << rhs.get_val();
+  out << "LBAMapping(" << rhs.get_key() << "~" << rhs.get_length()
+      << "->" << rhs.get_val();
+  if (rhs.is_indirect()) {
+    out << " indirect(" << rhs.get_intermediate_base() << "~"
+	<< rhs.get_intermediate_key() << "~"
+	<< rhs.get_intermediate_length() << ")";
+  }
+  out << ")";
+  return out;
 }
 
 std::ostream &operator<<(std::ostream &out, const lba_pin_list_t &rhs)

src/crimson/os/seastore/cached_extent.h

@@ -595,7 +595,7 @@ class CachedExtent
 
   // a rewrite extent has an invalid prior_instance,
   // and a mutation_pending extent has a valid prior_instance
-  CachedExtentRef get_prior_instance() {
+  CachedExtentRef get_prior_instance() const {
     return prior_instance;
   }
 
@@ -1046,6 +1046,8 @@ class PhysicalNodeMapping {
     child_pos->link_child(c);
   }
 
+  virtual bool is_stable() const = 0;
+
   virtual ~PhysicalNodeMapping() {}
 protected:
   std::optional<child_pos_t> child_pos = std::nullopt;

src/crimson/os/seastore/collection_manager/flat_collection_manager.cc

@@ -84,7 +84,7 @@ FlatCollectionManager::create(coll_root_t &coll_root, Transaction &t,
 	    get_coll_context(t), cid, info.split_bits
 	  ).si_then([=, this, &t](auto result) {
 	    assert(result == CollectionNode::create_result_t::SUCCESS);
-	    return tm.dec_ref(t, extent->get_laddr());
+	    return tm.remove(t, extent->get_laddr());
 	  }).si_then([] (auto) {
             return create_iertr::make_ready_future<>();
           });

src/crimson/os/seastore/lba_manager.h

@@ -89,7 +89,7 @@ class LBAManager {
     paddr_t addr,
     LogicalCachedExtent &nextent) = 0;
 
-  virtual alloc_extent_ret clone_extent(
+  virtual alloc_extent_ret clone_mapping(
     Transaction &t,
     laddr_t hint,
     extent_len_t len,

src/crimson/os/seastore/lba_manager/btree/btree_lba_manager.cc

@@ -197,7 +197,7 @@ BtreeLBAManager::_get_original_mappings(
 	    pin->get_key(), pin->get_length(),
 	    pin->get_raw_val().get_laddr());
 	  auto &btree_new_pin = static_cast<BtreeLBAMapping&>(*new_pin);
-	  btree_new_pin.set_key_for_indirect(
+	  btree_new_pin.make_indirect(
 	    pin->get_key(),
 	    pin->get_length(),
 	    pin->get_raw_val().get_laddr());
@@ -287,7 +287,7 @@ BtreeLBAManager::_get_mapping(
 	      c.trans, pin->get_raw_val().get_laddr()
 	    ).si_then([&pin](auto new_pin) {
 	      ceph_assert(pin->get_length() == new_pin->get_length());
-	      new_pin->set_key_for_indirect(
+	      new_pin->make_indirect(
 		pin->get_key(),
 		pin->get_length());
 	      return new_pin;
@@ -307,7 +307,6 @@ BtreeLBAManager::_alloc_extent(
   extent_len_t len,
   pladdr_t addr,
   paddr_t actual_addr,
-  laddr_t intermediate_base,
   LogicalCachedExtent* nextent)
 {
   struct state_t {
@@ -321,6 +320,8 @@ BtreeLBAManager::_alloc_extent(
 
   LOG_PREFIX(BtreeLBAManager::_alloc_extent);
   TRACET("{}~{}, hint={}", t, addr, len, hint);
+
+  ceph_assert(actual_addr != P_ADDR_NULL ? addr.is_laddr() : addr.is_paddr());
   auto c = get_context(t);
   ++stats.num_alloc_extents;
   auto lookup_attempts = stats.num_alloc_extents_iter_nexts;
@@ -384,17 +385,9 @@ BtreeLBAManager::_alloc_extent(
 	    state.ret = iter;
 	  });
 	});
-    }).si_then([c, actual_addr, addr, intermediate_base](auto &&state) {
-      auto ret_pin = state.ret->get_pin(c);
-      if (actual_addr != P_ADDR_NULL) {
-	ceph_assert(addr.is_laddr());
-	ret_pin->set_paddr(actual_addr);
-	ret_pin->set_intermediate_base(intermediate_base);
-      } else {
-	ceph_assert(addr.is_paddr());
-      }
-      return alloc_extent_iertr::make_ready_future<LBAMappingRef>(
-	std::move(ret_pin));
+    }).si_then([c](auto &&state) {
+      return alloc_extent_iertr::make_ready_future<
+	LBAMappingRef>(state.ret->get_pin(c));
     });
 }
 
@@ -496,7 +489,9 @@ BtreeLBAManager::scan_mappings(
 	      seastar::stop_iteration::yes);
 	  }
 	  ceph_assert((pos.get_key() + pos.get_val().len) > begin);
-	  f(pos.get_key(), pos.get_val().pladdr.get_paddr(), pos.get_val().len);
+	  if (pos.get_val().pladdr.is_paddr()) {
+	    f(pos.get_key(), pos.get_val().pladdr.get_paddr(), pos.get_val().len);
+	  }
 	  return LBABtree::iterate_repeat_ret_inner(
 	    interruptible::ready_future_marker{},
 	    seastar::stop_iteration::no);
@@ -554,7 +549,8 @@ BtreeLBAManager::update_mapping(
       return ret;
     },
     nextent
-  ).si_then([&t, laddr, prev_addr, addr, FNAME](auto result) {
+  ).si_then([&t, laddr, prev_addr, addr, FNAME](auto p) {
+      auto &result = p.first;
       DEBUGT("laddr={}, paddr {} => {} done -- {}",
              t, laddr, prev_addr, addr, result);
     },
@@ -685,7 +681,9 @@ BtreeLBAManager::update_refcount(
       return out;
     },
     nullptr
-  ).si_then([&t, addr, delta, FNAME, this, cascade_remove](auto result) {
+  ).si_then([&t, addr, delta, FNAME, this, cascade_remove](auto p) {
+    auto &result = p.first;
+    auto &mapping = p.second;
     DEBUGT("laddr={}, delta={} done -- {}", t, addr, delta, result);
     auto fut = ref_iertr::make_ready_future<
       std::optional<std::pair<paddr_t, extent_len_t>>>();
@@ -696,19 +694,23 @@ BtreeLBAManager::update_refcount(
 	result.len
       );
     }
-    return fut.si_then([result](auto removed) {
+    return fut.si_then([result, mapping=std::move(mapping)]
+		       (auto removed) mutable {
       if (result.pladdr.is_laddr()
 	  && removed) {
-	return ref_update_result_t{
-	  result.refcount,
-	  removed->first,
-	  removed->second};
+	return std::make_pair(
+	    ref_update_result_t{
+	      result.refcount,
+	      removed->first,
+	      removed->second},
+	    std::move(mapping));
       } else {
-	return ref_update_result_t{
-	  result.refcount,
-	  result.pladdr,
-	  result.len
-	};
+	return std::make_pair(
+	    ref_update_result_t{
+	      result.refcount,
+	      result.pladdr,
+	      result.len},
+	    std::move(mapping));
       }
     });
   });
@@ -722,7 +724,7 @@ BtreeLBAManager::_update_mapping(
   LogicalCachedExtent* nextent)
 {
   auto c = get_context(t);
-  return with_btree_ret<LBABtree, lba_map_val_t>(
+  return with_btree_ret<LBABtree, _update_mapping_ret_bare>(
     cache,
     c,
     [f=std::move(f), c, addr, nextent](auto &btree) mutable {
@@ -742,16 +744,18 @@ BtreeLBAManager::_update_mapping(
 	    c,
 	    iter
 	  ).si_then([ret] {
-	    return ret;
+	    return std::make_pair(
+		std::move(ret), BtreeLBAMappingRef(nullptr));
 	  });
 	} else {
 	  return btree.update(
 	    c,
 	    iter,
 	    ret,
 	    nextent
-	  ).si_then([ret](auto) {
-	    return ret;
+	  ).si_then([c, ret](auto iter) {
+	    return std::make_pair(
+		std::move(ret), iter.get_pin(c));
 	  });
 	}
       });