matanper opened a new pull request, #69619:
URL: https://github.com/apache/doris/pull/69619
### What problem does this PR solve?
Issue Number: close #68893
Related PR: #69587 (bounds the eviction scan under the same lock;
independent of this change)
Problem Summary:
A reservation that must evict right now (the reactive path, `sync =
!evict_in_advance`) removes
the evicted block files inline: `remove()` calls `_storage->remove(key)`
while the caller still
holds the `BlockFileCache` mutex taken by `SCOPED_CACHE_LOCK` in
`get_or_set`. For the
filesystem storage that is FD-cache invalidation, unlink of the v3 (and v2)
file, the meta store
delete, and two directory listings plus possible rmdirs. Once the cache is
full almost every
reservation takes this path, so every user of that cache path queues behind
disk I/O and
throughput declines as the cache fills.
The points raised in the issue's triage comment are addressed as follows.
**1. Selection and accounting stay under the lock; only owned records leave
it.**
`SCOPED_CACHE_LOCK` opens a `DeferredStorageRemoveScope` before taking the
lock. `remove()` still
does victim selection, LRU and logical size accounting under the lock and
parks an owned
`FileCacheKey` (not a `FileBlockCell*`) in the scope. The scope's destructor
runs the removals on
the same thread after the lock is released, before the caller continues, so
explicit
invalidation keeps its semantics: `remove_if_cached` and reactive eviction
still free the space
before they return, just not under the lock. Every path that removes under
the cache lock takes
it through `SCOPED_CACHE_LOCK`: reactive eviction (by time, by size, own
queue), the query-limit
path, `try_reserve_during_async_load`, `remove_if_cached`, holder release,
`reset_capacity`.
The two raw `lock_guard`s on the cache mutex (a probe lookup and the
consistency check) do not
remove anything. Without an owning scope, removal falls back to inline as
before.
**2. Same-key eviction and re-admission.** Every pending removal is
registered in
`PendingStorageRemoves`, keyed by (hash, offset). Only the thread that
claims an entry runs it.
A block downloader drains any pending removal at its position before its
first write
(`appendv`, `append_iobuf`) and before `finalize` (`set_downloaded`), and a
removal marked again
while running is run again by the same claimer. So a late removal of an
evicted block cannot
delete the block that replaced it at the same position, its meta, or its
FD-cache entry. Covered
by a test that stalls the stale removal while the same position is
downloaded again, and by a
concurrent reserve/evict/re-insert stress test.
**3. Delayed reclamation, backlog, enqueue failure, errors, shutdown.**
Physical space and
inodes are reclaimed when the evicting thread releases the lock, right after
the logical
release. The async recycle path (evict-in-advance, holder release) is
unchanged; if its queue is
full, a removal that has a scope is now deferred to that scope instead of
being run inline under
the lock. Deletion errors are logged and the pending entry is cleared; a
file left behind is
handled by the existing leak cleaner, as before. Removals parked in a scope
finish before the
scope's thread continues, so shutdown is unchanged.
`FSFileCacheStorage::remove` now unlinks the known paths (a missing file
counts as removed) and
rmdirs the block directory, which only succeeds when it is empty, instead of
listing the
directory twice. A writer that loses the race with that rmdir recreates the
directory once.
`enable_file_cache_async_evict_io` (mutable, default true) switches back to
inline removal under
the lock.
**Not attached yet:** production traces (off-CPU stacks, lock-wait and
removal latency before /
full-cache / after) and the workload details the triage comment asks for.
### Release note
File cache eviction removes block files after releasing the cache lock
instead of while holding
it. New BE config `enable_file_cache_async_evict_io` (default true) restores
inline removal.
### Check List (For Author)
- Test: Unit Test (`be/test/io/cache/block_file_cache_evict_io_test.cpp`):
removal runs after the
lock is released (and under it with the config off); `remove_if_cached`
removes after unlock
and before returning; an unrelated lookup completes while a removal is
stalled; a block
re-created at the same position survives a stale removal; a queued removal
is drained by the
replacement's downloader; concurrent reserve/evict/re-insert keeps every
downloaded block's
file; a missing file is treated as removed; `PendingStorageRemoves`
claim/finish/wait. Run under
ASAN: pending.
- Behavior changed: Yes. Evicted block files are removed right after the
cache lock is released
instead of while holding it.
- Does this need documentation: No
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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]
