Merge pull request ceph#63431 from Matan-B/wip-matanb-crimson-tentacle-63219

crimson/os/seastore/linked_tree_node: tentacle: correct balance pivots calculation for both fixed-kv tree and omap tree

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

Author: Matan-B

src/crimson/common/fixed_kv_node_layout.h

@@ -558,26 +558,35 @@ class FixedKVNodeLayout {
     set_meta(Meta::merge_from(left.get_meta(), right.get_meta()));
   }
 
+  static uint32_t get_balance_pivot_idx(
+    const FixedKVNodeLayout &left,
+    const FixedKVNodeLayout &right)
+  {
+    auto l_size = left.get_size();
+    auto r_size = right.get_size();
+    auto total = l_size + r_size;
+    auto pivot_idx = total / 2;
+    assert(pivot_idx > std::min(l_size, r_size)
+      && pivot_idx < std::max(l_size, r_size));
+    return pivot_idx;
+  }
+
   /**
    * balance_into_new_nodes
    *
    * Takes the contents of left and right and copies them into
-   * replacement_left and replacement_right such that in the
-   * event that the number of elements is odd the extra goes to
-   * the left side iff prefer_left.
+   * replacement_left and replacement_right such that the size
+   * of both are balanced.
    */
   static K balance_into_new_nodes(
     const FixedKVNodeLayout &left,
     const FixedKVNodeLayout &right,
-    bool prefer_left,
+    uint32_t pivot_idx,
     FixedKVNodeLayout &replacement_left,
     FixedKVNodeLayout &replacement_right)
   {
+    assert(pivot_idx != left.get_size() && pivot_idx != right.get_size());
     auto total = left.get_size() + right.get_size();
-    auto pivot_idx = (left.get_size() + right.get_size()) / 2;
-    if (total % 2 && prefer_left) {
-      pivot_idx++;
-    }
     auto replacement_pivot = pivot_idx >= left.get_size() ?
       right.iter_idx(pivot_idx - left.get_size())->get_key() :
       left.iter_idx(pivot_idx)->get_key();

src/crimson/os/seastore/backref/backref_tree_node.h

@@ -173,7 +173,7 @@ class BackrefLeafNode
     Transaction &t,
     BackrefLeafNode &left,
     BackrefLeafNode &right,
-    bool prefer_left,
+    uint32_t pivot_idx,
     BackrefLeafNode &replacement_left,
     BackrefLeafNode &replacement_right) final {}
 
@@ -191,7 +191,7 @@ class BackrefLeafNode
     Transaction &t,
     BackrefLeafNode &left,
     BackrefLeafNode &right,
-    bool prefer_left,
+    uint32_t pivot_idx,
     BackrefLeafNode &replacement_left,
     BackrefLeafNode &replacement_right) final {}
   // backref leaf nodes don't have to resolve relative addresses

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

@@ -312,17 +312,6 @@ class FixedKVBtree {
       }
       return get_depth();
     }
-
-    depth_t check_merge() const {
-      if (!leaf.node->below_min_capacity()) {
-	return 0;
-      }
-      for (depth_t merge_from = 1; merge_from < get_depth(); ++merge_from) {
-	if (!get_internal(merge_from + 1).node->below_min_capacity())
-	  return merge_from;
-      }
-      return get_depth();
-    }
   };
 
   FixedKVBtree(RootBlockRef &root_block) : root_block(root_block) {}
