Skip to content

blockring: the arena holds only what is on the wire, and a flush keeps many writes there - #1113

Merged
Japabu merged 1 commit into
mainfrom
disk-path-write
Nov 1, 2026
Merged

Japabu merged 1 commit into
mainfrom
disk-path-write

Conversation

@Japabu

@Japabu Japabu commented Oct 28, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

persist.seq_write_fsync, 8 MiB written in 1 MiB calls and then fsynced, read 316 MB/s on the AMD bench and 148 MB/s on the T14. Linux reads 506–520 and 527–598 MB/s on the same NVMe.

A measurement-only boot per bench attributed each 8 MiB round (probe/attribution.txt, medians of three rounds; AMD then T14):

part AMD T14
the server's WRITE handling 5.0 ms (allocation 2.4, cache copy 1.5) 9.8 ms (6.7, 2.3)
device writes 10.0 ms, 169 requests, one at a time 20.4 ms, 170 requests
device Flushes 6 × 0.95 ms, 4 of them forced by a full arena 6 × 3.58 ms, 4 forced

Copying into the cache is 1.5–2.3 ms, not the 18 ms first supposed. The cost is in two places:

  • Forced Flushes. An acknowledged write kept its arena blocks until a flush covered it, so it could be issued again after a loss. A full arena, 511 blocks, made Served ask the disk for a Flush before the next request.
  • Serial writes. The file server's cache wrote its runs one at a time.

Change

