[Japabu]

Problem

bcachefs::Mounted::sync committed unconditionally: flush, write both superblock copies, flush again. The file server's cache already skips a device Flush it doesn't owe, but the superblock write always created one. So every fsync on DATA paid two block writes and an NVMe Flush, even with nothing to make durable. Before this change, persist.fsync_clean read 1.23–1.26 ms on the AMD bench and 0.46–3.4 ms on the T14, against Linux's 57–70 µs and 23 µs on the same disks.

Change

A mount keeps the superblock its last commit made durable (landed). A commit whose superblock names the same commit (Superblock::same_commit: equal apart from the allocation cursor) writes no superblock and issues no second flush.

• Why an equal superblock means an equal tree. With shadowing on, no node the landed root reaches is written over, and none of its blocks is handed out again before the next commit. So an equal root is the landed tree.
• Durability is unchanged. The first io.flush() still runs, so everything written before the sync, data pages and bitmap blocks included, is on the medium before sync returns. Under the file server's cache that flush asks the disk for nothing when nothing is dirty. The frees committed() makes in a sync reach the medium at the next flush, as before.
• Why the cursor is excluded. A commit's own frees move it back after its superblock lands. Comparing it would make the first sync after every commit write the superblock again.
• When landed is None. At mount, after mkfs, after into_formatted, and after a sync refused before its superblock landed. Each of those commits whole. A sync whose superblock landed but whose committed() frees were then refused keeps landed: the commit did land, and pending keeps the frees for the next sync.

The sync right after a commit still writes the bitmap blocks that commit freed, and so owes one Flush. Every later clean sync asks the disk for nothing.

The defect's record, issues/a-clean-fsync-on-data-writes-the-superblock-and-flushes-the-disk.md, is deleted: both halves of its exit are met (the host request count, and AMD in tens of µs). The T14 durable-write record (toyos-durable-writes-on-the-t14s-nvme-run-six-times-slower-in-some-boots.md) cited it. It now states the measured 15.9/16.1 µs, sourced to evidence pr-1099 at ac6b89a, metal-t14/. It also notes that the slow-mode boot B-run2 still read 16 µs, so the slow mode appears only where a Flush or a write is owed.

Checks

Evidence:

• host checks, red controls and AMD: https://github.com/ToyOSOrg/evidence/tree/a45b2742bc6fd009d4c68fd5560a48a0b906cabd (r2/, metal-amd/)

• T14: https://github.com/ToyOSOrg/evidence/tree/ac6b89a373b6dc24fbdf64ba1089a8c2d8c3b708/metal-t14 (the same boots serve fileserver: a volume's cache keeps an eighth of physical memory clean #1099)

• Host, at 59dd113 (rebased onto a5b330f); the head 2a599bb is the same code rebased onto 28fdfd7, its only change under issues/: cargo run -- --ci host, exit 0, 105 steps green (r2/ci-host.log). The error: test failed lines are all inside the CI's control sections.

• New host tests:

  • fs::tests::a_sync_with_nothing_uncommitted_writes_nothing: a clean sync writes no block and asks for exactly one device sync; a later change commits and remounts.
  • fs::tests::a_sync_after_a_refused_one_commits_again: a commit whose superblock was written and whose sync was refused is written again by the next.
  • data::tests::a_sync_with_nothing_new_asks_the_disk_for_nothing: through the file server's cache, a clean sync issues no disk write and no Flush, and a write over a page the file holds is written and flushed.

• Red controls, each applied as a patch and restored in the same run:

  • same_commit always false (r2/red.patch): both "nothing" tests fail.
  • From the first round, the superblock recorded as landed before its write (red/red2.patch): the refused-sync test fails.

• Metal, base four boots of osbench: persist and the disk's cached rows run on ToyOS's DATA, the bench's NVMe #1097 at 069cbbe against two boots of 34409c9, which is osbench: persist and the disk's cached rows run on ToyOS's DATA, the bench's NVMe #1097's head plus this change and fileserver: a volume's cache keeps an eighth of physical memory clean #1099. All passed.

  • AMD (metal-amd/): persist.fsync_clean 1,225,111–1,263,124 ns down to 39,787 and 38,645 ns.
  • T14 (pr-1099 metal-t14/): 464,505–3,418,250 ns down to 15,898 and 16,124 ns.
  • Rows that owe a Flush stay within the base's spread on AMD.
  • The bcachefs code in that treatment is this PR's; mem.anon.map and proc.spawn_wait.large, which moved on AMD, do not call Mounted::sync and are fileserver: a volume's cache keeps an eighth of physical memory clean #1099's to explain.

Coordination and risks

