mirrors/scylladb
1
0
Fork 0
mirror of https://github.com/scylladb/scylladb.git synced 2026-04-22 01:20:39 +00:00
Code Issues Packages Projects Releases Wiki Activity

commitlog: Ensure segment deletion is re-entrant

Browse Source 
Fixes #25709

If we have large allocations, spanning more than one segment, and
the internal segment references from lead to secondary are the
only thing keeping a segment alive, the implicit drop in
discard_unused_segments and orphan_all can cause a recursive call
to discard_unused_segments, which in turn can lead to vector
corruption/crash, or even double free of segment (iterator confusion).

Need to separate the modification of the vector (_segments) from
actual releasing of objects. Using temporaries is the easiest
solution.

To further reduce recursion, we can also do an early clear of
segment dependencies in callbacks from segment release (cf release).

Closes scylladb/scylladb#25719

(cherry picked from commit cc9eb321a1)

Closes scylladb/scylladb#25755
This commit is contained in:
Calle Wilund
2025-08-28 07:48:42 +00:00
committed by Avi Kivity
parent 1ee00069e7
commit a2bc1a7c6b
2 changed files with 72 additions and 15 deletions
Download Patch File Download Diff File Expand all files Collapse all files

41
db/commitlog/commitlog.cc
View File

@@ -800,6 +800,8 @@ class db::commitlog::segment : public enable_shared_from_this<segment>, public c
void end_flush() {
_segment_manager->end_flush();
if (can_delete()) {
// #25709 - do this early if possible
_extended_segments.clear();
_segment_manager->discard_unused_segments();
}
}
@@ -875,6 +877,8 @@ public:
void release_cf_count(const cf_id_type& cf) {
mark_clean(cf, 1);
if (can_delete()) {
// #25709 - do this early if possible
_extended_segments.clear();
_segment_manager->discard_unused_segments();
}
}
@@ -2576,20 +2580,24 @@ struct fmt::formatter<db::commitlog::segment::cf_mark> {
void db::commitlog::segment_manager::discard_unused_segments() noexcept {
clogger.trace("Checking for unused segments ({} active)", _segments.size());
std::erase_if(_segments, [=](sseg_ptr s) {
if (s->can_delete()) {
clogger.debug("Segment {} is unused", *s);
return true;
}
if (s->is_still_allocating()) {
clogger.debug("Not safe to delete segment {}; still allocating.", *s);
} else if (!s->is_clean()) {
clogger.debug("Not safe to delete segment {}; dirty is {}", *s, segment::cf_mark {*s});
} else {
clogger.debug("Not safe to delete segment {}; disk ops pending", *s);
}
return false;
});
// #25709 ensure we don't free any segment until after prune.
{
auto tmp = _segments;
std::erase_if(_segments, [=](sseg_ptr s) {
if (s->can_delete()) {
clogger.debug("Segment {} is unused", *s);
return true;
}
if (s->is_still_allocating()) {
clogger.debug("Not safe to delete segment {}; still allocating.", *s);
} else if (!s->is_clean()) {
clogger.debug("Not safe to delete segment {}; dirty is {}", *s, segment::cf_mark {*s});
} else {
clogger.debug("Not safe to delete segment {}; disk ops pending", *s);
}
return false;
});
}
// launch in background, but guard with gate so this deletion is
// sure to finish in shutdown, because at least through this path,
@@ -2878,7 +2886,10 @@ future<> db::commitlog::segment_manager::do_pending_deletes() {
}
future<> db::commitlog::segment_manager::orphan_all() {
_segments.clear();
// #25709. the actual process of destroying the elements here
// might cause a call into discard_unused_segments.
// ensure the target vector is empty when we get to destructors
auto tmp = std::exchange(_segments, {});
return clear_reserve_segments();
}