crimson/onode-staged-tree: fix ownership issues at node-level

Signed-off-by: Yingxin Cheng <yingxin.cheng@intel.com>

diff --git a/src/crimson/os/seastore/onode_manager/staged-fltree/node.cc b/src/crimson/os/seastore/onode_manager/staged-fltree/node.cc
index 8121e6f625c5456673181fbedd729c22d6843f7d..b0fa5304bc3a304917beefdb19edb33fa09cef97 100644 (file)
--- a/src/crimson/os/seastore/onode_manager/staged-fltree/node.cc
+++ b/src/crimson/os/seastore/onode_manager/staged-fltree/node.cc
@@ -477,12 +477,16 @@ template void Node::as_child<true>(const search_position_t&, Ref<InternalNode>);
 template void Node::as_child<false>(const search_position_t&, Ref<InternalNode>);
 
 node_future<> Node::apply_split_to_parent(
-    context_t c, Ref<Node> split_right, bool update_right_index)
+    context_t c,
+    Ref<Node>&& this_ref,
+    Ref<Node>&& split_right,
+    bool update_right_index)
 {
   assert(!is_root());
+  assert(this == this_ref.get());
   // TODO(cross-node string dedup)
   return parent_info().ptr->apply_child_split(
-      c, this, split_right, update_right_index);
+      c, std::move(this_ref), std::move(split_right), update_right_index);
 }
 
 node_future<Ref<tree_cursor_t>>
@@ -495,15 +499,16 @@ Node::get_next_cursor_from_parent(context_t c)
 
 template <bool FORCE_MERGE>
 node_future<>
