From 2e5874f8396432c8e07218dc5279ef98a3b236ba Mon Sep 17 00:00:00 2001 From: houseme Date: Wed, 12 Aug 2026 14:53:59 +0800 Subject: [PATCH] fix(get): give UringBackend the only fd cache for its disk (#5974) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit fix(get): give UringBackend the only fd cache for its disk (#1801) #1801 made `StdBackend::new` always build a descriptor cache. `UringBackend` wraps a `StdBackend` (`inner`), so under io_uring a disk ended up with TWO `FdCache`s: the wrapper's and the inner's. `UringBackend::pread_bytes` delegates to `inner.pread_bytes` on four fallback paths (latch-off, O_DIRECT unsupported / error, buffered-read error), which populated `inner.fd_cache` — but `UringBackend`'s invalidation only touches its own cache, so the inner cache was never invalidated. For up to `FD_CACHE_TTL` (5s) after a heal/rename/ delete, a fallback read could serve the pre-mutation inode: exactly the stale-descriptor hazard `FdCache`'s generation guard exists to close (rustfs/backlog#1176). It also double-counted `FD_CACHE_CAPACITY` (512 fds) against `RLIMIT_NOFILE` per disk (backlog#1178). Fix: `UringBackend` now constructs its inner `StdBackend` with the new `StdBackend::new_without_fd_cache`, so the wrapper owns the only cache for the disk. The inner backend opens per read on fallback, leaving nothing unguarded. `StdBackend::new` (standalone default) is unchanged; a private `build(root, build_fd_cache)` holds the shared construction. - Default (non-io_uring) path: byte-for-byte unchanged. - io_uring path: one cache per disk, fully covered by the wrapper's invalidation; halves the per-disk fd budget under `RLIMIT_NOFILE`. - `RUSTFS_IO_URING_FD_CACHE` / `RUSTFS_LOCAL_FD_CACHE` semantics preserved. - Regression test pins `new_without_fd_cache` -> no cache. Found by a post-merge re-review of the Wave 1 GET PRs. cargo check/clippy clean; Linux compile + the io_uring fd-cache suite deferred to CI. Co-authored-by: heihutu --- crates/ecstore/src/disk/local.rs | 45 ++++++++++++++++++++++++++++++-- 1 file changed, 43 insertions(+), 2 deletions(-) diff --git a/crates/ecstore/src/disk/local.rs b/crates/ecstore/src/disk/local.rs index 7ac651a3a..5a42245d8 100644 --- a/crates/ecstore/src/disk/local.rs +++ b/crates/ecstore/src/disk/local.rs @@ -2955,11 +2955,31 @@ impl std::fmt::Debug for StdBackend { impl StdBackend { pub(crate) fn new(root: PathBuf) -> Self { + Self::build(root, true) + } + + /// Construct without the descriptor cache. + /// + /// `UringBackend` wraps a `StdBackend` and runs its own `FdCache` over the + /// same positioned reads. If the inner `StdBackend` also built a cache, a + /// fallback read (`UringBackend::pread_bytes` delegates to the inner backend + /// on latch-off / O_DIRECT / buffered errors) would populate a *second* + /// cache that `UringBackend`'s invalidation never touches — re-opening the + /// stale-inode hazard `FdCache` exists to close (rustfs/backlog#1176/#1801). + /// The wrapper therefore owns the only cache for the disk; the inner backend + /// opens per read. This also avoids double-counting `FD_CACHE_CAPACITY` + /// against `RLIMIT_NOFILE` (rustfs/backlog#1178). + #[cfg(target_os = "linux")] + pub(crate) fn new_without_fd_cache(root: PathBuf) -> Self { + Self::build(root, false) + } + + fn build(root: PathBuf, build_fd_cache: bool) -> Self { // Gate the fd cache on RLIMIT_NOFILE headroom (rustfs/backlog#1178): // 512 fds/disk with a low soft limit and several disks would hit EMFILE. // Fall back to open-per-read when the limit is too small. #[cfg(target_os = "linux")] - let fd_cache = if is_local_fd_cache_enabled() { + let fd_cache = if build_fd_cache && is_local_fd_cache_enabled() { if rlimit_allows_fd_cache() { Some(FdCache::new()) } else { @@ -2973,6 +2993,10 @@ impl StdBackend { } else { None }; + // `build_fd_cache` is only consulted on Linux (for the fd cache); on + // other platforms it has no effect and would trip the unused-variable lint. + #[cfg(not(target_os = "linux"))] + let _ = build_fd_cache; Self { root, #[cfg(target_os = "linux")] @@ -4120,7 +4144,7 @@ impl UringBackend { // struct (rustfs/backlog#1185). let root_label = root.display().to_string(); Some(Self { - inner: StdBackend::new(root.clone()), + inner: StdBackend::new_without_fd_cache(root.clone()), root, root_label, driver: std::mem::ManuallyDrop::new(driver), @@ -19918,6 +19942,23 @@ mod test { assert_eq!(cache.entry_count().await, 0, "prefix invalidation must drop the cached descriptor"); } + /// `StdBackend::new_without_fd_cache` must not build a descriptor cache. + /// `UringBackend` wraps a `StdBackend` and owns the only cache for the disk, + /// so an inner cache would be populated by fallback reads + /// (`UringBackend::pread_bytes` delegates inward) yet never invalidated — + /// the stale-inode hazard `FdCache` exists to close (backlog#1176/#1801). + /// This pins the contract so a future constructor change cannot regress it. + #[cfg(target_os = "linux")] + #[test] + fn new_without_fd_cache_builds_no_descriptor_cache() { + let root_dir = tempfile::tempdir().expect("operation should succeed"); + let backend = StdBackend::new_without_fd_cache(root_dir.path().to_path_buf()); + assert!( + backend.fd_cache.is_none(), + "new_without_fd_cache must not build a descriptor cache — UringBackend owns the only cache for the disk" + ); + } + /// The mutation paths on `LocalDisk` must actually call /// `invalidate_cached_fds`, not merely have it available (backlog#1145). /// `rename_file` replaces the inode at a path a reader has already cached;