Skip to content

salad: reserve block before bps_tree_insert_first_elem#11857

Merged
sergepetrenko merged 1 commit intotarantool:masterfrom
Astronomax:gh-11788-sigsegv-in-bps_tree_memtx_tree_garbage_pop
Oct 7, 2025
Merged

salad: reserve block before bps_tree_insert_first_elem#11857
sergepetrenko merged 1 commit intotarantool:masterfrom
Astronomax:gh-11788-sigsegv-in-bps_tree_memtx_tree_garbage_pop

Conversation

@Astronomax
Copy link
Contributor

@Astronomax Astronomax commented Sep 25, 2025

The commit 51c56d9 ("salad: reserve extents for matras_touch on BPS tree operations") already made the tree self-sufficient - reserved blocks in garbage and extents for matras_touch calls prior to insertion and deletion. However, it didn't account for the addition of the first element to the tree, which creates the root leaf - this could also trigger matras_touch in bps_tree_garbage_pop.

However, it turns out there is no need to touch blocks before taking them out of the garbage. There is an invariant that blocks in the garbage are definitely not used by any read views. Although from the point of view of the matras, they require copying and could potentially have been needed by these read views. Let's just remove this matras_touch from bps_tree_garbage_pop. Also let's reserve a block in the garbage before calling bps_tree_insert_first_elem to bring the tree even closer to self-sufficiency.

Closes #11788

NO_DOC=bugfix

@Astronomax Astronomax requested a review from a team as a code owner September 25, 2025 12:54
@coveralls
Copy link

coveralls commented Sep 25, 2025

Coverage Status

coverage: 87.604% (-0.02%) from 87.62%
when pulling bf4bc6a on Astronomax:gh-11788-sigsegv-in-bps_tree_memtx_tree_garbage_pop
into 309409e
on tarantool:master
.

