From 812150444e89760346c52a28cb60c401937a0e11 Mon Sep 17 00:00:00 2001 From: japabu Date: Wed, 28 Oct 2026 18:06:59 +0100 Subject: [PATCH 1/2] bcachefs: a sync with nothing uncommitted writes no superblock Mounted::sync committed unconditionally: it flushed the device, wrote both superblock copies and flushed again. Under the file server's cache the first flush costs nothing when nothing is dirty, but the superblock write always left one owed, so every fsync on DATA paid two block writes and an NVMe Flush. #1097's AMD readback has persist.fsync_clean at 1.23 ms against 57-70 us under Linux on the same disk. A mount now keeps the superblock its last commit made durable. A commit whose superblock equals it, the allocation cursor aside, writes none: under shadowing no node the landed root reaches is written over, so an equal root is the landed tree. The device is still flushed first, for the data pages and bitmap blocks written in place, so a sync still returns only once everything written before it is on the medium. The cursor is excluded because a commit's own frees move it back after its superblock lands, and where the next scan starts is no reason to commit. Nothing is known landed at mount, after mkfs or after a refused sync, so each of those commits whole. Co-Authored-By: Claude Opus 5.5 --- lib/bcachefs/src/fs.rs | 114 ++++++++++++++++++++++++++++++--- lib/bcachefs/src/superblock.rs | 2 +- system/fileserver/src/data.rs | 30 ++++++++- 3 files changed, 133 insertions(+), 13 deletions(-) diff --git a/lib/bcachefs/src/fs.rs b/lib/bcachefs/src/fs.rs index ddf3da0179..79d976eca9 100644 --- a/lib/bcachefs/src/fs.rs +++ b/lib/bcachefs/src/fs.rs @@ -157,6 +157,9 @@ pub struct Mounted { 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 one is refused. + landed: Option, _mode: PhantomData, } @@ -557,7 +560,7 @@ impl Formatted { 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 +591,7 @@ impl Formatted { /// Mount this formatted filesystem for read-only access. pub fn mount_readonly(self) -> Mounted { 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 +630,7 @@ impl Mounted { fn read_superblock(io: IO) -> Result { 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 +762,7 @@ impl Mounted { pub fn into_formatted(self) -> Formatted { 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 +926,30 @@ impl Mounted { }) } - /// 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. The allocation + /// cursor alone is no change: it is only where a scan begins, and a + /// commit's own frees move it after its superblock lands. 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()?; + let unchanged = landed.as_ref().is_some_and(|l| Superblock { next_alloc: l.next_alloc, ..self.sb.clone() } == *l); + if unchanged { + 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 +1799,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>, + syncs: core::cell::Cell, + refuse: core::cell::Cell>, + } + + 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 { + 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"); + } } diff --git a/lib/bcachefs/src/superblock.rs b/lib/bcachefs/src/superblock.rs index 8b3209943a..8a8211a28a 100644 --- a/lib/bcachefs/src/superblock.rs +++ b/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, diff --git a/system/fileserver/src/data.rs b/system/fileserver/src/data.rs index a19b3eb3c8..f384f15e0c 100644 --- a/system/fileserver/src/data.rs +++ b/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>, writes: std::cell::Cell, + flushes: std::cell::Cell, keep: usize, refuse: Option, } 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) -> 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| (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 { let DataVolume { fs, cache, .. } = v; drop(fs); From 2a599bb22654c3197355d3447c4d8cec262f7058 Mon Sep 17 00:00:00 2001 From: japabu Date: Wed, 28 Oct 2026 22:37:59 +0100 Subject: [PATCH 2/2] review r1: the clean-fsync record goes, and `landed` says when it is None The record's exit is met: the host test counts no request from a clean sync, and the AMD bench reads persist.fsync_clean at 39.8 and 38.6 us against 1.23-1.26 ms before (the T14: 15.9 and 16.1 us). The T14 record's sentence that named it as the remaining cost now says what the clean fsync reads. `landed` is still Some when committed() refuses a free: the superblock did land. Its comment said None after any refused sync, which was false. The comparison is Superblock::same_commit, with the cursor's exclusion stated where it is defined. Co-Authored-By: Claude Opus 5.5 --- ...tes-the-superblock-and-flushes-the-disk.md | 31 ------------------- ...nvme-run-six-times-slower-in-some-boots.md | 12 ++++--- lib/bcachefs/src/fs.rs | 10 +++--- lib/bcachefs/src/superblock.rs | 7 +++++ 4 files changed, 19 insertions(+), 41 deletions(-) delete mode 100644 issues/a-clean-fsync-on-data-writes-the-superblock-and-flushes-the-disk.md diff --git a/issues/a-clean-fsync-on-data-writes-the-superblock-and-flushes-the-disk.md b/issues/a-clean-fsync-on-data-writes-the-superblock-and-flushes-the-disk.md deleted file mode 100644 index 45d793b892..0000000000 --- a/issues/a-clean-fsync-on-data-writes-the-superblock-and-flushes-the-disk.md +++ /dev/null @@ -1,31 +0,0 @@ ---- -status: open -kind: defect -opened: 2026-10-28 ---- - -# A clean fsync on DATA writes the superblock and flushes the disk - -An fsync of a file on DATA syncs the whole volume -(`issues/an-fsync-of-a-served-file-syncs-its-whole-volume.md`), and the -volume's sync, `bcachefs::Mounted::sync` (`lib/bcachefs/src/fs.rs`), commits -unconditionally. It flushes the cache, writes the superblock, and flushes -again. The file server's cache issues a device flush only where it owes one, -after a write it made. The superblock write makes one owed every time, so an -fsync with nothing new to make durable still costs a block write and an NVMe -Flush. - -Measured on the AMD bench (2026-10-28, `osbench persist` at PR #1097's -c8f3fbfb0, DATA on its designated NVMe partition): `persist.fsync_clean` -p50 1,233,821 ns under ToyOS, against 56,649 and 69,703 ns under the -bench's Linux on the same NVMe. That is about the cost of one Flush on that -disk, and `persist.write_4k_fsync`, which owes one, reads 2.9 ms on ToyOS -and 3.0 to 3.6 ms on Linux. - -**Exit**: a sync of a volume with nothing uncommitted writes and flushes -nothing, shown by counting the requests a clean sync issues on a host disk. -`persist.fsync_clean` on the AMD bench then reads within the file server's -round trip, tens of µs and not a Flush's ms. - -**Owner**: the DATA format (`lib/bcachefs/src/fs.rs`) and the file server's -DATA volume (`system/fileserver/src/data.rs`). diff --git a/issues/toyos-durable-writes-on-the-t14s-nvme-run-six-times-slower-in-some-boots.md b/issues/toyos-durable-writes-on-the-t14s-nvme-run-six-times-slower-in-some-boots.md index dac95da620..5f0ed8f195 100644 --- a/issues/toyos-durable-writes-on-the-t14s-nvme-run-six-times-slower-in-some-boots.md +++ b/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 diff --git a/lib/bcachefs/src/fs.rs b/lib/bcachefs/src/fs.rs index 79d976eca9..2841387cf7 100644 --- a/lib/bcachefs/src/fs.rs +++ b/lib/bcachefs/src/fs.rs @@ -158,7 +158,8 @@ pub struct Mounted { 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 one is refused. + /// mount's last commit; `None` until one lands, and after a sync refused + /// before its superblock landed. landed: Option, _mode: PhantomData, } @@ -929,9 +930,7 @@ impl Mounted { /// 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. The allocation - /// cursor alone is no change: it is only where a scan begins, and a - /// commit's own frees move it after its superblock lands. + /// 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()?; @@ -939,8 +938,7 @@ impl Mounted { self.sb.free_blocks = self.alloc.free_blocks + self.alloc.pending(); self.sb.next_alloc = self.alloc.next_alloc; self.sb.set_clean(true); - let unchanged = landed.as_ref().is_some_and(|l| Superblock { next_alloc: l.next_alloc, ..self.sb.clone() } == *l); - if unchanged { + 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 diff --git a/lib/bcachefs/src/superblock.rs b/lib/bcachefs/src/superblock.rs index 8a8211a28a..2462012247 100644 --- a/lib/bcachefs/src/superblock.rs +++ b/lib/bcachefs/src/superblock.rs @@ -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 }