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 182022b593a [fix](file cache) keep the cache monitor off the LRU
recorder lock (#67315)
182022b593a is described below
commit 182022b593acfc959178d80cd9b7d110b93c2fe6
Author: deardeng <[email protected]>
AuthorDate: Thu Sep 3 19:57:22 2026 +0800
[fix](file cache) keep the cache monitor off the LRU recorder lock (#67315)
run_background_monitor() ended its loop with
update_shadow_queue_element_count_metrics(), which takes _mutex_lru_log.
The LRU log replay thread holds that lock for as long as it takes to
drain the log queue, so a slow consumer froze the monitor with it:
check_disk_resource_limit(), check_need_evict_cache_in_advance() and
every gauge stopped running, and the disk resource limit mode stayed at
whatever value it happened to hold. Gauges were observed frozen for up
to 40 minutes in production.
The call was redundant from the start. #64798 added the shadow queue
element count gauge and published it in two places: inside
replay_queue_event(), under the same lock that mutates the shadow queue,
and again from the monitor every
file_cache_background_monitor_interval_ms as a periodic refresh. Nothing
outside replay_queue_event() mutates a shadow queue, so that refresh
could only rewrite a value that had just been published and could not
have changed since. What it did add was a dependency from the disk
protection loop onto a lock owned by a background consumer.
Drop the call, and update_shadow_queue_element_count_metrics() with it:
it existed only for that refresh, and leaving a public method that takes
_mutex_lru_log invites the next background loop to reintroduce the
coupling. The gauge is still published by replay, now on the replay
interval instead of the monitor interval. Its test is replaced by one
asserting that replay publishes the gauge on its own.
How long replay holds the lock is a separate problem, addressed
separately.
---
be/src/io/cache/block_file_cache.cpp | 1 -
be/src/io/cache/lru_queue_recorder.cpp | 15 ---------------
be/src/io/cache/lru_queue_recorder.h | 3 ---
be/test/io/cache/cache_lru_dumper_test.cpp | 29 +++++++++++++++--------------
4 files changed, 15 insertions(+), 33 deletions(-)
diff --git a/be/src/io/cache/block_file_cache.cpp
b/be/src/io/cache/block_file_cache.cpp
index d0ea528bb3e..6b9ad1d84e4 100644
--- a/be/src/io/cache/block_file_cache.cpp
+++ b/be/src/io/cache/block_file_cache.cpp
@@ -2441,7 +2441,6 @@ void BlockFileCache::run_background_monitor() {
(double)_no_warmup_num_read_blocks_1h->get_value());
}
}
- _lru_recorder->update_shadow_queue_element_count_metrics();
}
}
diff --git a/be/src/io/cache/lru_queue_recorder.cpp
b/be/src/io/cache/lru_queue_recorder.cpp
index 82edc5f6070..d18ac047002 100644
--- a/be/src/io/cache/lru_queue_recorder.cpp
+++ b/be/src/io/cache/lru_queue_recorder.cpp
@@ -175,11 +175,6 @@ size_t LRUQueueRecorder::lru_log_queue_size(FileCacheType
type) const {
return
_lru_log_queue_size[file_cache_type_index(type)].load(std::memory_order_relaxed);
}
-void LRUQueueRecorder::update_shadow_queue_element_count_metrics() {
- std::lock_guard<std::mutex> lru_log_lock(_mutex_lru_log);
- update_shadow_queue_element_count_metrics_unlocked(lru_log_lock);
-}
-
void LRUQueueRecorder::limit_shadow_queue_size(LRUQueue& shadow_queue,
std::lock_guard<std::mutex>&
lru_log_lock) {
int64_t queue_limit =
config::file_cache_background_lru_dump_tail_record_num;
@@ -196,16 +191,6 @@ void LRUQueueRecorder::limit_shadow_queue_size(LRUQueue&
shadow_queue,
}
}
-void LRUQueueRecorder::update_shadow_queue_element_count_metrics_unlocked(
- std::lock_guard<std::mutex>& lru_log_lock) {
- for (FileCacheType type : {FileCacheType::DISPOSABLE,
FileCacheType::NORMAL,
- FileCacheType::INDEX, FileCacheType::TTL}) {
- size_t idx = file_cache_type_index(type);
- _mgr->_lru_recorder_shadow_queue_element_count_metrics[idx]->set_value(
- get_shadow_queue(type).get_elements_num(lru_log_lock));
- }
-}
-
bool LRUQueueRecorder::reserve_lru_log_queue_slot(FileCacheType type) {
int64_t queue_limit = config::file_cache_background_lru_log_queue_max_size;
if (queue_limit <= 0) {
diff --git a/be/src/io/cache/lru_queue_recorder.h
b/be/src/io/cache/lru_queue_recorder.h
index 1edd0f5ab85..058b78194ff 100644
--- a/be/src/io/cache/lru_queue_recorder.h
+++ b/be/src/io/cache/lru_queue_recorder.h
@@ -67,7 +67,6 @@ public:
size_t get_lru_queue_update_cnt_from_last_dump(FileCacheType type);
void reset_lru_queue_update_cnt_from_last_dump(FileCacheType type);
size_t lru_log_queue_size(FileCacheType type) const;
- void update_shadow_queue_element_count_metrics();
CacheLRULogQueue& get_lru_log_queue(FileCacheType type);
LRUQueue& get_shadow_queue(FileCacheType type);
@@ -94,8 +93,6 @@ private:
bool reserve_lru_log_queue_slot(FileCacheType type);
void release_lru_log_queue_slot(FileCacheType type);
void limit_shadow_queue_size(LRUQueue& shadow_queue,
std::lock_guard<std::mutex>& lru_log_lock);
- void update_shadow_queue_element_count_metrics_unlocked(
- std::lock_guard<std::mutex>& lru_log_lock);
};
} // namespace doris::io
diff --git a/be/test/io/cache/cache_lru_dumper_test.cpp
b/be/test/io/cache/cache_lru_dumper_test.cpp
index 6256f52d74a..e49a142bb2c 100644
--- a/be/test/io/cache/cache_lru_dumper_test.cpp
+++ b/be/test/io/cache/cache_lru_dumper_test.cpp
@@ -311,28 +311,29 @@ TEST_F(CacheLRUDumperTest,
test_remove_event_trims_existing_oversized_shadow_que
EXPECT_EQ(offsets, std::vector<size_t>({1, 2}));
}
-TEST_F(CacheLRUDumperTest,
test_update_shadow_queue_metric_does_not_trim_queue) {
+TEST_F(CacheLRUDumperTest, test_replay_publishes_shadow_queue_metric) {
const auto old_tail_record_num =
config::file_cache_background_lru_dump_tail_record_num;
- Defer defer {[old_tail_record_num] {
+ const auto old_queue_limit =
config::file_cache_background_lru_log_queue_max_size;
+ Defer defer {[old_tail_record_num, old_queue_limit] {
config::file_cache_background_lru_dump_tail_record_num =
old_tail_record_num;
+ config::file_cache_background_lru_log_queue_max_size = old_queue_limit;
}};
- config::file_cache_background_lru_dump_tail_record_num = 1;
+ config::file_cache_background_lru_dump_tail_record_num = 100;
+ config::file_cache_background_lru_log_queue_max_size = 100;
- UInt128Wrapper hash(778899ULL);
- {
- std::lock_guard lru_log_lock(recorder->_mutex_lru_log);
- auto& shadow_queue = recorder->get_shadow_queue(FileCacheType::INDEX);
- for (size_t offset = 0; offset < 3; ++offset) {
- shadow_queue.add(hash, offset, 4096, lru_log_lock);
- }
+ UInt128Wrapper hash(556677ULL);
+ for (size_t offset = 0; offset < 5; ++offset) {
+ recorder->record_queue_event(FileCacheType::NORMAL,
CacheLRULogType::ADD, hash, offset,
+ 4096);
}
- recorder->update_shadow_queue_element_count_metrics();
-
-
EXPECT_EQ(recorder->get_shadow_queue(FileCacheType::INDEX).get_elements_num_unsafe(),
3);
+ // replay_queue_event() is the only thing that publishes this gauge, and
it does so under the
+ // same lock that mutates the shadow queue. That is what lets
run_background_monitor() stay
+ // off _mutex_lru_log.
+ EXPECT_EQ(recorder->replay_queue_event(FileCacheType::NORMAL), 5);
auto stats = mock_cache->get_stats_unsafe();
- EXPECT_EQ(stats["lru_recorder_index_shadow_queue_curr_elements"], 3);
+ EXPECT_EQ(stats["lru_recorder_normal_shadow_queue_curr_elements"], 5);
}
TEST_F(CacheLRUDumperTest, test_remove_event_still_obeys_replay_queue_cap) {
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
