From c6427de4db284256ad13e6f8be54d2ab6a153a7e Mon Sep 17 00:00:00 2001 From: Yingxin Cheng Date: Fri, 23 Apr 2021 10:17:31 +0800 Subject: [PATCH] crimson/onode-staged-tree: fix ownership issues at node-level Signed-off-by: Yingxin Cheng --- .../onode_manager/staged-fltree/node.cc | 109 ++++++++++-------- .../onode_manager/staged-fltree/node.h | 10 +- .../seastore/onode_tree/test_staged_fltree.cc | 7 +- 3 files changed, 73 insertions(+), 53 deletions(-) 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 8121e6f625c54..b0fa5304bc3a3 100644 --- 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(const search_position_t&, Ref); template void Node::as_child(const search_position_t&, Ref); node_future<> Node::apply_split_to_parent( - context_t c, Ref split_right, bool update_right_index) + context_t c, + Ref&& this_ref, + Ref&& 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> @@ -495,15 +499,16 @@ Node::get_next_cursor_from_parent(context_t c) template 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&& this_ref) { + assert(this == this_ref.get()); impl->validate_non_empty(); assert(!is_root()); - Ref 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 left_for_merge; Ref right_for_merge; + Ref* 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(context_t, bool); -template node_future<> Node::try_merge_adjacent(context_t, bool); +template node_future<> Node::try_merge_adjacent(context_t, bool, Ref&&); +template node_future<> Node::try_merge_adjacent(context_t, bool, Ref&&); node_future<> Node::erase_node(context_t c, Ref&& this_ref) { @@ -607,18 +618,19 @@ node_future<> Node::erase_node(context_t c, Ref&& this_ref) template node_future<> Node::fix_parent_index( - context_t c, bool check_downgrade) + context_t c, Ref&& 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(c, this, check_downgrade); + return parent->fix_index(c, std::move(this_ref), check_downgrade); } -template node_future<> Node::fix_parent_index(context_t, bool); -template node_future<> Node::fix_parent_index(context_t, bool); +template node_future<> Node::fix_parent_index(context_t, Ref&&, bool); +template node_future<> Node::fix_parent_index(context_t, Ref&&, bool); node_future> 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 left_child, Ref right_child, + context_t c, Ref&& left_child, Ref&& 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 this_ref = this; + Ref 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&& child_ref) } do_untrack_child(*child_ref); - Ref this_ref = this; + Ref 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&& child_ref) // no child should be referencing this node now, this_ref is the last one. assert(this_ref->use_count() == 1); - Ref 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&& 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&& child_ref) template node_future<> InternalNode::fix_index( - context_t c, Ref child, bool check_downgrade) + context_t c, Ref&& child, bool check_downgrade) { impl->validate_non_empty(); @@ -926,14 +940,15 @@ node_future<> InternalNode::fix_index( : update_parent_index = false; } - Ref this_ref = this; + Ref 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(c, update_parent_index); + return try_merge_adjacent( + 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 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(c, true); + return left_child->fix_parent_index(c, std::move(left_child), true); } else { - left_child.reset(); validate_tracked_children(); + Ref 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(c, false); + return try_merge_adjacent( + 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 this_ref = this; + Ref 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 this_ref = this; + Ref 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> InternalNode::get_or_track_child( bool level_tail = position.is_end(); Ref child; auto found = tracked_child_nodes.find(position); - Ref this_ref = this; + Ref 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 this_ref = this; + Ref 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_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(c, update_parent_index); + return try_merge_adjacent( + 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 this_ref = this; + Ref 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> LeafNode::insert_value( return node_ertr::make_ready_future>(ret); } // split and insert - Ref this_ref = this; + Ref 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> 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 ce8b64fe5945c..cd6451a15e143 100644 --- 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, bool); + node_future<> apply_split_to_parent(context_t, Ref&&, Ref&&, bool); node_future> get_next_cursor_from_parent(context_t); template - node_future<> try_merge_adjacent(context_t, bool); + node_future<> try_merge_adjacent(context_t, bool, Ref&&); node_future<> erase_node(context_t, Ref&&); template - node_future<> fix_parent_index(context_t, bool); + node_future<> fix_parent_index(context_t, Ref&&, bool); node_future rebuild_extent(context_t); node_future<> retire(context_t, Ref&&); void make_tail(context_t); @@ -485,7 +485,7 @@ class InternalNode final : public Node { node_future> get_next_cursor(context_t, const search_position_t&); - node_future<> apply_child_split(context_t, Ref left, Ref right, bool); + node_future<> apply_child_split(context_t, Ref&& left, Ref&& right, bool); template void do_track_child(Node& child) { @@ -520,7 +520,7 @@ class InternalNode final : public Node { node_future<> erase_child(context_t, Ref&&); template - node_future<> fix_index(context_t, Ref, bool); + node_future<> fix_index(context_t, Ref&&, bool); template 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 74cc23364e0f7..1000bbb9f649c 100644 --- 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 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 new_keys; new_keys.insert(new_key); impl->reset(new_keys, impl->is_level_tail()); - return fix_parent_index(c, false); + Ref this_ref = this; + return fix_parent_index(c, std::move(this_ref), false); } bool match_pos(const search_position_t& pos) const { -- 2.39.5