This is an automated email from the ASF dual-hosted git repository. yiguolei pushed a commit to branch branch-4.2 in repository https://gitbox.apache.org/repos/asf/doris.git
commit 57ae1dc73615d7a47c6a85a5ee29a518bbad596c Author: deardeng <[email protected]> AuthorDate: Tue Oct 13 10:22:28 2026 +0800 [fix](file cache) keep disk resource limit mode hysteresis across checks (#67313) (#68945) pick from https://github.com/apache/doris/pull/67313 Problem Summary: The disk-pressure protection mode is meant to enter at `enter` (85%) and stay on until usage drops below `exit` (80%), holding its previous state in between. That hold is implemented by `_disk_resource_limit_mode` remembering the previous round — and `check_disk_resource_limit()` wiped it before reading `statfs`: ```cpp if (_capacity > _cur_cache_size) { _disk_resource_limit_mode = false; // runs before statfs is read } ... } else if (_disk_resource_limit_mode && space < exit && inode < exit) { ``` With the memory gone, the exit branch is dead code: the mode dropped as soon as usage fell below `enter`, never holding down to `exit`. The `disk_resource_limit_mode` bvar was unusable for the same reason — a single round wrote 0 and then 1 whenever the mode was immediately re-entered, so it could not be used to tell whether a node was in the mode. 1. Drop the pre-clear so the band holds, and publish `_disk_limit_mode_metrics` once, after the decision. 2. Drop the `_disk_resource_limit_mode = true` that `reset_capacity()` forces on when shrinking — see below. 3. Make `_disk_resource_limit_mode` and `_need_evict_cache_in_advance` `std::atomic<bool>`. `run_background_monitor()` writes them without the cache lock while `try_reserve()`, `is_overflow()` and `run_background_gc()` read them, so both plain bools were already racy on master. Both lines are halves of a mechanism that no longer exists. They arrived together in #37484, when shrinking was asynchronous: ```cpp cell->is_deleted = true; // tagged only, nothing freed here ... _disk_resource_limit_mode = true; _async_clear_file_cache = true; // a background thread does the removal ``` `reset_capacity()` returned with `_cur_cache_size` still above the new capacity, so the force made `try_reserve()` drain aggressively while the background cleanup caught up, and `if (_capacity > _cur_cache_size)` was the matching "drained, stop" condition. Shrinking is synchronous now — `reset_capacity()` calls `remove()` under the cache lock and returns at the new capacity, and `_async_clear_file_cache` no longer exists in `be/src`. So the force has nothing left to accelerate, while the pre-clear kept firing on its own condition, which holds in ordinary operation whenever the cache is under its budget, not only while a shrink drains. That is the bug. Removing only the pre-clear would leave the force with nothing to turn it off: a shrink on a node whose disk sits in `[exit, enter)` for unrelated reasons — BE logs, another data dir on the same mount — would pin the mode on permanently, and every reservation would evict five times the requested size. The force was also not capacity enforcement during async metadata load: - The loader inserts restored cells through `add_cell()` and never calls `try_reserve()` (`fs_file_cache_storage.cpp:818`, `:1007`, `:1179`). - `try_reserve_during_async_load()` does not consult `_capacity`, and cannot: while loading, `_cur_cache_size` is a partial count growing from zero toward the restored total, so comparing it against the full budget is meaningless. `statfs` is the only signal valid at that point, which is why the mode is the only thing that function checks. Real, pre-existing on master, independent of the state machine touched here, each getting its own PR: - **Capacity admission during async metadata load.** Making it work needs the loader to publish the on-disk total up front, or admission deferred until loading completes — a change to the loader's publication protocol, not an `if` in the reservation path. - **The eviction target reused as the admitted size.** `try_reserve()` does `size = 5 * size` under disk pressure and passes that to `QueryFileCacheContext::reserve()`, while `add_cell()` creates a cell of the original size, so `DCHECK(iter->size == cell_size)` at `block_file_cache.cpp:1483` can fire and release builds mis-account query usage. Already reachable on master whenever the disk is at or above `enter` with `enable_file_cache_query_limit` on. - **Republication over deleted files during load.** `load_cache_info_into_memory_from_db()` buffers 10k `BatchLoadArgs` outside `_mutex` (`fs_file_cache_storage.cpp:995`) and `handle_already_loaded_block()` checks only whether a cell exists, never whether the file is still on disk. (cherry picked from commit 4de121c3b6208d9b85cd6530876a7d86d2b5ffa7) ### What problem does this PR solve? Issue Number: close #xxx Related PR: #xxx Problem Summary: ### Release note None ### Check List (For Author) - Test <!-- At least one of them must be included. --> - [ ] Regression test - [ ] Unit Test - [ ] Manual test (add detailed scripts or steps below) - [ ] No need to test or manual test. Explain why: - [ ] This is a refactor/code format and no logic has been changed. - [ ] Previous test can cover this change. - [ ] No code files have been changed. - [ ] Other reason <!-- Add your reason? --> - Behavior changed: - [ ] No. - [ ] Yes. <!-- Explain the behavior change --> - Does this need documentation? - [ ] No. - [ ] Yes. <!-- Add document PR link here. eg: https://github.com/apache/doris-website/pull/1214 --> ### Check List (For Reviewer who merge this PR) - [ ] Confirm the release note - [ ] Confirm test cases - [ ] Confirm document - [ ] Add branch pick label <!-- Add branch pick label that this PR should merge into --> --- be/src/io/cache/block_file_cache.cpp | 29 ++++---- be/src/io/cache/block_file_cache.h | 4 +- be/test/io/cache/block_file_cache_test.cpp | 108 +++++++++++++++++++++++++---- 3 files changed, 112 insertions(+), 29 deletions(-) diff --git a/be/src/io/cache/block_file_cache.cpp b/be/src/io/cache/block_file_cache.cpp index f78fe10ef41..c5ebc7cdbfb 100644 --- a/be/src/io/cache/block_file_cache.cpp +++ b/be/src/io/cache/block_file_cache.cpp @@ -2204,8 +2204,6 @@ std::string BlockFileCache::reset_capacity(size_t new_capacity) { queue_released = remove_blocks(_ttl_queue); ss << " ttl_queue released " << queue_released; - _disk_resource_limit_mode = true; - _disk_limit_mode_metrics->set_value(1); ss << " total_space_released=" << space_released; } old_capacity = _capacity; @@ -2224,10 +2222,7 @@ void BlockFileCache::check_disk_resource_limit() { if (_storage->get_type() != FileCacheStorageType::DISK) { return; } - if (_capacity > _cur_cache_size) { - _disk_resource_limit_mode = false; - _disk_limit_mode_metrics->set_value(0); - } + std::pair<int, int> percent; int ret = disk_used_percentage(_cache_base_path, &percent); if (ret != 0) { @@ -2252,20 +2247,24 @@ void BlockFileCache::check_disk_resource_limit() { config::file_cache_enter_disk_resource_limit_mode_percent = 88; config::file_cache_exit_disk_resource_limit_mode_percent = 80; } - if (is_insufficient(space_percentage) || is_insufficient(inode_percentage)) { + bool is_space_insufficient = is_insufficient(space_percentage); + bool is_inode_insufficient = is_insufficient(inode_percentage); + // Enter when either resource reaches the enter threshold, but exit only after both + // resources fall below the exit threshold. Values in [exit, enter) preserve the previous + // mode through _disk_resource_limit_mode. + if (is_space_insufficient || is_inode_insufficient) { _disk_resource_limit_mode = true; - _disk_limit_mode_metrics->set_value(1); } else if (_disk_resource_limit_mode && (space_percentage < config::file_cache_exit_disk_resource_limit_mode_percent) && (inode_percentage < config::file_cache_exit_disk_resource_limit_mode_percent)) { _disk_resource_limit_mode = false; - _disk_limit_mode_metrics->set_value(0); } + _disk_limit_mode_metrics->set_value(_disk_resource_limit_mode.load()); if (_disk_resource_limit_mode) { LOG(WARNING) << "file_cache=" << get_base_path() << " space_percent=" << space_percentage << " inode_percent=" << inode_percentage - << " is_space_insufficient=" << is_insufficient(space_percentage) - << " is_inode_insufficient=" << is_insufficient(inode_percentage) + << " is_space_insufficient=" << is_space_insufficient + << " is_inode_insufficient=" << is_inode_insufficient << " mode run in resource limit"; } } @@ -2726,8 +2725,8 @@ std::map<std::string, double> BlockFileCache::get_stats() { (double)_lru_recorder_shadow_queue_element_count_metrics[FileCacheType::DISPOSABLE] ->get_value(); - stats["need_evict_cache_in_advance"] = (double)_need_evict_cache_in_advance; - stats["disk_resource_limit_mode"] = (double)_disk_resource_limit_mode; + stats["need_evict_cache_in_advance"] = (double)_need_evict_cache_in_advance.load(); + stats["disk_resource_limit_mode"] = (double)_disk_resource_limit_mode.load(); stats["total_removed_counts"] = (double)_num_removed_blocks->get_value(); stats["total_hit_counts"] = (double)_num_hit_blocks->get_value(); @@ -2781,8 +2780,8 @@ std::map<std::string, double> BlockFileCache::get_stats_unsafe() { (double)_lru_recorder_shadow_queue_element_count_metrics[FileCacheType::DISPOSABLE] ->get_value(); - stats["need_evict_cache_in_advance"] = (double)_need_evict_cache_in_advance; - stats["disk_resource_limit_mode"] = (double)_disk_resource_limit_mode; + stats["need_evict_cache_in_advance"] = (double)_need_evict_cache_in_advance.load(); + stats["disk_resource_limit_mode"] = (double)_disk_resource_limit_mode.load(); stats["total_removed_counts"] = (double)_num_removed_blocks->get_value(); stats["total_hit_counts"] = (double)_num_hit_blocks->get_value(); diff --git a/be/src/io/cache/block_file_cache.h b/be/src/io/cache/block_file_cache.h index caf9fed6b2a..9b32b4764b4 100644 --- a/be/src/io/cache/block_file_cache.h +++ b/be/src/io/cache/block_file_cache.h @@ -554,8 +554,8 @@ private: std::thread _cache_background_block_lru_update_thread; std::atomic_bool _async_open_done {false}; // disk space or inode is less than the specified value - bool _disk_resource_limit_mode {false}; - bool _need_evict_cache_in_advance {false}; + std::atomic<bool> _disk_resource_limit_mode {false}; + std::atomic<bool> _need_evict_cache_in_advance {false}; bool _is_initialized {false}; // strategy diff --git a/be/test/io/cache/block_file_cache_test.cpp b/be/test/io/cache/block_file_cache_test.cpp index 8d8dee523dd..dbd0b623b97 100644 --- a/be/test/io/cache/block_file_cache_test.cpp +++ b/be/test/io/cache/block_file_cache_test.cpp @@ -5997,7 +5997,7 @@ TEST_F(BlockFileCacheTest, test_check_disk_reource_limit_2) { std::this_thread::sleep_for(std::chrono::milliseconds(10)); EXPECT_EQ(config::file_cache_enter_disk_resource_limit_mode_percent, 2); EXPECT_EQ(config::file_cache_exit_disk_resource_limit_mode_percent, 1); - EXPECT_TRUE(cache._disk_resource_limit_mode); + EXPECT_TRUE(cache._disk_resource_limit_mode.load()); config::file_cache_enter_disk_resource_limit_mode_percent = 99; if (fs::exists(cache_base_path)) { fs::remove_all(cache_base_path); @@ -6026,13 +6026,93 @@ TEST_F(BlockFileCacheTest, test_check_disk_reource_limit_3) { std::this_thread::sleep_for(std::chrono::milliseconds(1)); } std::this_thread::sleep_for(std::chrono::milliseconds(10)); - EXPECT_FALSE(cache._disk_resource_limit_mode); + EXPECT_FALSE(cache._disk_resource_limit_mode.load()); config::file_cache_exit_disk_resource_limit_mode_percent = 80; if (fs::exists(cache_base_path)) { fs::remove_all(cache_base_path); } } +TEST_F(BlockFileCacheTest, test_check_disk_resource_limit_hysteresis) { + if (fs::exists(cache_base_path)) { + fs::remove_all(cache_base_path); + } + fs::create_directories(cache_base_path); + + const auto origin_enter = config::file_cache_enter_disk_resource_limit_mode_percent; + const auto origin_exit = config::file_cache_exit_disk_resource_limit_mode_percent; + auto* sp = SyncPoint::get_instance(); + Defer defer {[&] { + config::file_cache_enter_disk_resource_limit_mode_percent = origin_enter; + config::file_cache_exit_disk_resource_limit_mode_percent = origin_exit; + sp->disable_processing(); + sp->clear_call_back("BlockFileCache::disk_used_percentage:1"); + if (fs::exists(cache_base_path)) { + fs::remove_all(cache_base_path); + } + }}; + + config::file_cache_enter_disk_resource_limit_mode_percent = 85; + config::file_cache_exit_disk_resource_limit_mode_percent = 80; + + io::FileCacheSettings settings; + settings.capacity = 100_mb; + settings.storage = "disk"; + io::BlockFileCache cache(cache_base_path, settings); + + std::pair<int, int> disk_usage {90, 70}; + sp->set_call_back("BlockFileCache::disk_used_percentage:1", [&](auto&& values) { + *try_any_cast<std::pair<int, int>*>(values.back()) = disk_usage; + }); + sp->enable_processing(); + + cache.check_disk_resource_limit(); + EXPECT_TRUE(cache._disk_resource_limit_mode.load()); + EXPECT_EQ(cache._disk_limit_mode_metrics->get_value(), cache._disk_resource_limit_mode.load()); + + cache._disk_resource_limit_mode = false; + disk_usage = {70, 90}; + cache.check_disk_resource_limit(); + EXPECT_TRUE(cache._disk_resource_limit_mode.load()); + EXPECT_EQ(cache._disk_limit_mode_metrics->get_value(), cache._disk_resource_limit_mode.load()); + + ASSERT_GT(cache._capacity, cache._cur_cache_size); + disk_usage = {82, 70}; + cache.check_disk_resource_limit(); + EXPECT_TRUE(cache._disk_resource_limit_mode.load()); + EXPECT_EQ(cache._disk_limit_mode_metrics->get_value(), cache._disk_resource_limit_mode.load()); + + disk_usage = {70, 70}; + cache.check_disk_resource_limit(); + EXPECT_FALSE(cache._disk_resource_limit_mode.load()); + EXPECT_EQ(cache._disk_limit_mode_metrics->get_value(), cache._disk_resource_limit_mode.load()); +} + +TEST_F(BlockFileCacheTest, test_check_disk_resource_limit_statfs_failure_preserves_state) { + if (fs::exists(cache_base_path)) { + fs::remove_all(cache_base_path); + } + fs::create_directories(cache_base_path); + Defer cleanup {[&] { + if (fs::exists(cache_base_path)) { + fs::remove_all(cache_base_path); + } + }}; + + io::FileCacheSettings settings; + settings.capacity = 100_mb; + settings.storage = "disk"; + io::BlockFileCache cache(cache_base_path, settings); + cache._disk_resource_limit_mode = true; + cache._disk_limit_mode_metrics->set_value(1); + cache._cache_base_path = "/non/existent/path/OOXXOO"; + + cache.check_disk_resource_limit(); + + EXPECT_TRUE(cache._disk_resource_limit_mode.load()); + EXPECT_EQ(cache._disk_limit_mode_metrics->get_value(), 1); +} + TEST_F(BlockFileCacheTest, test_align_size) { const size_t total_size = 10_mb + 10086; { @@ -6504,9 +6584,13 @@ TEST_F(BlockFileCacheTest, reset_capacity) { assert_range(1, segments[0], io::FileBlock::Range(offset, offset + 4), io::FileBlock::State::DOWNLOADED); } + cache._disk_resource_limit_mode = false; + cache._disk_limit_mode_metrics->set_value(0); std::cout << cache.reset_capacity(30) << std::endl; EXPECT_EQ(cache._cur_cache_size, 30); + EXPECT_FALSE(cache._disk_resource_limit_mode.load()); + EXPECT_EQ(cache._disk_limit_mode_metrics->get_value(), 0); if (fs::exists(cache_base_path)) { fs::remove_all(cache_base_path); } @@ -8334,9 +8418,9 @@ TEST_F(BlockFileCacheTest, test_check_need_evict_cache_in_advance) { { settings.storage = "memory"; io::BlockFileCache cache(cache_base_path, settings); - ASSERT_FALSE(cache._need_evict_cache_in_advance); + ASSERT_FALSE(cache._need_evict_cache_in_advance.load()); cache.check_need_evict_cache_in_advance(); - ASSERT_FALSE(cache._need_evict_cache_in_advance); + ASSERT_FALSE(cache._need_evict_cache_in_advance.load()); } // the rest for disk @@ -8345,17 +8429,17 @@ TEST_F(BlockFileCacheTest, test_check_need_evict_cache_in_advance) { // bad disk path { io::BlockFileCache cache(cache_base_path, settings); - ASSERT_FALSE(cache._need_evict_cache_in_advance); + ASSERT_FALSE(cache._need_evict_cache_in_advance.load()); cache._cache_base_path = "/non/existent/path/OOXXOO"; cache.check_need_evict_cache_in_advance(); - ASSERT_FALSE(cache._need_evict_cache_in_advance); + ASSERT_FALSE(cache._need_evict_cache_in_advance.load()); } // conditions for enter need evict cache in advance { io::BlockFileCache cache(cache_base_path, settings); - ASSERT_FALSE(cache._need_evict_cache_in_advance); + ASSERT_FALSE(cache._need_evict_cache_in_advance.load()); // condition1 space usage rate exceed threshold config::file_cache_enter_need_evict_cache_in_advance_percent = 70; @@ -8370,7 +8454,7 @@ TEST_F(BlockFileCacheTest, test_check_need_evict_cache_in_advance) { SyncPoint::get_instance()->enable_processing(); cache.check_need_evict_cache_in_advance(); - ASSERT_TRUE(cache._need_evict_cache_in_advance); + ASSERT_TRUE(cache._need_evict_cache_in_advance.load()); SyncPoint::get_instance()->disable_processing(); SyncPoint::get_instance()->clear_all_call_backs(); @@ -8386,7 +8470,7 @@ TEST_F(BlockFileCacheTest, test_check_need_evict_cache_in_advance) { SyncPoint::get_instance()->enable_processing(); cache.check_need_evict_cache_in_advance(); - ASSERT_TRUE(cache._need_evict_cache_in_advance); + ASSERT_TRUE(cache._need_evict_cache_in_advance.load()); SyncPoint::get_instance()->disable_processing(); SyncPoint::get_instance()->clear_all_call_backs(); @@ -8394,7 +8478,7 @@ TEST_F(BlockFileCacheTest, test_check_need_evict_cache_in_advance) { cache._need_evict_cache_in_advance = false; cache._cur_cache_size = 80_mb; // set high cache.check_need_evict_cache_in_advance(); - ASSERT_TRUE(cache._need_evict_cache_in_advance); + ASSERT_TRUE(cache._need_evict_cache_in_advance.load()); } // conditions for exit need evict cache in advance @@ -8412,7 +8496,7 @@ TEST_F(BlockFileCacheTest, test_check_need_evict_cache_in_advance) { SyncPoint::get_instance()->enable_processing(); cache.check_need_evict_cache_in_advance(); - ASSERT_FALSE(cache._need_evict_cache_in_advance); + ASSERT_FALSE(cache._need_evict_cache_in_advance.load()); SyncPoint::get_instance()->disable_processing(); SyncPoint::get_instance()->clear_all_call_backs(); } @@ -8486,7 +8570,7 @@ TEST_F(BlockFileCacheTest, test_evict_cache_in_advance_skip) { ASSERT_TRUE(cache.get_async_open_success()); cache.check_need_evict_cache_in_advance(); - ASSERT_TRUE(cache._need_evict_cache_in_advance); + ASSERT_TRUE(cache._need_evict_cache_in_advance.load()); // Set recycle keys threshold and fill with enough keys config::file_cache_evict_in_advance_recycle_keys_num_threshold = 10; --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
