[fix](file cache) keep disk resource limit mode hysteresis across checks - #67313
[fix](file cache) keep disk resource limit mode hysteresis across checks#67313deardeng wants to merge 2 commits into
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Found two blocking correctness regressions; details are in the inline comments.
Goal and test proof: the revised state machine correctly retains disk-resource-limit mode inside the exit/enter band, enters inclusively for either resource, exits only when both resources are below the exit threshold, and publishes the final metric state. The new tests prove that local transition table and the ready-state reset side effect, with appropriate config and SyncPoint cleanup, but they do not exercise the two blocking scenarios.
Scope, data correctness, and lifecycle: removing reset's mode assignment leaves the pre-readiness admission path without enforcement of a newly reduced capacity. The cache is published while metadata loading is still asynchronous, so this is production-reachable and can grow beyond the reset capacity.
Parallel query path: retaining true mode in the hysteresis band exposes an existing size-reuse bug on a newly reachable path: the five-times eviction target is persisted in per-query accounting while the cell retains its actual size. That can hit a DCHECK and produces incorrect quota and eviction decisions in release builds. Non-query paths do not persist the inflated value.
Concurrency and ownership: no new thread, lock-order edge, or ownership path is introduced. The plain-bool monitor/foreground race predates this PR and the patch removes a writer/readers, so it is not reported here.
Configuration, boundaries, errors, and observability: no config is added; dynamic thresholds, invalid-threshold repair, exact boundary behavior, memory-storage and statfs early returns, transition logs, and final-state metric publication are otherwise consistent.
Compatibility, persistence, transactions, and performance: there is no protocol, storage-format, rolling-upgrade, durable metadata, transaction, or allocation surface. Monitor work remains constant-time with one negligible metric assignment per successful interval.
Validation: three review rounds converged with no third distinct finding, and there was no additional user-specified focus. Per the review-runner instructions, I did not run local builds or tests. At the submission snapshot, style, formatting, title, license, and secret checks passed; BE UT, compile, and performance checks were still pending.
|
/review |
There was a problem hiding this comment.
Request changes.
Scope and goal: This patch preserves disk-resource hysteresis and adds async-load capacity checks. The state transition and metric publication are coherent, but capacity enforcement remains incomplete on persisted metadata insertion paths.
Findings:
- P1: The new async reservation target is bypassed by persisted metadata batches and lazy direct loads; those paths can repopulate entries after reset_capacity() shrinks the cache and leave _cur_cache_size above _capacity. See the inline comment in block_file_cache.cpp.
- P2: The new async tests mutate the plain-bool disk-resource mode while the monitor thread may still be performing its first check, so setup can race under TSAN. See the inline comment in block_file_cache_test.cpp.
Review checkpoints:
- Correctness: Hysteresis transitions are retained, but the capacity invariant is not maintained across all async publication paths.
- Concurrency and lifecycle: No new production synchronization is introduced; the tests need an explicit barrier for the monitor's initial check.
- Error, memory, and data handling: No new error/status or format changes were found; the P1 can cause over-capacity memory retention.
- Parallel paths and configuration: Direct loader, lazy metadata, and restoration paths were checked; no new configuration surface is introduced.
- Observability: The metric is published from the final mode after each disk check and is consistent with that state.
- Tests: The added tests cover the split-range path and hysteresis transitions, but do not cover persisted-loader/direct-load admission and have the synchronization issue noted above. No builds or tests were run during this review per the review instructions.
The existing live threads for post-reset miss admission and inflated query accounting were treated as duplicate context and are not resubmitted. No additional review focus was supplied. Reviewed bundle head: 9f7f68c.
|
/review |
There was a problem hiding this comment.
Request changes: one blocking async-load lifecycle/performance issue remains; see the inline comment.
Critical checkpoint conclusions:
- Goal and proof: the disk/inode enter-hold-exit state machine is now coherent, reset no longer masquerades as disk pressure, and the atomic flags correctly remove the monitor/request races. The hysteresis test covers disk enter, inode enter, true-side hold, exit, and bvar agreement. The async tests prove only the synthetic one-block capacity gap and zero-target fast path; they do not exercise the real loader lifecycle that exposes the finding.
- Scope and parallel paths: the hysteresis and atomic changes are focused. The new async reserve policy crosses DB/FS batch restoration, lazy direct loads, queued metadata deletion, LRU eviction, and foreground cache reads. All of those paths were traced; persisted publication above capacity and inflated query accounting were hard-deduplicated against the existing threads. No additional user focus was supplied.
- Concurrency and lifecycle: the monitor is the sole production writer of both changed flags, default sequentially consistent atomics are sufficient, cells/capacity/LRUs remain under the cache mutex, and threads are joined before member destruction. The blocking issue is a different lifecycle gap: physical eviction can race stale loader or queued-metadata publication before readiness.
- Data, persistence, and errors: no committed table data, transaction protocol, storage format, FE/BE variable, or rolling-upgrade contract changes. Normal cached readers fall back to remote data and self-heal after the stale cache entry, so this is not evidence of committed-data loss; it is invalid cache state, failed local/peer work, extra remote I/O, and lost cache warmth.
- Performance and observability: mode logs and bvars are adequate and the atomic overhead is negligible. The accepted path performs O(restored overage / block size) synchronous deletes/listings under the global cache mutex and is not bounded by the request size. Existing latency metrics can observe the stall but do not prevent it.
- Tests and validation: the added tests miss a held loader, queued-delete reload, and a large restored overage. Per the review-runner instruction, no build or unit test was run locally. The repository header-hygiene gate passed; live style, formatter, license, large-file, and secret checks pass on the reviewed head, while this automated review check is still running.
Review completion: complete after two rounds. Round 2 normal full reviews and the focused falsification audit all returned NO_NEW_VALUABLE_FINDINGS, every candidate is accepted, dismissed with evidence, or duplicate-fenced, and the final changed-file sweep found no unresolved point. Reviewed head: 450bcd3.
|
/review |
There was a problem hiding this comment.
Review completed against bundle head 1b592cc (base 2689e0d).
Blocking findings:
- The async-load capacity-gap path now walks all disposable/normal/index LRU entries under the global cache mutex when no entries are releasable. The byte target is capped, but traversal is not; repeated misses with held blocks can make startup/request latency O(total cached blocks). Inline P1 below.
- The new async capacity tests do not order the monitor's first capacity read. In a shuffled run, the existing evict_in_advance test can leave the global enable flag true; the monitor then reads plain _capacity/_cur_cache_size while these tests mutate them. Inline P2 below.
Checkpoint conclusions:
- Task goal/state machine: disk-resource mode now correctly enters on either resource at the enter threshold, holds in the hysteresis band, exits only when both are below exit, and publishes its metric after the decision. The statfs-failure path preserves state.
- Concurrency: converting both mode flags to atomics covers the production flag races and all call sites use the atomic values. The new async eviction scan still performs synchronous work under _mutex and needs bounded/deferred traversal.
- Lifecycle/config/compatibility: monitor and cache lifecycles remain joined normally; no new protocol or storage-format compatibility issue was found; dynamic threshold validation remains in place.
- Parallel paths/duplicates: direct metadata insertion and loader republication concerns, and the query-size inflation path, were checked and are already covered by existing review threads rather than repeated here.
- Tests/observability: hysteresis, statfs failure, and async-capacity tests were added, and the mode metric update is now per-round; the test synchronization issue above must be fixed for deterministic TSAN-safe coverage.
- User focus: no additional focus was provided.
Please address both inline findings and add regressions for a large held queue during async load and for monitor/test synchronization before rerunning the review.
… band
check_disk_resource_limit() cleared _disk_resource_limit_mode before it read
statfs, whenever the cache had not yet filled its configured capacity:
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) {
The [exit, enter) band is implemented by that member remembering the previous
round, so wiping it up front made the exit branch unreachable. The mode was
dropped as soon as usage fell below enter instead of holding until it fell
below exit, i.e. only the enter half of the state machine worked. The
disk_resource_limit_mode bvar could not be trusted either, because a single
round wrote 0 and then 1 whenever the mode was immediately re-entered.
Remove the pre-clear so the band holds, and publish _disk_limit_mode_metrics
once, after the decision.
reset_capacity() also force-enabled the mode when shrinking, and the pre-clear
is what undid it. Removing only the pre-clear would pin the mode on forever
after a shrink, so drop the force as well. It was not an enforcement mechanism
for the reduced capacity: reset_capacity() already evicts down to the new
capacity under the cache lock before it returns, the metadata loader inserts
restored cells through add_cell() without ever calling try_reserve(), and
try_reserve_during_async_load() admits unconditionally unless the mode is set.
On master the force also survived at most one monitor interval, because the
next check cleared it either through the pre-clear or through the exit branch.
Restore the is_insufficient lambda so both threshold checks read the same way
as the one in check_need_evict_cache_in_advance, and state the enter/hold/exit
contract on the branch that implements it.
Tests drive the state machine with injected statfs results: entering on space,
entering on inode, holding at 82% while the cache still has capacity to spare,
exiting only once both resources are below exit, the metric agreeing with the
mode after every round, and a shrink no longer forcing the mode on. A statfs
failure now preserves the mode instead of clearing it, which the pre-clear
used to do before the early return, so that path is covered as well.
run_background_monitor() writes _disk_resource_limit_mode and _need_evict_cache_in_advance without holding the cache lock, while try_reserve() and is_overflow() read them under the cache lock and run_background_gc() reads _need_evict_cache_in_advance without it. Both plain bools were therefore racy on master, independently of the hysteresis fix. Make them std::atomic<bool>. The default sequentially consistent ordering is deliberate: these are mode flags on the reservation path rather than counters, and the monitor writes them once per interval, so the ordering costs nothing measurable. Loads are explicit where the value feeds a metric, a stats map or a gtest assertion.
1b592cc to
bb59a1e
Compare
|
/review |
There was a problem hiding this comment.
Review completed against head bb59a1e74a2ee8867d2d7010491cd791a216483c (base 2689e0d7fdb111bf822cebc26d7d5765a563e276). I am not adding duplicate inline comments: the two blocking final-head concerns are already anchored on the current diff.
Blocking existing threads
- Preserve shrink enforcement while metadata is loading: the final head removes the reset-time mode force, while the unchanged pre-readiness reservation branch still admits without consulting
_capacitywhenever the monitor-owned mode is false. The capacity-aware async implementation described in the reply is not present in the authoritative final diff, so a shrink can still be followed immediately by admission above the new capacity. Post-load or proactive convergence does not restore that removed guard. - Keep the eviction target separate from the admitted size:
try_reserve()still multipliessizeby five and passes that value to query accounting even thoughadd_cell()creates the original-size cell. This PR deliberately keeps disk-limit mode active throughout[exit, enter), expanding that mismatch into the newly reachable hold band.
Critical checkpoint conclusions
- Goal and focused scope: the enter/hold/exit state-machine change itself is small and implements the intended two-resource hysteresis correctly, but the reset coupling and the newly extended reachability above prevent the overall change from being safe as submitted.
- Concurrency and lifecycle: the monitor is the production writer of the two mode flags; reservation, overflow, proactive eviction, and stats are consumers. Sequentially consistent atomics remove the direct flag races, cache structures remain under
_mutex, and shutdown joins the relevant threads. The outstanding shrink issue is the separate async-loader/admission lifecycle described above. - Configuration and conditions: no new configuration is added. Existing dynamic thresholds are re-read by the monitor; entry occurs when either resource reaches
enter, state holds in the band, exit requires both resources belowexit, invalidenter < exitvalues are reset, and failedstatfspreserves state and metric. - Parallel paths, compatibility, and persistence: proactive eviction, normal reservation, stats, disk/memory storage selection, DB/FS metadata restoration, and direct lazy loading were traced. There is no wire, storage-format, FE/BE, transaction, EditLog, or rolling-upgrade surface in this patch.
- Tests and observability: the direct unit tests cover space entry, inode entry, hold, exit, successful state/bvar agreement, failed-sample preservation, and removal of the reset force. They do not prove safe post-shrink admission before readiness or correct query accounting in the hold band. The bvar now reflects the final state after each successful sample. No build or test was run in this review-only environment, per the review prompt.
- Performance and memory: the atomic operations are negligible relative to
statfs, and the patch adds no allocations. The five-times accounting/eviction path remains the material performance and correctness concern referenced above.
User focus: no additional review focus was provided.
Convergence status: the main scan, two complete-review agents, and the separate risk-focused agent covered all three changed files and the upstream/downstream lifecycle. All three agents returned NO_NEW_VALUABLE_FINDINGS; every other candidate was dismissed with code evidence or deduplicated against existing threads. The review is complete in Round 1, with the two existing blockers above remaining unresolved.
|
run buildall |
2 participants
What problem does this PR solve?
Problem Summary:
check_disk_resource_limit()cleared_disk_resource_limit_modebefore it readstatfs, whenever the cache had not yet filled its configured capacity:The
[exit, enter)hysteresis band is implemented by that member remembering the previous round, so wiping it up front made the exit branch unreachable. The mode was dropped as soon as usage fell belowenterinstead of holding until it fell belowexit— only the enter half of the state machine worked. Thedisk_resource_limit_modebvar was unreliable for the same reason: a single round wrote 0 and then 1 whenever the mode was immediately re-entered, so the metric could not be used to tell whether a node was in the mode.What this PR changes
_disk_limit_mode_metricsonce, after the decision._disk_resource_limit_mode = truethatreset_capacity()forced on when shrinking. The pre-clear is what undid it, so removing only the pre-clear would pin the mode on forever after a shrink. That force was not an enforcement mechanism for the reduced capacity — see the scope note below._disk_resource_limit_modeand_need_evict_cache_in_advancestd::atomic<bool>.run_background_monitor()writes them without the cache lock whiletry_reserve(),is_overflow()andrun_background_gc()read them, so both plain bools were racy on master, independently of this fix.Why dropping the
reset_capacity()force is safeIt never enforced the new capacity:
reset_capacity()already evicts down to the new capacity under the cache lock before it returns.add_cell()and never callstry_reserve()(fs_file_cache_storage.cpp:818,:1007,:1179), so capacity was never consulted on that path.try_reserve_during_async_load()admits unconditionally unless the mode is set, on master as well as here.file_cache_background_monitor_interval_ms, because the next check cleared it either through the pre-clear or through the exit branch.Any over-capacity state converges once loading completes:
try_reserve()takes the normal branch, andenable_evict_file_cache_in_advancedefaults to true, socheck_need_evict_cache_in_advance()seessize_percentageabove 100 and drives eviction from the monitor loop.Deliberately out of scope
These are real and were checked while working on this change. Each is pre-existing on master, independent of the state machine touched here, and gets its own PR:
try_reserve_during_async_load()has never consulted_capacity, and the loader bypassestry_reserve()entirely. Adding admission control there interacts with the loader's publication protocol and needs to be designed as such, not folded into a state-machine fix.try_reserve()doessize = 5 * sizeunder disk pressure and passes that value toQueryFileCacheContext::reserve(), whileadd_cell()creates a cell of the original size, soDCHECK(iter->size == cell_size)atblock_file_cache.cpp:1483can fire and release builds mis-account query usage. Reachable on master today whenever the disk is at or above the enter threshold withenable_file_cache_query_limiton.load_cache_info_into_memory_from_db()buffers 10kBatchLoadArgsoutside_mutex(fs_file_cache_storage.cpp:995) andhandle_already_loaded_block()only checks whether a cell exists, never whether the file is still on disk.Release note
None
Check List (For Author)
BlockFileCacheTest.test_check_disk_resource_limit_hysteresisdrives the state machine with injectedstatfsresults through the existingBlockFileCache::disk_used_percentage:1sync point: entering on space, entering on inode, holding at 82% while the cache still has capacity to spare, exiting only once both resources are belowexit, and the metric agreeing with the mode after every round. The hold case fails without this change.BlockFileCacheTest.test_check_disk_resource_limit_statfs_failure_preserves_statecovers thestatfsfailure path.BlockFileCacheTest.reset_capacityadditionally asserts a shrink no longer forces the mode on.Existing
test_check_disk_reource_limit_1/2/3are unchanged and still pass.Behavior changed:
[exit, enter)instead of clearing belowenter, which is the documented intent of the two thresholds.reset_capacity()no longer turns the mode on.statfsnow preserves the mode and its metric; previously the pre-clear had already run before the early return.Does this need documentation?