Doris-Breakwater commented on issue #68893: URL: https://github.com/apache/doris/issues/68893#issuecomment-6098926717
The reported lock-scope problem is supported by source inspection and merits a BE/file-cache performance fix. The throughput impact is plausible, but its magnitude and attribution still need the reporter's traces and benchmark. This is a static investigation; I have not reproduced the workload or tested the proposed change. The issue is currently open, with no labels, assignee, milestone, or existing comments. Suggested routing: BE storage/file-cache maintainers; classify as a performance bug. I checked the local `4.1.4` tag against the public tag (commit `ad35a140c7fd0b842f18c23300bac581f7d04326`) and compared the relevant local master files with public master at `dd456ab7a894f54c6553415223c6d5c9c7d4e035`. Verified mechanism: - `get_or_set()` acquires `_mutex` through `SCOPED_CACHE_LOCK` and calls `split_range_into_cells()` → `try_reserve()` within that scope. The macro uses `std::lock_guard`; these eviction calls do not release the cache mutex. See [reservation entry](https://github.com/apache/doris/blob/ad35a140c7fd0b842f18c23300bac581f7d04326/be/src/io/cache/block_file_cache.cpp#L1183-L1214) and [lock macro](https://github.com/apache/doris/blob/ad35a140c7fd0b842f18c23300bac581f7d04326/be/src/io/cache/block_file_cache.h#L66-L80). - Reactive cross-queue eviction by time/size and same-queue LRU eviction select `sync = !evict_in_advance`. `remove_file_blocks()` also takes the block mutex, then `remove(..., sync=true)` invokes `_storage->remove(key)` while the caller still holds the cache mutex. [LRU eviction and removal](https://github.com/apache/doris/blob/ad35a140c7fd0b842f18c23300bac581f7d04326/be/src/io/cache/block_file_cache.cpp#L1759-L1905) confirm this; [current master](https://github.com/apache/doris/blob/dd456ab7a894f54c6553415223c6d5c9c7d4e035/be/src/io/cache/block_file_cache.cpp#L1760-L1906) retains the mechanism. - For filesystem storage, removal includes FD-cache invalidation, file deletion, persistent metadata deletion, directory listing, and possible empty-directory removal. `LocalFileSystem::delete_file_impl()` uses `std::filesystem::remove`. Thus the critical section includes more filesystem work than the unlink alone. See [storage removal](https://github.com/apache/doris/blob/ad35a140c7fd0b842f18c23300bac581f7d04326/be/src/io/cache/fs_file_cache_storage.cpp#L404-L444) and [local deletion](https://github.com/apache/doris/blob/ad35a140c7fd0b842f18c23300bac581f7d04326/be/src/io/fs/local_file_system.cpp#L130-L145). - The recycle worker removes files outside `_mutex`, but enqueue failure falls back to synchronous removal inside the caller's lock. Async removal is also used by `remove_if_cached_async()`, so it is not exclusive to advance eviction. See [fallback](https://github.com/apache/doris/blob/ad35a140c7fd0b842f18c23300bac581f7d04326/be/src/io/cache/block_file_cache.cpp#L1868-L1887) and [worker](https://github.com/apache/doris/blob/ad35a140c7fd0b842f18c23300bac581f7d04326/be/src/io/cache/block_file_cache.cpp#L2404-L2435). The contention domain is one `BlockFileCache` instance/cache path. Cache lookups and reservations sharing it can queue behind removal; this does not mean every ongoing file read holds that mutex. Frequent eviction after saturation is workload-dependent. The issue reports off-CPU evidence and a successful experiment, but provides neither artifact, so these remain reported observations rather than independently verified performance results. For the proposed PR, moving physical removal beyond the cache-lock scope is a justified direction, with these review requirements: 1. Keep victim selection, LRU updates, and logical size accounting atomic under the lock; carry owned deletion records beyond it, not `FileBlockCell*` pointers invalidated by erasure. Include query-limit and startup/disk-resource eviction paths in the scope audit, while preserving explicit invalidation semantics. 2. Demonstrate safe same-key eviction/re-admission. `finalize()` renames a new block into the same hash/offset path that deferred removal targets, and removal also deletes its metadata and FD-cache entry. A late deletion must not remove a replacement block. This is a code-grounded race to test, not a claim that the reporter observed it; simply changing `sync` to false is insufficient proof of correctness. 3. Account for delayed physical space/inode reclamation, recycler backlog, enqueue failure, deletion errors, and shutdown. Logical capacity is decremented before queued deletion finishes; the recycler has a configured removal rate limit. Any fallback must uphold the intended lock-scope guarantee. Please attach the exact BE build SHA and proposed patch; a reproducible ingestion script with concurrency, block sizes, cache capacity and filesystem/device details; effective write-through/async-write, advance-eviction, recycler and query-limit settings; and aligned before/full-cache/after-patch measurements. Useful evidence includes throughput, `file_cache_cache_lock_wait_time_us`, `file_cache_storage_sync_remove_latency_us`, retry/async removal latency, recycle queue length, physical free bytes/inodes, disk latency, and the symbolized off-CPU stacks plus BE lock-held/deletion-error logs. A deterministic test that pauses deletion and proves an unrelated cache lookup proceeds, followed by same-key replacement and backlog/failure tests, would make the PR directly reviewable. No configuration tuning or backport claim is established by this investigation. Breakwater-GitHub-Analysis-Slot: slot_ed1a6f8f2446 -- 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]
