deardeng opened a new pull request, #68945: URL: https://github.com/apache/doris/pull/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 --> -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
