[Japabu]

Problem

Found in review of #1118. DATA's write allocates every page past the file's extents, then writes the pages through the cache. When a page's cache write, or the read of a page written in part, failed, the call returned the error with the pages it never wrote still in the file's extents, past its length. Those blocks held what their last file left there.

A later extending truncate, or a write beyond them, brought them under the file's length. allocate zeroes only from the extents' end, which already included them, so a read returned a deleted file's bytes: one file's contents shown to another's reader. This predates #1118.

Change

DataVolume::write's allocation and its copy feed one result. Any refusal drops the extents back to their length before the call (keep_blocks) and frees the rest (free_extents), as a refused allocation already did. Pages written over in place keep what the call wrote, as before. The record #1118 added is deleted as fixed.

Checks

Evidence: https://github.com/ToyOSOrg/evidence/tree/fd1fd531843b009e0d6eb58be1ce2d8522ec19ee

• Restacked onto bcachefs: a write's pages are allocated at once, one bitmap write per block of bits #1118 at 1eb1a9f (the straddle join deleted), head 185a190. Clean; cargo test -p fileserver 91 passed.
• Earlier, restacked onto bcachefs: a write's pages are allocated at once, one bitmap write per block of bits #1118 r3 (bea91a3, on blockring: the arena holds only what is on the wire, and a flush keeps many writes there #1113's 4ea1637), head dacf972. Clean; the file server's allocate call carries bcachefs: a write's pages are allocated at once, one bitmap write per block of bits #1118's new min_run argument. cargo test -p fileserver 91 passed; cargo run -- --ci host exit 0, 107 green.
• Earlier, restacked onto bcachefs: a write's pages are allocated at once, one bitmap write per block of bits #1118 r2 (8be159d), head eb77322. The rebase was clean; bcachefs: a write's pages are allocated at once, one bitmap write per block of bits #1118 r2 touches only bcachefs's allocator under this change. cargo test -p fileserver --release 91 passed on the restacked head.
• Review r1, head 8c3bfd2. The test asserts Err(Error::Io) and the same free room before and after the refused write. Every block of the test disk holds 0xAB before formatting, so the test no longer depends on placement. Undoing the drop-back fails the room check. cargo test -p fileserver 91 passed; --ci host exit 0, 105 green (evidence https://github.com/ToyOSOrg/evidence/tree/6e7ef4652914fca6e3544d8743aa3d86c6f323c4, r1/). The reviewer's unrelated finding is recorded: issues/a-zero-length-write-past-the-end-extends-a-data-file.md.
• New host test data::tests::a_write_refused_after_allocating_leaves_no_old_bytes_past_the_end.
  • Round 0: a 512-page file of 0xAB is written, synced and deleted. A new file then writes half of its page 0, whose read the disk refuses, and page 1, which is allocated first in a block the deleted file left. Round 1 replaced the deleted file with a disk whose every block holds 0xAB.
  • The write is refused. The file is extended to two pages, and page 1 must read zeros.
• Control: the same test before the fix (before-fix.log), exit 101: "page 1 reads 0xab, not zeros".
• cargo test -p fileserver --release: 91 passed. cargo run -- --ci host at 41efb19: exit 0, 105 green.
• No integration test: the path is a refused disk read or write, which the host's test disk drives directly.

🤖 Generated with Claude Code

[Japabu]

Review of #1120 at 41efb19 (base disk-path-alloc af6d088)

Architecture

The fix belongs in DataVolume::write. That function owns both the extents grown by Mounted::allocate and the cache writes that fill them, so the decision to roll back has to be made there. The change does not add a new mechanism. It routes copy failures through the rollback a refused allocation already used: keep_blocks(covered) then free_extents. This is the exit the deleted record specified. Production code grows by about 7 lines. The test adds about 40 lines, and the issue file removes 30.

Failure points traced

• Range overflow (u32::try_from): this refuses before allocate runs. keep_blocks returns nothing and the free is a no-op.
• Allocation refusals: these include alloc_first_fit running out of space mid-loop after earlier runs were pushed, a short run (the loop simply continues), a refused bitmap mark (set_range_used undoes its own bits, and reserve records nothing in fresh or the counters), and a refused hole-zeroing write in allocate. In each case the extents hold only appended runs past covered, and all of them are dropped.
• file_entry_fits refusal: handled as before.
• Copy-phase refusals (whole-page cache.write, the partial-page cache.read, the partial-page cache.write): these are the new coverage. Every page past covered returns to the allocator.
• Tree update: write performs none. persist and update_metadata run later inside atomic, which restores the root and frees what it took. An entry is never written naming blocks that were rolled back.

Rollback exactness

• allocate changes the extent list only through push_extent, which either appends or extends the last extent. Both are append-only in page order.
• keep_blocks(extents, covered) therefore restores exactly the old list. When the old last extent was merged into, it is split back at its original count.
• The dropped runs are exactly the blocks this call reserved. No block from before the call is dropped, so a double-free is impossible. That matters because set_free has no guard of its own.
• free_extents runs with no operation open, so it goes free_range → give. Every dropped block is in fresh, because nothing between reserve and the rollback calls seal(). They are freed at once, which is correct because no commit names them.
• If give is itself refused, the affected blocks leak: their bits stay set and the extents no longer name them. Nothing is exposed. This is the same "record first, free second" outcome as before.

Crash consistency

• The bitmap is written in place. A kill between reserve and the rollback leaves bits set that no committed tree names. That is the leak already recorded in a-crash-leaks-the-blocks-of-its-last-commit, not an exposure.
• After a rollback, the root and free_blocks are back to their earlier values. same_commit ignores next_alloc, so a following sync may legitimately skip the superblock write and still flush the restored bitmap.
• Data that already reached the cache for freed blocks is flushed to unnamed blocks. A later owner's write or zeroing of those blocks goes to the same cache slot as a newer version.

Other paths

• Extending truncate changes only size, and reads past the extents are zero.
• A shrinking truncate zeroes the tail of the last kept page before dropping the rest.
• Sparse writes are zeroed in allocate.
• A partial new page is zero-filled.
• The format's own write_data runs inside atomic, which frees what it took on refusal.

With the fix, the extents cover no more than ceil(size) pages on every path I traced. I found no other way to reach a stale block. A refused write can still leave its own bytes in a covered page beyond size. That predates this PR and only ever shows the same file's data.

Evidence

• fix.diff matches the commit's data.rs diff exactly.
• The control failed with exit 101, "page 1 reads 0xab". It fails at data.rs:1472, which is the head's assert line (1479) minus the 7 production lines the fix adds. That corroborates the control tree, although the log does not name its revision.
• cargo test -p fileserver --release: 91 passed.
• --ci host at 41efb19 (ci-host.rev): exit 0, 105 steps green.
• A host test is the right level for a refused disk read.

Findings

No blockers.

• SUGGESTION system/fileserver/src/data.rs:1473 — assert!(v.write(..).is_err()) also passes if the write is refused before it allocates, for example on a future limit check. In that case page 1 is never given a block and the test passes without exercising the fix. assert_eq!(.., Err(Error::Io)) would pin the path. Adding assert_eq!(v.room(), before) around the refused write would also check by test that exactly the new blocks come back, with no leak and no double-free. Today that rests on inspection.
• SUGGESTION system/fileserver/src/data.rs:1456-1458 — the 512-page size and its comment tie the test to SPREAD and the current placement. If placement changes, page 1 could land in a block that was never used and the test would pass without testing anything. Filling the volume with 0xAB until NoSpace, as fragmented() loops, then deleting it, would make every block stale. The test would then hold regardless of placement and the comment could go.
• Unrelated, pre-existing, recorded only: DataVolume::write with empty data still runs open.size = open.size.max(end). A zero-length write past EOF therefore extends the file, where POSIX says it has no effect. Nothing stale is exposed, because reads past the extents are zero.

LAND