Merge pull request ceph#57368 from myoungwon/wip-seastore-is-data-stable

crimson/os/seastore: add is_data_stable() to allow delta-overwrite on EXIST_CLEAN

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

Author: cyx1231st

src/common/options/crimson.yaml.in

@@ -108,6 +108,8 @@ options:
   level: advanced
   desc: split extent if ratio of total extent size to write size exceeds this value
   default: 1.25
+# TODO: seastore_obj_data_write_amplification is no longer correct if 
+#       seastore_data_delta_based_overwrite is enabled. So, this should be reconsidered.
 - name: seastore_max_concurrent_transactions
   type: uint
   level: advanced

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

@@ -36,6 +36,16 @@ bool BtreeNodeMapping<key_t, val_t>::is_stable() const
   return p.is_child_stable(ctx, pos);
 }
 
+template <typename key_t, typename val_t>
+bool BtreeNodeMapping<key_t, val_t>::is_data_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_data_stable(ctx, 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

@@ -196,6 +196,7 @@ class BtreeNodeMapping : public PhysicalNodeMapping<key_t, val_t> {
 
   get_child_ret_t<LogicalCachedExtent> get_logical_extent(Transaction&) final;
   bool is_stable() const final;
+  bool is_data_stable() const final;
 };
 
 }

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

@@ -227,6 +227,7 @@ struct FixedKVNode : ChildableCachedExtent {
   }
 
   virtual bool is_child_stable(op_context_t<node_key_t>, uint16_t pos) const = 0;
+  virtual bool is_child_data_stable(op_context_t<node_key_t>, uint16_t pos) const = 0;
 
   template <typename T>
   get_child_ret_t<T> get_child(
@@ -597,6 +598,10 @@ struct FixedKVInternalNode
     ceph_abort("impossible");
     return false;
   }
+  bool is_child_data_stable(op_context_t<NODE_KEY>, uint16_t pos) const final {
+    ceph_abort("impossible");
+    return false;
+  }
 
   bool validate_stable_children() final {
     LOG_PREFIX(FixedKVInternalNode::validate_stable_children);
@@ -986,6 +991,13 @@ struct FixedKVLeafNode
   //
   // For reserved mappings, the return values are undefined.
   bool is_child_stable(op_context_t<NODE_KEY> c, uint16_t pos) const final {
+    return _is_child_stable(c, pos);
+  }
+  bool is_child_data_stable(op_context_t<NODE_KEY> c, uint16_t pos) const final {
+    return _is_child_stable(c, pos, true);
+  }
+
+  bool _is_child_stable(op_context_t<NODE_KEY> c, uint16_t pos, bool data_only = false) const {
     auto child = this->children[pos];
     if (is_reserved_ptr(child)) {
       return true;
@@ -994,15 +1006,23 @@ struct FixedKVLeafNode
       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);
+      if (data_only) {
+	return c.cache.is_viewable_extent_data_stable(c.trans, child);
+      } else {
+	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);
       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 c.cache.is_viewable_extent_stable(c.trans, child);
+	if (data_only) {
+	  return c.cache.is_viewable_extent_data_stable(c.trans, child);
+	} else {
+	  return c.cache.is_viewable_extent_stable(c.trans, child);
+	}
       } else {
 	return true;
       }

src/crimson/os/seastore/cache.h

@@ -452,6 +452,15 @@ class Cache {
     return view->is_stable();
   }
 
+  bool is_viewable_extent_data_stable(
+    Transaction &t,
+    CachedExtentRef extent)
+  {
+    assert(extent);
+    auto view = extent->get_transactional_view(t);
+    return view->is_data_stable();
+  }
+
   using get_extent_ertr = base_ertr;
   get_extent_ertr::future<CachedExtentRef>
   get_extent_viewable_by_trans(

src/crimson/os/seastore/cached_extent.h

@@ -436,6 +436,10 @@ class CachedExtent
             is_pending_io());
   }
 
