From 5b92c4549effd065459ad7e1b30cd821973ddf70 Mon Sep 17 00:00:00 2001 From: Pavel Emelyanov Date: Wed, 12 Feb 2025 16:04:38 +0300 Subject: [PATCH 1/5] sstable_directory: Relax toc file dumping to deletion log The current code takes sstable prefix() (e.g. the /foo/bar string), then trims from its fron the basedir (e.g. the /foo/ string) and then writes the remainder, a slash and TOC component name (e.g. the xxx-TOC.txt string). The final result is "bar/xxx-TOC.txt" string. The taking into account sstable.toc_filename() renders into sstable.prefix + \slash + component-name, the above result can be achieved by trimming basedir directory from toc_filename(). Signed-off-by: Pavel Emelyanov --- sstables/sstable_directory.cc | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/sstables/sstable_directory.cc b/sstables/sstable_directory.cc index c0b7979dd0..e1b491f57e 100644 --- a/sstables/sstable_directory.cc +++ b/sstables/sstable_directory.cc @@ -653,17 +653,13 @@ future sstable_directory::create_pendi try { auto trim_size = base_dir.native().size() + 1; // Account for the '/' delimiter for (const auto& sst : ssts) { - auto prefix = sst->_storage->prefix(); - if (prefix.size() > trim_size) { - out.write(prefix.begin() + trim_size, prefix.size() - trim_size).get(); - out.write("/").get(); + auto toc = sst->toc_filename(); + if (toc.size() <= trim_size) { + on_internal_error(dirlog, fmt::format("Sstable {} outside of basedir {} is scheduled for deletion", toc, base_dir.native())); } - auto toc = sst->component_basename(component_type::TOC); - out.write(toc).get(); + out.write(toc.begin() + trim_size, toc.size() - trim_size).get(); out.write("\n").get(); - dirlog.trace("Wrote '{}{}' to {}", - prefix.size() > trim_size ? sstring(prefix.begin() + trim_size, prefix.size() - trim_size) + "/" : "", - sst->component_basename(component_type::TOC), tmp_pending_delete_log); + dirlog.trace("Wrote '{}' to {}", sstring(toc.begin() + trim_size, toc.size() - trim_size), tmp_pending_delete_log); } out.flush().get(); From b0c1a775289f072f5de69b653a1c366f4c8409db Mon Sep 17 00:00:00 2001 From: Pavel Emelyanov Date: Wed, 12 Feb 2025 16:06:01 +0300 Subject: [PATCH 2/5] sstable_directory: Introduce local pending_delete_log variable This is simply to reduce the churn in the next patch, nothing special here. Signed-off-by: Pavel Emelyanov --- sstables/sstable_directory.cc | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/sstables/sstable_directory.cc b/sstables/sstable_directory.cc index e1b491f57e..7cebc8cb00 100644 --- a/sstables/sstable_directory.cc +++ b/sstables/sstable_directory.cc @@ -629,6 +629,7 @@ bool sstable_directory::compare_sstable_storage_prefix(const sstring& prefix_a, future sstable_directory::create_pending_deletion_log(opened_directory& base_dir, const std::vector& ssts) { return seastar::async([&] { min_max_tracker gen_tracker; + sstring pending_delete_log; pending_delete_result res; for (const auto& sst : ssts) { @@ -638,8 +639,9 @@ future sstable_directory::create_pendi } sstring pending_delete_dir = (base_dir.path() / sstables::pending_delete_dir).native(); - res.pending_delete_log = format("{}/sstables-{}-{}.log", pending_delete_dir, gen_tracker.min(), gen_tracker.max()); - sstring tmp_pending_delete_log = res.pending_delete_log + ".tmp"; + pending_delete_log = format("{}/sstables-{}-{}.log", pending_delete_dir, gen_tracker.min(), gen_tracker.max()); + res.pending_delete_log = pending_delete_log; + sstring tmp_pending_delete_log = pending_delete_log + ".tmp"; dirlog.trace("Writing {}", tmp_pending_delete_log); touch_directory(pending_delete_dir).get(); @@ -669,11 +671,11 @@ future sstable_directory::create_pendi } // Once flushed and closed, the temporary log file can be renamed. - io_check(rename_file, tmp_pending_delete_log, res.pending_delete_log).get(); + io_check(rename_file, tmp_pending_delete_log, pending_delete_log).get(); // Guarantee that the changes above reached the disk. base_dir.sync(general_disk_error_handler).get(); - dirlog.debug("{} written successfully.", res.pending_delete_log); + dirlog.debug("{} written successfully.", pending_delete_log); return res; }); From f6de6d6887e458a1ab95e06b74f0e936141647bc Mon Sep 17 00:00:00 2001 From: Pavel Emelyanov Date: Wed, 12 Feb 2025 16:07:03 +0300 Subject: [PATCH 3/5] sstable_directory: Calculate prefixes outside of create_pending_deletion_log() The method in question walks the list of sstables and accumulates sstables' prefixes into a set on pending_delete_result object. The set in question is not used at all in this method and is in fact alien to it -- the p.d._result object is used by the filesystem storage driver as atomic deletion prepare/commit transparent context. Said that, move the whole pending_delete_result to where it belongs and relax the create_pending_deletion_log() to only return the log directory path string. Signed-off-by: Pavel Emelyanov --- sstables/sstable_directory.cc | 8 ++------ sstables/sstable_directory.hh | 2 +- sstables/storage.cc | 10 +++++++++- 3 files changed, 12 insertions(+), 8 deletions(-) diff --git a/sstables/sstable_directory.cc b/sstables/sstable_directory.cc index 7cebc8cb00..09b9aa14e7 100644 --- a/sstables/sstable_directory.cc +++ b/sstables/sstable_directory.cc @@ -626,21 +626,17 @@ bool sstable_directory::compare_sstable_storage_prefix(const sstring& prefix_a, return size_a == size_b && sstring::traits_type::compare(prefix_a.begin(), prefix_b.begin(), size_a) == 0; } -future sstable_directory::create_pending_deletion_log(opened_directory& base_dir, const std::vector& ssts) { +future sstable_directory::create_pending_deletion_log(opened_directory& base_dir, const std::vector& ssts) { return seastar::async([&] { min_max_tracker gen_tracker; sstring pending_delete_log; - pending_delete_result res; for (const auto& sst : ssts) { - auto prefix = sst->_storage->prefix(); - res.prefixes.insert(prefix); gen_tracker.update(sst->generation()); } sstring pending_delete_dir = (base_dir.path() / sstables::pending_delete_dir).native(); pending_delete_log = format("{}/sstables-{}-{}.log", pending_delete_dir, gen_tracker.min(), gen_tracker.max()); - res.pending_delete_log = pending_delete_log; sstring tmp_pending_delete_log = pending_delete_log + ".tmp"; dirlog.trace("Writing {}", tmp_pending_delete_log); @@ -677,7 +673,7 @@ future sstable_directory::create_pendi base_dir.sync(general_disk_error_handler).get(); dirlog.debug("{} written successfully.", pending_delete_log); - return res; + return pending_delete_log; }); } diff --git a/sstables/sstable_directory.hh b/sstables/sstable_directory.hh index fbc3561429..4289063a5e 100644 --- a/sstables/sstable_directory.hh +++ b/sstables/sstable_directory.hh @@ -312,7 +312,7 @@ public: // Creates the deletion log for atomic deletion of sstables at `base_dir` (helper for the // above function that's also used by tests) // Returns the name of the pending_delete_log and an unordered_set of sstable prefixes. - static future create_pending_deletion_log(opened_directory& base_dir, const std::vector& ssts); + static future create_pending_deletion_log(opened_directory& base_dir, const std::vector& ssts); static bool compare_sstable_storage_prefix(const sstring& a, const sstring& b) noexcept; sstable_state state() const noexcept { return _state; } diff --git a/sstables/storage.cc b/sstables/storage.cc index 095e9af70f..aa89f35ea0 100644 --- a/sstables/storage.cc +++ b/sstables/storage.cc @@ -480,7 +480,15 @@ future<> filesystem_storage::wipe(const sstable& sst, sync_dir sync) noexcept { } future filesystem_storage::atomic_delete_prepare(const std::vector& ssts) const { - co_return co_await sstable_directory::create_pending_deletion_log(_base_dir, ssts); + sstable_directory::pending_delete_result res; + + for (const auto& sst : ssts) { + auto prefix = sst->_storage->prefix(); + res.prefixes.insert(prefix); + } + + res.pending_delete_log = co_await sstable_directory::create_pending_deletion_log(_base_dir, ssts); + co_return std::move(res); } future<> filesystem_storage::atomic_delete_complete(atomic_delete_context ctx) const { From 96a867c8697284ce4faef0b8978404d7016dd9ca Mon Sep 17 00:00:00 2001 From: Pavel Emelyanov Date: Wed, 12 Feb 2025 16:08:34 +0300 Subject: [PATCH 4/5] sstable_directory: Move sstable_directory::pending_delete_result ... to where it belongs -- to the filesystem storage driver itself. Continuation of the previous patch. Signed-off-by: Pavel Emelyanov --- sstables/sstable_directory.hh | 5 ----- sstables/storage.cc | 2 +- sstables/storage.hh | 6 ++++-- 3 files changed, 5 insertions(+), 8 deletions(-) diff --git a/sstables/sstable_directory.hh b/sstables/sstable_directory.hh index 4289063a5e..18d80decb6 100644 --- a/sstables/sstable_directory.hh +++ b/sstables/sstable_directory.hh @@ -291,11 +291,6 @@ public: using can_be_remote = bool_class; future<> collect_output_unshared_sstables(std::vector resharded_sstables, can_be_remote); - struct pending_delete_result { - sstring pending_delete_log; - std::unordered_set prefixes; - }; - // When we compact sstables, we have to atomically instantiate the new // sstable and delete the old ones. Otherwise, if we compact A+B into C, // and if A contained some data that was tombstoned by B, and if B was diff --git a/sstables/storage.cc b/sstables/storage.cc index aa89f35ea0..3222960bec 100644 --- a/sstables/storage.cc +++ b/sstables/storage.cc @@ -480,7 +480,7 @@ future<> filesystem_storage::wipe(const sstable& sst, sync_dir sync) noexcept { } future filesystem_storage::atomic_delete_prepare(const std::vector& ssts) const { - sstable_directory::pending_delete_result res; + atomic_delete_context res; for (const auto& sst : ssts) { auto prefix = sst->_storage->prefix(); diff --git a/sstables/storage.hh b/sstables/storage.hh index 206217b335..8041b21dbf 100644 --- a/sstables/storage.hh +++ b/sstables/storage.hh @@ -23,7 +23,6 @@ #include "sstables/component_type.hh" #include "sstables/generation_type.hh" #include "utils/disk-error-handler.hh" -#include "sstables/sstable_directory.hh" class schema; @@ -41,7 +40,10 @@ class sstable; class sstables_manager; class entry_descriptor; -using atomic_delete_context = sstable_directory::pending_delete_result; +struct atomic_delete_context { + sstring pending_delete_log; + std::unordered_set prefixes; +}; class opened_directory final { std::filesystem::path _pathname; From d79eec2e76ac2cbb015a823906ced53450a5c89e Mon Sep 17 00:00:00 2001 From: Pavel Emelyanov Date: Wed, 12 Feb 2025 16:08:53 +0300 Subject: [PATCH 5/5] sstable: Unfriend sstable_directory class It was only needed there for create_pending_deletion_log() method to get private "_storage" from sstable. Now it's all gone and friendship can be broken. Signed-off-by: Pavel Emelyanov --- sstables/sstables.hh | 1 - 1 file changed, 1 deletion(-) diff --git a/sstables/sstables.hh b/sstables/sstables.hh index 68fabc9e5b..930d216c18 100644 --- a/sstables/sstables.hh +++ b/sstables/sstables.hh @@ -526,7 +526,6 @@ private: } friend class sstable_stream_sink_impl; - friend class sstable_directory; friend class filesystem_storage; friend class s3_storage; friend class tiered_storage;