From c088c4c73c733a9632c2da187d19581346b02ad5 Mon Sep 17 00:00:00 2001 From: deardeng Date: Thu, 3 Sep 2026 19:57:22 +0800 Subject: [PATCH] [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 4ba15fafaa75b8..b8f4f3984eb398 100644 --- a/be/src/io/cache/block_file_cache.cpp +++ b/be/src/io/cache/block_file_cache.cpp @@ -2397,7 +2397,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 c8227129bdac57..8bd202ee8476bb 100644 --- a/be/src/io/cache/lru_queue_recorder.cpp +++ b/be/src/io/cache/lru_queue_recorder.cpp @@ -181,11 +181,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 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& lru_log_lock) { int64_t queue_limit = config::file_cache_background_lru_dump_tail_record_num; @@ -202,16 +197,6 @@ void LRUQueueRecorder::limit_shadow_queue_size(LRUQueue& shadow_queue, } } -void LRUQueueRecorder::update_shadow_queue_element_count_metrics_unlocked( - std::lock_guard& 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 1edd0f5ab85bc9..058b78194ff82a 100644 --- a/be/src/io/cache/lru_queue_recorder.h +++ b/be/src/io/cache/lru_queue_recorder.h @@ -67,7 +67,6 @@ class LRUQueueRecorder { 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 @@ class LRUQueueRecorder { 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& lru_log_lock); - void update_shadow_queue_element_count_metrics_unlocked( - std::lock_guard& 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 6256f52d74a8be..e49a142bb2c9e6 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({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) {