From 6155628693eed6510607e949be0c9784552d0b3b Mon Sep 17 00:00:00 2001 From: Matthew Jackson Date: Sun, 17 May 2026 07:38:34 -0700 Subject: [PATCH] Revert "iter2: restore FileSectorSource readahead buffer (32 MiB)" This reverts commit 36a2b3af68268b69ae53656c5be56ee734ce716c. --- src/io/file_sector_source/mod.rs | 64 +++++++++++++++----------------- 1 file changed, 29 insertions(+), 35 deletions(-) diff --git a/src/io/file_sector_source/mod.rs b/src/io/file_sector_source/mod.rs index ecec53b..99b2b98 100644 --- a/src/io/file_sector_source/mod.rs +++ b/src/io/file_sector_source/mod.rs @@ -103,10 +103,14 @@ pub struct FileSectorSource { /// Total file size in sectors. Constant after construction; /// surfaced via [`SectorSource::capacity_sectors`]. capacity: u32, - /// 32 MiB application-level readahead buffer. Restored 2026-05-17 - /// after iter1 baseline (18.4 MB/s mean) showed per-sector pread is - /// the bottleneck. + /// 0.21.3+: the app-level buffer is no longer touched on the hot + /// path (every `read_sectors` is a direct pread). The fields are + /// retained so a future per-source-type policy (e.g. a local-disk + /// source where batched reads ARE beneficial) can re-enable + /// buffering cleanly without re-plumbing the struct. + #[allow(dead_code)] buf: Box<[u8]>, + #[allow(dead_code)] buf_start_lba: u32, buf_len_sectors: u32, /// 0.21.6: bytes read since the last DONTNEED drop. Drives the @@ -163,6 +167,7 @@ impl FileSectorSource { /// True if `[lba, lba + count)` is wholly inside the current /// buffer window. `count == 0` is vacuously true. + #[allow(dead_code)] fn buffer_covers(&self, lba: u32, count: u32) -> bool { if self.buf_len_sectors == 0 { return false; @@ -178,6 +183,7 @@ impl FileSectorSource { /// Refill the buffer so it starts at `lba`. Read as many sectors /// as we have buffer space AND file capacity for. Caller has /// already checked `lba < capacity`. + #[allow(dead_code)] fn refill(&mut self, lba: u32) -> Result<()> { debug_assert!(lba < self.capacity, "refill past capacity"); // Don't read past EOF — clamp the request to remaining @@ -220,40 +226,28 @@ impl SectorSource for FileSectorSource { if count == 0 { return Ok(0); } - // Iteration 2 (2026-05-17): restore the 32 MiB readahead buffer. + // 0.21.3: bypass the application-level buffer entirely. // - // The 0.21.3 bypass was justified by an A/B test taken under the - // 0.21.7 producer-side polling cap. Under that cap the producer - // couldn't push fast enough to saturate the channel regardless of - // read strategy, so the comparison "buffer vs no-buffer" measured - // the cap, not the read path. Iter1 baseline with bypass + Phase - // 2.5 + DONTNEED = 18.4 MB/s mean — well below the rig's measured - // concurrent-r+w ceiling (37 MB/s). Per-sector pread costs ~50 us - // each ≈ 19k syscalls/sec = 38 MB/s ceiling just in syscall - // overhead. The 32 MiB app buffer amortises that to 1 pread per - // 16k sectors and lets the kernel's readahead operate on a wider - // window. DONTNEED below still evicts the page cache so we don't - // pin the ISO in RAM. + // Empirically the 32 MiB readahead window (0.21.0–0.21.1) and the + // 4 MiB shrink (0.21.2) both regressed mux throughput vs the + // pre-Phase-1 0.20.7 baseline on NFS bidirectional workloads + // (sweep ~25 MB/s OK; mux dropped from 18 → 7-8 → 5-6 MB/s). + // Direct pread per call lets the kernel's own readahead policy + // run, which interleaves naturally with concurrent NFS writes on + // the same TCP connection. // - // Pathological-large requests (> BUF_SECTORS = 16384) fall back - // to direct pread so callers can't deadlock the source. - if count > BUF_SECTORS { - let offset = lba as u64 * SECTOR_SIZE as u64; - self.file - .seek(SeekFrom::Start(offset)) - .map_err(|e| Error::IoError { source: e })?; - self.file - .read_exact(&mut out[..bytes]) - .map_err(|e| Error::IoError { source: e })?; - self.buf_len_sectors = 0; - } else { - if !self.buffer_covers(lba, count) { - self.refill(lba)?; - } - let off_sectors = (lba - self.buf_start_lba) as usize; - let off_bytes = off_sectors * SECTOR_SIZE; - out[..bytes].copy_from_slice(&self.buf[off_bytes..off_bytes + bytes]); - } + // Buffer fields are retained (currently unused on this path) so + // any future per-source policy can be reintroduced without + // re-plumbing structure. `refill` / `buffer_covers` are kept too + // (still exercised by the tests so the API contract is locked). + let offset = lba as u64 * SECTOR_SIZE as u64; + self.file + .seek(SeekFrom::Start(offset)) + .map_err(|e| Error::IoError { source: e })?; + self.file + .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