+  bool is_data_stable() const {
+    return is_stable() || is_exist_clean();
+  }
+
   /// Returns true if extent has a pending delta
   bool is_mutation_pending() const {
     return state == extent_state_t::MUTATION_PENDING;
@@ -1087,6 +1091,7 @@ class PhysicalNodeMapping {
   // For reserved mappings, the return values are
   // undefined although it won't crash
   virtual bool is_stable() const = 0;
+  virtual bool is_data_stable() const = 0;
   virtual bool is_clone() const = 0;
   bool is_zero_reserved() const {
     return !get_val().is_real();

src/crimson/os/seastore/object_data_handler.cc

@@ -297,14 +297,13 @@ overwrite_ops_t prepare_ops_list(
   interval_set<uint64_t> pre_alloc_addr_removed, pre_alloc_addr_remapped;
   if (delta_based_overwrite_max_extent_size) {
     for (auto &r : ops.to_remove) {
-      // TODO: Introduce LBAMapping::is_data_stable() to include EXIST_CLEAN extents
-      if (r->is_stable() && !r->is_zero_reserved()) {
+      if (r->is_data_stable() && !r->is_zero_reserved()) {
 	pre_alloc_addr_removed.insert(r->get_key(), r->get_length());
 
       }
     }
     for (auto &r : ops.to_remap) {
-      if (r.pin && r.pin->is_stable() && !r.pin->is_zero_reserved()) {
+      if (r.pin && r.pin->is_data_stable() && !r.pin->is_zero_reserved()) {
 	pre_alloc_addr_remapped.insert(r.pin->get_key(), r.pin->get_length());
       }
     }
@@ -641,8 +640,8 @@ struct overwrite_plan_t {
 
   // helper member
   extent_len_t block_size;
-  bool is_left_stable;
-  bool is_right_stable;
+  bool is_left_fresh;
+  bool is_right_fresh;
 
 public:
   extent_len_t get_left_size() const {
@@ -692,8 +691,8 @@ struct overwrite_plan_t {
 	       << ", left_operation=" << overwrite_plan.left_operation
 	       << ", right_operation=" << overwrite_plan.right_operation
 	       << ", block_size=" << overwrite_plan.block_size
-	       << ", is_left_stable=" << overwrite_plan.is_left_stable
-	       << ", is_right_stable=" << overwrite_plan.is_right_stable
+	       << ", is_left_fresh=" << overwrite_plan.is_left_fresh
+	       << ", is_right_fresh=" << overwrite_plan.is_right_fresh
 	       << ")";
   }
 
@@ -712,8 +711,10 @@ struct overwrite_plan_t {
       left_operation(overwrite_operation_t::UNKNOWN),
       right_operation(overwrite_operation_t::UNKNOWN),
       block_size(block_size),
-      is_left_stable(pins.front()->is_stable()),
-      is_right_stable(pins.back()->is_stable()) {
+      // TODO: introduce PhysicalNodeMapping::is_fresh()
+      // Note: fresh write can be merged with overwrite if they overlap.
+      is_left_fresh(!pins.front()->is_stable()),
+      is_right_fresh(!pins.back()->is_stable()) {
     validate();
     evaluate_operations();
     assert(left_operation != overwrite_operation_t::UNKNOWN);
@@ -742,6 +743,9 @@ struct overwrite_plan_t {
    * seastore_obj_data_write_amplification; otherwise, split the
    * original extent into at most three parts: origin-left, part-to-be-modified
    * and origin-right.
+   *
+   * TODO: seastore_obj_data_write_amplification needs to be reconsidered because
+   * delta-based overwrite is introduced
    */
   void evaluate_operations() {
     auto actual_write_size = get_pins_size();
@@ -753,7 +757,7 @@ struct overwrite_plan_t {
       actual_write_size -= left_ext_size;
       left_ext_size = 0;
       left_operation = overwrite_operation_t::OVERWRITE_ZERO;
-    } else if (!is_left_stable) {
+    } else if (is_left_fresh) {
       aligned_data_size += left_ext_size;
       left_ext_size = 0;
       left_operation = overwrite_operation_t::MERGE_EXISTING;
@@ -763,7 +767,7 @@ struct overwrite_plan_t {
       actual_write_size -= right_ext_size;
       right_ext_size = 0;
       right_operation = overwrite_operation_t::OVERWRITE_ZERO;
-    } else if (!is_right_stable) {
+    } else if (is_right_fresh) {
       aligned_data_size += right_ext_size;
       right_ext_size = 0;
       right_operation = overwrite_operation_t::MERGE_EXISTING;

src/crimson/os/seastore/transaction_manager.h

@@ -365,7 +365,7 @@ class TransactionManager : public ExtentCallbackInterface {
   read_extent_ret<T> get_mutable_extent_by_laddr(Transaction &t, laddr_t laddr, extent_len_t len) {
     return get_pin(t, laddr
     ).si_then([this, &t, len](auto pin) {
-      ceph_assert(pin->is_stable() && !pin->is_zero_reserved());
+      ceph_assert(pin->is_data_stable() && !pin->is_zero_reserved());
       ceph_assert(!pin->is_clone());
       ceph_assert(pin->get_length() == len);
       return this->read_pin<T>(t, std::move(pin));

src/test/crimson/seastore/test_object_data_handler.cc

@@ -800,22 +800,9 @@ TEST_P(object_data_handler_test_t, overwrite_then_read_within_transaction) {
       auto pin1 = remap_pin(*t, std::move(pins.front()), 4096, 8192);
       auto ext = get_extent(*t, base + 4096, 4096 * 2);
       ASSERT_TRUE(ext->is_exist_clean());
-      ext = tm->get_mutable_extent(*t, ext)->cast<ObjectDataBlock>();
-
-      auto l = 4096;
-      memset(
-	known_contents.c_str() + base + 4096,
-	'z',
-	l);
-      bufferlist bl;
-      bl.append(
-	bufferptr(
-	  known_contents,
-	  base + 4096,
-	  l));
-
-      ext->overwrite(0, bl);
+      write(*t, base + 4096, 4096, 'y');
       ASSERT_TRUE(ext->is_exist_mutation_pending());
+      write(*t, base + 8092, 4096, 'z');
     }
     submit_transaction(std::move(t));
     read(base + 4096, 4096);