• issues/an-fsync-of-a-served-file-syncs-its-whole-volume.md is not changed. The format commits one root, so a one-file fsync would still commit the whole tree, and no persist row needs a narrower one.

🤖 Generated with Claude Code

[Japabu]

Review of #1098 at 55970f7

Architecture

The decision sits with the right owner. The format knows whether its tree changed, and the file server's cache knows whether the disk owes a Flush. Comparing whole superblocks, cursor aside, is better than a dirty flag: it covers every mutation without each mutation site having to remember to set something. The change adds about 16 production lines (the field, four constructors, the branch in sync, a PartialEq derive) and about 110 test lines. Mounted has one caller outside the crate, system/fileserver/src/data.rs. Formatted never syncs except in into_io, which consumes it.

Durability and crash consistency, traced

• Data written before the sync. The first io.flush() runs on both paths, so nothing written earlier is skipped. Through the cache, Cache::flush sets unflushed before every disk write, including writes that DIRTY_LIMIT forces. Only the running flush writes the disk, and unflushed is cleared only after the disk Flush is answered. So a cache flush that asks the disk for nothing really does owe nothing. The old second Flush was the only thing hiding a gap here, and there is none.
• Equal superblock means equal tree. Under shadowing, in_place is true only for blocks in the current op's taken. The landed root was taken before the last seal, so any change to the tree copies it, and the landed root block goes to pending, not fresh. It cannot be reallocated before a commit, so root_node cannot come back to it. A refused op restores the root, and fail gives back what it took. A data overwrite always dirties the open node, so persist → update_metadata → btree::insert copies the root, and that sync commits in full. The only shadow-off state is Formatted, and landed is None at every construction site.
• Cursor exclusion and the previous commit's bitmap frees. committed() frees pending after the superblock has landed. free_blocks + pending() is therefore unchanged, and only next_alloc moves. The next sync's first flush makes those bitmap writes durable. The cursor that is left on the medium is only where a scan starts, and the allocator re-checks the bitmap. Mounted<ReadWrite>::open recounts the free blocks from the bitmap. A later mount therefore sees a superblock that agrees with durable state.
• Skipped seal(). fresh keeps blocks taken since the last superblock write. Since no superblock was written, no tree on the medium names them, so freeing them at once is correct.
• Refused sync. If the first flush, either superblock copy, or the second flush fails, landed stays None and the next sync commits whole. If committed() fails, landed is Some, and that is correct: the superblock did land, pending keeps the rest, and the next sync skips and retries the frees. See finding 2 for the comment.
• Concurrency. sync takes &mut self, and the DataVolume methods are serialized. The cache's single-flush rule covers the device.

I found no path where sync returns before earlier writes are on the medium, and no path where a later mount sees a superblock that disagrees with durable state.

Evidence checked

• host/ci-host.diff matches git diff 370d7d9f6 55970f7d4 apart from index lines. ci-host.log ends 105 step(s), all green / exit 0. The three new tests run in it.
• The red controls fail as claimed: (9,4) against (7,3); (14,5) against (12,4); "the retry wrote [1], no superblock". Test 1 also exercises the cursor exclusion, because the commit's frees move next_alloc back.
• lib/bcachefs in the metal treatment 34409c9 is identical to 55970f7. The fileserver differences there are fileserver: a volume's cache keeps an eighth of physical memory clean #1099. Both B verdicts are passed. persist.fsync_clean p50 is 39,787 and 38,645 ns, against 1.23–1.26 ms in A.
• Correction to the handoff and the brief: not every other row stayed within A's spread in the combined treatment. mem.anon.map is 1.90x (B 388/382 against A 201–207 ns/MiB). proc.spawn_wait.large is 1.23x (B 211,008/208,091 against A 155,211–186,138 ns). Neither row calls Mounted::sync, so by inspection they are not this change. They belong to fileserver: a volume's cache keeps an eighth of physical memory clean #1099's review. The PR body itself claims only the durable rows, and that claim is true.

Findings

