bcachefs: a sync with nothing uncommitted writes no superblock

issues/toyos-durable-writes-on-the-t14s-nvme-run-six-times-slower-in-some-boots.md

@@ -30,10 +30,14 @@ A third turned slow partway, between `append_4k_fsync` and
 `create_fsync_unlink`. No change to the NVMe driver, the block service, the
 file server or the DATA format lies between the two heads. The boots' NVMe
 and DATA log lines do not tell the slow ones apart, and the cause is
-unknown. A fast boot's clean fsync, a superblock write and a Flush, takes
-0.46 to 0.58 ms where Linux's takes 23 µs
-(`issues/a-clean-fsync-on-data-writes-the-superblock-and-flushes-the-disk.md`).
-The figures are evidence branch `scoreboard-2026-10-28`'s and PR #1097's.
+unknown. The figures are evidence branch `scoreboard-2026-10-28`'s and PR
+#1097's.
+
+A clean fsync now writes nothing (#1098): two boots of 34409c9c2 read it at
+15.9 and 16.1 µs, against Linux's 23 µs (evidence branch `pr-1099` at
+ac6b89a, `metal-t14/`). The second of those boots ran slow, its
+`write_4k_fsync` at 6.26 ms, and its clean fsync still read 16 µs: the slow
+mode appears only where a Flush or a write is owed.
 
 **Exit**: repeated boots on the T14 read every durable `persist` row
 (`write_4k_fsync`, `append_4k_fsync`, `create_fsync_unlink` and

lib/bcachefs/src/fs.rs

@@ -157,6 +157,10 @@ pub struct Mounted<IO: BlockIO, Mode = ReadWrite> {
     io: IO,
     sb: Superblock,
     alloc: BitmapAllocator,
+    /// The superblock both copies hold on the medium, made durable by this
+    /// mount's last commit; `None` until one lands, and after a sync refused
+    /// before its superblock landed.
+    landed: Option<Superblock>,
     _mode: PhantomData<Mode>,
 }
 
@@ -557,7 +561,7 @@ impl<IO: BlockIO> Formatted<IO> {
 
         sb.write(&io)?;
 
-        Ok(Self(Mounted { io, sb, alloc, _mode: PhantomData }))
+        Ok(Self(Mounted { io, sb, alloc, landed: None, _mode: PhantomData }))
     }
 
     /// Name this filesystem, so whoever serves it can say which one it is.
@@ -588,7 +592,7 @@ impl<IO: BlockIO> Formatted<IO> {
     /// Mount this formatted filesystem for read-only access.
     pub fn mount_readonly(self) -> Mounted<IO, ReadOnly> {
         let Mounted { io, sb, alloc, .. } = self.0;
-        Mounted { io, sb, alloc, _mode: PhantomData }
+        Mounted { io, sb, alloc, landed: None, _mode: PhantomData }
     }
 
     /// Consume and return the underlying IO (for extracting the image bytes).
@@ -627,7 +631,7 @@ impl<IO: BlockIO, Mode> Mounted<IO, Mode> {
     fn read_superblock(io: IO) -> Result<Self, FsError> {
         let sb = Superblock::read(&io)?;
         let alloc = BitmapAllocator::open(&sb);
-        Ok(Mounted { io, sb, alloc, _mode: PhantomData })
+        Ok(Mounted { io, sb, alloc, landed: None, _mode: PhantomData })
     }
 
     /// The device this filesystem was opened over.
@@ -759,7 +763,7 @@ impl<IO: BlockIO, Mode> Mounted<IO, Mode> {
     pub fn into_formatted(self) -> Formatted<IO> {
         let Self { io, sb, mut alloc, .. } = self;
         alloc.shadow = false;
-        Formatted(Mounted { io, sb, alloc, _mode: PhantomData })
+        Formatted(Mounted { io, sb, alloc, landed: None, _mode: PhantomData })
     }
 
     /// Return the extents and file size for a file.
@@ -923,18 +927,27 @@ impl<IO: BlockIO> Mounted<IO, ReadWrite> {
         })
     }
 
-    /// Commit the tree as it stands: see this block's header.
+    /// Commit the tree as it stands: see this block's header. A commit whose
+    /// superblock is the one the medium already holds writes none, since no
+    /// node its root reaches has been written over since it landed; the
+    /// device is still flushed, for what is written in place.
     pub fn sync(&mut self) -> Result<(), FsError> {
+        let landed = self.landed.take();
         self.io.flush()?;
         // What the volume holds free once this commit's own frees are made.
         self.sb.free_blocks = self.alloc.free_blocks + self.alloc.pending();
         self.sb.next_alloc = self.alloc.next_alloc;
         self.sb.set_clean(true);
-        // From the first superblock write on, this commit may be what a mount
-        // finds, even where the write is refused.
-        self.alloc.seal();
-        self.sb.write(&self.io)?;
-        self.io.flush()?;
+        if landed.as_ref().is_some_and(|l| self.sb.same_commit(l)) {
+            self.landed = landed;
+        } else {
+            // From the first superblock write on, this commit may be what a
+            // mount finds, even where the write is refused.
+            self.alloc.seal();
+            self.sb.write(&self.io)?;
+            self.io.flush()?;
+            self.landed = Some(self.sb.clone());
+        }
         self.alloc.committed(&self.io)
     }
 
@@ -1784,4 +1797,83 @@ mod tests {
             assert!(fs.is_dir(&dir).expect("a walk of a sound tree"), "{dir}");
         }
     }
+
+    /// A device that records the blocks it is asked to write and counts its
+    /// syncs, and refuses the sync `refuse` numbers.
+    struct Counted {
+        io: VecBlockIO,
+        written: core::cell::RefCell<Vec<u64>>,
+        syncs: core::cell::Cell<u32>,
+        refuse: core::cell::Cell<Option<u32>>,
+    }
+
+    impl Counted {
+        fn counts(&self) -> (usize, u32) {
+            (self.written.borrow().len(), self.syncs.get())
+        }
+    }
+
+    impl BlockIO for Counted {
+        fn read_block(&self, block: BlockNum, buf: &mut BlockBuf) -> Result<(), DeviceError> {
+            self.io.read_block(block, buf)
+        }
+        fn write_block(&self, block: BlockNum, buf: &BlockBuf) -> Result<(), DeviceError> {
+            self.written.borrow_mut().push(block.raw());
+            self.io.write_block(block, buf)
+        }
+        fn block_count(&self) -> u64 {
+            self.io.block_count()
+        }
+        fn sync(&self) -> Result<(), DeviceError> {
+            let n = self.syncs.get();
+            self.syncs.set(n + 1);
+            if self.refuse.get() == Some(n) { Err(DeviceError::FAILED) } else { Ok(()) }
+        }
+    }
+
+    fn counted(blocks: u64) -> Mounted<Counted, ReadWrite> {
+        let io = Counted {
+            io: VecBlockIO::from_vec(image(blocks)),
+            written: core::cell::RefCell::default(),
+            syncs: core::cell::Cell::default(),
+            refuse: core::cell::Cell::default(),
+        };
+        Mounted::<_, ReadWrite>::open(io).expect("mount")
+    }
+
+    /// A sync with nothing changed since the last commit writes no block and
+    /// asks the device for one sync, for whatever was written in place; a
+    /// change after it is committed again.
+    #[test]
+    fn a_sync_with_nothing_uncommitted_writes_nothing() {
+        let mut fs = counted(128);
+        fs.create("a", b"one", 2).expect("create");
+        fs.sync().expect("the commit");
+        let (writes, syncs) = fs.io().counts();
+        fs.sync().expect("a clean sync");
+        assert_eq!(fs.io().counts(), (writes, syncs + 1));
+
+        fs.create("b", b"two", 3).expect("create");
+        fs.sync().expect("the next commit");
+        let (after, _) = fs.io().counts();
+        assert!(after > writes + 2, "a change and its commit wrote {} blocks", after - writes);
+        let raw = mount_rw(fs.io.io.into_vec()).expect("remount");
+        assert_eq!(raw.read_file("b").expect("read"), b"two");
+    }
+
+    /// A commit whose superblock was written and whose sync was refused is
+    /// made again by the next, though nothing changed between them.
+    #[test]
+    fn a_sync_after_a_refused_one_commits_again() {
+        let mut fs = counted(128);
+        fs.create("a", b"one", 2).expect("create");
+        fs.sync().expect("the commit");
+        fs.create("b", b"two", 3).expect("create");
+        fs.io().refuse.set(Some(fs.io().syncs.get() + 1));
+        assert!(fs.sync().is_err(), "the device refused the sync after the superblock");
+        let (writes, _) = fs.io().counts();
+        fs.sync().expect("the commit again");
+        let again = fs.io().written.borrow()[writes..].to_vec();
+        assert!(again.contains(&0) && again.contains(&127), "the retry wrote {again:?}, no superblock");
+    }
 }

