deardeng commented on code in PR #67313:
URL: https://github.com/apache/doris/pull/67313#discussion_r3888811018
##########
be/src/io/cache/block_file_cache.cpp:
##########
@@ -2577,6 +2569,15 @@ BlockFileCache::get_hot_blocks_meta(const
UInt128Wrapper& hash) const {
bool BlockFileCache::try_reserve_during_async_load(size_t size,
std::lock_guard<std::mutex>& cache_lock) {
+ size_t evict_target =
Review Comment:
You're right, and this one is on the change: the target I added used the
full gap between _cur_cache_size and _capacity, so a loader-restored overage
amplified it without bound. A BE restarted with a smaller total_size
republishes everything already on disk, and the first foreground miss during
load would then synchronously delete the whole overage under the global cache
lock. Master's version of this path never evicted more than the request; making
it gap-sized was a regression.
The target is now capped at the request size. When the gap is at least size,
evicting size and admitting size leaves _cur_cache_size unchanged — an
over-capacity cache stops growing, which is the invariant that actually matters
here — and when the gap is smaller, evicting it lands exactly on capacity.
Foreground work per reservation is constant in the restored overage. Added
test_async_load_overage_eviction_is_bounded_by_request_size, which builds a
100-byte cache, drops _capacity to 30 to simulate a restored overage, and
asserts a 5-byte miss removes exactly one block rather than draining the
75-byte gap.
Delete-then-stale-republication half — real, but not introduced here and not
confined to this path.
Confirmed the mechanism: load_cache_info_into_memory_from_db() buffers 10k
BatchLoadArgs outside _mutex (fs_file_cache_storage.cpp:995), and
handle_already_loaded_block() only checks whether a cell exists, never whether
the file is still on disk — so any block evicted between buffering and
add_cell_batch_func() gets republished as DOWNLOADED over a deleted file.
But the trigger is "eviction happens while _async_open_done is false", not
this reservation policy. Master already evicts on that path whenever
_disk_resource_limit_mode is set, which is any disk at or above the enter
threshold, and holder-destruction removes run there too. With the target now
bounded by the request, the exposure is the same shape and the same magnitude
as master's existing disk-pressure path.
Generation-fencing loader-visible cells, or making the batch republication
check storage, is a fix to the loader's publication protocol. It is worth doing
and it will fix master's existing exposure as well, but it does not belong in a
hysteresis fix — folding it in would mean touching fs_file_cache_storage.cpp's
batching under the same PR that changes the disk-pressure state machine. I'd
rather file it separately.
--
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]