1. BLOCKER — issues/a-clean-fsync-on-data-writes-the-superblock-and-flushes-the-disk.md is now on main. osbench: persist and the disk's cached rows run on ToyOS's DATA, the bench's NVMe #1097 landed as e4bdb0a. This PR therefore lands second, and by its own coordination plan it must delete the record. Both halves of the record's exit are met: the host request count in data::tests::a_sync_with_nothing_new_asks_the_disk_for_nothing, and AMD at 39 µs. The T14 is not part of the exit. Rebase onto main, delete the record, and fix the sentence in issues/toyos-durable-writes-on-the-t14s-nvme-took-1-to-6-ms-across-two-boots.md that cites it as the cause of "the clean fsync's remaining 20x". Without this the change leaves a resolved defect open and a dangling reference. The rebase touches only issues/; no overlapping code changed on main, so the host evidence still holds.
2. BLOCKER — lib/bcachefs/src/fs.rs:160-161: "None until one lands, and after one is refused" is false. When committed() refuses a free, sync returns Err with landed already Some. That outcome is correct, but this comment states the contract of a durability invariant and gets it wrong. Suggested wording: "None until one lands, and after a sync refused before its superblock landed." The PR body's "restored only on success" has the same error; that correction is the orchestrator's.
3. PR body (orchestrator) — the "Coordination and risks" section is stale: the record is on main now. "Durability is unchanged … bitmap blocks written in place are on the medium before sync returns" should say "written before the sync". committed()'s frees in that same sync land at the next flush, as before.
4. SUGGESTION — fs.rs:942: give the comparison a name, for example a Superblock method like same_commit(&self, other) that documents the cursor exclusion where it is defined. Optional.

SEND BACK

[Japabu]

Review r2 of #1098 at 59dd113

The r1 fixes

• landed comment (r1 blocker 2): resolved. fs.rs:160-162 now says "None until one lands, and after a sync refused before its superblock landed". That matches the code: landed is set once the second flush succeeds, before committed().
• Superblock::same_commit (superblock.rs:120-125): correct. *self == Self { next_alloc: self.next_alloc, ..other.clone() } compares every field except the cursor. The caller in sync reads clearly. The cursor rationale moved with the comparison, and sync's doc comment no longer repeats it.
• Red control. r2/red.patch makes same_commit always false. Both "nothing" tests fail with exit 101, with the same counts as r1. r2/ci-host.diff matches git diff a5b330f39 59dd113ec apart from index lines. The log ends with 105 steps green and exit 0, and all three new tests run in it.
• Range-diff. The feature commit 8417449 is identical to 55970f7, so the r1 durability trace still holds.
• T14 figures. I checked them against ToyOSOrg/evidence branch pr-1099 at ac6b89a, under metal-t14/. B-run1 and B-run2 read persist.fsync_clean p50 15,898 and 16,124 ns, and both verdicts are passed. A-boot1..4 read 464,505 / 3,418,250 / 497,773 / 575,789 ns. These match the PR body and the main record's table.

Findings

1. BLOCKER: the PR is not rebased onto issues: the scoreboard's unrecorded gaps, and the T14's durable writes across five boots #1105, and the record edit lands in a file main no longer has. The merge base is a5b330f, which does not contain 965bb73. GitHub reports mergeable: CONFLICTING. issues: the scoreboard's unrecorded gaps, and the T14's durable writes across five boots #1105 renamed the record to issues/toyos-durable-writes-on-the-t14s-nvme-run-six-times-slower-in-some-boots.md, and line 35 of that file on main still cites the deleted issues/a-clean-fsync-on-data-writes-the-superblock-and-flushes-the-disk.md. Its sentence is also now false: "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". The PR's own coordination note says the PR that lands second carries the edit. issues: the scoreboard's unrecorded gaps, and the T14's durable writes across five boots #1105 has landed, so that is this PR.
   • Rebase onto main, drop the edit to the old filename, and replace that sentence in the renamed record. The new sentence should give the 15.9 and 16.1 µs readings, say that the clean fsync now writes nothing, and remove the citation.
   • The record names where its figures come from ("evidence branch scoreboard-2026-10-28's and PR osbench: persist and the disk's cached rows run on ToyOS's DATA, the bench's NVMe #1097's"), so the new sentence should name the source of the T14 figures as well: pr-1099 metal-t14/.
   • The rebase touches only issues/, so the host evidence is not invalidated.
2. PR body (orchestrator): the T14 figures are cited, but no T14 files are under the linked evidence tree a45b2742. They are on pr-1099 (ac6b89a, metal-t14/), so link that. Once the rebase is done, the "Coordination and risks" bullet about issues: the scoreboard's unrecorded gaps, and the T14's durable writes across five boots #1105 is settled and should go.
3. SUGGESTION: same_commit could be pub(crate), since it has no caller outside the crate. Its doc comment could also drop ", which the medium holds", which describes sync's use rather than the method. This is optional.
4. Observation, not for this PR: T14 B-run2 was a slow-mode boot, with write_4k_fsync at 6.26 ms. It still read fsync_clean at 16 µs. So the slow mode appears only where a Flush or a write is owed. That narrows the renamed record's open question, and the record may want to say so.

SEND BACK

[Japabu]

Orchestrator: r2's blocker checked at 2a599bb, rebased onto main after #1105. The renamed T14 record states the measured 15.9/16.1 µs with its source, plus the slow-mode observation. The deleted record is gone and no citation dangles. The code is byte-identical to the host-checked 59dd113. Landing.