From 7dcd74ac098201d2ea6f9d1c723f9944ade6bf33 Mon Sep 17 00:00:00 2001
From: Xuehan Xu <xuxuehan@qianxin.com>
Date: Thu, 21 Mar 2024 09:54:42 +0800
Subject: [PATCH] crimson/os/seastore/btree: always check the stability of
 extents within the current transaction's view

Signed-off-by: Xuehan Xu <xuxuehan@qianxin.com>
---
 src/crimson/os/seastore/btree/btree_range_pin.cc |  2 +-
 src/crimson/os/seastore/btree/fixed_kv_node.h    | 14 +++++++++-----
 src/crimson/os/seastore/cache.h                  |  9 +++++++++
 src/crimson/os/seastore/cached_extent.h          | 10 +++++-----
 4 files changed, 24 insertions(+), 11 deletions(-)

diff --git a/src/crimson/os/seastore/btree/btree_range_pin.cc b/src/crimson/os/seastore/btree/btree_range_pin.cc
index 1fe79eafa8177..3da1cac970ae7 100644
--- a/src/crimson/os/seastore/btree/btree_range_pin.cc
+++ b/src/crimson/os/seastore/btree/btree_range_pin.cc
@@ -29,7 +29,7 @@ bool BtreeNodeMapping<key_t, val_t>::is_stable() const
   assert(parent->is_valid());
   assert(pos != std::numeric_limits<uint16_t>::max());
   auto &p = (FixedKVNode<key_t>&)*parent;
-  return p.is_child_stable(pos);
+  return p.is_child_stable(ctx, pos);
 }
 
 template class BtreeNodeMapping<laddr_t, paddr_t>;
diff --git a/src/crimson/os/seastore/btree/fixed_kv_node.h b/src/crimson/os/seastore/btree/fixed_kv_node.h
index de3569c0c5bcf..ab8775ad8aa83 100644
--- a/src/crimson/os/seastore/btree/fixed_kv_node.h
+++ b/src/crimson/os/seastore/btree/fixed_kv_node.h
@@ -229,13 +229,14 @@ 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;
+  virtual bool is_child_stable(op_context_t<node_key_t>, 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();
     assert(children.capacity());
     auto child = children[pos];
+    ceph_assert(!is_reserved_ptr(child));
     if (is_valid_child_ptr(child)) {
       ceph_assert(child->get_type() == T::TYPE);
       return c.cache.template get_extent_viewable_by_trans<T>(c.trans, (T*)child);
@@ -596,7 +597,7 @@ struct FixedKVInternalNode
     return get_child_ret_t<LogicalCachedExtent>(child_pos_t(nullptr, 0));
   }
 
-  bool is_child_stable(uint16_t pos) const final {
+  bool is_child_stable(op_context_t<NODE_KEY>, uint16_t pos) const final {
     ceph_abort("impossible");
     return false;
   }
@@ -999,13 +1000,16 @@ struct FixedKVLeafNode
   // 2. The child extent is stable
   //
   // For reserved mappings, the return values are undefined.
-  bool is_child_stable(uint16_t pos) const final {
+  bool is_child_stable(op_context_t<NODE_KEY> c, uint16_t pos) const final {
     auto child = this->children[pos];
     if (is_reserved_ptr(child)) {
       return true;
     } else if (is_valid_child_ptr(child)) {
       ceph_assert(child->is_logical());
-      return child->is_stable();
+      ceph_assert(
+	child->is_pending_in_trans(c.trans.get_trans_id())
+	|| this->is_stable_written());
+      return c.cache.is_viewable_extent_stable(c.trans, child);
     } else if (this->is_pending()) {
       auto key = this->iter_idx(pos).get_key();
       auto &sparent = this->get_stable_for_key(key);
@@ -1013,7 +1017,7 @@ struct FixedKVLeafNode
       auto child = sparent.children[spos];
       if (is_valid_child_ptr(child)) {
 	ceph_assert(child->is_logical());
-	return child->is_stable();
+	return c.cache.is_viewable_extent_stable(c.trans, child);
       } else {
 	return true;
       }
diff --git a/src/crimson/os/seastore/cache.h b/src/crimson/os/seastore/cache.h
index 28471bbcd7ea3..c3dbda1dd8ada 100644
--- a/src/crimson/os/seastore/cache.h
+++ b/src/crimson/os/seastore/cache.h
@@ -443,6 +443,15 @@ class Cache {
     return get_absent_extent<T>(t, offset, length, [](T &){});
   }
 
+  bool is_viewable_extent_stable(
+    Transaction &t,
+    CachedExtentRef extent)
+  {
+    assert(extent);
+    auto view = extent->get_transactional_view(t);
+    return view->is_stable();
+  }
+
   using get_extent_ertr = base_ertr;
   get_extent_ertr::future<CachedExtentRef>
   get_extent_viewable_by_trans(
diff --git a/src/crimson/os/seastore/cached_extent.h b/src/crimson/os/seastore/cached_extent.h
index 730a0ace9a0b7..fee7ca74514cd 100644
--- a/src/crimson/os/seastore/cached_extent.h
+++ b/src/crimson/os/seastore/cached_extent.h
@@ -618,6 +618,11 @@ class CachedExtent
     return last_committed_crc;
   }
 
+  /// Returns true if the extent part of the open transaction
+  bool is_pending_in_trans(transaction_id_t id) const {
+    return is_pending() && pending_for_transaction == id;
+  }
+
 private:
   template <typename T>
   friend class read_set_item_t;
@@ -648,11 +653,6 @@ class CachedExtent
     ptr = nptr;
   }
 
-  /// Returns true if the extent part of the open transaction
-  bool is_pending_in_trans(transaction_id_t id) const {
-    return is_pending() && pending_for_transaction == id;
-  }
-
   /// hook for intrusive ref list (mainly dirty or lru list)
   boost::intrusive::list_member_hook<> primary_ref_list_hook;
   using primary_ref_list_member_options = boost::intrusive::member_hook<