@Astronomax Astronomax force-pushed the gh-11788-sigsegv-in-bps_tree_memtx_tree_garbage_pop branch 3 times, most recently from 0aa3f48 to 6baf87e Compare September 26, 2025 06:12
@Astronomax Astronomax requested a review from nshy September 26, 2025 06:14
@nshy nshy assigned Astronomax and unassigned nshy Sep 26, 2025
@Astronomax Astronomax force-pushed the gh-11788-sigsegv-in-bps_tree_memtx_tree_garbage_pop branch 3 times, most recently from 6047faa to 42c9e5d Compare September 28, 2025 19:32
@Astronomax Astronomax changed the title salad: reserve extents for matras_touch in bps_tree_insert_first_elem salad: reserve blocks before bps_tree_insert_first_elem Sep 28, 2025
@Astronomax Astronomax force-pushed the gh-11788-sigsegv-in-bps_tree_memtx_tree_garbage_pop branch from 42c9e5d to 6bb2426 Compare September 28, 2025 19:48
@Astronomax Astronomax changed the title salad: reserve blocks before bps_tree_insert_first_elem salad: reserve block before bps_tree_insert_first_elem Sep 29, 2025
The commit 51c56d9 ("salad: reserve extents for matras_touch on BPS
tree operations") already made the tree self-sufficient - reserved
blocks in garbage and extents for `matras_touch` calls prior to insertion
and deletion. However, it didn't account for the addition of the first
element to the tree, which creates the root leaf - this could also trigger
`matras_touch` in `bps_tree_garbage_pop`.

However, it turns out there is no need to touch blocks before taking them
out of the garbage. There is an invariant that blocks in the garbage are
definitely not used by any read views. Although from the point of view of
the matras, they require copying and could potentially have been needed by
these read views. Let's just remove this `matras_touch` from
`bps_tree_garbage_pop`. Also let's reserve a block in the garbage before
calling `bps_tree_insert_first_elem` to bring the tree even closer to
self-sufficiency.

Closes tarantool#11788

NO_DOC=bugfix
@Astronomax Astronomax force-pushed the gh-11788-sigsegv-in-bps_tree_memtx_tree_garbage_pop branch from 6bb2426 to bf4bc6a Compare September 29, 2025 07:52
@Astronomax Astronomax requested a review from nshy September 29, 2025 08:34
@Astronomax Astronomax assigned nshy and unassigned Astronomax Sep 29, 2025
@nshy nshy assigned mkostoevr and unassigned nshy Sep 29, 2025
@nshy nshy requested a review from mkostoevr September 29, 2025 10:13
Copy link
Contributor

@mkostoevr mkostoevr left a comment

Choose a reason for hiding this comment

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

Nice solution! Also, a good step towards #10322.

@mkostoevr mkostoevr assigned Astronomax and unassigned mkostoevr Sep 30, 2025
@sergepetrenko sergepetrenko added full-ci Enables all tests for a pull request backport/3.3 Automatically create a 3.3 backport PR backport/3.4 Automatically create a 3.4 backport PR backport/3.5 Automatically create a 3.5 backport PR and removed full-ci Enables all tests for a pull request labels Oct 2, 2025
@sergepetrenko sergepetrenko merged commit 52b3d4b into tarantool:master Oct 7, 2025
62 of 83 checks passed
@TarantoolBot
Copy link
Collaborator

Successfully created backport PR for release/3.3:

@TarantoolBot
Copy link
Collaborator

Successfully created backport PR for release/3.4:

@TarantoolBot
Copy link
Collaborator

Successfully created backport PR for release/3.5:

@TarantoolBot
Copy link
Collaborator

Backport summary

mkostoevr added a commit to mkostoevr/tarantool that referenced this pull request Oct 24, 2025
We've removed CoW'ing of garbage blocks for creating new inner/leaf
blocks in commit 05f708ba6b17b1e7e9a269131139d98c9debcb59 ("salad:
reserve block before bps_vec_insert_first_elem") as the blocks are
not required to be preserved in views and can be used without CoW.

The problem is that in some circumstances this approach can lead to
a tricky problem: if we've created a block, CoW'ed another one and
then update values of the created one, the new block could've been
CoW'ed by the previous `matras_touch` (as the blocks are located in
extents of greater size than the blocks themselves). So following
updates in the created block gone into the view while the current
matras head had the old view's values remained.

The problem is that if we don't touch the garbage on block creation,
we need to make sure there's no any `matras_touch` between the block
creation and the last assignment of its field. Let's fix the only
place where the guarantee is not preserved: the leaf insert routine.

Removed the target leaf touch from the `bps_tree_process_insert_leaf`
by the way as it's not required cause the leaf is touched on lookup.

Follows-up tarantool#11857
Closes tarantool#11979

NO_DOC=bugfix
NO_CHANGELOG=was not released
mkostoevr added a commit to mkostoevr/tarantool that referenced this pull request Oct 24, 2025
We've removed CoW'ing of garbage blocks for creating new inner/leaf
blocks in commit 52b3d4b ("salad:
reserve block before bps_tree_insert_first_elem") as the blocks are
not required to be preserved in views and can be used without CoW.

The problem is that in some circumstances this approach can lead to
a tricky problem: if we've created a block, CoW'ed another one and
then update values of the created one, the new block could've been
CoW'ed by the previous `matras_touch` (as the blocks are located in
extents of greater size than the blocks themselves). So following
updates in the created block gone into the view while the current
matras head had the old view's values remained.

The problem is that if we don't touch the garbage on block creation,
we need to make sure there's no any `matras_touch` between the block
creation and the last assignment of its field. Let's fix the only
place where the guarantee is not preserved: the leaf insert routine.

Removed the target leaf touch from the `bps_tree_process_insert_leaf`
by the way as it's not required cause the leaf is touched on lookup.

Follows-up tarantool#11857
Closes tarantool#11979

NO_DOC=bugfix
NO_CHANGELOG=was not released
mkostoevr added a commit to mkostoevr/tarantool that referenced this pull request Oct 24, 2025
We've removed CoW'ing of garbage blocks for creating new inner/leaf
blocks in commit 52b3d4b ("salad:
reserve block before bps_tree_insert_first_elem") as the blocks are
not required to be preserved in views and can be used without CoW.

The problem is that in some circumstances this approach can lead to
a tricky problem: if we've created a block, CoW'ed another one and
then update values of the created one, the new block could've been
CoW'ed by the previous `matras_touch` (as the blocks are located in
extents of greater size than the blocks themselves). So following
updates in the created block gone into the view while the current
matras head had the old view's values remained.

The problem is that if we don't touch the garbage on block creation,
we need to make sure there's no any `matras_touch` between the block
creation and the last assignment of its field. We could fix the only
place where the guarantee is not preserved: the leaf insert routine,
but let's keep the tree similar to the BPS vector and simply revert
the change and touch poped garbage blocks too.

Since now the garbage pop invokes CoW, we might need to reserve more
memory for touches, as some new garbage blocks can be allocated on the
`bps_vec_reserve_blocks`, but other ones can be existing in the vector
previously, and, for these ones, we must reserve touches. Also, one
more touch might be required if a new block allocated on reserve.

Removed the target leaf touch from the `bps_tree_process_insert_leaf`
by the way as it's not required cause the leaf is touched on lookup.

Follows-up tarantool#11857
Closes tarantool#11979

NO_DOC=bugfix
NO_CHANGELOG=was not released
mkostoevr added a commit to mkostoevr/tarantool that referenced this pull request Oct 24, 2025
We've removed CoW'ing of garbage blocks for creating new inner/leaf
blocks in commit 52b3d4b ("salad:
reserve block before bps_tree_insert_first_elem") as the blocks are
not required to be preserved in views and can be used without CoW.

The problem is that in some circumstances this approach can lead to
a tricky problem: if we've created a block, CoW'ed another one and
then update values of the created one, the new block could've been
CoW'ed by the previous `matras_touch` (as the blocks are located in
extents of greater size than the blocks themselves). So following
updates in the created block gone into the view while the current
matras head had the old view's values remained.

The problem is that if we don't touch the garbage on block creation,
we need to make sure there's no any `matras_touch` between the block
creation and the last assignment of its field. We could fix the only
place where the guarantee is not preserved: the leaf insert routine,
but let's keep the tree similar to the BPS vector and simply revert
the change and touch popped garbage blocks too.

Since now the garbage pop invokes CoW, we might need to reserve more
memory for touches, as some new garbage blocks can be allocated on the
`bps_vec_reserve_blocks`, but other ones can be existing in the vector
previously, and, for these ones, we must reserve touches. Also, one
more touch might be required if a new block allocated on reserve.

Removed the target leaf touch from the `bps_tree_process_insert_leaf`
by the way as it's not required cause the leaf is touched on lookup.

Follows-up tarantool#11857
Closes tarantool#11979

NO_DOC=bugfix
NO_CHANGELOG=was not released
mkostoevr added a commit to mkostoevr/tarantool that referenced this pull request Oct 28, 2025
We've removed CoW'ing of garbage blocks for creating new inner/leaf
blocks in commit 52b3d4b ("salad:
reserve block before bps_tree_insert_first_elem") as the blocks are
not required to be preserved in views and can be used without CoW.

The problem is that in some circumstances this approach can lead to
a tricky problem: if we've created a block, CoW'ed another one and
then update values of the created one, the new block could've been
CoW'ed by the previous `matras_touch` (as the blocks are located in
extents of greater size than the blocks themselves). So following
updates in the created block gone into the view while the current
matras head had the old view's values remained.

The problem is that if we don't touch the garbage on block creation,
we need to make sure there's no any `matras_touch` between the block
creation and the last assignment of its field. We could fix the only
place where the guarantee is not preserved: the leaf insert routine,
but let's keep the tree similar to the BPS vector and simply revert
the change and touch popped garbage blocks too.

Since now the garbage pop invokes CoW, we might need to reserve more
memory for touches, as some new garbage blocks can be allocated on the
`bps_vec_reserve_blocks`, but other ones can be existing in the vector
previously, and, for these ones, we must reserve touches. Also, one
more touch might be required if a new block allocated on reserve.

Removed the target leaf touch from the `bps_tree_process_insert_leaf`
by the way as it's not required cause the leaf is touched on lookup.

Follows-up tarantool#11857
Closes tarantool#11979

NO_DOC=bugfix
NO_CHANGELOG=was not released
sergepetrenko pushed a commit that referenced this pull request Oct 31, 2025
We've removed CoW'ing of garbage blocks for creating new inner/leaf
blocks in commit 52b3d4b ("salad:
reserve block before bps_tree_insert_first_elem") as the blocks are
not required to be preserved in views and can be used without CoW.

The problem is that in some circumstances this approach can lead to
a tricky problem: if we've created a block, CoW'ed another one and
then update values of the created one, the new block could've been
CoW'ed by the previous `matras_touch` (as the blocks are located in
extents of greater size than the blocks themselves). So following
updates in the created block gone into the view while the current
matras head had the old view's values remained.

The problem is that if we don't touch the garbage on block creation,
we need to make sure there's no any `matras_touch` between the block
creation and the last assignment of its field. We could fix the only
place where the guarantee is not preserved: the leaf insert routine,
but let's keep the tree similar to the BPS vector and simply revert
the change and touch popped garbage blocks too.

Since now the garbage pop invokes CoW, we might need to reserve more
memory for touches, as some new garbage blocks can be allocated on the
`bps_vec_reserve_blocks`, but other ones can be existing in the vector
previously, and, for these ones, we must reserve touches. Also, one
more touch might be required if a new block allocated on reserve.

Removed the target leaf touch from the `bps_tree_process_insert_leaf`
by the way as it's not required cause the leaf is touched on lookup.

Follows-up #11857
Closes #11979

NO_DOC=bugfix
NO_CHANGELOG=was not released
github-actions bot pushed a commit that referenced this pull request Oct 31, 2025
We've removed CoW'ing of garbage blocks for creating new inner/leaf
blocks in commit 52b3d4b ("salad:
reserve block before bps_tree_insert_first_elem") as the blocks are
not required to be preserved in views and can be used without CoW.

The problem is that in some circumstances this approach can lead to
a tricky problem: if we've created a block, CoW'ed another one and
then update values of the created one, the new block could've been
CoW'ed by the previous `matras_touch` (as the blocks are located in
extents of greater size than the blocks themselves). So following
updates in the created block gone into the view while the current
matras head had the old view's values remained.

The problem is that if we don't touch the garbage on block creation,
we need to make sure there's no any `matras_touch` between the block
creation and the last assignment of its field. We could fix the only
place where the guarantee is not preserved: the leaf insert routine,
but let's keep the tree similar to the BPS vector and simply revert
the change and touch popped garbage blocks too.

Since now the garbage pop invokes CoW, we might need to reserve more
memory for touches, as some new garbage blocks can be allocated on the
`bps_vec_reserve_blocks`, but other ones can be existing in the vector
previously, and, for these ones, we must reserve touches. Also, one
more touch might be required if a new block allocated on reserve.

Removed the target leaf touch from the `bps_tree_process_insert_leaf`
by the way as it's not required cause the leaf is touched on lookup.

Follows-up #11857
Closes #11979

NO_DOC=bugfix
NO_CHANGELOG=was not released

(cherry picked from commit 24cdd89)
github-actions bot pushed a commit that referenced this pull request Oct 31, 2025
We've removed CoW'ing of garbage blocks for creating new inner/leaf
blocks in commit 52b3d4b ("salad:
reserve block before bps_tree_insert_first_elem") as the blocks are
not required to be preserved in views and can be used without CoW.

The problem is that in some circumstances this approach can lead to
a tricky problem: if we've created a block, CoW'ed another one and
then update values of the created one, the new block could've been
CoW'ed by the previous `matras_touch` (as the blocks are located in
extents of greater size than the blocks themselves). So following
updates in the created block gone into the view while the current
matras head had the old view's values remained.

The problem is that if we don't touch the garbage on block creation,
we need to make sure there's no any `matras_touch` between the block
creation and the last assignment of its field. We could fix the only
place where the guarantee is not preserved: the leaf insert routine,
but let's keep the tree similar to the BPS vector and simply revert
the change and touch popped garbage blocks too.

Since now the garbage pop invokes CoW, we might need to reserve more
memory for touches, as some new garbage blocks can be allocated on the
`bps_vec_reserve_blocks`, but other ones can be existing in the vector
previously, and, for these ones, we must reserve touches. Also, one
more touch might be required if a new block allocated on reserve.

Removed the target leaf touch from the `bps_tree_process_insert_leaf`
by the way as it's not required cause the leaf is touched on lookup.

Follows-up #11857
Closes #11979

NO_DOC=bugfix
NO_CHANGELOG=was not released

(cherry picked from commit 24cdd89)
github-actions bot pushed a commit that referenced this pull request Oct 31, 2025
We've removed CoW'ing of garbage blocks for creating new inner/leaf
blocks in commit 52b3d4b ("salad:
reserve block before bps_tree_insert_first_elem") as the blocks are
not required to be preserved in views and can be used without CoW.

The problem is that in some circumstances this approach can lead to
a tricky problem: if we've created a block, CoW'ed another one and
then update values of the created one, the new block could've been
CoW'ed by the previous `matras_touch` (as the blocks are located in
extents of greater size than the blocks themselves). So following
updates in the created block gone into the view while the current
matras head had the old view's values remained.

The problem is that if we don't touch the garbage on block creation,
we need to make sure there's no any `matras_touch` between the block
creation and the last assignment of its field. We could fix the only
place where the guarantee is not preserved: the leaf insert routine,
but let's keep the tree similar to the BPS vector and simply revert
the change and touch popped garbage blocks too.

Since now the garbage pop invokes CoW, we might need to reserve more
memory for touches, as some new garbage blocks can be allocated on the
`bps_vec_reserve_blocks`, but other ones can be existing in the vector
previously, and, for these ones, we must reserve touches. Also, one
more touch might be required if a new block allocated on reserve.

Removed the target leaf touch from the `bps_tree_process_insert_leaf`
by the way as it's not required cause the leaf is touched on lookup.

Follows-up #11857
Closes #11979

NO_DOC=bugfix
NO_CHANGELOG=was not released

(cherry picked from commit 24cdd89)
sergepetrenko pushed a commit that referenced this pull request Oct 31, 2025
We've removed CoW'ing of garbage blocks for creating new inner/leaf
blocks in commit 52b3d4b ("salad:
reserve block before bps_tree_insert_first_elem") as the blocks are
not required to be preserved in views and can be used without CoW.

The problem is that in some circumstances this approach can lead to
a tricky problem: if we've created a block, CoW'ed another one and
then update values of the created one, the new block could've been
CoW'ed by the previous `matras_touch` (as the blocks are located in
extents of greater size than the blocks themselves). So following
updates in the created block gone into the view while the current
matras head had the old view's values remained.

The problem is that if we don't touch the garbage on block creation,
we need to make sure there's no any `matras_touch` between the block
creation and the last assignment of its field. We could fix the only
place where the guarantee is not preserved: the leaf insert routine,
but let's keep the tree similar to the BPS vector and simply revert
the change and touch popped garbage blocks too.

Since now the garbage pop invokes CoW, we might need to reserve more
memory for touches, as some new garbage blocks can be allocated on the
`bps_vec_reserve_blocks`, but other ones can be existing in the vector
previously, and, for these ones, we must reserve touches. Also, one
more touch might be required if a new block allocated on reserve.

Removed the target leaf touch from the `bps_tree_process_insert_leaf`
by the way as it's not required cause the leaf is touched on lookup.

Follows-up #11857
Closes #11979

NO_DOC=bugfix
NO_CHANGELOG=was not released

(cherry picked from commit 24cdd89)
sergepetrenko pushed a commit that referenced this pull request Oct 31, 2025
We've removed CoW'ing of garbage blocks for creating new inner/leaf
blocks in commit 52b3d4b ("salad:
reserve block before bps_tree_insert_first_elem") as the blocks are
not required to be preserved in views and can be used without CoW.

The problem is that in some circumstances this approach can lead to
a tricky problem: if we've created a block, CoW'ed another one and
then update values of the created one, the new block could've been
CoW'ed by the previous `matras_touch` (as the blocks are located in
extents of greater size than the blocks themselves). So following
updates in the created block gone into the view while the current
matras head had the old view's values remained.

The problem is that if we don't touch the garbage on block creation,
we need to make sure there's no any `matras_touch` between the block
creation and the last assignment of its field. We could fix the only
place where the guarantee is not preserved: the leaf insert routine,
but let's keep the tree similar to the BPS vector and simply revert
the change and touch popped garbage blocks too.

Since now the garbage pop invokes CoW, we might need to reserve more
memory for touches, as some new garbage blocks can be allocated on the
`bps_vec_reserve_blocks`, but other ones can be existing in the vector
previously, and, for these ones, we must reserve touches. Also, one
more touch might be required if a new block allocated on reserve.

Removed the target leaf touch from the `bps_tree_process_insert_leaf`
by the way as it's not required cause the leaf is touched on lookup.

Follows-up #11857
Closes #11979

NO_DOC=bugfix
NO_CHANGELOG=was not released

(cherry picked from commit 24cdd89)
sergepetrenko pushed a commit that referenced this pull request Oct 31, 2025
We've removed CoW'ing of garbage blocks for creating new inner/leaf
blocks in commit 52b3d4b ("salad:
reserve block before bps_tree_insert_first_elem") as the blocks are
not required to be preserved in views and can be used without CoW.

The problem is that in some circumstances this approach can lead to
a tricky problem: if we've created a block, CoW'ed another one and
then update values of the created one, the new block could've been
CoW'ed by the previous `matras_touch` (as the blocks are located in
extents of greater size than the blocks themselves). So following
updates in the created block gone into the view while the current
matras head had the old view's values remained.

The problem is that if we don't touch the garbage on block creation,
we need to make sure there's no any `matras_touch` between the block
creation and the last assignment of its field. We could fix the only
place where the guarantee is not preserved: the leaf insert routine,
but let's keep the tree similar to the BPS vector and simply revert
the change and touch popped garbage blocks too.

Since now the garbage pop invokes CoW, we might need to reserve more
memory for touches, as some new garbage blocks can be allocated on the
`bps_vec_reserve_blocks`, but other ones can be existing in the vector
previously, and, for these ones, we must reserve touches. Also, one
more touch might be required if a new block allocated on reserve.

Removed the target leaf touch from the `bps_tree_process_insert_leaf`
by the way as it's not required cause the leaf is touched on lookup.

Follows-up #11857
Closes #11979

NO_DOC=bugfix
NO_CHANGELOG=was not released

(cherry picked from commit 24cdd89)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport/3.3 Automatically create a 3.3 backport PR backport/3.4 Automatically create a 3.4 backport PR backport/3.5 Automatically create a 3.5 backport PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

SIGSEGV in bps_tree_memtx_tree_garbage_pop

7 participants