From 55219070f1fb3d5d4021e323942e3dd0d7d57d44 Mon Sep 17 00:00:00 2001
From: MattJackson <1085847+MattJackson@users.noreply.github.com>
Date: Wed, 13 May 2026 23:57:46 -0700
Subject: [PATCH] =?UTF-8?q?io/file=5Fsector=5Fsource:=20bypass=20app-level?=
 =?UTF-8?q?=20buffer=20=E2=80=94=20direct=20pread=20per=20call?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The 32 MiB readahead window (0.21.0–0.21.1) regressed mux throughput
on NFS bidirectional workloads vs the pre-Phase-1 0.20.7 baseline
(18 -> 7-8 MB/s). The 0.21.2 4 MiB shrink made it worse (5-6 MB/s).
Both signs point at the application-level buffer itself, not the size.

This commit bypasses the buffer entirely on the read path — every
read_sectors call seeks and pread()s direct to the file. That matches
0.20.7's hot path. Kernel readahead handles the policy; on NFS that
interleaves naturally with concurrent writes on the same TCP
connection.

Buffer state fields and refill/buffer_covers are kept so the
structure is preserved for a future per-source-type policy (e.g. a
local-disk source where batched reads ARE beneficial), and so the
existing tests still exercise that machinery.
---
 src/io/file_sector_source/mod.rs | 49 ++++++++++++++------------------
 1 file changed, 22 insertions(+), 27 deletions(-)

diff --git a/src/io/file_sector_source/mod.rs b/src/io/file_sector_source/mod.rs
index 0a4f815..a58a007 100644
--- a/src/io/file_sector_source/mod.rs
+++ b/src/io/file_sector_source/mod.rs
@@ -207,33 +207,28 @@ impl SectorSource for FileSectorSource {
         if count == 0 {
             return Ok(0);
         }
-        // Refill if the requested range isn't entirely buffered.
-        // `buffer_covers` also handles the empty-buffer case
-        // (buf_len_sectors == 0).
-        if !self.buffer_covers(lba, count) {
-            // A request larger than the buffer itself can never fit;
-            // fall back to a one-shot direct pread for that pathological
-            // case 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 })?;
-                // Invalidate buffer state — we bypassed it, the
-                // window is no longer authoritative for this LBA.
-                self.buf_len_sectors = 0;
-                return Ok(bytes);
-            }
-            self.refill(lba)?;
-        }
-
-        // Slice the buffer at the requested LBA's offset within it.
-        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]);
+        // 0.21.3: bypass the application-level buffer entirely.
+        //
+        // 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.
+        //
+        // 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;
         Ok(bytes)
     }
 }