From ce2aa15d197d14b6026b81c6820b71d1e186c70b Mon Sep 17 00:00:00 2001 From: Lakshmi Narayanan Sreethar Date: Wed, 22 Jan 2025 15:31:45 +0530 Subject: [PATCH 01/12] sstables_manager: extract out `maybe_reload_components()` Extract the logic that reloads reclaimed components into memory in the `components_reloader_fiber()` method into a separate method. This is in preparation for moving the reclaim logic into the same fiber. Signed-off-by: Lakshmi Narayanan Sreethar --- sstables/sstables_manager.cc | 6 +++++- sstables/sstables_manager.hh | 2 ++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/sstables/sstables_manager.cc b/sstables/sstables_manager.cc index ababcfade5..559fbb662f 100644 --- a/sstables/sstables_manager.cc +++ b/sstables/sstables_manager.cc @@ -192,6 +192,11 @@ future<> sstables_manager::components_reloader_fiber() { co_return; } + co_await maybe_reload_components(); + } +} + +future<> sstables_manager::maybe_reload_components() { // Reload bloom filters from the smallest to largest so as to maximize // the number of bloom filters being reloaded. auto memory_available = get_memory_available_for_reclaimable_components(); @@ -224,7 +229,6 @@ future<> sstables_manager::components_reloader_fiber() { _total_memory_reclaimed -= reclaimed_memory; memory_available = get_memory_available_for_reclaimable_components(); } - } } void sstables_manager::reclaim_memory_and_stop_tracking_sstable(sstable* sst) { diff --git a/sstables/sstables_manager.hh b/sstables/sstables_manager.hh index 6bf0307b9a..6342141da9 100644 --- a/sstables/sstables_manager.hh +++ b/sstables/sstables_manager.hh @@ -218,6 +218,8 @@ private: void increment_total_reclaimable_memory_and_maybe_reclaim(sstable* sst); // Fiber to reload reclaimed components back into memory when memory becomes available. future<> components_reloader_fiber(); + // Reloads components from reclaimed SSTables if memory is available. + future<> maybe_reload_components(); size_t get_memory_available_for_reclaimable_components(); // Reclaim memory from the SSTable and remove it from the memory tracking metrics. // The method is idempotent and for an sstable that is deleted, it is called both From 59cbee6fc7ccb48984a1ec0e03e8add8a418498d Mon Sep 17 00:00:00 2001 From: Lakshmi Narayanan Sreethar Date: Wed, 22 Jan 2025 15:37:27 +0530 Subject: [PATCH 02/12] sstables_manager: fix `maybe_reload_components()` indentation Signed-off-by: Lakshmi Narayanan Sreethar --- sstables/sstables_manager.cc | 62 ++++++++++++++++++------------------ 1 file changed, 31 insertions(+), 31 deletions(-) diff --git a/sstables/sstables_manager.cc b/sstables/sstables_manager.cc index 559fbb662f..c0f0a6c029 100644 --- a/sstables/sstables_manager.cc +++ b/sstables/sstables_manager.cc @@ -197,38 +197,38 @@ future<> sstables_manager::components_reloader_fiber() { } future<> sstables_manager::maybe_reload_components() { - // Reload bloom filters from the smallest to largest so as to maximize - // the number of bloom filters being reloaded. - auto memory_available = get_memory_available_for_reclaimable_components(); - while (!_reclaimed.empty() && memory_available > 0) { - auto sstable_to_reload = _reclaimed.begin(); - const size_t reclaimed_memory = sstable_to_reload->total_memory_reclaimed(); - if (reclaimed_memory > memory_available) { - // cannot reload anymore sstables - break; - } - - // Increment the total memory before reloading to prevent any parallel - // fibers from loading new bloom filters into memory. - _total_reclaimable_memory += reclaimed_memory; - _reclaimed.erase(sstable_to_reload); - // Use a lw_shared_ptr to prevent the sstable from getting deleted when - // the components are being reloaded. - auto sstable_ptr = sstable_to_reload->shared_from_this(); - try { - co_await sstable_ptr->reload_reclaimed_components(); - } catch (...) { - // reload failed due to some reason - sstlog.warn("Failed to reload reclaimed SSTable components : {}", std::current_exception()); - // revert back changes made before the reload - _total_reclaimable_memory -= reclaimed_memory; - _reclaimed.insert(*sstable_to_reload); - break; - } - - _total_memory_reclaimed -= reclaimed_memory; - memory_available = get_memory_available_for_reclaimable_components(); + // Reload bloom filters from the smallest to largest so as to maximize + // the number of bloom filters being reloaded. + auto memory_available = get_memory_available_for_reclaimable_components(); + while (!_reclaimed.empty() && memory_available > 0) { + auto sstable_to_reload = _reclaimed.begin(); + const size_t reclaimed_memory = sstable_to_reload->total_memory_reclaimed(); + if (reclaimed_memory > memory_available) { + // cannot reload anymore sstables + break; } + + // Increment the total memory before reloading to prevent any parallel + // fibers from loading new bloom filters into memory. + _total_reclaimable_memory += reclaimed_memory; + _reclaimed.erase(sstable_to_reload); + // Use a lw_shared_ptr to prevent the sstable from getting deleted when + // the components are being reloaded. + auto sstable_ptr = sstable_to_reload->shared_from_this(); + try { + co_await sstable_ptr->reload_reclaimed_components(); + } catch (...) { + // reload failed due to some reason + sstlog.warn("Failed to reload reclaimed SSTable components : {}", std::current_exception()); + // revert back changes made before the reload + _total_reclaimable_memory -= reclaimed_memory; + _reclaimed.insert(*sstable_to_reload); + break; + } + + _total_memory_reclaimed -= reclaimed_memory; + memory_available = get_memory_available_for_reclaimable_components(); + } } void sstables_manager::reclaim_memory_and_stop_tracking_sstable(sstable* sst) { From 4d12ae433a68e15f8363add953b721b4e7d7babc Mon Sep 17 00:00:00 2001 From: Lakshmi Narayanan Sreethar Date: Thu, 30 Jan 2025 14:43:22 +0530 Subject: [PATCH 03/12] sstables_manager: extract `maybe_reclaim_components()` Extract the code from `increment_total_reclaimable_memory_and_maybe_reclaim()` that reclaims the components memory into `maybe_reclaim_components()`. The extracted new method will be used by a following patch to handle reclaim within the components reload fiber. Signed-off-by: Lakshmi Narayanan Sreethar --- sstables/sstables_manager.cc | 3 +++ sstables/sstables_manager.hh | 2 ++ 2 files changed, 5 insertions(+) diff --git a/sstables/sstables_manager.cc b/sstables/sstables_manager.cc index c0f0a6c029..c1a6439712 100644 --- a/sstables/sstables_manager.cc +++ b/sstables/sstables_manager.cc @@ -154,7 +154,10 @@ sstable_writer_config sstables_manager::configure_writer(sstring origin) const { void sstables_manager::increment_total_reclaimable_memory_and_maybe_reclaim(sstable* sst) { _total_reclaimable_memory += sst->total_reclaimable_memory_size(); + maybe_reclaim_components(); +} +void sstables_manager::maybe_reclaim_components() { size_t memory_reclaim_threshold = _available_memory * _db_config.components_memory_reclaim_threshold(); if (_total_reclaimable_memory <= memory_reclaim_threshold) { // total memory used is within limit; no need to reclaim. diff --git a/sstables/sstables_manager.hh b/sstables/sstables_manager.hh index 6342141da9..4e9a0d151f 100644 --- a/sstables/sstables_manager.hh +++ b/sstables/sstables_manager.hh @@ -218,6 +218,8 @@ private: void increment_total_reclaimable_memory_and_maybe_reclaim(sstable* sst); // Fiber to reload reclaimed components back into memory when memory becomes available. future<> components_reloader_fiber(); + // Reclaims components from SSTables if total memory usage exceeds the threshold. + void maybe_reclaim_components(); // Reloads components from reclaimed SSTables if memory is available. future<> maybe_reload_components(); size_t get_memory_available_for_reclaimable_components(); From 30184ead794b213db78bbf8457b8cbdc275cb786 Mon Sep 17 00:00:00 2001 From: Lakshmi Narayanan Sreethar Date: Thu, 30 Jan 2025 15:12:50 +0530 Subject: [PATCH 04/12] sstables_manager: introduce `get_components_memory_reclaim_threshold()` Introduce `get_components_memory_reclaim_threshold()`, which returns the components' memory threshold based on the total available memory. Signed-off-by: Lakshmi Narayanan Sreethar --- sstables/sstables_manager.cc | 12 +++++++----- sstables/sstables_manager.hh | 3 ++- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/sstables/sstables_manager.cc b/sstables/sstables_manager.cc index c1a6439712..1cbc10d0a4 100644 --- a/sstables/sstables_manager.cc +++ b/sstables/sstables_manager.cc @@ -158,8 +158,7 @@ void sstables_manager::increment_total_reclaimable_memory_and_maybe_reclaim(ssta } void sstables_manager::maybe_reclaim_components() { - size_t memory_reclaim_threshold = _available_memory * _db_config.components_memory_reclaim_threshold(); - if (_total_reclaimable_memory <= memory_reclaim_threshold) { + if (_total_reclaimable_memory <= get_components_memory_reclaim_threshold()) { // total memory used is within limit; no need to reclaim. return; } @@ -179,9 +178,12 @@ void sstables_manager::maybe_reclaim_components() { memory_reclaimed, sst_with_max_memory->get_filename(), _total_memory_reclaimed); } -size_t sstables_manager::get_memory_available_for_reclaimable_components() { - size_t memory_reclaim_threshold = _available_memory * _db_config.components_memory_reclaim_threshold(); - return memory_reclaim_threshold - _total_reclaimable_memory; +size_t sstables_manager::get_components_memory_reclaim_threshold() const { + return _available_memory * _db_config.components_memory_reclaim_threshold(); +} + +size_t sstables_manager::get_memory_available_for_reclaimable_components() const { + return get_components_memory_reclaim_threshold() - _total_reclaimable_memory; } future<> sstables_manager::components_reloader_fiber() { diff --git a/sstables/sstables_manager.hh b/sstables/sstables_manager.hh index 4e9a0d151f..aba86480af 100644 --- a/sstables/sstables_manager.hh +++ b/sstables/sstables_manager.hh @@ -222,7 +222,8 @@ private: void maybe_reclaim_components(); // Reloads components from reclaimed SSTables if memory is available. future<> maybe_reload_components(); - size_t get_memory_available_for_reclaimable_components(); + size_t get_components_memory_reclaim_threshold() const; + size_t get_memory_available_for_reclaimable_components() const; // Reclaim memory from the SSTable and remove it from the memory tracking metrics. // The method is idempotent and for an sstable that is deleted, it is called both // during unlink and during deactivation. From f53fd40ff079abd7877412fdc9dc1792b702559c Mon Sep 17 00:00:00 2001 From: Lakshmi Narayanan Sreethar Date: Thu, 30 Jan 2025 18:09:11 +0530 Subject: [PATCH 05/12] sstables_manager: reclaim components memory until usage falls below threshold The current implementation reclaims memory from SSTables only when a new SSTable is created. An upcoming patch will move this reclaim logic into the existing component reloader fiber. To support this change, the `maybe_reclaim_components()` method is updated to reclaim memory until the total memory consumption falls below the configured threshold. Signed-off-by: Lakshmi Narayanan Sreethar --- sstables/sstables_manager.cc | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/sstables/sstables_manager.cc b/sstables/sstables_manager.cc index 1cbc10d0a4..734db20e35 100644 --- a/sstables/sstables_manager.cc +++ b/sstables/sstables_manager.cc @@ -158,13 +158,10 @@ void sstables_manager::increment_total_reclaimable_memory_and_maybe_reclaim(ssta } void sstables_manager::maybe_reclaim_components() { - if (_total_reclaimable_memory <= get_components_memory_reclaim_threshold()) { - // total memory used is within limit; no need to reclaim. - return; - } - - // Memory consumption has crossed threshold. Reclaim from the SSTable that + while(_total_reclaimable_memory > get_components_memory_reclaim_threshold()) { + // Memory consumption is above threshold. Reclaim from the SSTable that // has the most reclaimable memory to get the total consumption under limit. + // FIXME: Take SSTable usage into account during reclaim - see https://github.com/scylladb/scylladb/issues/21897 auto sst_with_max_memory = std::max_element(_active.begin(), _active.end(), [](const sstable& sst1, const sstable& sst2) { return sst1.total_reclaimable_memory_size() < sst2.total_reclaimable_memory_size(); }); @@ -176,6 +173,7 @@ void sstables_manager::maybe_reclaim_components() { // TODO: As of now only bloom filter is reclaimed. Print actual component names when adding support for more components. smlogger.info("Reclaimed {} bytes of memory from components of {}. Total memory reclaimed so far is {} bytes", memory_reclaimed, sst_with_max_memory->get_filename(), _total_memory_reclaimed); + } } size_t sstables_manager::get_components_memory_reclaim_threshold() const { From 4d396b9578723d1f9ec5dbabd7253005cbc92c70 Mon Sep 17 00:00:00 2001 From: Lakshmi Narayanan Sreethar Date: Thu, 30 Jan 2025 18:20:41 +0530 Subject: [PATCH 06/12] sstables_manager: fix `maybe_reclaim_components()` indentation Signed-off-by: Lakshmi Narayanan Sreethar --- sstables/sstables_manager.cc | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/sstables/sstables_manager.cc b/sstables/sstables_manager.cc index 734db20e35..c751b48639 100644 --- a/sstables/sstables_manager.cc +++ b/sstables/sstables_manager.cc @@ -159,21 +159,21 @@ void sstables_manager::increment_total_reclaimable_memory_and_maybe_reclaim(ssta void sstables_manager::maybe_reclaim_components() { while(_total_reclaimable_memory > get_components_memory_reclaim_threshold()) { - // Memory consumption is above threshold. Reclaim from the SSTable that - // has the most reclaimable memory to get the total consumption under limit. - // FIXME: Take SSTable usage into account during reclaim - see https://github.com/scylladb/scylladb/issues/21897 - auto sst_with_max_memory = std::max_element(_active.begin(), _active.end(), [](const sstable& sst1, const sstable& sst2) { - return sst1.total_reclaimable_memory_size() < sst2.total_reclaimable_memory_size(); - }); + // Memory consumption is above threshold. Reclaim from the SSTable that + // has the most reclaimable memory to get the total consumption under limit. + // FIXME: Take SSTable usage into account during reclaim - see https://github.com/scylladb/scylladb/issues/21897 + auto sst_with_max_memory = std::max_element(_active.begin(), _active.end(), [](const sstable& sst1, const sstable& sst2) { + return sst1.total_reclaimable_memory_size() < sst2.total_reclaimable_memory_size(); + }); - auto memory_reclaimed = sst_with_max_memory->reclaim_memory_from_components(); - _total_memory_reclaimed += memory_reclaimed; - _total_reclaimable_memory -= memory_reclaimed; - _reclaimed.insert(*sst_with_max_memory); - // TODO: As of now only bloom filter is reclaimed. Print actual component names when adding support for more components. - smlogger.info("Reclaimed {} bytes of memory from components of {}. Total memory reclaimed so far is {} bytes", - memory_reclaimed, sst_with_max_memory->get_filename(), _total_memory_reclaimed); - } + auto memory_reclaimed = sst_with_max_memory->reclaim_memory_from_components(); + _total_memory_reclaimed += memory_reclaimed; + _total_reclaimable_memory -= memory_reclaimed; + _reclaimed.insert(*sst_with_max_memory); + // TODO: As of now only bloom filter is reclaimed. Print actual component names when adding support for more components. + smlogger.info("Reclaimed {} bytes of memory from components of {}. Total memory reclaimed so far is {} bytes", + memory_reclaimed, sst_with_max_memory->get_filename(), _total_memory_reclaimed); + } } size_t sstables_manager::get_components_memory_reclaim_threshold() const { From 35a4de3eebcc2a452fff5af1d88c0f96fc02beb4 Mon Sep 17 00:00:00 2001 From: Lakshmi Narayanan Sreethar Date: Thu, 30 Jan 2025 17:30:39 +0530 Subject: [PATCH 07/12] sstables_manager: rename `components_reloader_fiber()` A future patch will move components reclaim logic into the current `components_reloader_fiber()`, so to reflect its new purpose, rename it to `components_reclaim_reload_fiber()`. Signed-off-by: Lakshmi Narayanan Sreethar --- sstables/sstables_manager.cc | 4 ++-- sstables/sstables_manager.hh | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/sstables/sstables_manager.cc b/sstables/sstables_manager.cc index c751b48639..db755b1e4c 100644 --- a/sstables/sstables_manager.cc +++ b/sstables/sstables_manager.cc @@ -43,7 +43,7 @@ sstables_manager::sstables_manager( , _maintenance_sg(std::move(maintenance_sg)) , _abort(abort) { - _components_reloader_status = components_reloader_fiber(); + _components_reloader_status = components_reclaim_reload_fiber(); } sstables_manager::~sstables_manager() { @@ -184,7 +184,7 @@ size_t sstables_manager::get_memory_available_for_reclaimable_components() const return get_components_memory_reclaim_threshold() - _total_reclaimable_memory; } -future<> sstables_manager::components_reloader_fiber() { +future<> sstables_manager::components_reclaim_reload_fiber() { co_await coroutine::switch_to(_maintenance_sg); sstlog.trace("components_reloader_fiber start"); diff --git a/sstables/sstables_manager.hh b/sstables/sstables_manager.hh index aba86480af..a88ba6646c 100644 --- a/sstables/sstables_manager.hh +++ b/sstables/sstables_manager.hh @@ -217,7 +217,7 @@ private: // reclaim it from the SSTable that has the most reclaimable memory. void increment_total_reclaimable_memory_and_maybe_reclaim(sstable* sst); // Fiber to reload reclaimed components back into memory when memory becomes available. - future<> components_reloader_fiber(); + future<> components_reclaim_reload_fiber(); // Reclaims components from SSTables if total memory usage exceeds the threshold. void maybe_reclaim_components(); // Reloads components from reclaimed SSTables if memory is available. From f73b6abcc76f8e92c483dff709603fe0dfba6848 Mon Sep 17 00:00:00 2001 From: Lakshmi Narayanan Sreethar Date: Thu, 30 Jan 2025 17:42:01 +0530 Subject: [PATCH 08/12] sstables_manager: rename `_sstable_deleted_event` condition variable Rename the `_sstable_deleted_event` condition variable to `_components_memory_change_event` as it will be used by future patches to signal changes in sstable component memory consumption, (i.e.) during sstable create and delete, and also when the `components_memory_reclaim_threshold` config value is changed. Signed-off-by: Lakshmi Narayanan Sreethar --- sstables/sstables_manager.cc | 6 +++--- sstables/sstables_manager.hh | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/sstables/sstables_manager.cc b/sstables/sstables_manager.cc index db755b1e4c..bd76a4bbe5 100644 --- a/sstables/sstables_manager.cc +++ b/sstables/sstables_manager.cc @@ -189,7 +189,7 @@ future<> sstables_manager::components_reclaim_reload_fiber() { sstlog.trace("components_reloader_fiber start"); while (true) { - co_await _sstable_deleted_event.when(); + co_await _components_memory_change_event.when(); if (_closing) { co_return; @@ -269,7 +269,7 @@ void sstables_manager::deactivate(sstable* sst) { void sstables_manager::remove(sstable* sst) { _undergoing_close.erase(_undergoing_close.iterator_to(*sst)); delete sst; - _sstable_deleted_event.signal(); + _components_memory_change_event.signal(); maybe_done(); } @@ -304,7 +304,7 @@ future<> sstables_manager::close() { co_await _done.get_future(); co_await _sstable_metadata_concurrency_sem.stop(); // stop the components reload fiber - _sstable_deleted_event.signal(); + _components_memory_change_event.signal(); co_await std::move(_components_reloader_status); } diff --git a/sstables/sstables_manager.hh b/sstables/sstables_manager.hh index a88ba6646c..f7418e29a2 100644 --- a/sstables/sstables_manager.hh +++ b/sstables/sstables_manager.hh @@ -108,8 +108,8 @@ private: size_t _total_memory_reclaimed{0}; // Set of sstables from which memory has been reclaimed set_type _reclaimed; - // Condition variable that gets notified when an sstable is deleted - seastar::condition_variable _sstable_deleted_event; + // Condition variable that needs to be notified when an sstable is created or deleted + seastar::condition_variable _components_memory_change_event; future<> _components_reloader_status = make_ready_future<>(); bool _closing = false; From 7f0f839d6d1bb45b9a15134bffc6e5ea558f3438 Mon Sep 17 00:00:00 2001 From: Lakshmi Narayanan Sreethar Date: Mon, 3 Feb 2025 14:51:10 +0530 Subject: [PATCH 09/12] sstables_manager: move reclaim logic into `components_reclaim_reload_fiber()` Move the sstable reclaim logic into `components_reclaim_reload_fiber()` in preparation for the fix for #21947. This also simplifies the overall reclaim/reload logic by preventing multiple fibers from attempting to reclaim/reload component memory concurrently. Also, update the existing test cases to adapt to this change. Signed-off-by: Lakshmi Narayanan Sreethar --- sstables/sstables_manager.cc | 10 ++++++++-- test/boost/bloom_filter_test.cc | 29 +++++++++++++++-------------- 2 files changed, 23 insertions(+), 16 deletions(-) diff --git a/sstables/sstables_manager.cc b/sstables/sstables_manager.cc index bd76a4bbe5..e6159fddae 100644 --- a/sstables/sstables_manager.cc +++ b/sstables/sstables_manager.cc @@ -154,7 +154,7 @@ sstable_writer_config sstables_manager::configure_writer(sstring origin) const { void sstables_manager::increment_total_reclaimable_memory_and_maybe_reclaim(sstable* sst) { _total_reclaimable_memory += sst->total_reclaimable_memory_size(); - maybe_reclaim_components(); + _components_memory_change_event.signal(); } void sstables_manager::maybe_reclaim_components() { @@ -195,7 +195,13 @@ future<> sstables_manager::components_reclaim_reload_fiber() { co_return; } - co_await maybe_reload_components(); + if (_total_reclaimable_memory > get_components_memory_reclaim_threshold()) { + // reclaim memory to bring total memory usage under threshold + maybe_reclaim_components(); + } else { + // memory available for reloading components of previously reclaimed SSTables + co_await maybe_reload_components(); + } } } diff --git a/test/boost/bloom_filter_test.cc b/test/boost/bloom_filter_test.cc index 5abb201798..9f9805bf5f 100644 --- a/test/boost/bloom_filter_test.cc +++ b/test/boost/bloom_filter_test.cc @@ -78,8 +78,8 @@ SEASTAR_TEST_CASE(test_sstable_manager_auto_reclaim_and_reload_of_bloom_filter) // Verify manager reclaims from the largest sst when the total usage crosses thresold. auto [sst3, sst3_bf_memory] = create_sstable_with_bloom_filter(env, sst_mgr, schema_ptr, 50); - // sst1 has the most reclaimable memory - BOOST_REQUIRE_EQUAL(sst1->filter_memory_size(), 0); + // sst1 has the most reclaimable memory, so its filter should be reclaimed + REQUIRE_EVENTUALLY_EQUAL([&] { return sst1->filter_memory_size(); }, 0); BOOST_REQUIRE_EQUAL(sst2->filter_memory_size(), sst2_bf_memory); BOOST_REQUIRE_EQUAL(sst3->filter_memory_size(), sst3_bf_memory); BOOST_REQUIRE_EQUAL(sst_mgr.get_total_memory_reclaimed(), sst1_bf_memory); @@ -87,10 +87,10 @@ SEASTAR_TEST_CASE(test_sstable_manager_auto_reclaim_and_reload_of_bloom_filter) // Reclaim should also work on the latest sst being added auto [sst4, sst4_bf_memory] = create_sstable_with_bloom_filter(env, sst_mgr, schema_ptr, 100); // sst4 should have been reclaimed + REQUIRE_EVENTUALLY_EQUAL([&] { return sst4->filter_memory_size(); }, 0); BOOST_REQUIRE_EQUAL(sst1->filter_memory_size(), 0); BOOST_REQUIRE_EQUAL(sst2->filter_memory_size(), sst2_bf_memory); BOOST_REQUIRE_EQUAL(sst3->filter_memory_size(), sst3_bf_memory); - BOOST_REQUIRE_EQUAL(sst4->filter_memory_size(), 0); BOOST_REQUIRE_EQUAL(sst_mgr.get_total_memory_reclaimed(), sst1_bf_memory + sst4_bf_memory); // Test auto reload - disposing sst3 should trigger reload of the @@ -152,7 +152,7 @@ SEASTAR_TEST_CASE(test_bloom_filter_reclaim_during_reload) { auto [sst2, sst2_bf_memory] = create_sstable_with_bloom_filter(env, sst_mgr, schema_ptr, 60); // total memory used by the bloom filters has crossed the threshold, so sst1's // filter, which occupies the most memory, will be discarded from memory. - BOOST_REQUIRE_EQUAL(sst1->filter_memory_size(), 0); + REQUIRE_EVENTUALLY_EQUAL([&] { return sst1->filter_memory_size(); }, 0); BOOST_REQUIRE_EQUAL(sst2->filter_memory_size(), sst2_bf_memory); BOOST_REQUIRE_EQUAL(sst_mgr.get_total_memory_reclaimed(), sst1_bf_memory); @@ -164,20 +164,21 @@ SEASTAR_TEST_CASE(test_bloom_filter_reclaim_during_reload) { // _total_reclaimable_memory will be updated when the reload begins; wait for it. REQUIRE_EVENTUALLY_EQUAL([&] { return sst_mgr.get_total_reclaimable_memory(); }, sst1_bf_memory); - // now that the reload is midway and paused, create new sst to verify that its - // filter gets evicted immediately as the memory that became available is reserved - // for sst1's filter reload. + // now that the reload is midway and paused, create new sst; + // it will not be reclaimed immediately as another reload is in progress auto [sst3, sst3_bf_memory] = create_sstable_with_bloom_filter(env, sst_mgr, schema_ptr, 80); - BOOST_REQUIRE_EQUAL(sst3->filter_memory_size(), 0); - // confirm sst1 is not reloaded yet + REQUIRE_EVENTUALLY_EQUAL([&] { return sst3->filter_memory_size(); }, sst3_bf_memory); + BOOST_REQUIRE_EQUAL(sst_mgr.get_total_reclaimable_memory(), sst1_bf_memory + sst3_bf_memory); + // verify sst1 is not actually reloaded yet BOOST_REQUIRE_EQUAL(sst1->filter_memory_size(), 0); - BOOST_REQUIRE_EQUAL(sst_mgr.get_total_memory_reclaimed(), sst1_bf_memory + sst3_bf_memory); + BOOST_REQUIRE_EQUAL(sst_mgr.get_total_memory_reclaimed(), sst1_bf_memory); - // resume reloading sst1 filter + // Resume reloading sst1 filter - it will eventually be reclaimed again utils::get_local_injector().receive_message("reload_reclaimed_components/pause"); - REQUIRE_EVENTUALLY_EQUAL([&] { return sst1->filter_memory_size(); }, sst1_bf_memory); - REQUIRE_EVENTUALLY_EQUAL([&] { return sst_mgr.get_total_memory_reclaimed(); }, sst3_bf_memory); - BOOST_REQUIRE_EQUAL(sst_mgr.get_total_reclaimable_memory(), sst1_bf_memory); + // Eventually only sst3's bloom filter will be in memory + REQUIRE_EVENTUALLY_EQUAL([&] { return sst_mgr.get_total_reclaimable_memory(); }, sst3_bf_memory); + BOOST_REQUIRE_EQUAL(sst_mgr.get_total_memory_reclaimed(), sst1_bf_memory); + BOOST_REQUIRE_EQUAL(sst1->filter_memory_size(), 0); utils::get_local_injector().disable("reload_reclaimed_components/pause"); }, { From 77107ddaa3a7b10e14fb4e9eed3221e8c5d0beef Mon Sep 17 00:00:00 2001 From: Lakshmi Narayanan Sreethar Date: Mon, 3 Feb 2025 15:09:19 +0530 Subject: [PATCH 10/12] sstables_manager: rename `increment_total_reclaimable_memory_and_maybe_reclaim()` Renamed the aboved mentioned method to `increment_total_reclaimable_memory()` as it doesn't directly reclaim memory anymore. Signed-off-by: Lakshmi Narayanan Sreethar --- sstables/sstables.cc | 4 ++-- sstables/sstables_manager.cc | 2 +- sstables/sstables_manager.hh | 6 ++---- test/lib/sstable_test_env.hh | 2 +- 4 files changed, 6 insertions(+), 8 deletions(-) diff --git a/sstables/sstables.cc b/sstables/sstables.cc index 58d2d52beb..9525fc037b 100644 --- a/sstables/sstables.cc +++ b/sstables/sstables.cc @@ -1364,7 +1364,7 @@ future<> sstable::open_data(sstable_open_config cfg) noexcept { _stats.on_open_for_reading(); _total_reclaimable_memory.reset(); - _manager.increment_total_reclaimable_memory_and_maybe_reclaim(this); + _manager.increment_total_reclaimable_memory(this); } future<> sstable::update_info_for_opened_data(sstable_open_config cfg) { @@ -1608,7 +1608,7 @@ future<> sstable::load(sstables::foreign_sstable_open_info info) noexcept { validate_partitioner(); co_await update_info_for_opened_data(); _total_reclaimable_memory.reset(); - _manager.increment_total_reclaimable_memory_and_maybe_reclaim(this); + _manager.increment_total_reclaimable_memory(this); } future sstable::get_open_info() & { diff --git a/sstables/sstables_manager.cc b/sstables/sstables_manager.cc index e6159fddae..08ed145d2f 100644 --- a/sstables/sstables_manager.cc +++ b/sstables/sstables_manager.cc @@ -152,7 +152,7 @@ sstable_writer_config sstables_manager::configure_writer(sstring origin) const { return cfg; } -void sstables_manager::increment_total_reclaimable_memory_and_maybe_reclaim(sstable* sst) { +void sstables_manager::increment_total_reclaimable_memory(sstable* sst) { _total_reclaimable_memory += sst->total_reclaimable_memory_size(); _components_memory_change_event.signal(); } diff --git a/sstables/sstables_manager.hh b/sstables/sstables_manager.hh index f7418e29a2..8f0e0f018d 100644 --- a/sstables/sstables_manager.hh +++ b/sstables/sstables_manager.hh @@ -212,10 +212,8 @@ private: // Allow at most 10% of memory to be filled with such reads. size_t max_memory_sstable_metadata_concurrent_reads(size_t available_memory) { return available_memory * 0.1; } - // Increment the _total_reclaimable_memory with the new SSTable's reclaimable - // memory and if the total memory usage exceeds the pre-defined threshold, - // reclaim it from the SSTable that has the most reclaimable memory. - void increment_total_reclaimable_memory_and_maybe_reclaim(sstable* sst); + // Increment the _total_reclaimable_memory with the new SSTable's reclaimable memory + void increment_total_reclaimable_memory(sstable* sst); // Fiber to reload reclaimed components back into memory when memory becomes available. future<> components_reclaim_reload_fiber(); // Reclaims components from SSTables if total memory usage exceeds the threshold. diff --git a/test/lib/sstable_test_env.hh b/test/lib/sstable_test_env.hh index 4e6b77f743..ddd89daea0 100644 --- a/test/lib/sstable_test_env.hh +++ b/test/lib/sstable_test_env.hh @@ -53,7 +53,7 @@ public: } void increment_total_reclaimable_memory_and_maybe_reclaim(sstable *sst) { - sstables_manager::increment_total_reclaimable_memory_and_maybe_reclaim(sst); + sstables_manager::increment_total_reclaimable_memory(sst); } size_t get_total_memory_reclaimed() { From 10fffcd6462f569c87fab12b898b95b7b283f424 Mon Sep 17 00:00:00 2001 From: Lakshmi Narayanan Sreethar Date: Mon, 3 Feb 2025 15:13:29 +0530 Subject: [PATCH 11/12] sstables_manager: maybe_reclaim_components: yield between iterations Signed-off-by: Lakshmi Narayanan Sreethar --- sstables/sstables_manager.cc | 5 +++-- sstables/sstables_manager.hh | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/sstables/sstables_manager.cc b/sstables/sstables_manager.cc index 08ed145d2f..ea53f27246 100644 --- a/sstables/sstables_manager.cc +++ b/sstables/sstables_manager.cc @@ -157,7 +157,7 @@ void sstables_manager::increment_total_reclaimable_memory(sstable* sst) { _components_memory_change_event.signal(); } -void sstables_manager::maybe_reclaim_components() { +future<> sstables_manager::maybe_reclaim_components() { while(_total_reclaimable_memory > get_components_memory_reclaim_threshold()) { // Memory consumption is above threshold. Reclaim from the SSTable that // has the most reclaimable memory to get the total consumption under limit. @@ -174,6 +174,7 @@ void sstables_manager::maybe_reclaim_components() { smlogger.info("Reclaimed {} bytes of memory from components of {}. Total memory reclaimed so far is {} bytes", memory_reclaimed, sst_with_max_memory->get_filename(), _total_memory_reclaimed); } + co_await coroutine::maybe_yield(); } size_t sstables_manager::get_components_memory_reclaim_threshold() const { @@ -197,7 +198,7 @@ future<> sstables_manager::components_reclaim_reload_fiber() { if (_total_reclaimable_memory > get_components_memory_reclaim_threshold()) { // reclaim memory to bring total memory usage under threshold - maybe_reclaim_components(); + co_await maybe_reclaim_components(); } else { // memory available for reloading components of previously reclaimed SSTables co_await maybe_reload_components(); diff --git a/sstables/sstables_manager.hh b/sstables/sstables_manager.hh index 8f0e0f018d..9612b96794 100644 --- a/sstables/sstables_manager.hh +++ b/sstables/sstables_manager.hh @@ -217,7 +217,7 @@ private: // Fiber to reload reclaimed components back into memory when memory becomes available. future<> components_reclaim_reload_fiber(); // Reclaims components from SSTables if total memory usage exceeds the threshold. - void maybe_reclaim_components(); + future<> maybe_reclaim_components(); // Reloads components from reclaimed SSTables if memory is available. future<> maybe_reload_components(); size_t get_components_memory_reclaim_threshold() const; From 064bf2fd857684d1dfefab564f4a681e39bf7a96 Mon Sep 17 00:00:00 2001 From: Lakshmi Narayanan Sreethar Date: Tue, 4 Feb 2025 21:39:03 +0530 Subject: [PATCH 12/12] sstables_manager: trigger reclaim/reload on `components_memory_reclaim_threshold` update The config variable `components_memory_reclaim_threshold` limits the memory available to the sstable bloom filters. Any change to its value is not immediately propagated to the sstable manager, despite it being a LiveUpdate variable. The updated value takes effect only when a new sstable is created or deleted. This patch updates the sstable manager to subscribe to any changes in the above mentioned config value and immediately trigger the reclaim/reload fiber when a change occurs. Also, adds a testcase to verify the fix. Fixes #21947 Signed-off-by: Lakshmi Narayanan Sreethar --- sstables/sstables_manager.cc | 6 ++++ test/boost/bloom_filter_test.cc | 58 +++++++++++++++++++++++++++++++++ 2 files changed, 64 insertions(+) diff --git a/sstables/sstables_manager.cc b/sstables/sstables_manager.cc index ea53f27246..f8a6df1344 100644 --- a/sstables/sstables_manager.cc +++ b/sstables/sstables_manager.cc @@ -186,9 +186,15 @@ size_t sstables_manager::get_memory_available_for_reclaimable_components() const } future<> sstables_manager::components_reclaim_reload_fiber() { + auto components_memory_reclaim_threshold_observer = _db_config.components_memory_reclaim_threshold.observe([&] (double) { + // any change to the components_memory_reclaim_threshold config should trigger reload/reclaim + _components_memory_change_event.signal(); + }); + co_await coroutine::switch_to(_maintenance_sg); sstlog.trace("components_reloader_fiber start"); + while (true) { co_await _components_memory_change_event.when(); diff --git a/test/boost/bloom_filter_test.cc b/test/boost/bloom_filter_test.cc index 9f9805bf5f..4e83a095d5 100644 --- a/test/boost/bloom_filter_test.cc +++ b/test/boost/bloom_filter_test.cc @@ -13,6 +13,7 @@ #include "test/lib/sstable_test_env.hh" #include "test/lib/sstable_utils.hh" +#include "db/config.hh" #include "readers/from_mutations_v2.hh" #include "utils/bloom_filter.hh" #include "utils/error_injection.hh" @@ -347,3 +348,60 @@ SEASTAR_TEST_CASE(test_bloom_filter_reclaim_after_unlink) { .available_memory = 100 }); }; + +SEASTAR_TEST_CASE(test_components_memory_reclaim_threshold_liveupdateness) { + return test_env::do_with_async([] (test_env& env) { + simple_schema ss; + auto schema_ptr = ss.schema(); + auto& sst_mgr = env.manager(); + BOOST_REQUIRE_EQUAL(env.db_config().components_memory_reclaim_threshold(), 0.2); + + // create a few sstables and verify their bloom filters are still in memory + auto [sst1, sst1_bf_memory] = create_sstable_with_bloom_filter(env, sst_mgr, schema_ptr, 70); + auto [sst2, sst2_bf_memory] = create_sstable_with_bloom_filter(env, sst_mgr, schema_ptr, 50); + auto [sst3, sst3_bf_memory] = create_sstable_with_bloom_filter(env, sst_mgr, schema_ptr, 20); + BOOST_REQUIRE_EQUAL(sst1->filter_memory_size(), sst1_bf_memory); + BOOST_REQUIRE_EQUAL(sst2->filter_memory_size(), sst2_bf_memory); + BOOST_REQUIRE_EQUAL(sst3->filter_memory_size(), sst3_bf_memory); + BOOST_REQUIRE_EQUAL(sst_mgr.get_total_memory_reclaimed(), 0); + BOOST_REQUIRE_EQUAL(sst_mgr.get_total_reclaimable_memory(), sst1_bf_memory + sst2_bf_memory + sst3_bf_memory); + + // reduce the threshold to 0.1 and verify that sst1's bloom filter, which occupies most memory, gets evicted + env.db_config().components_memory_reclaim_threshold.set(0.1); + REQUIRE_EVENTUALLY_EQUAL([&] { return sst1->filter_memory_size(); }, 0); + BOOST_REQUIRE_EQUAL(sst_mgr.get_total_memory_reclaimed(), sst1_bf_memory); + // the other two ssts are untouched + BOOST_REQUIRE_EQUAL(sst2->filter_memory_size(), sst2_bf_memory); + BOOST_REQUIRE_EQUAL(sst3->filter_memory_size(), sst3_bf_memory); + BOOST_REQUIRE_EQUAL(sst_mgr.get_total_reclaimable_memory(), sst2_bf_memory + sst3_bf_memory); + + // reduce the threshold to 0 and verify that no bloom filter is in memory + env.db_config().components_memory_reclaim_threshold.set(0); + REQUIRE_EVENTUALLY_EQUAL([&] { return sst_mgr.get_total_memory_reclaimed(); }, sst1_bf_memory + sst2_bf_memory + sst3_bf_memory); + BOOST_REQUIRE_EQUAL(sst_mgr.get_total_reclaimable_memory(), 0); + BOOST_REQUIRE_EQUAL(sst1->filter_memory_size(), 0); + BOOST_REQUIRE_EQUAL(sst2->filter_memory_size(), 0); + BOOST_REQUIRE_EQUAL(sst3->filter_memory_size(), 0); + + // increase threshold back 0.1 and expect sst2 and sst3's bloom filter to be reloaded + env.db_config().components_memory_reclaim_threshold.set(0.1); + REQUIRE_EVENTUALLY_EQUAL([&] { return sst3->filter_memory_size(); }, sst3_bf_memory); + REQUIRE_EVENTUALLY_EQUAL([&] { return sst2->filter_memory_size(); }, sst2_bf_memory); + BOOST_REQUIRE_EQUAL(sst_mgr.get_total_reclaimable_memory(), sst2_bf_memory + sst3_bf_memory); + // sst1's bloom filter is not reloaded yet due to lack of available memory + REQUIRE_EVENTUALLY_EQUAL([&] { return sst_mgr.get_total_memory_reclaimed(); }, sst1_bf_memory); + BOOST_REQUIRE_EQUAL(sst1->filter_memory_size(), 0); + + // increase threshold back to 0.2 and expect sst1 to be reloaded + env.db_config().components_memory_reclaim_threshold.set(0.2); + REQUIRE_EVENTUALLY_EQUAL([&] { return sst_mgr.get_total_memory_reclaimed(); }, 0); + BOOST_REQUIRE_EQUAL(sst1->filter_memory_size(), sst1_bf_memory); + BOOST_REQUIRE_EQUAL(sst2->filter_memory_size(), sst2_bf_memory); + BOOST_REQUIRE_EQUAL(sst3->filter_memory_size(), sst3_bf_memory); + BOOST_REQUIRE_EQUAL(sst_mgr.get_total_reclaimable_memory(), sst1_bf_memory + sst2_bf_memory + sst3_bf_memory); + }, { + // limit available memory to the sstables_manager to test reclaiming. + // this will set the reclaim threshold to 200 bytes. + .available_memory = 1000 + }); +}