[Japabu]

Problem

persist.seq_write_fsync writes 8 MiB in 1 MiB calls and fsyncs. #1113's probe boots attributed 2.4 ms per 8 MiB round on AMD, and 6.7 ms on the T14, to the file server's allocation (write/attribution.txt in #1113's evidence). DataVolume::write resolved each page past the file's end on its own (resolve_or_alloc_block). Each page cost a bitmap read to see whether the block after the file was free, another to find a run of one, and a read-modify-write of the bitmap block to mark it, all through the cache: four 4 KiB copies per 4 KiB written.

Change

• Mounted::allocate(extents, pages) covers a write's whole inclusive range of pages. Each run asks the allocator for every page still missing, and only the hole before the write's first page is zeroed, since the caller writes the rest. resolve_or_alloc_block is its one-page case.
• set_range_used marks a run with one read-modify-write per bitmap block its bits lie in, where it took one per block.
• DataVolume::write makes one allocate and one file_entry_fits check per call. If either refuses, it frees the call's blocks, as before. A write of no bytes allocates nothing.
• The four records that named resolve_or_alloc_block as the write path now name allocate, or block_for where they meant the lookup.

Placement deliberately departs from a page at a time (review r2, the coordinator's decision). Each run must be at least min_run blocks long, or the pages still missing if fewer. The file server passes the block ring's request size (toyos_blockring::MAX_REQUEST_BLOCKS, 32). alloc_first_fit takes the first free run that long from the file's end, or the longest it saw once its scan budget is spent, so single-block holes do not each become a run.

• Why: the probe boots (evidence r2/). Page-at-a-time placement, and r1's first fit that reproduced it, left each 8 MiB round's file in 103–106 runs, about 100 of them single blocks: holes behind the allocator's cursor, where a commit's freed nodes are the likely source. So a round's flush took 163–169 requests, 100–105 of them single blocks. Round 0's whole-remainder ask gave 8 runs of 256 and 64 full requests. persist.seq_write_fsync on the T14 read 175 MB/s page at a time, 208 with r1's single allocation call, and 236 with long runs; on the AMD, 524/527, 657 and 640.
• The bound (review r3; r2's "two passes" was false, because the jump past a file's taken next block could wrap a lap per run). Each phase examines at most one lap of the bitmap for the whole call. The first phase looks for runs min_run long, with one budget across all its runs. Once that is spent, the second phase takes runs as they come, sweeping forward from the last run's end without the jump, within one more lap. A call reads at most two laps of the bitmap, besides the blocks it takes.
• A refused run is unmarked (review r1). set_range_used can now mark several bitmap blocks for one run; if the device refuses one, the blocks already marked are cleared again.
• resolve_or_alloc_block is deleted: only tests called it, and they call allocate(p..=p). allocate's doc states the caller's duty: write every page of the range, zero-filling what it does not cover. Two stale comments are fixed: the commit header's in-place write names block_for, and write_pages no longer claims a kernel path.
• A pre-existing defect the review found is recorded here (issues/a-write-that-fails-after-allocating-leaves-another-files-bytes-past-the-end.md) and fixed in the PR stacked on this one: a write that fails after allocating left another file's bytes past the end.

Checks

Evidence: https://github.com/ToyOSOrg/evidence/tree/57c9e8245fe88f24e7c8d81c711bd3cfabccfc87 (round 0) and https://github.com/ToyOSOrg/evidence/tree/6e645aafbeeb932ad9748d2ddc34a3bf6c8102e8 (review r1, r1/) and https://github.com/ToyOSOrg/evidence/tree/22d33ce808000fc08b5d353d25bc418bbfde975f (review r2, r2/)

• Review r3 follow-up, head 1eb1a9f: the straddle join is deleted, as decided: its code, its doc sentence, its test and its claims here and in the commit message. Under allocate, phase one's budget is exactly one lap, so the join never ran, and phase two's minimum of one never needs it. Also, allocate's two conditions on sweep are one match, and the file server's call says why a run is one request long. X-F (bea91a3 plus the probe) still measures this behaviour, because the deleted code never ran. --ci host exit 0, 107 green at 09203d2; the head only rewraps one doc comment (evidence https://github.com/ToyOSOrg/evidence/tree/9eadefc77139ff66bf6de52ae4967de529bc4045, r4/).
• Review r3, head bea91a3, on blockring: the arena holds only what is on the wire, and a flush keeps many writes there #1113's 4ea1637: --ci host exit 0, 107 green. The first run was red on a clippy lint in two new tests, fixed by the amend (evidence https://github.com/ToyOSOrg/evidence/tree/ddef8f0232552826f5e80151c41293071276dd5a, r3/). New tests, each with a red control:
  • the reviewer's clustered layouts, each held to two laps of reads besides three a run on a volume of eight bitmap blocks:
    • a_write_into_one_cluster_of_single_holes_reads_two_laps_at_most, 64 single holes in 128 blocks;
    • a_write_into_one_cluster_of_long_runs_reads_two_laps_at_most, eight runs of 40 one block apart;
    • a_write_on_a_volume_of_single_holes_reads_two_laps_at_most, replacing r2's single-holes test.
    • Red: phase two jumping SPREAD (red-r7) fails both clusters; a budget per run (red-r8) reads 71 times for 7 runs, against 39.
  • The skip test now says 31 holes; red with runs of any length (red-r5).
  • At r3 (bea91a3), bcachefs 183 unit and 57 integration tests passed (ci-host-r3.log). After the straddle join's deletion, 182 unit tests and fileserver 90 pass.
• Metal X-D (r2's placement, both benches passed): T14 10–11 runs, 68–73 flush requests (63 full), seq_write_fsync 233 MB/s. AMD 9 runs, 70 requests, seq_write_fsync 639 and seq_write.1048576 913. r3 changes the search, so the head reran on metal:
• Metal X-F (bea91a3 plus probe2, against blockring: the arena holds only what is on the wire, and a flush keeps many writes there #1113's X-E; both passed; judged a pass, and the coordinator accepted both strict breaches below), judged item by item against the rule fixed before the run (r4/):
  • The 8 MiB file is in 8–9 runs and a round's flush in 68–71 requests, on both benches.
  • seq_write_fsync: T14 493 MB/s against 181, AMD 631 against 588. The T14 boot ran in its fast durable-write mode.
  • Strictly, two persist.* rows lie more than 5% past blockring: the arena holds only what is on the wire, and a flush keeps many writes there #1113's spread. T14 fsync_clean 15.84 µs follows the T14's durable-write mode: the other fast-mode boot read 16.07. AMD seq_read_cached 2,713 reads from the cache without allocating, and that day's AMD boots of unrelated trees range 2,217–3,290.
  • Files other than the 8 MiB one: 4 KiB writes ask for one block, so fs.disk.seq_write.4096's file has 15–20 runs. The minimum run cannot apply to a one-page write.
• Review r2, head 8be159d: --ci host exit 0, 105 green. The first run was red on a clippy lint in the new test (ci-host-r2.try1.log), fixed before the push. New tests, each with a red control:
  • fs::tests::a_write_skips_single_holes_for_a_run_a_request_long: 31 single holes at the cursor, then a long free run, and a 64-page write takes one extent in the long run, where a page at a time takes the first hole. Red: runs of any length (red-r5).
  • fs::tests::a_write_on_a_volume_of_single_holes_passes_its_bitmap_twice_at_most: eight bitmap blocks, only single holes. A 16-page write reads the bitmap 55 times, within two passes and three reads a run (64). Red: a pass per run, 167 reads (red-r6).
  • The integration test that held page-at-a-time placement (r1's a_range_is_placed_as_its_pages_one_at_a_time_would_be) is deleted: that placement is no longer the contract.
  • bcachefs 180 unit and 57 integration tests pass; fileserver 90.
• Metal A/B: staged, not run. One boot per bench of this head with probe2 (X-D), against X-B2, blockring: the arena holds only what is on the wire, and a flush keeps many writes there #1113's head with the same probe.
• Review r1, head af6d088: --ci host exit 0, 105 green. New tests:
  • a_range_is_placed_as_its_pages_one_at_a_time_would_be: twin volumes with a 4-block hole before a long free run give a 100-page file the same extents page by page and in one call. Red: the longest-run ask (red-m3).
  • alloc_bitmap::tests::a_run_across_two_bitmap_blocks_writes_each_once, on a 40,000-block volume, which has two bitmap blocks.
  • a_run_refused_halfway_leaves_nothing_marked. Red: no undo (red-m4).
  • bcachefs passes (178 unit, 58 integration), and so does fileserver (90).
• Round 0, at fb5adb5: cargo run -- --ci host exit 0, 105 steps green (alloc/ci-host.log).
• New host test: fs::tests::a_range_of_pages_is_allocated_with_one_write_per_bitmap_block, over the Counted device that records each block written.
  • 256 pages on an empty volume are one extent, and the only block written is the bitmap block.
  • A write after a 4-page hole writes the bitmap block and those 4 hole blocks, and none of its own pages.
• Red controls (evidence alloc/), each a patch on the change, applied, run with cargo test -p bcachefs --lib, then reverted:
  • red-m1-per-block-bitmap-writes: set_range_used writes per block. Exit 101; the new test fails.
  • red-m2-one-page-per-run: allocate asks for one page per run, the per-page loop. Exit 101; the new test fails.
  • DataVolume::write's single call is checked by inspection: it calls allocate once, outside any page loop.
• cargo test -p bcachefs (176 + 57 + 2 + 2) and cargo test -p fileserver (90) pass.
• Metal: staged with blockring: the arena holds only what is on the wire, and a flush keeps many writes there #1113's short A/B. Base: blockring: the arena holds only what is on the wire, and a flush keeps many writes there #1113 + the probe. Head: this + the same probe. One boot per bench. Rows: persist.seq_write_fsync and fs.disk.seq_write.*. The probe logs each round's file runs and flush run lengths, for the fragmentation question (below).

Not in this change

The write-failure leak the r1 review found is fixed in #1120, stacked on this.

🤖 Generated with Claude Code

[Japabu]

Review r1 of #1118 (head fb5adb5, base disk-path-write 558ab77)

Architecture

The change is in the right place. Range allocation belongs to Mounted, run marking belongs to the allocator, and DataVolume::write makes one call. Net change: +69/−29. That is about +20 lines of production code and +21 of test, plus four one-line record edits. No dependency, ABI or CI change.

Crash consistency is unchanged, as I traced it.

• The bitmap is still marked in place through the cache before any entry names the blocks. An entry reaches the tree only at close, fsync or sync.
• A refusal from allocate (NoSpace on a later run, a failed hole zero) or from file_entry_fits drops back to covered with keep_blocks, which splits a merged last extent correctly. The dropped blocks go through free_extents → give. No seal can come between the allocation and the free inside one call, so every dropped block is still fresh and is freed at once.
• After the free, free_blocks is back to its value before the call. A following sync therefore still meets Superblock::same_commit (bcachefs: a sync with nothing uncommitted writes no superblock #1098), and nothing the last commit reaches is handed out again.
• The NoSpace refusal partway through a run is covered at the fileserver level by every_file_reads_back_as_a_plain_map_of_its_accepted_writes (its 64 << 20 write). That test ran at head inside --ci host (system/fileserver step in alloc/ci-host.log).

The zero-length write no longer allocates a page. That is correct and simpler.

Findings

BLOCKER 1: lib/bcachefs/src/fs.rs:1084 with alloc_bitmap.rs:300-306,350. On a fragmented or nearly full volume, each run of a write scans the whole volume.

• longest_free_run stops early only when it finds a run of at least wanted. Otherwise it scans every bit of the volume and wraps.
• With want set to the whole remainder, each short run costs one full scan. A call needing k runs costs about k × total_blocks bit tests.
• The base asked for 1 per page (except across a hole). Each scan then stopped at the first free bit, so a whole call was about one pass over the blocks it walked.
• Out of space partway through a run is the worst case. When spare < want, wanted is clamped to spare, and unless all of spare is contiguous, every iteration is a full scan. Example: a volume with 200 scattered free blocks and a 1 MiB write does about 184 full-volume scans, then refuses NoSpace and frees them all. DATA's server is one thread, so every client waits behind that.
• The body says "one call may scan further than before". This finding gives the bound: a full scan per run instead of a scan to the next free block.
• Cheapest fix (inference from the code): give this caller first-fit semantics, "the first free run at or after from, up to want".
  • That reproduces the base's per-page placement block for block. Per page, the base took the next free block at or after from, and after a used block it jumped by +SPREAD. A first-fit run ends at a used block or at want, and the next iteration then takes the same +SPREAD.
  • The call stays one bounded pass, and the placement change the body flags goes away. write_data can keep longest-run semantics.
• Alternatively, refute this with a stated bound and a host measurement on a fragmented volume of a realistic DATA size.

BLOCKER 2: lib/bcachefs/src/fs.rs:1054. resolve_or_alloc_block has no production caller left.

• Its only callers are tests: fs.rs tests, tests/integration.rs and fileserver's fragmented() (data.rs:1133,1141). The kernel does not use bcachefs.
• A public API kept only for tests breaks the rule against production machinery that exists solely to satisfy a test.
• Delete it. Tests call allocate(&mut e, p..=p), then block_for where they need the block.

BLOCKER 3: stale comments naming the old path.

• lib/bcachefs/src/fs.rs:795: the commit-rules header says "A page written over where the file already holds it ([Mounted::resolve_or_alloc_block]) is written in place". The PR corrected the same sentence in issues/a-write-over-a-files-committed-page-is-not-shadowed.md to block_for, but not this one. The body's "the four records … now name allocate, or block_for" is therefore incomplete.
• lib/bcachefs/tests/integration.rs:580: "The path the kernel's flush takes: one resolve_or_alloc_block per dirty page" is false twice over. No kernel path exists, and the write path no longer goes page by page. Say what write_pages models (pages grown one at a time), or nothing.

SUGGESTION 1: lib/bcachefs/src/fs.rs:1847. The test covers only one bitmap block.

• counted(4096) has a single bitmap block (one block covers 32,768). No host test uses a volume larger than 32,768 blocks.
• So the boundary break in set_range_used (alloc_bitmap.rs:271-273) is reached by no test. A mistake there would set a bit in the wrong bitmap block, and that block would be handed out twice.
• Inspection says the loop is correct. A cheap test would still hold it:
  • a BitmapAllocator::format over a 2-block VecBlockIO with total_blocks of, say, 40,000 (only bitmap blocks are touched);
  • a run that straddles block 32,768;
  • assert each bitmap block is written once, and the result of count_free.
• The test's present claim (one write for one bitmap block; hole blocks zeroed, pages the caller writes untouched) is real. Both red controls fail it as the evidence shows (alloc/red-m1…, red-m2…, exit 101).

SUGGESTION 2: alloc_bitmap.rs:262 and 333. A run's partial failure leaks bits.

• If set_range_used fails after writing an earlier bitmap block, reserve returns the error. The bits already set then belong to no record: not fresh, not op.taken, and not counted in free_blocks.
• They are leaked until a sweep, and free_blocks reads high. This behaviour already exists for write_data. Before this change the data write path only marked one-block runs, so it never hit it; now it marks multi-block runs and can.
• Either clear the bits already set before returning, or note it in issues/a-crash-leaks-the-blocks-of-its-last-commit.md.

SUGGESTION 3: fs.rs:1062-1071. The allocate doc.

• The sentence "Reading the short run as a complete one returned … another file" is history, and Run's doc already says the same.
• "Writes whole or in part" leaves out the caller's duty. Each newly allocated page of pages must be written whole, with any part the write does not cover zero-filled, or it reads the previous tenant's bytes. data.rs does this (page_buf[0].fill(0)).
• Suggested replacement: "the caller writes every page of pages, zero-filling what it does not cover".

Unrelated discovery (pre-existing, not blocking)

system/fileserver/src/data.rs:577-596: if the write loop fails after allocating (a cache.write or read error), the new blocks stay in open.extents past open.size without being written.

• A later extending truncate, or a write beyond them, brings them under the file's length. allocate zeroes only from covered, which already includes them, so a read then returns the previous tenant's bytes.
• The base had the same ordering: allocate everything, then write.
• Worth an issue. Dropping back to covered on any error from the loop, as the refusal path does, would close it.

Evidence

• --ci host passed at fb5adb5: exit 0, 105 steps. It includes bcachefs and system/fileserver, so the standalone crate runs from before the rebase are not needed.
• The red controls are faithful. m1 restores per-block set_used. m2 restores want=1, which is exactly the per-page loop.
• The motivating 2.4–6.7 ms per 8 MiB is blockring: the arena holds only what is on the wire, and a flush keeps many writes there #1113's measurement. This PR's metal A/B is staged and not yet run, so no gain is claimed yet, correctly. That A/B runs on a lightly used volume and cannot show Blocker 1.

SEND BACK

[Japabu]

Review r2 of #1118 (head af6d088, base disk-path-write 20bdd14)

I reviewed the new r1 commit. The rebased step-1 commit 8bad77f has the same patch as r0's fb5adb5 (diff of the two patches is empty).

r1 blockers

• B1, the scan bound: resolved. alloc_first_fit (alloc_bitmap.rs:336) takes the first free block at or after from, wrapping once, then extends the run while blocks stay free, up to want. The run stops at the volume's end.
  • I traced it against the base's per-page loop. Within a free run the base took next because is_free(next). At a used block, or at the volume's end, both versions go to next % total + SPREAD and take the first free block there. The spare clamp gives the same total.
  • So placement and scan length match a page at a time, and the full-volume rescan per short run is gone.
  • The out-of-space case is now one walk to NoSpace, followed by keep_blocks/free_extents as before.
  • alloc_up_to, with longest-run semantics, stays for write_data, its only caller.
• B2: resolved. resolve_or_alloc_block is deleted. Its test callers now call allocate(p..=p), then block_for, or block_of in integration.rs, because block_for is private to the crate.
• B3: resolved. The comment at fs.rs:795 now names block_for. write_pages (integration.rs:580) no longer claims a kernel path.

Suggestions taken

• Run across two bitmap blocks. a_run_across_two_bitmap_blocks_writes_each_once uses a 2-block device under a 40,000-block bitmap. It covers the boundary I flagged and asserts the written list [0, 1], the bit edges and count_free.
• Undo of a refused run. set_range_used now clears what it marked when a later bitmap block is refused. That is correct: the scan had just read those bits as free, so clearing them restores the prior state. If the undo is itself refused, it stops, as its doc says.
  • a_run_refused_halfway_leaves_nothing_marked tests this, and the red-m4-no-undo control fails it (exit 101).
• allocate doc. It now states the zero-fill duty, and the history sentence is gone.
• Data-exposure defect. It is recorded in issues/a-write-that-fails-after-allocating-leaves-another-files-bytes-past-the-end.md, with frontmatter valid under issues/README.md and an exit and owner. It is fixed in fileserver: a write refused after allocating gives back the blocks it took #1120, which is under separate review.

Evidence

• Host CI. --ci host at af6d088: exit 0, 105 steps (r1/ci-host-r1.log, ci-host.rev). cargo test -p bcachefs --release (178 + 58 + 2 + 2) and -p fileserver (90) pass on the same code.
• m3 control. red-m3-longest-run puts alloc_up_to back. a_range_is_placed_as_its_pages_one_at_a_time_would_be fails, exit 101.
  • The twin-volume test is a real discriminator. The 4-block hole at next_alloc is skipped by longest-run and taken by first-fit, and the test asserts that the first run is 4.
• Round-0 controls, not rerun: this does not matter. The assertions they proved able to fail are unchanged, and the new code does not weaken them.
  • m1 (per-block bitmap writes): both a_range_of_pages_… (one write for 256 pages) and the new two-bitmap-block test ([0, 1]) compare exact written lists. A per-block set_range_used would make those lists 256 and 16 entries long. Inspection establishes this; a rerun would add nothing.
  • m2 (want = 1): the line still applies. Through alloc_first_fit it still gives 256 one-block runs and 256 bitmap writes, and the unchanged test asserts one.

Findings

No blockers.

SUGGESTION: alloc_bitmap.rs:262-263. The doc says "a write the device refuses". A refused bitmap read also triggers the undo. "A bitmap block the device refuses" would cover both.

Note for the orchestrator (not a code finding):

• The staged metal X-C images were built from bcachefs: a write's pages are allocated at once, one bitmap write per block of bits #1118 at r0 (fb5adb5 plus the probe, 8e2bb97ce).
• At af6d088 the runs a write takes differ from r0's wherever the free run at the file's end is shorter than the write. r0 skipped such a run; r1 takes it, as the base did.
• So that A/B's probe readings on file layout and flush runs describe r0, not this head. Its timing of the one-bitmap-write change still applies.
• Whether to rebuild X-C before the run is the orchestrator's call.

LAND