lib/bcachefs/src/superblock.rs

@@ -98,7 +98,7 @@ impl fmt::Display for FsUuid {
 }
 
 /// On-disk superblock layout. Stored at block 0 and backed up at the last block.
-#[derive(Debug, Clone)]
+#[derive(Debug, Clone, PartialEq, Eq)]
 pub struct Superblock {
     pub block_count: u64,
     pub root_node: BlockNum,
@@ -117,6 +117,13 @@ pub struct Superblock {
 impl Superblock {
     pub(crate) const CRC_START: usize = 12; // CRC covers bytes [12..4096]
 
+    /// Whether `self` names the same commit as `other`, which the medium
+    /// holds. The allocation cursor is not compared: it is only where a scan
+    /// begins, and a commit's own frees move it after its superblock lands.
+    pub fn same_commit(&self, other: &Self) -> bool {
+        *self == Self { next_alloc: self.next_alloc, ..other.clone() }
+    }
+
     pub fn is_clean(&self) -> bool {
         self.flags & 1 != 0
     }

system/fileserver/src/data.rs

@@ -1319,17 +1319,18 @@ mod tests {
         /// Shared, so a test reads what the disk holds while the server runs.
         image: Rc<std::cell::RefCell<Image>>,
         writes: std::cell::Cell<usize>,
+        flushes: std::cell::Cell<usize>,
         keep: usize,
         refuse: Option<usize>,
     }
 
     impl Stops {
         fn new(blocks: u64) -> Self {
-            Self { blocks, image: Rc::default(), writes: Default::default(), keep: usize::MAX, refuse: None }
+            Self { blocks, image: Rc::default(), writes: Default::default(), flushes: Default::default(), keep: usize::MAX, refuse: None }
         }
 
         fn from(image: &Image, blocks: u64, keep: usize, refuse: Option<usize>) -> Self {
-            Self { blocks, image: Rc::new(image.clone().into()), writes: Default::default(), keep, refuse }
+            Self { blocks, image: Rc::new(image.clone().into()), writes: Default::default(), flushes: Default::default(), keep, refuse }
         }
     }
 
@@ -1364,12 +1365,37 @@ mod tests {
         }
 
         async fn flush(&self) -> Result<(), toyos_blockdev::Error> {
+            self.flushes.set(self.flushes.get() + 1);
             Ok(())
         }
     }
 
     impl Disk for Stops {}
 
+    /// A sync with nothing written since the last asks the disk for nothing,
+    /// and one after a write over a page the file holds writes the page and
+    /// makes it durable. The sync after a commit still writes the blocks that
+    /// commit freed, which are marked free only once it has landed.
+    #[test]
+    fn a_sync_with_nothing_new_asks_the_disk_for_nothing() {
+        let mut v = DataVolume::format(Stops::new(1024), &["home", "apps"], clock).unwrap();
+        let n = v.open("home/f", CREATE).unwrap();
+        v.write(n, 0, &[1; BLOCK]).unwrap();
+        assert_eq!(v.sync(), Ok(Vec::new()));
+        assert_eq!(v.sync(), Ok(Vec::new()));
+        let asked = |v: &DataVolume<Stops>| (v.cache.disk().writes.get(), v.cache.disk().flushes.get());
+        let synced = asked(&v);
+        assert_eq!(v.sync(), Ok(Vec::new()));
+        assert_eq!(asked(&v), synced);
+
+        v.write(n, 0, &[2; BLOCK]).unwrap();
+        assert_eq!(v.sync(), Ok(Vec::new()));
+        let (writes, flushes) = asked(&v);
+        assert!(writes > synced.0 && flushes > synced.1, "the write over the page asked {:?} after {synced:?}", (writes, flushes));
+        let block = block_for(&v.open[&n].extents, 0).unwrap();
+        assert_eq!(v.cache.disk().image.borrow()[&block][..], [2; BLOCK][..]);
+    }
+
     fn take(v: DataVolume<Stops>) -> Stops {
         let DataVolume { fs, cache, .. } = v;
         drop(fs);