diff --git a/Cargo.lock b/Cargo.lock index 126652cbce..3285301c15 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1197,6 +1197,7 @@ dependencies = [ "toyos-abi", "toyos-blockring", "toyos-fat32", + "toyos-fat32-check", "toyos-gpt", "toyos-manifest", ] diff --git a/issues/a-block-session-waits-forever-on-a-server-past-its-submission-ring.md b/issues/a-block-session-waits-forever-on-a-server-past-its-submission-ring.md new file mode 100644 index 0000000000..d2153d672e --- /dev/null +++ b/issues/a-block-session-waits-forever-on-a-server-past-its-submission-ring.md @@ -0,0 +1,24 @@ +--- +status: open +kind: defect +opened: 2026-10-13 +--- + +# A block session waits forever on a server past its submission ring + +A diskserver client trusts its server's cursor on the submission ring. Where +the server's consumer index is past what the client published, +`Session::pump` (`system/diskserver/src/session.rs`) meets the transport's +`Violation::HeadPastTail` from `space` and only stops pushing, on the comment +that "the next read of the connection is where that is acted on". Nothing +acts on it: `drain` reads only the completion ring, so with no completion to +refuse `wait` blocks in `poller.wait(1, u64::MAX, ..)` until the server hangs +up. A server that keeps its connection and writes that index holds every +caller of the session for good: an unbounded wait on an untrusted peer, where +the producer's violation should end the session as `drain`'s do. + +**Exit**: a submission-ring violation ends the session, so the caller's call +is refused; held by a host test of the client's ring half over a page whose +server cursor is past what the client published, red with that end removed. + +Owner: the orchestrator. diff --git a/issues/nothing-reopens-a-diskserver-session-over-a-used-page.md b/issues/nothing-reopens-a-diskserver-session-over-a-used-page.md index c03cc56c4e..897ff1d5c0 100644 --- a/issues/nothing-reopens-a-diskserver-session-over-a-used-page.md +++ b/issues/nothing-reopens-a-diskserver-session-over-a-used-page.md @@ -4,33 +4,27 @@ kind: tooling opened: 2026-10-02 --- -# Nothing reopens a blockd session over a used page +# Nothing reopens a diskserver session over a used page A nightly went red on `blockd_io: FAIL /AFTER.BIN after the restart: Io` (run 36753172688, guest 7). blockd answered an open and only then made its ends of the session page. A client that reconnects sends the page its last server -wrote, and one that looks before the new server's ends are made has no room to -ask (`Violation::HeadPastTail`) and hears the dead session's completion -(`Violation::Tag`). The order is a type now: `wire::Opened::over` makes a -server's ends and its answer together. +wrote, and one that looks before the new server's ends are made hears the dead +session's completion. The server's half is a type now: `wire::Opened::over` +makes a server's ends and its answer together, and diskserver sends the answer +it returns. -That was fixed by reading. Nothing reproduced the `Io` before it, and nothing -that runs reopens a session over a used page: +The client's half is read off the code and not run. `Session::pump`, `drain` +and `wait` (`system/diskserver/src/session.rs`) hold a `Region`, a +`Connection` and a `Poller`, so no host reaches them. A page whose completion +ring holds the dead session's answer ends the session only through `drain`'s +refusal of a completion for no ticket in flight (`Violation::Tag`, through +`client.complete`); `disk::Served` then answers its caller `Gone`, which a file +server's caller hears as `Io`. That nothing else ends it is +`issues/a-block-session-waits-forever-on-a-server-past-its-submission-ring.md`. -- blockd's host test reaches `Service::place` and `Service::listing`, never - `Service::open`; -- `toyos-blockring`'s model test holds what the transport answers over a page - a crashed session left, and is green with blockd's order reverted; -- no host runs the client's glue (`Session::pump`, `drain` and `wait` in - `userland/blockd/src/session.rs`), so that `HeadPastTail` and `Tag` end the - session and the caller's call is `Io` is read off it, not run; -- `520c0d129` cut `blockd_survives_its_death`, the guest test that killed - blockd under its client, and no guest test or metal row ends blockd. +**Exit**: a host test reopens a client's session over a page a killed server +left its answer on, and the session ends and the caller's call is `Io`, by +`drain`'s `Tag` refusal: red with that refusal removed, and green with it. -**Exit**: the restart test that Stage D of -`issues/the-guest-suite-runs-only-what-no-cheaper-tier-reaches.md` owes -for `blockd_survives_its_death`, at the tier that stage builds it on: red with -that `Io` under a mutation that moves blockd's two stores after its answer, -and green without it. - -Owner: the orchestrator, which dispatches Stage D. +Owner: the orchestrator. diff --git a/issues/the-kernel-is-small-interrupts-post-and-threads-wait.md b/issues/the-kernel-is-small-interrupts-post-and-threads-wait.md index 764d97701c..524b799505 100644 --- a/issues/the-kernel-is-small-interrupts-post-and-threads-wait.md +++ b/issues/the-kernel-is-small-interrupts-post-and-threads-wait.md @@ -67,11 +67,10 @@ Beside that order, each waiting on something outside it: `every_request_is_answered_exactly_once`, `what_a_flush_calls_durable_is_on_the_medium` and `what_was_on_the_wire_at_the_end_is_refused_and_what_was_not_waits` for - the transport, by the restart test - `issues/nothing-reopens-a-diskserver-session-over-a-used-page.md` owes - for diskserver's part, a session reopened over the page a killed server - left, red under the mutation that moves the server's stores after its - answer, by fileserver's + the transport, for diskserver's part by `wire::Opened::over`, whose type + makes a server's ends before its answer, and by the client test + `issues/nothing-reopens-a-diskserver-session-over-a-used-page.md` owes, a + session reopened over the page a killed server left, by fileserver's `a_directory_rename_is_whole_under_one_name_wherever_the_server_is_killed` for DATA, and for LOG by the FAT test step 10 owes. **Partly done** (#525, #536): diskserver drives NVMe, and the kernel's NVMe driver is @@ -208,10 +207,10 @@ Beside that order, each waiting on something outside it: same controller delivering every report, red on a usbd that waits on the stick before it reads the keyboard's ring. `/log`'s, what a killed usbd had on the wire: stage 3's `toyos-blockring` tests for the - transport, and for usbd's session open the reopen test + transport, and for usbd's session open `wire::Opened::over`, moved with + the open, and the client test `issues/nothing-reopens-a-diskserver-session-over-a-used-page.md` - owes, red under the mutation that moves usbd's stores after its - answer; with one test this step owes in + owes; with one test this step owes in `system/fileserver/src/fat.rs`: a disk stopped after each block write in turn of an append and its sync mounts at every stop and reads back all the last whole sync held, red on a sync that writes a growing diff --git a/system/diskserver/src/main.rs b/system/diskserver/src/main.rs index 4978fc349f..a95d854a6b 100644 --- a/system/diskserver/src/main.rs +++ b/system/diskserver/src/main.rs @@ -308,8 +308,7 @@ struct Service { /// A session decided on and not yet told to its client. struct Opening { region: Region, - rings: ServerRings, - opened: Opened, + blocks: u64, device_addr: u64, first: u64, writes: bool, @@ -358,10 +357,7 @@ impl Service { let (first, blocks) = self.place(guid, grant)?; let writes = grant.expect("placed through a grant").writes; match self.ctrl.up().claim().dma_map(region.handle()) { - Ok(mapping) => { - let (rings, opened) = Opened::over(region.words(), blocks, guid); - Ok(Opening { region, rings, opened, device_addr: mapping.device_addr, first, writes }) - } + Ok(mapping) => Ok(Opening { region, blocks, device_addr: mapping.device_addr, first, writes }), Err(why) => { self.holds.release(first); Err(match why { @@ -372,8 +368,9 @@ impl Service { } } - /// The client heard its session is open: from here it is served. - fn admit(&mut self, opening: Opening, conn: Connection) -> u64 { + /// The client heard its session is open, over the ends `Opened::over` + /// made: from here it is served. + fn admit(&mut self, opening: Opening, rings: ServerRings, opened: Opened, conn: Connection) -> u64 { let id = self.next_id; self.next_id += 1; self.sessions.insert( @@ -382,9 +379,9 @@ impl Service { conn, region: opening.region, device_addr: opening.device_addr, - rings: opening.rings, - state: ServerSession::new(opening.first, opening.opened.blocks(), opening.writes), - unique: opening.opened.unique(), + rings, + state: ServerSession::new(opening.first, opened.blocks(), opening.writes), + unique: opened.unique(), closing: false, requests: 0, posted: false, @@ -772,12 +769,13 @@ fn handshake(service: &mut Service, acceptor: &toyos::port::Acceptor, p: Pending refuse(&p.conn, why); } Ok(opening) => { - let (opened, writes) = (opening.opened, opening.writes); + let (rings, opened) = Opened::over(opening.region.words(), opening.blocks, guid); if p.conn.try_send_bytes(wire::MSG_OPENED, &opened.encode()).is_err() { service.abandon(opening); return; } - let id = service.admit(opening, p.conn); + let writes = opening.writes; + let id = service.admit(opening, rings, opened, p.conn); let access = if writes { "" } else { ", read-only" }; println!("diskserver: session {id} opened {} ({} blocks{access})", guid_text(guid), opened.blocks()); } diff --git a/system/fileserver/Cargo.toml b/system/fileserver/Cargo.toml index 0f04c1d813..e5d5ad66fc 100644 --- a/system/fileserver/Cargo.toml +++ b/system/fileserver/Cargo.toml @@ -24,5 +24,10 @@ toyos-fat32 = { path = "../../lib/fat32" } bcachefs = { path = "../../lib/bcachefs" } diskserver = { path = "../diskserver" } +[dev-dependencies] +# The volumes the FAT role leaves, judged by the checker that shares no code +# with the driver it wrote them with. +toyos-fat32-check = { path = "../../lib/fat32/check" } + [package.metadata.toyos.host] exempt.owns = "ToyOS's storage roles, served from the partitions ToyOS claims for it" diff --git a/system/fileserver/src/fat.rs b/system/fileserver/src/fat.rs index 7d797d6923..42a8a39be8 100644 --- a/system/fileserver/src/fat.rs +++ b/system/fileserver/src/fat.rs @@ -5,7 +5,10 @@ //! Between them the volume holds clusters its entry does not reach, which //! fatgen103 refuses, so every open file is brought level — its entry written, //! its chain reconciled — at its last close, at an fsync and at a sync, as -//! the kernel's adapter did. +//! the kernel's adapter did. **The two are a cache flush apart**: the cache +//! writes lowest block first and the disk keeps no order until a flush, so an +//! entry put into the cache beside its chain and data would reach the disk +//! before the clusters it names. //! //! **A create makes the directories on its way**, as the kernel's did: a log //! file's path is its own, and FAT has the directories to carry it. A @@ -181,6 +184,11 @@ impl FatVolume { if open.gone { return Ok(()); } + // The chain and the data on the disk before the entry that reaches + // them is in the cache. + if open.file.needs_reconcile() { + self.cache.flush().map_err(|_| logged("the chain", &open.path, Error::Io))?; + } self.fs.flush_meta(&mut open.file, time).map_err(|e| logged("the entry", &open.path, e))?; if open.file.needs_reconcile() { self.fs.reconcile(&mut open.file, time).map_err(|e| logged("a reconcile", &open.path, e))?; @@ -724,6 +732,135 @@ mod tests { assert_eq!(v.sync(), Ok(Vec::new())); } + /// A disk whose bytes are the volume's, whole in memory. + struct Image(Vec); + + impl Disk for Image { + fn blocks(&self) -> u64 { + (self.0.len() / BLOCK) as u64 + } + fn read(&mut self, first: u64, out: &mut [u8]) -> Result<(), DiskError> { + let at = first as usize * BLOCK; + out.copy_from_slice(&self.0[at..at + out.len()]); + Ok(()) + } + fn write(&mut self, first: u64, data: &[u8]) -> Result<(), DiskError> { + let at = first as usize * BLOCK; + self.0[at..at + data.len()].copy_from_slice(data); + Ok(()) + } + fn flush(&mut self) -> Result<(), DiskError> { + Ok(()) + } + } + + /// The blocks a disk was written, in order, one epoch to each flush: the + /// disk keeps no order among one epoch's writes, and has all of an epoch + /// before any of the next. + type Epochs = Rc)>>>>; + + /// A disk that keeps every block it is written in its flush epoch, so a + /// test can stop it with any of one epoch's writes on it. + struct Traced { + image: Image, + epochs: Epochs, + } + + impl Disk for Traced { + fn blocks(&self) -> u64 { + self.image.blocks() + } + fn read(&mut self, first: u64, out: &mut [u8]) -> Result<(), DiskError> { + self.image.read(first, out) + } + fn write(&mut self, first: u64, data: &[u8]) -> Result<(), DiskError> { + let mut epochs = self.epochs.borrow_mut(); + let epoch = epochs.last_mut().expect("an epoch is always open"); + for (i, block) in data.chunks_exact(BLOCK).enumerate() { + epoch.push((first + i as u64, block.to_vec())); + } + self.image.write(first, data) + } + fn flush(&mut self) -> Result<(), DiskError> { + self.epochs.borrow_mut().push(Vec::new()); + self.image.flush() + } + } + + fn written<'a>(image: &mut [u8], writes: impl IntoIterator)>) { + for (block, data) in writes { + let at = *block as usize * BLOCK; + image[at..at + BLOCK].copy_from_slice(data); + } + } + + /// An append and its sync, stopped with every flush epoch before one on + /// the disk and any of that one's writes, leave a volume fatgen103's + /// checker refuses only for clusters no file's size reaches, and that mounts and + /// holds the file as the last whole sync left it or as the append made + /// it: never an entry naming bytes its chain does not reach or its + /// clusters do not hold yet. With every epoch on the disk the checker + /// refuses nothing and the file is the append's. + #[test] + fn an_append_stopped_with_any_writes_of_a_flush_on_the_disk_reads_back_the_last_sync() { + use toyos_fat32_check::{check, describe, Complaint}; + const CREATE: OpenHow = OpenHow { create: true, create_new: false, truncate: false }; + const OPEN: OpenHow = OpenHow { create: false, create_new: false, truncate: false }; + let clock = || 1_717_245_296 * NANOS_PER_SEC; + let spec = spec_volume::fixture(); + let mut image = spec.bytes.clone(); + image.resize(spec.bytes.len().div_ceil(BLOCK) * BLOCK, 0); + let epochs: Epochs = Rc::new(std::cell::RefCell::new(vec![Vec::new()])); + let mut v = FatVolume::mount(Traced { image: Image(image.clone()), epochs: Rc::clone(&epochs) }, true, clock) + .unwrap(); + let held: Vec = (0..3000u32).map(|i| (i % 251 + 1) as u8).collect(); + let appended: Vec = (0..3 * BLOCK as u32).map(|i| (i % 241 + 2) as u8).collect(); + let a = v.open("a.txt", CREATE).unwrap(); + v.write(a, 0, &held).unwrap(); + assert_eq!(v.sync(), Ok(Vec::new())); + let synced = epochs.borrow().len() - 1; + v.write(a, held.len() as u64, &appended).unwrap(); + assert_eq!(v.sync(), Ok(Vec::new())); + let epochs = epochs.borrow().clone(); + let whole = [&held[..], &appended[..]].concat(); + written(&mut image, epochs[..synced].iter().flatten()); + + for (e, epoch) in epochs.iter().enumerate().skip(synced) { + for kept in 0..1u32 << epoch.len() { + let stop = format!("stopped in flush epoch {e} of {} with writes {kept:#b} of its {}", epochs.len(), epoch.len()); + let mut bytes = image.clone(); + written(&mut bytes, epoch.iter().enumerate().filter(|(i, _)| kept >> i & 1 == 1).map(|(_, w)| w)); + let synced_whole = kept == (1 << epoch.len()) - 1 && epochs[e + 1..].iter().all(Vec::is_empty); + // Clusters no entry reaches yet, and the last sync's entry over + // the append's chain: the window between the chain and its + // entry, closed once the sync is whole on the disk. + let refused: Vec<_> = check(&bytes) + .into_iter() + .filter(|c| match c { + _ if synced_whole => true, + Complaint::LostChain { .. } | Complaint::FsInfoFreeCount { .. } | Complaint::FatMirror { .. } => false, + Complaint::ChainTooLong { path, size, .. } => !(path == "/a.txt" && *size == held.len() as u64), + _ => true, + }) + .collect(); + assert!(refused.is_empty(), "{stop}: {}", describe(&refused)); + let mut after = FatVolume::mount(Image(bytes), false, clock).unwrap_or_else(|e| panic!("{stop}: {e}")); + let n = after.open("a.txt", OPEN).unwrap(); + let size = after.lstat("a.txt").unwrap().size as usize; + let mut out = vec![0u8; size]; + let read = after.read(n, 0, &mut crate::volume::Buf(&mut out)); + assert_eq!(read, Ok(size), "{stop}: a {size}-byte file"); + assert!( + out == whole || out == held && !synced_whole, + "{stop}: the file is {size} bytes and neither the {} the last sync held nor the {} the append made", + held.len(), + whole.len() + ); + } + written(&mut image, epoch); + } + } + /// What the driver says of a volume that stopped answering is `Io`, which /// no caller takes for a name that is not there. #[test]