Skip to content

Commit

Permalink
crimson/onode-staged-tree: fix ownership issues at node-level
Browse files Browse the repository at this point in the history 
Signed-off-by: Yingxin Cheng <[email protected]>
  • Loading branch information
@cyx1231st
cyx1231st committed Apr 29, 2021
1 parent 47b67ec commit c6427de
Show file tree
Hide file tree
Showing 3 changed files with 73 additions and 53 deletions.
109 changes: 63 additions & 46 deletions src/crimson/os/seastore/onode_manager/staged-fltree/node.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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>>
Expand All @@ -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();
Expand All @@ -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;
}
Expand Down Expand Up @@ -585,16 +596,16 @@ 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();
}
// 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)
{
Expand All @@ -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)
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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);
Expand All @@ -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();
}
Expand All @@ -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();

Expand Down Expand Up @@ -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()) {
Expand All @@ -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));
}
}
});
Expand Down Expand Up @@ -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));
}
}
});
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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);

Expand Down Expand Up @@ -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();
Expand All @@ -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;
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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 {} ...",
Expand All @@ -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;
});
Expand Down
10 changes: 5 additions & 5 deletions src/crimson/os/seastore/onode_manager/staged-fltree/node.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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(
Expand Down
Loading

0 comments on commit c6427de

Please sign in to comment.