@@ -2064,12 +2053,13 @@ class FixedKVBtree {
         c.cache.retire_extent(c.trans, r);
         get_tree_stats<self_type>(c.trans).extents_num_delta--;
       } else {
+        auto pivot_idx = l->get_balance_pivot_idx(*l, *r);
         LOG_PREFIX(FixedKVBtree::merge_level);
         auto [replacement_l, replacement_r, pivot] =
           l->make_balanced(
             c,
             r,
-            !donor_is_left);
+            pivot_idx);
 
         parent_pos.node->update(
           liter,

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

@@ -341,7 +341,7 @@ struct FixedKVInternalNode
   make_balanced(
     op_context_t<NODE_KEY> c,
     Ref &_right,
-    bool prefer_left) {
+    uint32_t pivot_idx) {
     ceph_assert(_right->get_type() == this->get_type());
     auto &right = *_right->template cast<node_type_t>();
     auto replacement_left = c.cache.template alloc_new_non_data_extent<node_type_t>(
@@ -353,13 +353,13 @@ struct FixedKVInternalNode
       c.trans,
       static_cast<node_type_t&>(*this),
       right,
-      prefer_left,
+      pivot_idx,
       *replacement_left,
       *replacement_right);
     auto pivot = this->balance_into_new_nodes(
       *this,
       right,
-      prefer_left,
+      pivot_idx,
       *replacement_left,
       *replacement_right);
     replacement_left->range = replacement_left->get_meta();
@@ -368,7 +368,7 @@ struct FixedKVInternalNode
       c.trans,
       static_cast<node_type_t&>(*this),
       right,
-      prefer_left,
+      pivot_idx,
       *replacement_left,
       *replacement_right);
     return std::make_tuple(
@@ -688,22 +688,22 @@ struct FixedKVLeafNode
     Transaction &t,
     node_type_t &left,
     node_type_t &right,
-    bool prefer_left,
+    uint32_t pivot_idx,
     node_type_t &replacement_left,
     node_type_t &replacement_right) = 0;
   virtual void adjust_copy_src_dest_on_balance(
     Transaction &t,
     node_type_t &left,
     node_type_t &right,
-    bool prefer_left,
+    uint32_t pivot_idx,
     node_type_t &replacement_left,
     node_type_t &replacement_right) = 0;
 
   std::tuple<Ref, Ref, NODE_KEY>
   make_balanced(
     op_context_t<NODE_KEY> c,
     Ref &_right,
-    bool prefer_left) {
+    uint32_t pivot_idx) {
     ceph_assert(_right->get_type() == this->get_type());
     auto &right = *_right->template cast<node_type_t>();
     auto replacement_left = c.cache.template alloc_new_non_data_extent<node_type_t>(
@@ -715,13 +715,13 @@ struct FixedKVLeafNode
       c.trans,
       static_cast<node_type_t&>(*this),
       right,
-      prefer_left,
+      pivot_idx,
       *replacement_left,
       *replacement_right);
     auto pivot = this->balance_into_new_nodes(
       *this,
       right,
-      prefer_left,
+      pivot_idx,
       *replacement_left,
       *replacement_right);
     replacement_left->range = replacement_left->get_meta();
@@ -730,7 +730,7 @@ struct FixedKVLeafNode
       c.trans,
       static_cast<node_type_t&>(*this),
       right,
-      prefer_left,
+      pivot_idx,
       *replacement_left,
       *replacement_right);
     return std::make_tuple(

src/crimson/os/seastore/lba_manager/btree/lba_btree_node.h

@@ -304,21 +304,21 @@ struct LBALeafNode
     Transaction &t,
     LBALeafNode &left,
     LBALeafNode &right,
-    bool prefer_left,
+    uint32_t pivot_idx,
     LBALeafNode &replacement_left,
     LBALeafNode &replacement_right) final {
     this->balance_child_ptrs(
-      t, left, right, prefer_left, replacement_left, replacement_right);
+      t, left, right, pivot_idx, replacement_left, replacement_right);
   }
   void adjust_copy_src_dest_on_balance(
     Transaction &t,
     LBALeafNode &left,
     LBALeafNode &right,
-    bool prefer_left,
+    uint32_t pivot_idx,
     LBALeafNode &replacement_left,
     LBALeafNode &replacement_right) final {
     this->parent_node_t::adjust_copy_src_dest_on_balance(
-      t, left, right, prefer_left, replacement_left, replacement_right);
+      t, left, right, pivot_idx, replacement_left, replacement_right);
   }
 
   CachedExtentRef duplicate_for_write(Transaction&) final {

src/crimson/os/seastore/linked_tree_node.h

@@ -632,18 +632,14 @@ class ParentNode {
     Transaction &t,
     T &left,
     T &right,
-    bool prefer_left,
+    uint32_t pivot_idx,
     T &replacement_left,
     T &replacement_right)
   {
     size_t l_size = left.get_size();
     size_t r_size = right.get_size();
-    size_t total = l_size + r_size;
-    size_t pivot_idx = (l_size + r_size) / 2;
-    if (total % 2 && prefer_left) {
-      pivot_idx++;
-    }
 
+    ceph_assert(pivot_idx != l_size && pivot_idx != r_size);
     replacement_left.maybe_expand_children(pivot_idx);
     replacement_right.maybe_expand_children(r_size + l_size - pivot_idx);
 
@@ -682,17 +678,11 @@ class ParentNode {
     Transaction &t,
     T &left,
     T &right,
-    bool prefer_left,
+    uint32_t pivot_idx,
     T &replacement_left,
     T &replacement_right)
   {
     size_t l_size = left.get_size();
-    size_t r_size = right.get_size();
-    size_t total = l_size + r_size;
-    size_t pivot_idx = (l_size + r_size) / 2;
-    if (total % 2 && prefer_left) {
-      pivot_idx++;
-    }
 
     if (left.is_initial_pending()) {
       for (auto &cs : left.copy_sources) {

src/crimson/os/seastore/omap_manager.h

@@ -68,15 +68,16 @@ class OMapManager {
    * @param string &key, omap string key
    * @param string &value, mapped value corresponding key
    */
-  using omap_set_key_iertr = base_iertr;
+  using omap_set_key_iertr = base_iertr::extend<
+    crimson::ct_error::value_too_large>;
   using omap_set_key_ret = omap_set_key_iertr::future<>;
   virtual omap_set_key_ret omap_set_key(
     omap_root_t &omap_root,
     Transaction &t,
     const std::string &key,
     const ceph::bufferlist &value) = 0;
 
-  using omap_set_keys_iertr = base_iertr;
+  using omap_set_keys_iertr = omap_set_key_iertr;
   using omap_set_keys_ret = omap_set_keys_iertr::future<>;
   virtual omap_set_keys_ret omap_set_keys(
     omap_root_t &omap_root,

src/crimson/os/seastore/omap_manager/btree/omap_btree_node.h

@@ -66,7 +66,8 @@ struct OMapNode : LogicalChildNode {
     omap_context_t oc,
     const std::string &key) = 0;
 
-  using insert_iertr = base_iertr;
+  using insert_iertr = base_iertr::extend<
+    crimson::ct_error::value_too_large>;
   using insert_ret = insert_iertr::future<mutation_result_t>;
   virtual insert_ret insert(
     omap_context_t oc,
@@ -101,7 +102,7 @@ struct OMapNode : LogicalChildNode {
 
   using make_balanced_iertr = base_iertr;
   using make_balanced_ret = make_balanced_iertr::future
-          <std::tuple<OMapNodeRef, OMapNodeRef, std::string>>;
+          <std::tuple<OMapNodeRef, OMapNodeRef, std::optional<std::string>>>;
   virtual make_balanced_ret make_balanced(
     omap_context_t oc,
     OMapNodeRef _right) = 0;
@@ -116,6 +117,10 @@ struct OMapNode : LogicalChildNode {
 
   virtual ~OMapNode() = default;
 
+  virtual bool exceeds_max_kv_limit(
+    const std::string &key,
+    const ceph::bufferlist &value) const = 0;
+
   void init_range(std::string _begin, std::string _end) {
     assert(begin.empty());
     assert(end.empty());