From 8e496a2f2febcaf4ea3cedddceb8fca4ab801c49 Mon Sep 17 00:00:00 2001 From: Pavel Emelyanov Date: Thu, 25 Dec 2025 10:53:53 +0300 Subject: [PATCH 1/4] sstables: Introduce storage::clone() And call it from sstable::clone() instead of storage::snapshot(). The snapshot arguements are: - target directory is storage::prefix(), that's _dir itself - new generation is always provided, no need for optional - leave_unsealed bool flag With that, the implementation of filesystem_storage::clone() is as simple as call create_links_common() forwarding args and _dir to it. The unification of leave_unsealed branches will come a bit later making this code even shorter. Signed-off-by: Pavel Emelyanov --- sstables/sstables.cc | 2 +- sstables/storage.cc | 14 ++++++++++++++ sstables/storage.hh | 1 + 3 files changed, 16 insertions(+), 1 deletion(-) diff --git a/sstables/sstables.cc b/sstables/sstables.cc index 1c919253ad..a34eca1b88 100644 --- a/sstables/sstables.cc +++ b/sstables/sstables.cc @@ -902,7 +902,7 @@ future> sstable::readable_file_for_all_ } future sstable::clone(generation_type new_generation, bool leave_unsealed) const { - co_await _storage->snapshot(*this, _storage->prefix(), storage::absolute_path::yes, new_generation, storage::leave_unsealed(leave_unsealed)); + co_await _storage->clone(*this, new_generation, leave_unsealed); co_return entry_descriptor(new_generation, _version, _format, component_type::TOC, _state); } diff --git a/sstables/storage.cc b/sstables/storage.cc index 1846d9af27..378500e36a 100644 --- a/sstables/storage.cc +++ b/sstables/storage.cc @@ -93,6 +93,7 @@ public: virtual future<> seal(const sstable& sst) override; virtual future<> snapshot(const sstable& sst, sstring dir, absolute_path abs, std::optional gen, storage::leave_unsealed) const override; + virtual future<> clone(const sstable& sst, generation_type gen, bool leave_unsealed) const override; virtual future<> change_state(const sstable& sst, sstable_state state, generation_type generation, delayed_commit_changes* delay) override; // runs in async context virtual void open(sstable& sst) override; @@ -438,6 +439,13 @@ future<> filesystem_storage::snapshot(const sstable& sst, sstring dir, absolute_ co_await create_links_common(sst, snapshot_dir, std::move(gen)); } } +future<> filesystem_storage::clone(const sstable& sst, generation_type gen, bool leave_unsealed) const { + if (leave_unsealed) { + co_await create_links_common(sst, _dir.path(), std::move(gen), leave_unsealed_tag{}); + } else { + co_await create_links_common(sst, _dir.path(), std::move(gen)); + } +} future<> filesystem_storage::move(const sstable& sst, sstring new_dir, generation_type new_generation, delayed_commit_changes* delay_commit) { co_await touch_directory(new_dir); @@ -630,6 +638,7 @@ public: future<> seal(const sstable& sst) override; future<> snapshot(const sstable& sst, sstring dir, absolute_path abs, std::optional, storage::leave_unsealed) const override; + future<> clone(const sstable& sst, generation_type gen, bool leave_unsealed) const override; future<> change_state(const sstable& sst, sstable_state state, generation_type generation, delayed_commit_changes* delay) override; // runs in async context void open(sstable& sst) override; @@ -851,6 +860,11 @@ future<> object_storage_base::snapshot(const sstable& sst, sstring dir, absolute co_return; } +future<> object_storage_base::clone(const sstable& sst, generation_type gen, bool leave_unsealed) const { + on_internal_error(sstlog, "Cloning S3 objects not implemented"); + co_return; +} + std::unique_ptr make_storage(sstables_manager& manager, const data_dictionary::storage_options& s_opts, sstable_state state) { return std::visit(overloaded_functor { [state] (const data_dictionary::storage_options::local& loc) mutable -> std::unique_ptr { diff --git a/sstables/storage.hh b/sstables/storage.hh index 569d6430e5..0c7f72a037 100644 --- a/sstables/storage.hh +++ b/sstables/storage.hh @@ -101,6 +101,7 @@ public: virtual future<> seal(const sstable& sst) = 0; virtual future<> snapshot(const sstable& sst, sstring dir, absolute_path abs, std::optional gen = {}, leave_unsealed lu = leave_unsealed::no) const = 0; + virtual future<> clone(const sstable& sst, generation_type gen, bool leave_unsealed) const = 0; virtual future<> change_state(const sstable& sst, sstable_state to, generation_type generation, delayed_commit_changes* delay) = 0; // runs in async context virtual void open(sstable& sst) = 0; From 32cf358f44afd6ba287791e1f0fbd8e3390dcd19 Mon Sep 17 00:00:00 2001 From: Pavel Emelyanov Date: Thu, 25 Dec 2025 10:57:44 +0300 Subject: [PATCH 2/4] sstable: Simplify storage::snapshot() Now there are only two callers left -- sstable::snapshot() and sstable::seal() that wants to auto-backup the sealed sstable. The snapshot arguments are: - relative path, use _base_dir - no new generation provided - no leave-unsealed tag With that, the implementation of filesystem_storage::snapshot() is as simple as - prepare full path relative to _base_dir - touch new directory - call create_links_common() Signed-off-by: Pavel Emelyanov --- sstables/sstables.cc | 4 ++-- sstables/storage.cc | 21 ++++++--------------- sstables/storage.hh | 2 +- 3 files changed, 9 insertions(+), 18 deletions(-) diff --git a/sstables/sstables.cc b/sstables/sstables.cc index a34eca1b88..9abaa97dc9 100644 --- a/sstables/sstables.cc +++ b/sstables/sstables.cc @@ -2123,7 +2123,7 @@ future<> sstable::seal_sstable(bool backup) _marked_for_deletion = mark_for_deletion::none; } if (backup) { - co_await _storage->snapshot(*this, "backups", storage::absolute_path::no); + co_await _storage->snapshot(*this, "backups"); } } @@ -2491,7 +2491,7 @@ std::vector> sstable::all_components() const future<> sstable::snapshot(const sstring& name) const { auto lock = co_await get_units(_mutate_sem, 1); - co_await _storage->snapshot(*this, format("{}/{}", sstables::snapshots_dir, name), storage::absolute_path::no); + co_await _storage->snapshot(*this, format("{}/{}", sstables::snapshots_dir, name)); } future<> sstable::change_state(sstable_state to, delayed_commit_changes* delay_commit) { diff --git a/sstables/storage.cc b/sstables/storage.cc index 378500e36a..6cdc739c33 100644 --- a/sstables/storage.cc +++ b/sstables/storage.cc @@ -92,7 +92,7 @@ public: {} virtual future<> seal(const sstable& sst) override; - virtual future<> snapshot(const sstable& sst, sstring dir, absolute_path abs, std::optional gen, storage::leave_unsealed) const override; + virtual future<> snapshot(const sstable& sst, sstring name) const override; virtual future<> clone(const sstable& sst, generation_type gen, bool leave_unsealed) const override; virtual future<> change_state(const sstable& sst, sstable_state state, generation_type generation, delayed_commit_changes* delay) override; // runs in async context @@ -425,19 +425,10 @@ future<> filesystem_storage::create_links(const sstable& sst, const std::filesys return create_links_common(sst, dir.native(), sst._generation, link_mode::default_mode); } -future<> filesystem_storage::snapshot(const sstable& sst, sstring dir, absolute_path abs, std::optional gen, storage::leave_unsealed leave_unsealed) const { - std::filesystem::path snapshot_dir; - if (abs) { - snapshot_dir = dir; - } else { - snapshot_dir = _base_dir.path() / dir; - } +future<> filesystem_storage::snapshot(const sstable& sst, sstring name) const { + std::filesystem::path snapshot_dir = _base_dir.path() / name; co_await sst.sstable_touch_directory_io_check(snapshot_dir); - if (leave_unsealed) { - co_await create_links_common(sst, snapshot_dir, std::move(gen), leave_unsealed_tag{}); - } else { - co_await create_links_common(sst, snapshot_dir, std::move(gen)); - } + co_await create_links_common(sst, snapshot_dir, {}); } future<> filesystem_storage::clone(const sstable& sst, generation_type gen, bool leave_unsealed) const { if (leave_unsealed) { @@ -637,7 +628,7 @@ public: {} future<> seal(const sstable& sst) override; - future<> snapshot(const sstable& sst, sstring dir, absolute_path abs, std::optional, storage::leave_unsealed) const override; + future<> snapshot(const sstable& sst, sstring name) const override; future<> clone(const sstable& sst, generation_type gen, bool leave_unsealed) const override; future<> change_state(const sstable& sst, sstable_state state, generation_type generation, delayed_commit_changes* delay) override; // runs in async context @@ -855,7 +846,7 @@ future<> object_storage_base::unlink_component(const sstable& sst, component_typ } } -future<> object_storage_base::snapshot(const sstable& sst, sstring dir, absolute_path abs, std::optional gen, storage::leave_unsealed) const { +future<> object_storage_base::snapshot(const sstable& sst, sstring name) const { on_internal_error(sstlog, "Snapshotting S3 objects not implemented"); co_return; } diff --git a/sstables/storage.hh b/sstables/storage.hh index 0c7f72a037..698cfba959 100644 --- a/sstables/storage.hh +++ b/sstables/storage.hh @@ -100,7 +100,7 @@ public: using leave_unsealed = bool_class; virtual future<> seal(const sstable& sst) = 0; - virtual future<> snapshot(const sstable& sst, sstring dir, absolute_path abs, std::optional gen = {}, leave_unsealed lu = leave_unsealed::no) const = 0; + virtual future<> snapshot(const sstable& sst, sstring name) const = 0; virtual future<> clone(const sstable& sst, generation_type gen, bool leave_unsealed) const = 0; virtual future<> change_state(const sstable& sst, sstable_state to, generation_type generation, delayed_commit_changes* delay) = 0; // runs in async context From 9e189da23a4312b05e8f940233fde15a99ffba61 Mon Sep 17 00:00:00 2001 From: Pavel Emelyanov Date: Thu, 25 Dec 2025 11:02:33 +0300 Subject: [PATCH 3/4] sstables/storage: Drop create_links_common() overloads There's a bunch of tagged create_links_common() overloads that call the most generic one with properly crafted arguments and the link_mode. Callers of those one-liners can craft the args themselves. As a result, there's only one create_links_common() overload and callers explicitly specifying what they want from it. Signed-off-by: Pavel Emelyanov --- sstables/storage.cc | 25 +++---------------------- 1 file changed, 3 insertions(+), 22 deletions(-) diff --git a/sstables/storage.cc b/sstables/storage.cc index 6cdc739c33..c69eb53041 100644 --- a/sstables/storage.cc +++ b/sstables/storage.cc @@ -69,9 +69,6 @@ private: future<> remove_temp_dir(); virtual future<> create_links(const sstable& sst, const std::filesystem::path& dir) const override; future<> create_links_common(const sstable& sst, sstring dst_dir, generation_type dst_gen, link_mode mode) const; - future<> create_links_common(const sstable& sst, sstring dst_dir, generation_type dst_gen, mark_for_removal_tag) const; - future<> create_links_common(const sstable& sst, const std::filesystem::path& dir, std::optional gen, leave_unsealed_tag) const; - future<> create_links_common(const sstable& sst, const std::filesystem::path& dir, std::optional dst_gen) const; future<> touch_temp_dir(const sstable& sst); future<> move(const sstable& sst, sstring new_dir, generation_type generation, delayed_commit_changes* delay) override; future<> rename_new_file(const sstable& sst, sstring from_name, sstring to_name) const; @@ -409,18 +406,6 @@ future<> filesystem_storage::create_links_common(const sstable& sst, sstring dst sstlog.trace("create_links: {} -> {} generation={}: done", sst.get_filename(), dst_dir, generation); } -future<> filesystem_storage::create_links_common(const sstable& sst, sstring dst_dir, generation_type dst_gen, mark_for_removal_tag) const { - return create_links_common(sst, dst_dir, dst_gen, link_mode::mark_for_removal); -} - -future<> filesystem_storage::create_links_common(const sstable& sst, const std::filesystem::path& dir, std::optional gen, leave_unsealed_tag) const { - return create_links_common(sst, dir.native(), gen.value_or(sst._generation), link_mode::leave_unsealed); -} - -future<> filesystem_storage::create_links_common(const sstable& sst, const std::filesystem::path& dir, std::optional gen) const { - return create_links_common(sst, dir.native(), gen.value_or(sst._generation), link_mode::default_mode); -} - future<> filesystem_storage::create_links(const sstable& sst, const std::filesystem::path& dir) const { return create_links_common(sst, dir.native(), sst._generation, link_mode::default_mode); } @@ -428,14 +413,10 @@ future<> filesystem_storage::create_links(const sstable& sst, const std::filesys future<> filesystem_storage::snapshot(const sstable& sst, sstring name) const { std::filesystem::path snapshot_dir = _base_dir.path() / name; co_await sst.sstable_touch_directory_io_check(snapshot_dir); - co_await create_links_common(sst, snapshot_dir, {}); + co_await create_links_common(sst, snapshot_dir.native(), sst._generation, link_mode::default_mode); } future<> filesystem_storage::clone(const sstable& sst, generation_type gen, bool leave_unsealed) const { - if (leave_unsealed) { - co_await create_links_common(sst, _dir.path(), std::move(gen), leave_unsealed_tag{}); - } else { - co_await create_links_common(sst, _dir.path(), std::move(gen)); - } + co_await create_links_common(sst, _dir.path().native(), std::move(gen), leave_unsealed ? link_mode::leave_unsealed : link_mode::default_mode); } future<> filesystem_storage::move(const sstable& sst, sstring new_dir, generation_type new_generation, delayed_commit_changes* delay_commit) { @@ -443,7 +424,7 @@ future<> filesystem_storage::move(const sstable& sst, sstring new_dir, generatio sstring old_dir = _dir.native(); sstlog.debug("Moving {} old_generation={} to {} new_generation={} do_sync_dirs={}", sst.get_filename(), sst._generation, new_dir, new_generation, delay_commit == nullptr); - co_await create_links_common(sst, new_dir, new_generation, mark_for_removal_tag{}); + co_await create_links_common(sst, new_dir, new_generation, link_mode::mark_for_removal); co_await change_dir(new_dir); generation_type old_generation = sst._generation; co_await coroutine::parallel_for_each(sst.all_components(), [&sst, old_generation, old_dir] (auto p) { From 712cc8b8f1c00279a59ddcc9890d12893d5537fd Mon Sep 17 00:00:00 2001 From: Pavel Emelyanov Date: Thu, 25 Dec 2025 11:06:06 +0300 Subject: [PATCH 4/4] sstables, storage: Drop unused bool classes and tags Signed-off-by: Pavel Emelyanov --- sstables/storage.cc | 3 --- sstables/storage.hh | 2 -- 2 files changed, 5 deletions(-) diff --git a/sstables/storage.cc b/sstables/storage.cc index c69eb53041..a58259b236 100644 --- a/sstables/storage.cc +++ b/sstables/storage.cc @@ -50,9 +50,6 @@ class filesystem_storage final : public sstables::storage { std::optional _temp_dir; // Valid while the sstable is being created, until sealed private: - struct mark_for_removal_tag {}; - struct leave_unsealed_tag {}; - enum class link_mode { default_mode, mark_for_removal, diff --git a/sstables/storage.hh b/sstables/storage.hh index 698cfba959..6f7ab27a0b 100644 --- a/sstables/storage.hh +++ b/sstables/storage.hh @@ -95,9 +95,7 @@ class storage { public: virtual ~storage() {} - using absolute_path = bool_class; // FIXME -- should go away eventually using sync_dir = bool_class; // meaningful only to filesystem storage - using leave_unsealed = bool_class; virtual future<> seal(const sstable& sst) = 0; virtual future<> snapshot(const sstable& sst, sstring name) const = 0;