mirror of
https://github.com/rustfs/rustfs.git
synced 2026-08-13 00:26:53 +00:00
fix(get): give UringBackend the only fd cache for its disk (#5974)
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 <heihutu@gmail.com>
This commit is contained in:
@@ -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;
|
||||
|
||||
Reference in New Issue
Block a user