From d478896d4603b831ee4452d341dceb5b0e2cda24 Mon Sep 17 00:00:00 2001 From: Calle Wilund Date: Tue, 17 Aug 2021 14:55:16 +0000 Subject: [PATCH] commitlog: kill non-recycled segment management It has been default for a while now. Makes no sense to not do it. Even hints can use it (even if it makes no difference there) --- db/commitlog/commitlog.cc | 20 +++----------------- db/commitlog/commitlog.hh | 1 - db/config.cc | 5 +++-- db/config.hh | 2 +- db/hints/manager.cc | 3 --- test/boost/commitlog_test.cc | 13 +------------ 6 files changed, 8 insertions(+), 36 deletions(-) diff --git a/db/commitlog/commitlog.cc b/db/commitlog/commitlog.cc index c89f27ca26..275fd97436 100644 --- a/db/commitlog/commitlog.cc +++ b/db/commitlog/commitlog.cc @@ -107,7 +107,6 @@ db::commitlog::config db::commitlog::config::from_db_config(const db::config& cf c.commitlog_sync_period_in_ms = cfg.commitlog_sync_period_in_ms(); c.mode = cfg.commitlog_sync() == "batch" ? sync_mode::BATCH : sync_mode::PERIODIC; c.extensions = &cfg.extensions(); - c.reuse_segments = cfg.commitlog_reuse_segments(); c.use_o_dsync = cfg.commitlog_use_o_dsync(); c.allow_going_over_size_limit = !cfg.commitlog_use_hard_size_limit(); @@ -260,7 +259,6 @@ public: using request_controller_type = basic_semaphore; using request_controller_units = semaphore_units; request_controller_type _request_controller; - shared_promise<> _disk_deletions; std::optional>> _segment_allocating; std::unordered_map _files_to_delete; @@ -398,7 +396,6 @@ private: future<> clear_reserve_segments(); void abort_recycled_list(std::exception_ptr); - void abort_deletion_promise(std::exception_ptr); future<> recalculate_footprint(); @@ -1566,7 +1563,7 @@ future db::commitlog::segment_manager: if (!cfg.allow_going_over_size_limit && max_disk_size != 0 && totals.total_size_on_disk >= max_disk_size) { clogger.debug("Disk usage ({} MB) exceeds maximum ({} MB) - allocation will wait...", totals.total_size_on_disk/(1024*1024), max_disk_size/(1024*1024)); - auto f = cfg.reuse_segments ? _recycled_segments.not_empty() : _disk_deletions.get_shared_future(); + auto f = _recycled_segments.not_empty(); if (!f.available()) { _new_counter = 0; // zero this so timer task does not duplicate the below flush flush_segments(0); // force memtable flush already @@ -1788,7 +1785,6 @@ future<> db::commitlog::segment_manager::shutdown() { if (_recycled_segments.empty()) { abort_recycled_list(ep); } - abort_deletion_promise(ep); auto f2 = std::exchange(_background_sync, make_ready_future<>()); co_await std::move(f); @@ -1843,8 +1839,6 @@ future<> db::commitlog::segment_manager::delete_file(const sstring& filename) { co_await seastar::remove_file(filename); clogger.trace("Reclaimed {} MB", size/(1024*1024)); totals.total_size_on_disk -= size; - auto p = std::exchange(_disk_deletions, {}); - p.set_value(); } catch (...) { commit_error_handler(std::current_exception()); throw; @@ -1875,7 +1869,7 @@ future<> db::commitlog::segment_manager::delete_segments(std::vector fi } // We allow reuse of the segment if the current disk size is less than shard max. - if (cfg.reuse_segments) { + { auto usage = totals.total_size_on_disk; auto recycle = usage <= max_disk_size; @@ -1910,6 +1904,7 @@ future<> db::commitlog::segment_manager::delete_segments(std::vector fi } } } + // last resort. co_await delete_file(filename); ++num_deleted; } catch (...) { @@ -1927,11 +1922,6 @@ future<> db::commitlog::segment_manager::delete_segments(std::vector fi if (recycle_error && _recycled_segments.empty()) { abort_recycled_list(recycle_error); } - // If recycle failed and turned into a delete, we should fake-wakeup waiters - // since we might still have cleaned up disk space. - if (!recycle_error && num_deleted && cfg.reuse_segments && _recycled_segments.empty()) { - abort_recycled_list(std::make_exception_ptr(std::runtime_error("deleted files"))); - } // #9348 - if we had an exception, we can't trust our bookeep any more. recalculate. if (except) { @@ -1947,10 +1937,6 @@ void db::commitlog::segment_manager::abort_recycled_list(std::exception_ptr ep) _recycled_segments = queue(std::numeric_limits::max()); } -void db::commitlog::segment_manager::abort_deletion_promise(std::exception_ptr ep) { - std::exchange(_disk_deletions, {}).set_exception(ep); -} - future<> db::commitlog::segment_manager::recalculate_footprint() { try { co_await do_pending_deletes(); diff --git a/db/commitlog/commitlog.hh b/db/commitlog/commitlog.hh index befd603bce..9894100757 100644 --- a/db/commitlog/commitlog.hh +++ b/db/commitlog/commitlog.hh @@ -109,7 +109,6 @@ public: sync_mode mode = sync_mode::PERIODIC; std::string fname_prefix = descriptor::FILENAME_PREFIX; - bool reuse_segments = true; bool use_o_dsync = false; bool warn_about_segments_left_on_disk_after_shutdown = true; bool allow_going_over_size_limit = true; diff --git a/db/config.cc b/db/config.cc index 1f8e3f2d2c..1d18ae33d8 100644 --- a/db/config.cc +++ b/db/config.cc @@ -424,8 +424,9 @@ db::config::config(std::shared_ptr exts) , commitlog_total_space_in_mb(this, "commitlog_total_space_in_mb", value_status::Used, -1, "Total space used for commitlogs. If the used space goes above this value, Scylla rounds up to the next nearest segment multiple and flushes memtables to disk for the oldest commitlog segments, removing those log segments. This reduces the amount of data to replay on startup, and prevents infrequently-updated tables from indefinitely keeping commitlog segments. A small total commitlog space tends to cause more flush activity on less-active tables.\n" "Related information: Configuring memtable throughput") - , commitlog_reuse_segments(this, "commitlog_reuse_segments", value_status::Used, true, - "Whether or not to re-use commitlog segments when finished instead of deleting them. Can improve commitlog latency on some file systems.\n") + /* Note: Unused. Retained for upgrade compat. Deprecate and remove in a cycle or two. */ + , commitlog_reuse_segments(this, "commitlog_reuse_segments", value_status::Unused, true, + "Whether or not to re-use commitlog segments when finished instead of deleting them. Can improve commitlog latency on some file systems.\n") , commitlog_use_o_dsync(this, "commitlog_use_o_dsync", value_status::Used, true, "Whether or not to use O_DSYNC mode for commitlog segments IO. Can improve commitlog latency on some file systems.\n") , commitlog_use_hard_size_limit(this, "commitlog_use_hard_size_limit", value_status::Used, false, diff --git a/db/config.hh b/db/config.hh index 709fc5b461..4ca4a77c75 100644 --- a/db/config.hh +++ b/db/config.hh @@ -172,7 +172,7 @@ public: named_value commitlog_sync_period_in_ms; named_value commitlog_sync_batch_window_in_ms; named_value commitlog_total_space_in_mb; - named_value commitlog_reuse_segments; + named_value commitlog_reuse_segments; // unused. retained for upgrade compat named_value commitlog_use_o_dsync; named_value commitlog_use_hard_size_limit; named_value compaction_preheat_key_cache; diff --git a/db/hints/manager.cc b/db/hints/manager.cc index 95517d9b36..3940e0c9bd 100644 --- a/db/hints/manager.cc +++ b/db/hints/manager.cc @@ -392,9 +392,6 @@ future manager::end_point_hints_manager::add_store() noexcept { cfg.fname_prefix = manager::FILENAME_PREFIX; cfg.extensions = &_shard_manager.local_db().extensions(); - // HH doesn't utilize the flow that benefits from reusing segments. - // Therefore let's simply disable it to avoid any possible confusion. - cfg.reuse_segments = false; // HH leaves segments on disk after commitlog shutdown, and later reads // them when commitlog is re-created. This is expected to happen regularly // during standard HH workload, so no need to print a warning about it. diff --git a/test/boost/commitlog_test.cc b/test/boost/commitlog_test.cc index e00b11df43..31b515d833 100644 --- a/test/boost/commitlog_test.cc +++ b/test/boost/commitlog_test.cc @@ -752,7 +752,6 @@ SEASTAR_TEST_CASE(test_commitlog_deadlock_in_recycle) { // ensure total size per shard is not multiple of segment size. cfg.commitlog_total_space_in_mb = 5 * smp::count; cfg.commitlog_sync_period_in_ms = 10; - cfg.reuse_segments = true; cfg.allow_going_over_size_limit = false; cfg.use_o_dsync = true; // make sure we pre-allocate. @@ -823,7 +822,6 @@ SEASTAR_TEST_CASE(test_commitlog_shutdown_during_wait) { // ensure total size per shard is not multiple of segment size. cfg.commitlog_total_space_in_mb = 5 * smp::count; cfg.commitlog_sync_period_in_ms = 10; - cfg.reuse_segments = true; cfg.allow_going_over_size_limit = false; cfg.use_o_dsync = true; // make sure we pre-allocate. @@ -891,7 +889,6 @@ SEASTAR_TEST_CASE(test_commitlog_deadlock_with_flush_threshold) { cfg.commitlog_segment_size_in_mb = max_size_mb; cfg.commitlog_total_space_in_mb = 2 * max_size_mb * smp::count; cfg.commitlog_sync_period_in_ms = 10; - cfg.reuse_segments = true; cfg.allow_going_over_size_limit = false; cfg.use_o_dsync = true; // make sure we pre-allocate. @@ -937,7 +934,7 @@ SEASTAR_TEST_CASE(test_commitlog_deadlock_with_flush_threshold) { } } -static future<> do_test_exception_in_allocate_ex(bool do_file_delete, bool reuse = true) { +static future<> do_test_exception_in_allocate_ex(bool do_file_delete) { commitlog::config cfg; constexpr auto max_size_mb = 1; @@ -945,7 +942,6 @@ static future<> do_test_exception_in_allocate_ex(bool do_file_delete, bool reuse cfg.commitlog_segment_size_in_mb = max_size_mb; cfg.commitlog_total_space_in_mb = 2 * max_size_mb * smp::count; cfg.commitlog_sync_period_in_ms = 10; - cfg.reuse_segments = reuse; cfg.allow_going_over_size_limit = false; // #9348 - now can enforce size limit always cfg.use_o_dsync = true; // make sure we pre-allocate. @@ -1030,18 +1026,11 @@ SEASTAR_TEST_CASE(test_commitlog_exceptions_in_allocate_ex) { co_await do_test_exception_in_allocate_ex(false); } -SEASTAR_TEST_CASE(test_commitlog_exceptions_in_allocate_ex_no_recycle) { - co_await do_test_exception_in_allocate_ex(false, false); -} - /** * Test generating an exception in segment file allocation, but also * delete the file, which in turn should cause follow-up exceptions * in cleanup delete. Which CL should handle */ -SEASTAR_TEST_CASE(test_commitlog_exceptions_in_allocate_ex_deleted_file) { - co_await do_test_exception_in_allocate_ex(true, false); -} SEASTAR_TEST_CASE(test_commitlog_exceptions_in_allocate_ex_deleted_file_no_recycle) { co_await do_test_exception_in_allocate_ex(true);