Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Robj/deukyeon deadlock #626

Merged
merged 11 commits into from
May 22, 2024
Merged

Robj/deukyeon deadlock #626

Changes from 10 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
98f9ac5
attempt to fix ci
rtjohnso Jul 24, 2023
f558209
Merge branch 'main' of github.com:vmware/splinterdb
rtjohnso Jul 24, 2023
e5bf3a2
Merge branch 'main' of github.com:vmware/splinterdb
rtjohnso Nov 7, 2023
9957837
Merge branch 'main' of github.com:vmware/splinterdb
rtjohnso Nov 18, 2023
e97b0f4
garbage merge
rtjohnso Feb 2, 2024
beb4145
Merge remote-tracking branch 'refs/remotes/origin/main'
rtjohnso Mar 21, 2024
d5ae0ec
Merge remote-tracking branch 'refs/remotes/origin/main'
rtjohnso May 10, 2024
cb5e6fb
Merge remote-tracking branch 'refs/remotes/origin/main'
rtjohnso May 16, 2024
e744de1
Merge remote-tracking branch 'refs/remotes/origin/main'
rtjohnso May 22, 2024
930961d
fix compact_bundle bugs when index node has split
rtjohnso May 22, 2024
4bb97e6
cleanup
rtjohnso May 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 8 additions & 9 deletions src/trunk.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5377,7 +5377,11 @@ trunk_compact_bundle(void *arg, void *scratch_buf)
}
trunk_log_node_if_enabled(&stream, spl, &node);

should_continue = trunk_compact_bundle_node_has_split(spl, req, &node);
should_continue = trunk_compact_bundle_node_has_split(spl, req, &node)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new test for splitting is based on the unique id for a node, which means that both the left and right nodes resulting from a split will pass that "has split" test. So we need to make sure we stop iterating over nodes at the right edge of the tree.

I suspect this test should actually be comparing with the end-key of the req. Is that correct?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that seems right to me.

Another approach that is a bit cuter, but less correct on its face would be to have the rightmost node keep the old id. I think I prefer your proposal though.

&& trunk_key_compare(spl,
trunk_max_key(spl, &node),
POSITIVE_INFINITY_KEY)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So with your above proposal, this should be the max key for the compaction, right?

< 0;
if (!should_continue && num_replacements != 0 && pack_req.num_tuples != 0)
{
key max_key = trunk_max_key(spl, &node);
Expand All @@ -5387,23 +5391,18 @@ trunk_compact_bundle(void *arg, void *scratch_buf)

debug_assert(trunk_verify_node(spl, &node));

// garbage collect the old path and bundle
trunk_garbage_collect_bundle(spl, old_root_addr, req);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the garbage collection should happen before we update the start key of the req. Otherwise we will garbage-collect the successor node. Confirmed that, in the original order, this caused a crash.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that makes sense.


if (should_continue) {
debug_assert(height != 0);
key_buffer_copy_key(&req->start_key, trunk_max_key(spl, &node));
}

// garbage collect the old path and bundle
trunk_garbage_collect_bundle(spl, old_root_addr, req);

// only release locks on node after the garbage collection is complete
trunk_node_unlock(spl->cc, &node);
trunk_node_unclaim(spl->cc, &node);
trunk_node_unget(spl->cc, &node);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not needed -- we get the node at the top of the loop.

(In fact, this causes a deterministic lockup.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeesh

if (should_continue) {
rc = trunk_compact_bundle_node_get(spl, req, &node);
platform_assert_status_ok(rc);
}
}

if (spl->cfg.use_stats) {
Expand Down