The contract is unchanged: an acknowledged write that no flush has covered is issued again after a loss, and Durable means on the medium. What changes is where a write's bytes wait.

  • toyos_blockring::client: the arena holds only what is on the wire.
    • A read or a write is given its arena blocks as it goes out (next_request's place) and gives them back when it is answered.
    • The client carries each write's payload (Ask::Write { payload }, generic: the session's shared blocks, a ticket in the model) and hands it to place each time the write goes out. A write answered anything but Done, covered by a flush, or given up drops its payload with it. Nothing outside the client keeps a write's bytes, so none can outlive it or be missing at a reissue (review r1).
    • What it keeps is bounded (review r1). Once acknowledged writes reach the limit the client was made with, the caller's next write waits while the client sends a flush of its own. If that flush fails past MAX_ATTEMPTS, writes meeting the limit are answered Device unsent until a caller's flush succeeds, as the arena path refused them. The session's limit, KEPT_BLOCKS, is 32 MiB, no less than the cache's own dirty bound, so a cache flushing by that bound never meets it; the cache asserts DIRTY_LIMIT <= KEPT_BLOCKS at compile time (review r2). A write at the limit with a caller's flush on the wire waits for that flush rather than asking one of the client's own (review r2); only a flush on the wire counts, since one queued behind the waiting write could never go out. The 511-block bound it replaces cost 4 Flushes per 8 MiB round (0.95 ms each on AMD, 3.58 ms on the T14). An update streaming ROOT keeps at most 32 MiB, where without the limit it kept the whole image.
    • Placing every transfer at send, not at submit, is what makes a full arena only a wait for an answer. Were a queued user write to hold arena blocks, it could hold what a reissue ahead of it needs, and nothing would answer.
    • The overlap, ordering, attempt and poison rules are untouched.
  • diskserver::Session hands the client the caller's blocks. A write's SharedBlocks (an Rc of a boxed block) are its payload, copied into the arena each time it goes out. The block is boxed so the cache can allocate a block it reads in fallibly (fileserver: a volume's cache keeps an eighth of physical memory clean #1099's boxed).
    • Unsent::ArenaFull is gone, and so is Served's flush-on-full path.
    • Served::write copies into shared blocks and Served::write_shared takes them as they are, the one write path.
    • Served::depth() is the full-size requests a session carries at once: min(ring depth 64, 511 / 32) = 15. The NVMe driver queues 63.
  • The file server's cache shares its blocks and keeps depth runs in flight.
    • Slots are SharedBlocks, so a flush hands the disk the cache's own blocks: one copy, into the arena, where there were two.
    • A write to a block the disk still keeps takes a block of its own.
    • The flush keeps up to Disk::depth runs in flight. Once one is refused no other is begun, and the blocks not written stay dirty.
    • A disk that keeps nothing (Disk::write_shared's default) is handed a copy, as before.
  • Swap. The swap design (issues/anonymous-memory-swaps-to-an-encrypted-partition.md) rested on acknowledged writes holding the arena. Its "free after durable" and arena-reserve bullets now say what the client keeps, so "free after durable" holds unchanged. Its 223-write flush threshold came from arena arithmetic that no longer applies, and is gone; the batch's own flush bounds what the service keeps. A swap-in read can now wait behind a write: requests go out in the order asked, so a read asked after a write waits while that write has no contiguous arena room or waits at the kept limit. The record says so and names the remedy, reads ahead of waiting writes, should swap-in latency need it.

Checks

Evidence: https://github.com/ToyOSOrg/evidence/tree/37b67d1013ce98de4fb496f93834162691676a41 (host, red controls, probe) and https://github.com/ToyOSOrg/evidence/tree/dc209f847cc2f2c1a919abf0b60bdaf3a040f455 (metal A/B, rebase, the next steps' host probes) and https://github.com/ToyOSOrg/evidence/tree/36653291e4c09bac905ef567c8d2fe2e193ae2a5 (review r1: r1/)

  • On main 24a538c, head 4ea1637, one commit (it squashes at merge; the rounds below are in its message). cargo run -- --ci host exit 0, 107 green. diskserver, blockring with the model, fileserver and blockdev tests all pass (evidence https://github.com/ToyOSOrg/evidence/tree/3dc5fbae756ffd6300fcd7a8af9bc1fa6aea378b, onmain/).
    • The one conflict is system/diskserver/src/session.rs, against main's 3e0fd67 (a session's doorbell pipe became two events, bells.ring and bells.answered) and f83957d (the handshake's timed wait). Both edit the functions this change edits.
    • The merged file keeps main's events: pump rings bells.ring, wait waits on bells.answered, and the submits check bells. Around them it keeps this change's payload placement, reads table, KEPT_BLOCKS and Pending::Read.
    • The swap record merged automatically. Every other file is byte-identical to 20bdd14's.
    • Metal (X-E, passed on both benches): one clean boot per bench of 4ea1637, under a rule fixed before the run. T14 seq_write_fsync 181 MB/s and AMD 588, within 20bdd14's boots (175–233 and 524–657). seq_write.4096 reads 170 and 297. Every persist.* row is within or better than that spread, but T14 fsync_clean sits 0.05 µs past it (14.38 against 14.33), within resolution. Evidence https://github.com/ToyOSOrg/evidence/tree/24e490febee1909deecd9c975fecba8db4e82ba6 (onmain/).
  • Review r2, head 20bdd14: the KEPT_BLOCKS doc keeps only its reason, with the counts and timings in the commit; the compile-time bound check; the wait for a caller's flush on the wire. cargo run -- --ci host exit 0, 105 green, and client::tests::a_write_at_the_limit_waits_for_a_callers_flush_on_the_wire with its red control (evidence https://github.com/ToyOSOrg/evidence/tree/ff9a0fea3e4ceac3122f8eb7d5b2eb9ce2a30940, r2/).
  • Review r1, head 558ab77, stacked on fileserver: a volume's cache keeps an eighth of physical memory clean #1099 (fe6d7d2). cargo run -- --ci host exit 0, 105 green (r1/ci-host.log). The stack's conflict is in cache.rs: fileserver: a volume's cache keeps an eighth of physical memory clean #1099's fallible boxed now goes into the shared slot, and fileserver: a volume's cache keeps an eighth of physical memory clean #1099 r3's order reservation for a flushed block moves into write_run. cargo test for blockring (31), diskserver (15), fileserver (90) and blockdev (13): all pass. The first commit alone does not build its tests on fileserver: a volume's cache keeps an eighth of physical memory clean #1099, because fileserver: a volume's cache keeps an eighth of physical memory clean #1099's Cache::new takes a limit; the third commit fixes that.
  • Earlier, rebased onto 0c7e62c (bcachefs: a sync with nothing uncommitted writes no superblock #1098) without conflict, head 6e85f74. cargo run -- --ci host exit 0 there, 105 steps green (rebase/). Before the rebase, at a122cad: --ci host exit 0, 105 green (host/), and cargo run -- --build-only exit 0. The rebase touches none of blockring, the disk server or the cache. Under them it brings bcachefs: a sync with nothing uncommitted writes no superblock #1098's clean-fsync change (bcachefs, data.rs), and beside them TCP, Wi-Fi and osbench changes. So the A/B below compares this change's own effect at a122cad; 6e85f74 itself has not been booted.
  • New host tests:
    • client::tests::a_write_holds_arena_blocks_only_on_the_wire, a_request_without_arena_room_waits_for_it, and a_lost_flush_reissues_then_asks_again, now placing the reissue with its own payload.
    • Review r1, the disk server's write path at its limit: client::tests::writes_past_the_limit_wait_for_the_clients_own_flush (32 writes, no caller flush: the client never keeps more than its limit, and the caller hears only its writes) and a_failed_own_flush_refuses_writes_until_a_flush_succeeds. The limit lives in the client, so these test it there. Served's Script link stands in for the session and has no client.
    • cache::tests::a_flush_keeps_as_many_runs_in_flight_as_the_disk_takes, a_flush_shares_the_cache_s_blocks_with_the_disk, a_refused_run_begins_no_other_and_leaves_the_rest_dirty.
  • The model (lib/blockring/src/model.rs, every interleaving of client, server, device, resets, crashes and errors) runs the changed client, a write's payload being its ticket. That payload is what the device writes, so a reissue with the wrong bytes breaks the durable law. Review r1: it runs a limit of one block, so every script meets it, and holds a third law: no caller's write first goes out while the client keeps its limit. All laws hold. Red controls (r1/): removing the wait breaks the new law (red-k1), and dropping a payload at Done rather than at the covering flush breaks the durable law (red-k2). The CI's three mutations still fail it (red/red-mutate-*).
  • Red controls, one per new claim, each failing its named test (red/red-m*): the depth ignored, copies handed instead of shares, a run begun after a refusal, a write's run held until the flush.
  • Not host-tested: the session's reads by ticket and its arena copies. They map shared memory and run only in a guest; the metal A/B runs them. held is gone.

Metal under the limit (X-B2, passed on both benches). Base #1099 fe6d7d2 against this head 20bdd14 with the measurement-only probe. Evidence https://github.com/ToyOSOrg/evidence/tree/95b0e454d9fe5dd7d00ab7bc69638ca56631c85a (r3/).

AMD:

  • persist.seq_write_fsync 286 → 527 MB/s, fs.disk.seq_write.1048576 332 → 737, .4096 201 → 269. Every persist.* latency row is level or better.
  • Over 10% worse: fs.disk.rename 62.1 → 72.5 µs, though the r1 boot with the same probe read 61.8 and differs only by r2; persist.seq_read_cached 3,983 → 3,132 MB/s, inside earlier AMD boots' 2,911–3,315. Boot spread, not attributable.

T14:

  • persist.seq_write_fsync 149 → 193 MB/s, within this code's other T14 boots (175–208).
  • write_4k_fsync and append_4k_fsync read 1.1 ms in the base boot and about 6 ms in every boot of this code. That is the T14's per-boot durable-write mode (issues/toyos-durable-writes-on-the-t14s-nvme-run-six-times-slower-in-some-boots.md); a base boot read 6.5 ms in round 0.
  • Unexplained, not attributed: fs.disk.seq_write.1048576 (64 MiB in 1 MiB writes) reads 258–260 in four of five rounds and 401 in one. Every other T14 boot of this code reads 435–501.
    • The probe shows the same cache flushes as the r1 boot: 8,192 blocks in 257–259 runs.
    • r2 changes behaviour only at 32 MiB kept with a caller's flush on the wire, which that flush pattern does not produce.
    • The same boot is also 26% slower on fs.disk.mkdir_rmdir, which never reaches the cap.
    • X-D-t14, also carrying r2, read it low too (282). But the drop is not r2's (evidence https://github.com/ToyOSOrg/evidence/tree/5647017c4e8ae82bc7a7c065aa9caaccd4f831ac, r3/t14-boots.txt, every T14 boot that day):
      • X-C2, whose tree also contains r2, read 490.
      • The low boots are the two that ran after 23:00, and mkdir_rmdir moved with them, 86.6 → 108–110 µs, on a path r2 cannot reach.
      • One unchanged tree (sched-place's base image) read 342 and 353, then 187 and 184, in consecutive boots.
      • So the row is per-boot bimodal on the T14, outside this change.

Metal A/B, round 0 (metal/): base f407918 against a122cad, one benchcase boot each per bench, plus a second AMD base boot from #1099's landing control. All passed. p50s:

row T14 base → head AMD base (2 boots) → head
persist.seq_write_fsync (MB/s) 154 → 207 313, 301 → 551
fs.disk.seq_write.1048576 (MB/s) 198 → 435 389, 345 → 762
fs.disk.seq_write.4096 (MB/s) 146 → 176 242, 220 → 303
  • Linux on the same NVMe read 506–520 MB/s (AMD) and 527–598 (T14) for seq_write_fsync in earlier boots; these boots ran no Linux. AMD is now at or above that; the T14 is at about a third of it.
  • No regression attributable to this change. On AMD every persist.* and fs.disk.* row is level or better. On the T14, create_write_unlink (126 → 160 µs) and open_close (22.3 → 26.2 µs) read high, but inside the range four boots of one tree show (123–159 and 21.9–26.9; t14-metadata-spread.txt). The T14's metadata rows are bimodal per boot. rename 96.6 µs is 7% above that range, on one boot.
  • Outside the disk rows, measured against every earlier boot of nearby trees, not just this base:
    • On the T14, the head boot's fs.mem.open_close, fs.mem.stat, pingpong.futex.same_cpu and proc.spawn_wait.large lie within those boots' range or at most 2% past it.
    • On AMD, pipe.throughput.4096 reads 1,904 MB/s against 2,061–2,110 in six boots, 8% low on one boot. This change touches no pipe.
  • AMD fs.disk.seq_read.4096 rose 165 and 152 → 447 MB/s, unexplained, and not claimed. The 64 MiB file sits exactly at the 16,384-block clean limit, so the row is on a knife edge. The host model of it reads the whole file from disk every round both before and after this change (plan/probe-*.log). fileserver: a volume's cache keeps an eighth of physical memory clean #1099 removes the edge.

Next, as separate changes

🤖 Generated with Claude Code

@Japabu

Japabu commented Oct 31, 2026

Copy link
Copy Markdown
Collaborator Author

Review of #1113 at 6e85f74 (round 1)

Architecture

The direction is right. Placing transfers at send rather than at submit puts the arena where it belongs: it is a staging area for the wire, not a store for writes the device has not yet made durable. I traced the deadlock argument and it holds. Reissues sit at the front of the outbox, and nothing else is sent while they are outstanding. The only arena runs the session holds are runs of requests on the wire (complete and session_ended release every placed run). So when nothing is on the wire, the whole arena is free, and any request of at most 32 blocks fits. A request with no room waits only for an answer that will come. pump runs again after every collect (session.rs:288, reconnect), so the wait cannot be lost. Moving the retained bytes into Rcs that the caller owns also eliminates one copy. The bytes cannot change: Rc::get_mut fails while the session holds a reference, so the cache's copy-on-write is enforced by the type.

Contract check against the client. I went through every complete/session_ended arm:

  • A User write answered Done either joins acked or is requeued under its own ticket.
  • A write answered anything else is never sent again, and the session drops its blocks.
  • Retirement happens in exactly two places, the covering-flush pop and the give-up.
  • A covering flush can only retire entries in acked. A flush is sent only once !reissuing(), and any later reissue bumps losses, which sends the flush down the "asked again" arm instead.

So no ticket is retired while it is still queued for reissue, and Durable still rests on the same rules. The model still checks reissued contents: a block is the ticket, so a reissue under the wrong ticket breaks Law::Durable.

The bounds hold:

  • depth() = min(64, 511/32) = 15.
  • 15 × 32 = 480 ≤ 511 arena blocks, leaving 31 for concurrent reads.
  • 15 entries < ring depth 64 < NVMe queue 63+1. Concurrent requests from other tasks are still capped at 64 by D.

The design misses one thing. The deleted flush-on-full was also the only bound on what a session retains, and nothing replaces it for callers other than the cache.

Size: about +300 lines of production code (client +~100, session +~40, cache +~90, Served and blockdev small) and about +200 lines of tests; one obsolete Served test removed with its behaviour.

Findings

BLOCKER 1 — system/diskserver/src/disk.rs:180,216 (with session.rs:197) — a session's retention is now unbounded.

  • Before: an acknowledged write held arena blocks. At 511 blocks, ask_room forced a Flush, and a refused Flush refused the write. That capped retained data at about 2 MiB per session and gave backpressure.
  • Now: held keeps every acknowledged write's blocks until some flush covers it, and nothing in Served or Session bounds it. The cache bounds itself, because every cache flush ends in disk.flush() and DIRTY_LIMIT triggers one. The other callers do not:
    • system/update/src/install/mod.rs:155 (stream_root) writes the whole of ROOT through Served::write and flushes only at line 116, after the boot volume.
    • Each 32-block write is now copied into fresh Rc<Box<Block>>s and kept, so the update process holds all of ROOT on its heap during an install. Today that is about 66 MiB of files, from the root: adding lines in host/build.log of the evidence. Before, it was at most about 2 MiB. It grows with the image.
  • A second case: a device that accepts writes but keeps failing flushes. After MAX_ATTEMPTS the flush is answered Device, but its writes stay in acked, and their blocks stay in held. Every later cache round adds more. The old path refused the write instead.
  • Required: restore a bound where the retention lives, so no caller can forget it. For example, Served asks a Flush before a write once the session keeps more than a limit set above what the cache already bounds, and refuses the write when that flush fails. Alternatively, give every caller its own bound (update flushing every N MiB) and document the obligation on Served::write/write_shared. Whichever is chosen, it needs a Served host test with the Script link: what is kept stays below the limit across a long run of writes with no flush.

BLOCKER 2 — lib/blockring/src/model.rs:376 — the model drains take_retired unchecked, so the new retention contract is not covered.

  • The PR says the model "runs the changed client" and its laws hold. That is true, but no law reads retirement.
  • The client's new obligation to the caller is the subtle part of this change:
    • every write placed is one whose bytes are still kept;
    • a write retired, or answered anything but Done, is never placed again;
    • by the end, every Done write has been retired.
  • That obligation holds only through the losses/reissuing() gating, on loss paths that no metal boot exercised: the A/B boots took no resets or crashes, so "every boot passed" says nothing here.
  • A violation panics the file server (session.rs:237, "a write going out whose blocks are gone") on exactly the rare interleavings the model exists to explore. The opposite violation leaks silently.
  • Cheapest sufficient check: mirror the session's held in World as a ticket set. Insert on Step::Write's submit, remove on a non-Done answer and on take_retired. In pump's place, fail a law if a write ticket is not in the set. When the run ends quiet, fail a law if the set is non-empty. Add a red control: for example, retire at Done instead of at the covering flush, which the new law catches.
  • That also covers the guest-only held bookkeeping well enough that no separate session host test is needed: it is three map operations, and the rest is shared-memory copying.
  • Optional in the same change: a bounded place in the model, say two blocks with real release from take_released, would let it explore the new "wait for room" path under reissue. Today only a_request_without_arena_room_waits_for_it covers that path.
  • A simpler alternative that removes the bookkeeping altogether: let Client carry the caller's payload in Ask::Write/Acked (generic, a ticket in the model) and hand it to place. Then held, take_retired and the expect disappear, and the existing laws cover the bytes directly. The implementer's choice.

BLOCKER 3 — lib/blockdev/src/lib.rs:37-39 — a false comment and the representation it justifies.

  • "The block is boxed on its own so its owner can allocate it fallibly": no owner does. Every construction is an infallible Rc::new(Box::new(..)) (cache.rs, disk.rs:217). The Rc's own allocation would stay infallible either way.
  • The double indirection costs a second heap allocation per block on every write and fetch.
  • Use Rc<Block>, or point to a real fallible allocation site. Either way, delete the sentence.

BLOCKER 4 — lib/blockring/src/client.rs:10-44 — the module header now says the same thing twice and lost its wrapping.

  • "An acknowledged write is kept until a flush covers it, by its ticket: the caller keeps its bytes until take_retired names it" (15–16) and the new final paragraph "A write's bytes are the caller's to keep … until take_retired names its ticket" (41–44) are one contract. Keep one: the final paragraph's wording, which includes the non-Done case, folded into the earlier one.
  • Rewrap line 19.
  • The same overlong line is at system/fileserver/src/cache.rs:7.

SUGGESTION

  • client.rs:71: use crate::layout::DEPTH; is stranded after the new type definitions. Move it to the imports.
  • client.rs:62-70: Placing { write: bool } has a doc comment ("a write's are filled with the bytes the caller keeps for ticket, the write's own, when it is the caller's or issued again") that is hard to parse. Consider carrying the transfer's kind as an enum, or the Ask itself, with a one-line doc.
  • cache.rs: self.disk.depth().max(1) guards a zero that no Disk returns. depth's contract could simply say "at least 1".
  • issues/anonymous-memory-swaps-to-an-encrypted-partition.md:
    • The "223 acknowledged writes" threshold came from the arena arithmetic, which no longer applies. It is now a memory bound with no stated reason. Either give the reason or leave the number to the swap implementer.
    • With placement at send in one FIFO outbox, a swap-in read queued behind a write that has no contiguous room waits for that write. The 64-block reserve bounds the count, not that wait. Record this if swap-in latency matters to the design.

Evidence

  • Host: --ci host passed (exit 0) at the head after the rebase, and before it at a122cad.
  • Red controls: the four new ones each fail their named test, and the three CI mutations still fail the model.
  • The rebase brings bcachefs: a sync with nothing uncommitted writes no superblock #1098's fsync change in under the measured path, and 6e85f74 has not been booted. The PR says both openly, and nothing in the rebase touches this change's code, so reusing the a122cad A/B for the performance claim is acceptable.
  • The metal gains, 154→207 / 313→551 MB/s and 198→435 / 389→762 MB/s, are well outside per-boot spread. The regression attribution (t14-metadata-spread.txt, the pipe row) is reasoned and not overstated.
  • The A/B is not evidence for loss paths or for the new retention; see Blockers 1 and 2.

SEND BACK

Japabu added a commit that referenced this pull request Oct 31, 2026
This change is now stacked on #1099, whose cache allocates a block it reads
in fallibly (`boxed`). Shared, that block goes into an `Rc` as its own box,
which an `Rc` of the block itself could not do on stable Rust. So
`SharedBlock` keeps its box, and its comment names that site instead of
claiming an owner none had.

The clean-blocks record's list of allocations with no refusal path follows
the flush as it now is: its runs, the writes it keeps in flight, and each
run's shares, plus a block's share count. A flushed block whose place in
the eviction order is refused is dropped in `write_run`, as #1099's r3 did
in the loop it replaces. #1113's cache tests pass the clean limit #1099's
constructor takes.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Japabu

Japabu commented Oct 31, 2026

Copy link
Copy Markdown
Collaborator Author

Review of #1113 at 558ab77 (round 2, on #1099 at fe6d7d2)

The r1 blockers

  • B1, unbounded retention: resolved.
    • The cap now lives in the client, which is where the payload is kept.
    • update's stream_root is now bounded at KEPT_BLOCKS plus its one write on the wire, about 32 MiB. Unbounded, it would have held all of ROOT, about 66 MiB today.
    • A device that accepts writes but keeps failing flushes now ends in Pushback::Failed, which refuses writes at the limit with Device. That is the backpressure the arena path used to give.
  • B2, the model did not check retention: resolved, by the simpler design.
    • The payload travels with Ask::Write/Acked. The session's held map, take_retired and the expect are gone.
    • The model's payload is the ticket, so Law::Durable now checks reissued bytes and how long they are kept. The red-k2-dropped-at-done control shows it: dropping the payload at Done fails what_a_flush_calls_durable_is_on_the_medium under a reset.
    • The model runs LIMIT = 1 across every bound, so every script with a second write explores the client's own flush (Pushback) under resets, crashes and errors.
    • Law::Answers ("every ticket answered by the end") also rules out a write left waiting at the cap with no flush pending.
  • B3, the SharedBlock comment: resolved. On fileserver: a volume's cache keeps an eighth of physical memory clean #1099 the box is the fallible allocation (cache.rs boxed, try_reserve_exact), which Rc::new(block) cannot be.
  • B4, the client header and wrapping: resolved. The payload contract is stated once, and the cache.rs header is rewrapped.

The cap's semantics

I went through every Pushback transition:

  • Idle → Asked(0) queues a Kind::Pushback. queue_flush places it after any reissues and flushes and before the waiting write, and continue then sends it.
  • Ok, with no loss since it was sent: pops the covered acked entries, decrements kept and returns to Idle.
  • Ok after a loss: asked again.
  • Failure: lost(), then Asked(n+1) or Failed.
  • Session end: it is requeued.
  • A caller's flush that succeeds also returns the state to Idle.

Whenever the state is Asked, a pushback is queued or on the wire, so a write waiting at the cap is never stranded. A stale pushback answered after Idle is harmless: on success it only covers, and on failure it is a loss as any flush failure is.

The accounting is consistent:

  • kept rises only when a caller's write is acknowledged.
  • It falls only at a covering flush (the caller's or the client's own) or at a give-up.
  • A reissue or a loss moves an entry but leaves the count unchanged.

Durable is unaffected. A pushback covers only what a device flush made durable after every loss it could have seen, which is the same rule as the caller's flush. The overshoot is bounded by what was on the wire when the cap was met, at most D × 32 blocks, and writes_past_the_limit_wait_for_the_clients_own_flush holds kept ≤ limit for sequential writes.

The Failed path needs more than MAX_ATTEMPTS failures of a pushback, which no model bound reaches. a_failed_own_flush_refuses_writes_until_a_flush_succeeds covers it, and that is enough.

Is 32 MiB justified?

Yes. It is the smallest cap that adds no flush to the file server.

  • The cache's flush writes at most DIRTY_LIMIT (8192 blocks) and ends with disk.flush(), which covers them.
  • When a round's last run goes out, kept is below 8192 minus that run, so the cache never meets the cap in steady state.
  • A smaller cap would bring back the forced Flushes this PR removes.
  • For update, it is a bound on a one-shot process. The 2 MiB arena bound was an accident of the arena's size, not a memory policy.

Findings

BLOCKER — system/diskserver/src/session.rs:35-41 — the KEPT_BLOCKS doc comment carries history and measurements.

SUGGESTION

  • That constraint now spans two crates, and nothing ties them together: if one constant changes, forced flushes come back silently. A const _: () = assert!(DIRTY_LIMIT as u64 <= diskserver::session::KEPT_BLOCKS); in cache.rs (the fileserver already depends on diskserver) states it once.
  • client.rs:280-290: with a caller's flush queued or on the wire, a write at the cap still queues a pushback behind it. That is a redundant device flush in a rare case. It could wait for the pending caller's flush instead, but this is optional.

Evidence

SEND BACK — for the comment only. With it fixed and the queued metal run green, this lands.

Japabu added a commit that referenced this pull request Oct 31, 2026
… to it

`KEPT_BLOCKS`'s doc keeps only its reason: no less than the file server
cache's dirty bound, so a cache flushing by that bound is never made to
flush by the session's. The counts and timings behind the choice are
#1113's: the 511-block arena bound forced 4 Flushes per 8 MiB of
persist.seq_write_fsync, 0.95 ms each on the AMD bench and 3.58 ms on the
T14. The cache now asserts `DIRTY_LIMIT <= KEPT_BLOCKS` at compile time, so
the two crates' constants cannot drift apart silently.

A write at the limit with a caller's flush on the wire waits for that flush
rather than asking one of the client's own beside it. Only a flush on the
wire counts: one queued behind the waiting write could never go out.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Japabu

Japabu commented Oct 31, 2026

Copy link
Copy Markdown
Collaborator Author

Review of #1113 at 20bdd14 (round 3)

The r2 items

  • Blocker, the KEPT_BLOCKS doc: resolved. It now gives only the reason ("no less than a file server cache's dirty bound…"). The history is in the commit message.
  • Suggestion, the drift guard: taken. cache.rs asserts DIRTY_LIMIT as u64 <= diskserver::session::KEPT_BLOCKS at compile time.
  • Suggestion, waiting on a caller's flush: taken. Its deadlock reasoning is checked below.

The new wait rule (client.rs:287-290)

When the client's own flush is Idle, kept >= limit, the front of the queue is a caller's write, and a Kind::Flush is on the wire, next_request returns None. A flush on the wire is always answered by complete or ended by session_ended, so the write waits only for an event that will come. Every outcome lets it move:

  • Ok, with no loss since it was sent. The flush covers what it can and kept falls. If writes acknowledged after it was sent still leave kept >= limit, the next call finds no flush on the wire and asks the client's own flush, as before.
  • Ok after a loss, or a failure within MAX_ATTEMPTS. queue_flush puts it back after any reissues and before every caller item. That is ahead of the waiting write, so it goes out and the write waits on it again. Reissues come first, under reissuing().
  • A failure past MAX_ATTEMPTS. The flush is answered Device and leaves the wire. With no flush there, the client asks its own.
  • Session end. The flush is requeued ahead of the write, and goes out once the session reconnects.

Counting only flushes on the wire is required. A caller's flush still queued sits behind the waiting write, because the outbox is in order, and could never go out. A flush the client puts back itself always goes ahead of the write, and is sent before the write is looked at again. So no state exists where the write waits on a flush that cannot be sent.

The model runs this with LIMIT = 1. SCRIPT's W3 is asked while F2 can be on the wire, and the next flush is asked after W3. So the unsafe variant, counting queued flushes too, would leave W3 and F4 unanswered and break Law::Answers. That is an inference: no control demonstrates it, and none is needed for the rule as written. red-k3-own-flush-beside-callers fails the new unit test when the guard is removed.

In Pushback::Failed, a write at the limit is still refused even while a caller's flush is on the wire. That matches the documented contract ("until a caller's flush succeeds").

The branch base

The pre-squash commits do not block. #1099 landed as a merge commit, 49559dd, whose second parent is fe6d7d2. The branch's base is therefore an ancestor of main.

  • git log origin/main..20bdd1401 shows only this PR's four commits.
  • git merge-tree origin/main 20bdd1401 merges cleanly.
  • The three-dot diff is this change alone: 9 files, +888/−380.

Evidence

  • --ci host at 20bdd14: exit 0, 105 steps green (r2/ci-host.rev).
  • The r2 control fails on the stated revision.
  • No build has yet booted the restructured client and session. The queued metal boot (X-B2) remains a condition for landing, which is the orchestrator's to verify.

No new findings.

LAND — once X-B2 passes.

Japabu added a commit that referenced this pull request Oct 31, 2026
…ds one

This deliberately departs from page-at-a-time placement, which r1 kept. The
probe boots of #1113's head show what that placement does to
persist.seq_write_fsync on metal. Each round's 8 MiB file lay in 103-106
runs, about 100 of them single blocks. Those are holes behind the
allocator's cursor, where a commit's freed nodes are the likely source. So
each round's flush took 163-169 requests, 100-105 of them single blocks.
Round 0's whole-remainder ask instead gave 8 runs of 256 blocks and 64 full
requests, and read 236 MB/s against 175 on the T14 (640 against 524 on the
AMD bench), but it could scan the whole bitmap for each short run.

`allocate` now asks each run to be at least `MIN_RUN` long, 32 blocks, the
most one block-ring request carries, or the pages still missing if fewer.
`alloc_first_fit` takes the first free run that long from the file's end,
remembering the longest it saw. If one pass finds none it returns that
longest, and the rest of the call takes runs as they come. A call is
therefore at most two passes over the bitmap.

Two host tests: single holes at the cursor before a long free run give a
64-page write one extent in the long run, where a page at a time takes the
first hole; and on a volume of eight bitmap blocks with only single holes,
a 16-page write reads the bitmap 55 times, within two passes and three
reads a run (64). Without the minimum the first fails; without the fallback
the second reads 167 times. The integration test that held page-at-a-time
placement goes, as that placement is no longer the contract.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…s many writes there

An acknowledged write kept its arena blocks until a flush covered it, so
it could be issued again after a loss. A session's arena is 511 blocks,
and a full one made Served ask the disk for a Flush before the next
request. On the AMD bench (probe at 21265a0) an 8 MiB fsync paid four
such Flushes, about 0.95 ms each, besides its own two; on the T14, about
3.5 ms each. And the file server's cache wrote its runs one at a time.

The contract is unchanged: a write acknowledged and not covered is issued
again after a loss, and Durable means on the medium. Only where its bytes
wait changes. The client now gives a read or a write its arena blocks as
it goes on the wire (`next_request`'s `place`) and takes them back when it
is answered, and keeps an acknowledged write by its ticket; the session
keeps the caller's blocks for that ticket until the client retires it,
copying them into the arena each time the write goes out. A full arena
now only means requests on the wire hold it, and a request waits for one
of their answers. Placing every transfer as it goes out, not at submit,
is what keeps that wait from deadlocking: a write queued behind a reissue
holds nothing the reissue needs.

The blocks are shared, not copied: SharedBlock is an Rc of a boxed block,
and the cache's slots are SharedBlocks, so a flush hands the disk the
cache's own blocks (one copy, into the arena, where there were two). A
write to a block the disk still keeps takes a block of its own. The flush
keeps up to Disk::depth runs in flight, which a served disk answers as the
full-size requests its session carries at once: min(DEPTH 64, 511 / 32) =
15, under the NVMe driver's 63 per queue.

The swap track's design rested on acknowledged writes holding the arena;
its two bullets now say what the session keeps instead.

Review r1: the client carries each write's payload and bounds what it
keeps

Two blockers from review: a session's retention had no bound once the
arena's was gone, and the model did not check what the session kept for a
write.

The client now carries the caller's payload with each write (`Ask::Write`'s
`payload`, generic) and hands it to `place` each time the write goes out.
The session's `held` map, `take_retired` and the `expect` for a write whose
blocks were gone are deleted: a covered or given-up write drops its payload
with it. No copy comes back; the session's payload is the cache's shared
blocks. `Placing` is an enum of a read's ticket and a write's payload.

What a client keeps is bounded where it is kept. Once acknowledged writes
reach the limit it was made with, the caller's next write waits while the
client sends a flush of its own. If that flush fails past MAX_ATTEMPTS,
writes meeting the limit are answered Device unsent until a caller's flush
succeeds, as the arena path refused them. The session's limit is 32 MiB,
the file server cache's own dirty bound. The 511-block bound it replaces
cost 4 Flushes per 8 MiB round on metal.

The model runs a limit of one block, its write's payload being its ticket,
and holds a new law: no caller's write first goes out while the client
keeps its limit. Removing the wait breaks it. Dropping a payload at Done
rather than at the covering flush breaks the durable law.

Also: the module header states the payload contract once; cache.rs's header
is rewrapped; `Disk::depth` says it is at least one and the `max(1)` goes;
the swap record no longer derives a flush threshold from the arena, and
says a swap-in read can wait behind a write.

Review r1, stacked on #1099:
This change is now stacked on #1099, whose cache allocates a block it reads
in fallibly (`boxed`). Shared, that block goes into an `Rc` as its own box,
which an `Rc` of the block itself could not do on stable Rust. So
`SharedBlock` keeps its box, and its comment names that site instead of
claiming an owner none had.

The clean-blocks record's list of allocations with no refusal path follows
the flush as it now is: its runs, the writes it keeps in flight, and each
run's shares, plus a block's share count. A flushed block whose place in
the eviction order is refused is dropped in `write_run`, as #1099's r3 did
in the loop it replaces. #1113's cache tests pass the clean limit #1099's
constructor takes.

Review r2:
`KEPT_BLOCKS`'s doc keeps only its reason: no less than the file server
cache's dirty bound, so a cache flushing by that bound is never made to
flush by the session's. The counts and timings behind the choice are
#1113's: the 511-block arena bound forced 4 Flushes per 8 MiB of
persist.seq_write_fsync, 0.95 ms each on the AMD bench and 3.58 ms on the
T14. The cache now asserts `DIRTY_LIMIT <= KEPT_BLOCKS` at compile time, so
the two crates' constants cannot drift apart silently.

A write at the limit with a caller's flush on the wire waits for that flush
rather than asking one of the client's own beside it. Only a flush on the
wire counts: one queued behind the waiting write could never go out.

On main after #1099 and the event doorbells (3e0fd67, f83957d), as one
change: those turned the session's doorbell pipe into two events in the
same functions this change edits (`pump`, `wait`, the submits and the
session's fields). The session keeps main's `bells` and their `ring` and
`answered` waits, and this change's payload placement and reads table
around them.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Japabu added a commit that referenced this pull request Nov 1, 2026
…ds one

This deliberately departs from page-at-a-time placement, which r1 kept. The
probe boots of #1113's head show what that placement does to
persist.seq_write_fsync on metal. Each round's 8 MiB file lay in 103-106
runs, about 100 of them single blocks. Those are holes behind the
allocator's cursor, where a commit's freed nodes are the likely source. So
each round's flush took 163-169 requests, 100-105 of them single blocks.
Round 0's whole-remainder ask instead gave 8 runs of 256 blocks and 64 full
requests, and read 236 MB/s against 175 on the T14 (640 against 524 on the
AMD bench), but it could scan the whole bitmap for each short run.

`allocate` now asks each run to be at least `MIN_RUN` long, 32 blocks, the
most one block-ring request carries, or the pages still missing if fewer.
`alloc_first_fit` takes the first free run that long from the file's end,
remembering the longest it saw. If one pass finds none it returns that
longest, and the rest of the call takes runs as they come. A call is
therefore at most two passes over the bitmap.

Two host tests: single holes at the cursor before a long free run give a
64-page write one extent in the long run, where a page at a time takes the
first hole; and on a volume of eight bitmap blocks with only single holes,
a 16-page write reads the bitmap 55 times, within two passes and three
reads a run (64). Without the minimum the first fails; without the fallback
the second reads 167 times. The integration test that held page-at-a-time
placement goes, as that placement is no longer the contract.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Japabu
Japabu marked this pull request as ready for review November 1, 2026 02:04
@Japabu
Japabu added this pull request to the merge queue Nov 1, 2026
Merged via the queue into main with commit f20ab03 Nov 1, 2026
10 checks passed
@Japabu
Japabu deleted the disk-path-write branch November 1, 2026 02:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant