This is an automated email from the ASF dual-hosted git repository.
deardeng pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/doris.git
The following commit(s) were added to refs/heads/master by this push:
new 4de121c3b62 [fix](file cache) keep disk resource limit mode hysteresis
across checks (#67313)
4de121c3b62 is described below
commit 4de121c3b6208d9b85cd6530876a7d86d2b5ffa7
Author: deardeng <[email protected]>
AuthorDate: Fri Sep 18 14:14:11 2026 +0800
[fix](file cache) keep disk resource limit mode hysteresis across checks
(#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.
### What this PR changes
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.
### Why the `reset_capacity()` force goes too
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.
### Deliberately out of scope
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.
---
be/src/io/cache/block_file_cache.cpp | 28 ++++----
be/src/io/cache/block_file_cache.h | 4 +-
be/test/io/cache/block_file_cache_test.cpp | 108 +++++++++++++++++++++++++----
3 files changed, 110 insertions(+), 30 deletions(-)
diff --git a/be/src/io/cache/block_file_cache.cpp
b/be/src/io/cache/block_file_cache.cpp
index e3eaa691b9f..a4c769ad9bb 100644
--- a/be/src/io/cache/block_file_cache.cpp
+++ b/be/src/io/cache/block_file_cache.cpp
@@ -2201,8 +2201,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;
@@ -2222,11 +2220,6 @@ void BlockFileCache::check_disk_resource_limit() {
return;
}
- bool previous_mode = _disk_resource_limit_mode;
- 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) {
@@ -2251,18 +2244,21 @@ 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;
}
+ bool previous_mode = _disk_resource_limit_mode.load();
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);
}
- if (previous_mode != _disk_resource_limit_mode) {
+ _disk_limit_mode_metrics->set_value(_disk_resource_limit_mode.load());
+ if (previous_mode != _disk_resource_limit_mode.load()) {
// add log for disk resource limit mode switching
if (_disk_resource_limit_mode) {
LOG(WARNING) << "Entering disk resource limit mode: file_cache="
<< get_base_path()
@@ -2319,7 +2315,7 @@ void BlockFileCache::check_need_evict_cache_in_advance() {
config::file_cache_enter_need_evict_cache_in_advance_percent = 78;
config::file_cache_exit_need_evict_cache_in_advance_percent = 75;
}
- bool previous_mode = _need_evict_cache_in_advance;
+ bool previous_mode = _need_evict_cache_in_advance.load();
bool is_space_insufficient = is_insufficient(space_percentage);
bool is_inode_insufficient = is_insufficient(inode_percentage);
bool is_size_insufficient = is_insufficient(size_percentage);
@@ -2333,7 +2329,7 @@ void BlockFileCache::check_need_evict_cache_in_advance() {
_need_evict_cache_in_advance = false;
_need_evict_cache_in_advance_metrics->set_value(0);
}
- if (previous_mode != _need_evict_cache_in_advance) {
+ if (previous_mode != _need_evict_cache_in_advance.load()) {
// add log for evict cache in advance mode switching
if (_need_evict_cache_in_advance) {
LOG(WARNING) << "Entering evict cache in advance mode: "
@@ -2770,8 +2766,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();
@@ -2825,8 +2821,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 2de0182ba7f..69c79c50e52 100644
--- a/be/src/io/cache/block_file_cache.h
+++ b/be/src/io/cache/block_file_cache.h
@@ -559,8 +559,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 68d1d09db15..a84288f4521 100644
--- a/be/test/io/cache/block_file_cache_test.cpp
+++ b/be/test/io/cache/block_file_cache_test.cpp
@@ -6000,7 +6000,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);
@@ -6029,13 +6029,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;
{
@@ -6507,9 +6587,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);
}
@@ -8337,9 +8421,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
@@ -8348,17 +8432,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;
@@ -8373,7 +8457,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();
@@ -8389,7 +8473,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();
@@ -8397,7 +8481,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
@@ -8415,7 +8499,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();
}
@@ -8489,7 +8573,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]