-Node::try_merge_adjacent(context_t c, bool update_parent_index)
+Node::try_merge_adjacent(
+    context_t c, bool update_parent_index, Ref<Node>&& this_ref)
 {
+  assert(this == this_ref.get());
   impl->validate_non_empty();
   assert(!is_root());
-  Ref<Node> this_ref = this;
   if constexpr (!FORCE_MERGE) {
     if (!impl->is_size_underflow()) {
       if (update_parent_index) {
-        return fix_parent_index(c, false);
+        return fix_parent_index(c, std::move(this_ref), false);
       } else {
         parent_info().ptr->validate_child_tracked(*this);
         return node_ertr::now();
@@ -517,26 +522,32 @@ Node::try_merge_adjacent(context_t c, bool update_parent_index)
     auto& [lnode, rnode] = lr_nodes;
     Ref<Node> left_for_merge;
     Ref<Node> right_for_merge;
+    Ref<Node>* p_this_ref;
     bool is_left;
     if (!lnode && !rnode) {
       // XXX: this is possible before node rebalance is implemented,
       // when its parent cannot merge with its peers and has only one child
       // (this node).
+      p_this_ref = &this_ref;
     } else if (!lnode) {
       left_for_merge = std::move(this_ref);
+      p_this_ref = &left_for_merge;
       right_for_merge = std::move(rnode);
       is_left = true;
     } else if (!rnode) {
       left_for_merge = std::move(lnode);
       right_for_merge = std::move(this_ref);
+      p_this_ref = &right_for_merge;
       is_left = false;
     } else { // lnode && rnode
       if (lnode->impl->free_size() > rnode->impl->free_size()) {
         left_for_merge = std::move(lnode);
         right_for_merge = std::move(this_ref);
+        p_this_ref = &right_for_merge;
         is_left = false;
       } else { // lnode free size <= rnode free size
         left_for_merge = std::move(this_ref);
+        p_this_ref = &left_for_merge;
         right_for_merge = std::move(rnode);
         is_left = true;
       }
@@ -585,7 +596,7 @@ Node::try_merge_adjacent(context_t c, bool update_parent_index)
 
     // cannot merge
     if (update_parent_index) {
-      return fix_parent_index(c, false);
+      return fix_parent_index(c, std::move(*p_this_ref), false);
     } else {
       parent_info().ptr->validate_child_tracked(*this);
       return node_ertr::now();
@@ -593,8 +604,8 @@ Node::try_merge_adjacent(context_t c, bool update_parent_index)
     // XXX: rebalance
   });
 }
-template node_future<> Node::try_merge_adjacent<true>(context_t, bool);
-template node_future<> Node::try_merge_adjacent<false>(context_t, bool);
+template node_future<> Node::try_merge_adjacent<true>(context_t, bool, Ref<Node>&&);
+template node_future<> Node::try_merge_adjacent<false>(context_t, bool, Ref<Node>&&);
 
 node_future<> Node::erase_node(context_t c, Ref<Node>&& this_ref)
 {
@@ -607,18 +618,19 @@ node_future<> Node::erase_node(context_t c, Ref<Node>&& this_ref)
 
 template <bool FORCE_MERGE = false>
 node_future<> Node::fix_parent_index(
-    context_t c, bool check_downgrade)
+    context_t c, Ref<Node>&& this_ref, bool check_downgrade)
 {
   assert(!is_root());
+  assert(this == this_ref.get());
   auto& parent = parent_info().ptr;
   // one-way unlink
   parent->do_untrack_child(*this);
   // the rest of parent tracks should be correct
   parent->validate_tracked_children();
-  return parent->fix_index<FORCE_MERGE>(c, this, check_downgrade);
+  return parent->fix_index<FORCE_MERGE>(c, std::move(this_ref), check_downgrade);
 }
-template node_future<> Node::fix_parent_index<true>(context_t, bool);
-template node_future<> Node::fix_parent_index<false>(context_t, bool);
+template node_future<> Node::fix_parent_index<true>(context_t, Ref<Node>&&, bool);
+template node_future<> Node::fix_parent_index<false>(context_t, Ref<Node>&&, bool);
 
 node_future<Ref<Node>> Node::load(
     context_t c, laddr_t addr, bool expect_is_level_tail)
@@ -714,7 +726,7 @@ InternalNode::get_next_cursor(context_t c, const search_position_t& pos)
 }
 
 node_future<> InternalNode::apply_child_split(
-    context_t c, Ref<Node> left_child, Ref<Node> right_child,
+    context_t c, Ref<Node>&& left_child, Ref<Node>&& right_child,
     bool update_right_index)
 {
   auto& left_pos = left_child->parent_info().position;
@@ -745,24 +757,27 @@ node_future<> InternalNode::apply_child_split(
   replace_track(right_child, left_child, update_right_index);
 
   auto left_key = *left_child->impl->get_pivot_index();
-  Ref<InternalNode> this_ref = this;
+  Ref<Node> this_ref = this;
   return insert_or_split(
       c, left_pos, left_key, left_child,
       (update_right_index ? right_child : nullptr)
-  ).safe_then([this, c, this_ref = std::move(this_ref)] (auto split_right) {
+  ).safe_then([this, c,
+               this_ref = std::move(this_ref)] (auto split_right) mutable {
     if (split_right) {
       // even if update_right_index could be true,
       // we haven't fixed the right_child index of this node yet,
       // so my parent index should be correct now.
-      return apply_split_to_parent(c, split_right, false);
+      return apply_split_to_parent(
+          c, std::move(this_ref), std::move(split_right), false);
     } else {
       return node_ertr::now();
     }
-  }).safe_then([c, update_right_index, right_child] {
+  }).safe_then([c, update_right_index,
+                right_child = std::move(right_child)] () mutable {
     if (update_right_index) {
       // right_child must be already untracked by insert_or_split()
       return right_child->parent_info().ptr->fix_index(
-          c, right_child, false);
+          c, std::move(right_child), false);
     } else {
       // there is no need to call try_merge_adjacent() because
       // the filled size of the inserted node or the split right node
@@ -814,7 +829,7 @@ node_future<> InternalNode::erase_child(context_t c, Ref<Node>&& child_ref)
     }
 
     do_untrack_child(*child_ref);
-    Ref<InternalNode> this_ref = this;
+    Ref<Node> this_ref = this;
     child_ref->_parent_info.reset();
     return child_ref->retire(c, std::move(child_ref)
     ).safe_then([c, this, child_pos, this_ref = std::move(this_ref)] () mutable {
@@ -853,9 +868,7 @@ node_future<> InternalNode::erase_child(context_t c, Ref<Node>&& child_ref)
 
         // no child should be referencing this node now, this_ref is the last one.
         assert(this_ref->use_count() == 1);
-        Ref<Node> node_ref = this_ref;
-        this_ref.reset();
-        return Node::erase_node(c, std::move(node_ref));
+        return Node::erase_node(c, std::move(this_ref));
       }
 
       impl->prepare_mutate(c);
@@ -880,13 +893,14 @@ node_future<> InternalNode::erase_child(context_t c, Ref<Node>&& child_ref)
           next_or_last_pos.is_end() ? update_parent_index = true
                                     : update_parent_index = false;
         }
-        return try_merge_adjacent(c, update_parent_index);
+        return try_merge_adjacent(c, update_parent_index, std::move(this_ref));
       }
-    }).safe_then([c, new_tail_child = std::move(new_tail_child)] {
+    }).safe_then([c, new_tail_child = std::move(new_tail_child)] () mutable {
       // finally, check if the new tail child needs to merge
       if (new_tail_child && !new_tail_child->is_root()) {
         assert(new_tail_child->impl->is_level_tail());
-        return new_tail_child->try_merge_adjacent(c, false);
+        return new_tail_child->try_merge_adjacent(
+            c, false, std::move(new_tail_child));
       } else {
         return node_ertr::now();
       }
@@ -896,7 +910,7 @@ node_future<> InternalNode::erase_child(context_t c, Ref<Node>&& child_ref)
 
 template <bool FORCE_MERGE>
 node_future<> InternalNode::fix_index(
-    context_t c, Ref<Node> child, bool check_downgrade)
+    context_t c, Ref<Node>&& child, bool check_downgrade)
 {
   impl->validate_non_empty();
 
@@ -926,14 +940,15 @@ node_future<> InternalNode::fix_index(
                       : update_parent_index = false;
   }
 
-  Ref<InternalNode> this_ref = this;
+  Ref<Node> this_ref = this;
   return insert_or_split(c, next_pos, new_key, child
   ).safe_then([this, c, update_parent_index, check_downgrade,
-               this_ref = std::move(this_ref)] (auto split_right) {
+               this_ref = std::move(this_ref)] (auto split_right) mutable {
     if (split_right) {
       // after split, the parent index to the split_right will be incorrect
       // if update_parent_index is true.
-      return apply_split_to_parent(c, split_right, update_parent_index);
+      return apply_split_to_parent(
+          c, std::move(this_ref), std::move(split_right), update_parent_index);
     } else {
       // no split path
       if (is_root()) {
@@ -948,7 +963,8 @@ node_future<> InternalNode::fix_index(
       } else {
         // for non-root, maybe need merge adjacent or fix parent,
         // because the filled node size may be reduced.
-        return try_merge_adjacent<FORCE_MERGE>(c, update_parent_index);
+        return try_merge_adjacent<FORCE_MERGE>(
+            c, update_parent_index, std::move(this_ref));
       }
     }
   });
@@ -1012,22 +1028,23 @@ node_future<> InternalNode::apply_children_merge(
   return right_child->retire(c, std::move(right_child)
   ).safe_then([c, this, update_index,
                left_child = std::move(left_child)] () mutable {
-    Ref<InternalNode> this_ref = this;
     if (update_index) {
       // I'm all good but:
       // - my number of keys is reduced by 1
       // - my size may underflow, but try_merge_adjacent() is already part of fix_index()
-      return left_child->fix_parent_index<FORCE_MERGE>(c, true);
+      return left_child->fix_parent_index<FORCE_MERGE>(c, std::move(left_child), true);
     } else {
-      left_child.reset();
       validate_tracked_children();
+      Ref<Node> this_ref = this;
+      left_child.reset();
       // I'm all good but:
       // - my number of keys is reduced by 1
       // - my size may underflow
       if (is_root()) {
         return try_downgrade_root(c, std::move(this_ref));
       } else {
-        return try_merge_adjacent<FORCE_MERGE>(c, false);
+        return try_merge_adjacent<FORCE_MERGE>(
+            c, false, std::move(this_ref));
       }
     }
   });
@@ -1175,7 +1192,7 @@ node_future<> InternalNode::do_get_tree_stats(
   stats.num_kvs_internal += nstats.num_kvs;
   stats.num_nodes_internal += 1;
 
-  Ref<const InternalNode> this_ref = this;
+  Ref<Node> this_ref = this;
   return seastar::do_with(
     search_position_t(), (const laddr_packed_t*)(nullptr),
     [this, this_ref, c, &stats](auto& pos, auto& p_child_addr) {
@@ -1268,7 +1285,7 @@ node_future<> InternalNode::test_clone_root(
   assert(is_root());
   assert(impl->is_level_tail());
   assert(impl->field_type() == field_type_t::N0);
-  Ref<const InternalNode> this_ref = this;
+  Ref<const Node> this_ref = this;
   return InternalNode::allocate(c_other, field_type_t::N0, true, impl->level()
   ).safe_then([this, c_other, &tracker_other](auto fresh_other) {
     impl->test_copy_to(fresh_other.mut);
@@ -1426,7 +1443,7 @@ node_future<Ref<Node>> InternalNode::get_or_track_child(
   bool level_tail = position.is_end();
   Ref<Node> child;
   auto found = tracked_child_nodes.find(position);
-  Ref<InternalNode> this_ref = this;
+  Ref<Node> this_ref = this;
   return (found == tracked_child_nodes.end()
     ? (Node::load(c, child_addr, level_tail
        ).safe_then([this, position] (auto child) {
@@ -1677,7 +1694,7 @@ LeafNode::erase(context_t c, const search_position_t& pos, bool get_next)
 {
   assert(!pos.is_end());
   assert(!impl->is_keys_empty());
-  Ref<LeafNode> this_ref = this;
+  Ref<Node> this_ref = this;
   logger().debug("OTree::Leaf::Erase: erase {}'s pos({}), get_next={} ...",
                  get_name(), pos, get_next);
 
@@ -1716,9 +1733,7 @@ LeafNode::erase(context_t c, const search_position_t& pos, bool get_next)
 
         // no cursor should be referencing this node now, this_ref is the last one.
         assert(this_ref->use_count() == 1);
-        Ref<Node> node_ref = this_ref;
-        this_ref.reset();
-        return Node::erase_node(c, std::move(node_ref));
+        return Node::erase_node(c, std::move(this_ref));
       }
 
       on_layout_change();
@@ -1737,7 +1752,8 @@ LeafNode::erase(context_t c, const search_position_t& pos, bool get_next)
           next_pos.is_end() ? update_parent_index = true
                             : update_parent_index = false;
         }
-        return try_merge_adjacent<FORCE_MERGE>(c, update_parent_index);
+        return try_merge_adjacent<FORCE_MERGE>(
+            c, update_parent_index, std::move(this_ref));
       }
     }).safe_then([next_cursor] {
       return next_cursor;
@@ -1881,7 +1897,7 @@ node_future<> LeafNode::test_clone_root(
   assert(is_root());
   assert(impl->is_level_tail());
   assert(impl->field_type() == field_type_t::N0);
-  Ref<const LeafNode> this_ref = this;
+  Ref<const Node> this_ref = this;
   return LeafNode::allocate(c_other, field_type_t::N0, true
   ).safe_then([this, c_other, &tracker_other](auto fresh_other) {
     impl->test_copy_to(fresh_other.mut);
@@ -1924,11 +1940,11 @@ node_future<Ref<tree_cursor_t>> LeafNode::insert_value(
     return node_ertr::make_ready_future<Ref<tree_cursor_t>>(ret);
   }
   // split and insert
-  Ref<LeafNode> this_ref = this;
+  Ref<Node> this_ref = this;
   return (is_root() ? upgrade_root(c) : node_ertr::now()
   ).safe_then([this, c] {
     return LeafNode::allocate(c, impl->field_type(), impl->is_level_tail());
-  }).safe_then([this_ref, this, c, &key, vconf,
+  }).safe_then([this_ref = std::move(this_ref), this, c, &key, vconf,
                 insert_pos, insert_stage=insert_stage, insert_size=insert_size](auto fresh_right) mutable {
     auto right_node = fresh_right.node;
     logger().info("OTree::Leaf::InsertValue: proceed split {} to fresh {} ...",
@@ -1950,7 +1966,8 @@ node_future<Ref<tree_cursor_t>> LeafNode::insert_value(
     validate_tracked_cursors();
     right_node->validate_tracked_cursors();
 
-    return apply_split_to_parent(c, right_node, false
+    return apply_split_to_parent(
+        c, std::move(this_ref), std::move(right_node), false
     ).safe_then([ret] {
       return ret;
     });

diff --git a/src/crimson/os/seastore/onode_manager/staged-fltree/node.h b/src/crimson/os/seastore/onode_manager/staged-fltree/node.h
index ce8b64fe5945c0f0942d2ea9a19ee8fdf1b9bba3..cd6451a15e1434a46105e0fe25f12cfdf64984d9 100644 (file)
--- a/src/crimson/os/seastore/onode_manager/staged-fltree/node.h
+++ b/src/crimson/os/seastore/onode_manager/staged-fltree/node.h
@@ -430,13 +430,13 @@ class Node
   };
   const parent_info_t& parent_info() const { return *_parent_info; }
 
-  node_future<> apply_split_to_parent(context_t, Ref<Node>, bool);
+  node_future<> apply_split_to_parent(context_t, Ref<Node>&&, Ref<Node>&&, bool);
   node_future<Ref<tree_cursor_t>> get_next_cursor_from_parent(context_t);
   template <bool FORCE_MERGE = false>
-  node_future<> try_merge_adjacent(context_t, bool);
+  node_future<> try_merge_adjacent(context_t, bool, Ref<Node>&&);
   node_future<> erase_node(context_t, Ref<Node>&&);
   template <bool FORCE_MERGE = false>
-  node_future<> fix_parent_index(context_t, bool);
+  node_future<> fix_parent_index(context_t, Ref<Node>&&, bool);
   node_future<NodeExtentMutable> rebuild_extent(context_t);
   node_future<> retire(context_t, Ref<Node>&&);
   void make_tail(context_t);
@@ -485,7 +485,7 @@ class InternalNode final : public Node {
 
   node_future<Ref<tree_cursor_t>> get_next_cursor(context_t, const search_position_t&);
 
-  node_future<> apply_child_split(context_t, Ref<Node> left, Ref<Node> right, bool);
+  node_future<> apply_child_split(context_t, Ref<Node>&& left, Ref<Node>&& right, bool);
 
   template <bool VALIDATE>
   void do_track_child(Node& child) {
@@ -520,7 +520,7 @@ class InternalNode final : public Node {
   node_future<> erase_child(context_t, Ref<Node>&&);
 
   template <bool FORCE_MERGE = false>
-  node_future<> fix_index(context_t, Ref<Node>, bool);
+  node_future<> fix_index(context_t, Ref<Node>&&, bool);
 
   template <bool FORCE_MERGE = false>
   node_future<> apply_children_merge(

diff --git a/src/test/crimson/seastore/onode_tree/test_staged_fltree.cc b/src/test/crimson/seastore/onode_tree/test_staged_fltree.cc
index 74cc23364e0f704b139d2f2c50922dc62f2d9367..1000bbb9f649cd2776b187bbc41b072314efeaf7 100644 (file)
--- a/src/test/crimson/seastore/onode_tree/test_staged_fltree.cc
+++ b/src/test/crimson/seastore/onode_tree/test_staged_fltree.cc
@@ -937,7 +937,9 @@ class DummyChildPool {
       if (right_child->can_split()) {
         splitable_nodes.insert(right_child);
       }
-      return apply_split_to_parent(c, right_child, false);
+      Ref<Node> this_ref = this;
+      return apply_split_to_parent(
+          c, std::move(this_ref), std::move(right_child), false);
     }
 
     node_future<> insert_and_split(
@@ -989,7 +991,8 @@ class DummyChildPool {
       std::set<ghobject_t> new_keys;
       new_keys.insert(new_key);
       impl->reset(new_keys, impl->is_level_tail());
-      return fix_parent_index<true>(c, false);
+      Ref<Node> this_ref = this;
+      return fix_parent_index<true>(c, std::move(this_ref), false);
     }
 
     bool match_pos(const search_position_t& pos) const {