From 3613b082bcbb2dffc80236b4082a1c5ca7e23cf7 Mon Sep 17 00:00:00 2001 From: "Raphael S. Carvalho" Date: Thu, 25 Feb 2021 14:26:41 -0300 Subject: [PATCH] compaction: Prevent cleanup and regular from compacting the same sstable Due to regression introduced by 463d0ab, regular can compact in parallel a sstable being compacted by cleanup, scrub or upgrade. This redundancy causes resources to be wasted, write amplification is increased and so does the operation time, etc. That's a potential source of data resurrection because the now-owned data from a sstable being compacted by both cleanup and regular will still exist in the node afterwards, so resurrection can happen if node regains ownership. Fixes #8155. Signed-off-by: Raphael S. Carvalho Message-Id: <20210225172641.787022-1-raphaelsc@scylladb.com> (cherry picked from commit 2cf0c4bbf1c48dad147c347b4c25de476b9f009b) Includes fixup patch: compaction_manager: Fix use-after-free in rewrite_sstables() Use-after-free introduced by 2cf0c4bbf1c. That's because compacting is moved into then_wrapped() lambda, so it's potentially freed on the next iteration of repeat(). Signed-off-by: Raphael S. Carvalho Message-Id: <20210309232940.433490-1-raphaelsc@scylladb.com> (cherry picked from commit f7cc431477e8f59d8a45d29d4539bdafcceee4c9) --- sstables/compaction_manager.cc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/sstables/compaction_manager.cc b/sstables/compaction_manager.cc index a959837000..61a7a842a0 100644 --- a/sstables/compaction_manager.cc +++ b/sstables/compaction_manager.cc @@ -629,10 +629,11 @@ future<> compaction_manager::rewrite_sstables(column_family* cf, sstables::compa _tasks.push_back(task); auto sstables = std::make_unique>(get_func(*cf)); + auto compacting = make_lw_shared(this, *sstables); auto sstables_ptr = sstables.get(); _stats.pending_tasks += sstables->size(); - task->compaction_done = do_until([sstables_ptr] { return sstables_ptr->empty(); }, [this, task, options, sstables_ptr] () mutable { + task->compaction_done = do_until([sstables_ptr] { return sstables_ptr->empty(); }, [this, task, options, sstables_ptr, compacting] () mutable { // FIXME: lock cf here if (!can_proceed(task)) { @@ -642,7 +643,7 @@ future<> compaction_manager::rewrite_sstables(column_family* cf, sstables::compa auto sst = sstables_ptr->back(); sstables_ptr->pop_back(); - return repeat([this, task, options, sst = std::move(sst)] () mutable { + return repeat([this, task, options, sst = std::move(sst), compacting] () mutable { column_family& cf = *task->compacting_cf; auto sstable_level = sst->get_sstable_level(); auto run_identifier = sst->run_identifier(); @@ -650,7 +651,6 @@ future<> compaction_manager::rewrite_sstables(column_family* cf, sstables::compa auto descriptor = sstables::compaction_descriptor({ sst }, cf.get_sstable_set(), service::get_local_compaction_priority(), sstable_level, sstables::compaction_descriptor::default_max_sstable_bytes, run_identifier, options); - auto compacting = make_lw_shared(this, descriptor.sstables); // Releases reference to cleaned sstable such that respective used disk space can be freed. descriptor.release_exhausted = [compacting] (const std::vector& exhausted_sstables) { compacting->release_compacting(exhausted_sstables); @@ -664,7 +664,7 @@ future<> compaction_manager::rewrite_sstables(column_family* cf, sstables::compa return with_scheduling_group(_scheduling_group, [this, &cf, descriptor = std::move(descriptor)] () mutable { return cf.run_compaction(std::move(descriptor)); }); - }).then_wrapped([this, task, compacting = std::move(compacting)] (future<> f) mutable { + }).then_wrapped([this, task, compacting] (future<> f) mutable { task->compaction_running = false; _stats.active_tasks--; if (!can_proceed(task)) {