From e3b2c9d8509dae1a67a31234b13e138f52d28a83 Mon Sep 17 00:00:00 2001 From: MattJackson <1085847+MattJackson@users.noreply.github.com> Date: Thu, 14 May 2026 09:16:23 -0700 Subject: [PATCH] file_sector_source: restore read-side DONTNEED + SEQUENTIAL (the actual fix) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Empirical: isolated NFS read 70 MB/s + write 93 MB/s on the rip1 setup right now, but mux throughput pinned at 2.7 MB/s on 0.21.5. NOT environmental — code regression. Root cause: Phase 1 silently dropped the read-side posix_fadvise(POSIX_FADV_DONTNEED) eviction that the pre-Phase-1 (0.20.7) hot path had. Without it, an 85 GB streaming ISO read pins the entire file in the kernel page cache, starving concurrent MKV writeback. 0.21.2 then also dropped the POSIX_FADV_SEQUENTIAL hint on the same theory, compounding the regression. Restored both, per-OS split: - linux: posix_fadvise(SEQUENTIAL) at open + posix_fadvise(DONTNEED) on consumed 32 MiB windows - macos: F_RDADVISE hint at open (kept); drop_window no-op (macOS unified buffer cache less prone to the pin pathology) - windows / other: both no-op stubs Target mux speed restored to 20+ MB/s (per concurrent-NFS math: 70/2 read × 0.73 MKV/ISO ratio ≈ 25 MB/s achievable). --- src/io/file_sector_source/linux.rs | 54 +++++++++++++++++++++------- src/io/file_sector_source/macos.rs | 8 +++++ src/io/file_sector_source/mod.rs | 35 ++++++++++++++++++ src/io/file_sector_source/other.rs | 2 ++ src/io/file_sector_source/windows.rs | 5 +++ 5 files changed, 91 insertions(+), 13 deletions(-) diff --git a/src/io/file_sector_source/linux.rs b/src/io/file_sector_source/linux.rs index 2575b69..0355cee 100644 --- a/src/io/file_sector_source/linux.rs +++ b/src/io/file_sector_source/linux.rs @@ -1,18 +1,46 @@ -//! Linux: kernel readahead hint for the ISO file. +//! Linux read-side platform hooks: sequential-access hint at open + +//! periodic page-cache eviction during streaming reads. //! -//! Originally `POSIX_FADV_SEQUENTIAL` to widen the readahead window. -//! On NFS that turned out to cause aggressive multi-MB readahead bursts -//! that saturated the TCP connection and starved concurrent writes -//! during mux — observed empirically as a ~3× drop in mux throughput -//! on the rip1/unraid-1 setup (0.21.0 vs 0.20.7 baseline). The kernel's -//! default readahead (~128 KiB on Linux) interleaves more naturally -//! with the muxer's concurrent NFS writes, so we no longer issue any -//! hint here. The per-OS file stays so the convention is honoured and -//! we can re-enable a hint cleanly if a different storage path benefits. +//! ## Why both +//! +//! `POSIX_FADV_SEQUENTIAL` at open widens the kernel's readahead window +//! so each pread aggregates into fewer NFS round-trips. `DONTNEED` on +//! the consumed window (called periodically by the caller) drops the +//! already-read pages from the page cache so an 85 GB streaming ISO +//! read doesn't fill memory and starve concurrent writes (the MKV +//! output during mux). Together they mirror the write-side +//! WritebackPipeline's policy. +//! +//! ## History +//! +//! Pre-Phase-1 (0.20.7 baseline) had both. Phase 1's introduction of +//! `FileSectorSource` silently dropped the read-side DONTNEED, and +//! 0.21.2's revert of `SEQUENTIAL` (mistakenly attributing a regression +//! to it) removed the hint. Net effect: 85 GB of ISO reads pinned in +//! the page cache + no readahead widening → mux throughput collapse +//! from 18 MB/s historical to 2.7-8 MB/s on 0.21.x. Restored in 0.21.6. use std::fs::File; +use std::os::unix::io::AsRawFd; -pub(super) fn hint_sequential(_file: &File, _len_bytes: u64) { - // No-op: see module-level comment. Kernel default readahead is - // what we want on NFS-backed ISOs, which is the dominant case. +pub(super) fn hint_sequential(file: &File, _len_bytes: u64) { + // Best-effort: return value ignored. A fadvise failure has no + // user-observable consequence. + unsafe { + libc::posix_fadvise(file.as_raw_fd(), 0, 0, libc::POSIX_FADV_SEQUENTIAL); + } +} + +/// Drop pages in the half-open byte range `[start, start+len)` from +/// the page cache. Called periodically by `read_sectors` to bound the +/// read-side page cache pressure. +pub(super) fn drop_window(file: &File, start: u64, len: u64) { + unsafe { + libc::posix_fadvise( + file.as_raw_fd(), + start as i64, + len as i64, + libc::POSIX_FADV_DONTNEED, + ); + } } diff --git a/src/io/file_sector_source/macos.rs b/src/io/file_sector_source/macos.rs index 0ef2350..a196d6c 100644 --- a/src/io/file_sector_source/macos.rs +++ b/src/io/file_sector_source/macos.rs @@ -36,3 +36,11 @@ pub(super) fn hint_sequential(file: &File, len_bytes: u64) { libc::fcntl(file.as_raw_fd(), F_RDADVISE, &mut ra); } } + +/// macOS has no direct `POSIX_FADV_DONTNEED` equivalent for a byte +/// range. `fcntl(F_NOCACHE)` would disable caching globally on the fd +/// (too coarse — we want the unread region to still benefit). Best +/// approximation: no-op. macOS's unified buffer cache is generally +/// less prone to the pin-everything pathology that triggers the +/// regression on Linux NFS clients. +pub(super) fn drop_window(_file: &File, _start: u64, _len: u64) {} diff --git a/src/io/file_sector_source/mod.rs b/src/io/file_sector_source/mod.rs index 2273f1f..99b2b98 100644 --- a/src/io/file_sector_source/mod.rs +++ b/src/io/file_sector_source/mod.rs @@ -87,6 +87,17 @@ const BUF_SECTORS: u32 = (READAHEAD_BUF_BYTES / SECTOR_SIZE) as u32; /// `read_sectors` is satisfied from the buffer when possible; otherwise /// a full-buffer refill is issued at the requested LBA's position and /// the call is re-tried against the freshly populated window. +/// Bytes-read threshold per `posix_fadvise(DONTNEED)` drop on the +/// read side. Mirrors `WRITEBACK_CHUNK_BYTES` so the read-side page +/// cache stays bounded the same way the write side does. +/// +/// 0.21.6: re-added after empirical discovery that Phase 1 had silently +/// dropped this from the pre-Phase-1 (0.20.7) hot path. Without it, +/// 85 GB of streaming ISO reads pin the entire file in the kernel page +/// cache, starving the MKV writeback and collapsing mux throughput +/// (observed: 2.7 MB/s mux on 0.21.5 vs. 70 MB/s isolated NFS reads). +const READ_DROP_CHUNK_BYTES: u64 = 32 * 1024 * 1024; + pub struct FileSectorSource { file: File, /// Total file size in sectors. Constant after construction; @@ -102,6 +113,13 @@ pub struct FileSectorSource { #[allow(dead_code)] buf_start_lba: u32, buf_len_sectors: u32, + /// 0.21.6: bytes read since the last DONTNEED drop. Drives the + /// per-`READ_DROP_CHUNK_BYTES` page-cache eviction in read_sectors. + bytes_read_since_drop: u64, + /// 0.21.6: file offset at which the current drop window starts. + /// The next DONTNEED drops from `drop_window_start` for + /// `bytes_read_since_drop` bytes. + drop_window_start: u64, } impl FileSectorSource { @@ -142,6 +160,8 @@ impl FileSectorSource { buf, buf_start_lba: 0, buf_len_sectors: 0, + bytes_read_since_drop: 0, + drop_window_start: 0, }) } @@ -228,6 +248,21 @@ impl SectorSource for FileSectorSource { .read_exact(&mut out[..bytes]) .map_err(|e| Error::IoError { source: e })?; self.buf_len_sectors = 0; + + // 0.21.6: periodic page-cache eviction on the read side. Without + // this, an 85 GB streaming ISO read pins the entire file in + // kernel page cache, which starves concurrent NFS writes (the + // MKV output) and collapses mux throughput. Mirrors the + // write-side WritebackPipeline's DONTNEED policy. + self.bytes_read_since_drop += bytes as u64; + if self.bytes_read_since_drop >= READ_DROP_CHUNK_BYTES { + let drop_start = self.drop_window_start; + let drop_len = self.bytes_read_since_drop; + platform::drop_window(&self.file, drop_start, drop_len); + self.drop_window_start = drop_start + drop_len; + self.bytes_read_since_drop = 0; + } + Ok(bytes) } } diff --git a/src/io/file_sector_source/other.rs b/src/io/file_sector_source/other.rs index b6e4f65..805192a 100644 --- a/src/io/file_sector_source/other.rs +++ b/src/io/file_sector_source/other.rs @@ -5,3 +5,5 @@ use std::fs::File; pub(super) fn hint_sequential(_file: &File, _len_bytes: u64) {} + +pub(super) fn drop_window(_file: &File, _start: u64, _len: u64) {} diff --git a/src/io/file_sector_source/windows.rs b/src/io/file_sector_source/windows.rs index 6e4b036..0cc76f3 100644 --- a/src/io/file_sector_source/windows.rs +++ b/src/io/file_sector_source/windows.rs @@ -17,3 +17,8 @@ pub(super) fn hint_sequential(_file: &File, _len_bytes: u64) { "FileSectorSource hint_sequential: windows stub (TODO: FILE_FLAG_SEQUENTIAL_SCAN at open)" ); } + +/// Windows page-cache eviction is not exposed via a posix_fadvise +/// equivalent. The kernel does its own working-set management. No-op +/// for now. +pub(super) fn drop_window(_file: &File, _start: u64, _len: u64) {}