From 7429e0afe9846d1314f1ceb75a4798c28712c4ee Mon Sep 17 00:00:00 2001 From: japabu Date: Fri, 9 Oct 2026 18:37:37 +0200 Subject: [PATCH 1/5] Carry the issue a directory rename on DATA is not atomic Verbatim from wt/toyos-pkgrepo (d7603368c), so the branch that fixes it closes it. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_017cSFvbD35xJ2kGANVdm23C --- ...-directory-rename-on-data-is-not-atomic.md | 32 +++++++++++++++++++ 1 file changed, 32 insertions(+) create mode 100644 issues/a-directory-rename-on-data-is-not-atomic.md diff --git a/issues/a-directory-rename-on-data-is-not-atomic.md b/issues/a-directory-rename-on-data-is-not-atomic.md new file mode 100644 index 00000000000..457e55f9211 --- /dev/null +++ b/issues/a-directory-rename-on-data-is-not-atomic.md @@ -0,0 +1,32 @@ +--- +status: open +kind: defect +opened: 2026-10-09 +--- + +# A directory rename on DATA is not atomic + +Fileserver's. `userland/fileserver/src/data.rs`'s `rename` of a directory +renames each entry under it one at a time, because the format keys every file +by its whole path and has no rename of a prefix; its own comment says a kill +in the middle leaves the directory in two halves. A refused write does the +same without a kill: a host test of `DataVolume` (scratch, not committed) made +`home/staged/a` (5 bytes) and `home/staged/z` (240 pages on a fragmented +volume), and renamed `home/staged` to a 300-byte name. `rename` answered +`ResourceExhausted` (the format's `EntryTooLarge { size: 4192, max: 4064 }` +for `z`), and the volume then held `/a` and `home/staged/z`: half the +directory under each name, and the call reported as failed. The format keeps +no journal either (`issues/bcachefs-crate-is-not-bcachefs.md`), so even one +entry's rename is only as whole as the sync that writes it. + +**What it blocks.** The package track's stage-then-commit +(`issues/a-package-is-a-directory-under-apps-and-the-installer-is-a-program.md`): +`/system/bin/pkg` stages `/apps/` privately and commits it in one step, +which a rename that can leave half a package under `/apps` is not. + +## Exit condition + +A directory rename on DATA leaves the directory whole under exactly one of its +names whatever write is refused and wherever the server is killed, measured by +a test that refuses every write of the rename in turn and kills the server at +every one. From 3adcf424efe0b65aea1193183d8c49e33c4291fb Mon Sep 17 00:00:00 2001 From: japabu Date: Fri, 9 Oct 2026 18:38:03 +0200 Subject: [PATCH 2/5] A directory rename on DATA moves whole or not at all, and a sync is a commit a kill cannot tear The issue named fileserver's per-entry rename, and that was half of it. The other half was the format's: its btree was updated in place and a sync wrote the superblock (block 0, so first in the cache's lowest-first flush) in the same flush as the nodes it names. A host test that stops the disk after each block write of a directory rename and its sync, run on main, tore 39 of 44 stops: 2 left the directory in two halves, 37 left a volume that would not mount at all (BadMagic on a root the superblock named and the disk did not yet hold). So the fix is at both owners. bcachefs (the interim format) now commits by shadow paging, with no change to a byte of its layout: - No node the last commit's tree reaches is written over. A node is written in place only when the operation in progress took its block; otherwise `btree::own` moves it to a new block and gives the old one up, up to a new root. Insert and delete both go through it; delete now returns the root. - Every read-write call is one operation (`Mounted::atomic`): refused anywhere, the root goes back and every block it took is freed; succeeded, what it gave up is given up. `split_node`'s and `write_data`'s own give-backs go, the operation's rollback covers both. - `Mounted::rename_all` renames a list of names as one operation. - A sync flushes everything first, then writes the primary superblock and flushes, then the backup and flushes: the primary landing is the commit, and a torn primary fails its CRC for the backup, still the last commit. - A block given up is free at once only if no commit has named it since it was taken; otherwise it waits for the next commit. From the first superblock write on, everything taken counts as named (`seal`), so a commit refused after its superblock reached the device keeps both trees until one lands. - A file's data leaves `NODE_RESERVE` (16) blocks free, so a volume its files filled can still copy the nodes a delete changes. - `Formatted` is a `Mounted` with shadowing off, its own create, symlink and sync paths gone; mkfs writes the same bytes as before: one image of kernel/src (232 files, 24 symlinks) built with main's crate and with this one is byte-identical. fileserver's directory rename persists the open files beneath it, then hands every entry beneath and the directory's own to `rename_all`, and moves its index only once that answers. What this keeps, filed: a crash leaks the blocks taken since the last commit (issues/a-crash-leaks-the-blocks-of-its-last-commit.md), and a full volume frees what an unlink gave up only at the next commit (issues/a-full-data-volume-frees-space-only-at-a-commit.md). Closes issues/a-directory-rename-on-data-is-not-atomic.md: its exit, a test that refuses every write of the rename in turn and kills the server at every one, is fileserver's two new tests and bcachefs/tests/crash.rs. The test fixtures that made a volume of one-block holes by filling it with files now take blocks from the allocator directly and assert their premise on the bitmap: filled with files, the 16 reserved blocks were one free run and the premise was silently false. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_017cSFvbD35xJ2kGANVdm23C --- bcachefs/src/alloc_bitmap.rs | 182 +++++++++- bcachefs/src/btree.rs | 205 ++++++------ bcachefs/src/fs.rs | 311 ++++++++--------- bcachefs/src/superblock.rs | 13 +- bcachefs/tests/crash.rs | 238 +++++++++++++ bcachefs/tests/integration.rs | 88 ++--- ...ash-leaks-the-blocks-of-its-last-commit.md | 27 ++ ...-directory-rename-on-data-is-not-atomic.md | 32 -- ...ata-volume-frees-space-only-at-a-commit.md | 28 ++ userland/fileserver/src/data.rs | 316 +++++++++++++++--- 10 files changed, 1050 insertions(+), 390 deletions(-) create mode 100644 bcachefs/tests/crash.rs create mode 100644 issues/a-crash-leaks-the-blocks-of-its-last-commit.md delete mode 100644 issues/a-directory-rename-on-data-is-not-atomic.md create mode 100644 issues/a-full-data-volume-frees-space-only-at-a-commit.md diff --git a/bcachefs/src/alloc_bitmap.rs b/bcachefs/src/alloc_bitmap.rs index c6a5db186f9..844e9b3243c 100644 --- a/bcachefs/src/alloc_bitmap.rs +++ b/bcachefs/src/alloc_bitmap.rs @@ -1,5 +1,9 @@ +use alloc::collections::BTreeMap; +use alloc::vec::Vec; + use crate::block_io::{BlockBuf, BlockNum, BlockIO, BlockIOExt, BLOCK_SIZE}; use crate::fs::FsError; +use crate::superblock::Superblock; const BITS_PER_BLOCK: u64 = (BLOCK_SIZE * 8) as u64; @@ -22,15 +26,162 @@ pub struct Run { /// /// The bitmap is stored on disk starting at `bitmap_start` and spanning /// `bitmap_blocks` blocks. Each bit represents one block: 1 = used, 0 = free. +/// +/// **With `shadow` on, no block the last committed tree reaches is written +/// over or handed out again before the next commit lands** (`fs::commit`): a +/// btree node is written in place only when the operation in progress took +/// its block ([`Self::in_place`]), and a block given up is free at once only +/// if no commit has named it since it was taken (`fresh`); any other waits in +/// `pending` for the commit. An operation ([`Self::begin`]) gives up its +/// blocks only when it succeeds, and refused gives back every one it took. +/// mkfs runs with `shadow` off: nothing it writes is committed until it ends. pub struct BitmapAllocator { pub bitmap_start: BlockNum, pub bitmap_blocks: u64, pub total_blocks: u64, pub free_blocks: u64, pub next_alloc: u64, // cursor — scan starts here, wraps once + pub shadow: bool, + /// Taken since the last commit's superblock was handed to the device. + fresh: Runs, + /// Given up, and reached by a tree a mount may still find. + pending: Vec<(u64, u32)>, + op: Option, +} + +/// Blocks a file's data never takes while `shadow` is on, so a volume its +/// files filled can still copy the nodes a delete changes: a rename in a tree +/// four levels deep copies four, splits four and adds a root for its insert, +/// and copies four more for its delete. +pub const NODE_RESERVE: u64 = 16; + +/// The operation in progress: what it took, and what it gives up if it succeeds. +#[derive(Default)] +struct Op { + taken: Runs, + given: Vec<(u64, u32)>, +} + +/// Disjoint runs of blocks, by first block, to one past the last. +#[derive(Default)] +struct Runs(BTreeMap); + +impl Runs { + fn insert(&mut self, start: u64, len: u64) { + self.0.insert(start, start + len); + } + + fn contains(&self, block: u64) -> bool { + self.0.range(..=block).next_back().is_some_and(|(_, &end)| block < end) + } + + /// Take `block` out, answering whether it was in. + fn remove(&mut self, block: u64) -> bool { + let Some((&start, &end)) = self.0.range(..=block).next_back() else { return false }; + if block >= end { + return false; + } + self.0.remove(&start); + if start < block { + self.0.insert(start, block); + } + if block + 1 < end { + self.0.insert(block + 1, end); + } + true + } } impl BitmapAllocator { + /// The allocator of a mounted volume, as its superblock left it. + pub fn open(sb: &Superblock) -> Self { + Self { + bitmap_start: sb.bitmap_start, + bitmap_blocks: sb.bitmap_blocks, + total_blocks: sb.block_count, + free_blocks: sb.free_blocks, + next_alloc: sb.next_alloc, + shadow: true, + fresh: Runs::default(), + pending: Vec::new(), + op: None, + } + } + + /// Begin one operation on the tree, which [`Self::succeed`] or + /// [`Self::fail`] ends. + pub fn begin(&mut self) { + assert!(self.op.replace(Op::default()).is_none(), "an operation began inside another"); + } + + /// Whether a node at `block` may be written where it is. + pub fn in_place(&self, block: BlockNum) -> bool { + !self.shadow || self.op.as_ref().is_some_and(|op| op.taken.contains(block.raw())) + } + + /// The operation took effect: what it gave up is given up now. A bit the + /// device would not clear is a leaked block, and no reason to call an + /// operation that took effect refused. + pub fn succeed(&mut self, io: &dyn BlockIO) { + let op = self.op.take().expect("an operation in progress"); + for (start, count) in op.given { + let _ = self.give(io, start, count); + } + } + + /// The operation was refused: every block it took is free again, and what + /// it would have given up stays its owner's. A bit the device would not + /// clear is a leaked block, and the refusal already in hand is the answer. + pub fn fail(&mut self, io: &dyn BlockIO) { + let op = self.op.take().expect("an operation in progress"); + for (start, end) in op.taken.0 { + for block in start..end { + self.fresh.remove(block); + let _ = self.set_free(io, BlockNum::new(block)); + } + } + } + + /// A superblock naming the tree as it stands may land from here on, so + /// nothing taken so far is free at once when given up. + pub fn seal(&mut self) { + self.fresh = Runs::default(); + } + + /// Blocks given up that a commit landing makes free. + pub fn pending(&self) -> u64 { + self.pending.iter().map(|&(_, count)| count as u64).sum() + } + + /// A commit landed: the blocks only an older tree reached are free. + pub fn committed(&mut self, io: &dyn BlockIO) -> Result<(), FsError> { + while let Some((start, count)) = self.pending.pop() { + for i in 0..count { + if let Err(e) = self.set_free(io, BlockNum::new(start + i as u64)) { + self.pending.push((start + i as u64, count - i)); + return Err(e); + } + } + } + Ok(()) + } + + /// Give up `count` blocks from `start`: free now if no commit has named + /// them, and otherwise once the next one lands. + fn give(&mut self, io: &dyn BlockIO, start: u64, count: u32) -> Result<(), FsError> { + for block in start..start + count as u64 { + if !self.shadow || self.fresh.remove(block) { + self.set_free(io, BlockNum::new(block))?; + } else { + match self.pending.last_mut() { + Some((at, n)) if *at + *n as u64 == block => *n += 1, + _ => self.pending.push((block, 1)), + } + } + } + Ok(()) + } + /// Where a block's bit lives, and which bit of that byte it is. fn bit_of(&self, block: BlockNum) -> (BlockNum, usize, u8) { let byte_idx = block.raw() / 8; @@ -91,13 +242,20 @@ impl BitmapAllocator { } /// Reserve as much of `wanted` as one contiguous run can cover, scanning - /// from block `from`. + /// from block `from`: a file's data, which leaves [`NODE_RESERVE`] free. /// /// The run is never empty and may be shorter than asked for, so every /// caller has to loop or has to be wrong. pub fn alloc_up_to(&mut self, io: &dyn BlockIO, from: u64, wanted: u32) -> Result { + let spare = match self.shadow { + true => self.free_blocks.saturating_sub(NODE_RESERVE), + false => self.free_blocks, + }; + if spare == 0 { + return Err(FsError::NoSpace { requested: wanted, available: 0 }); + } // A zero-length run would let a caller's loop spin without progress. - let wanted = wanted.max(1); + let wanted = wanted.max(1).min(u32::try_from(spare).unwrap_or(u32::MAX)); let (start, len) = self.longest_free_run(io, from % self.total_blocks, wanted)?; self.reserve(io, start, len.min(wanted)) } @@ -123,6 +281,10 @@ impl BitmapAllocator { fn reserve(&mut self, io: &dyn BlockIO, start: u64, len: u32) -> Result { let start_block = BlockNum::new(start); self.set_range_used(io, start_block, len as u64)?; + self.fresh.insert(start, len as u64); + if let Some(op) = &mut self.op { + op.taken.insert(start, len as u64); + } self.free_blocks -= len as u64; self.next_alloc = start + len as u64; if self.next_alloc >= self.total_blocks { @@ -216,17 +378,21 @@ impl BitmapAllocator { Ok((start, best_count)) } - /// Free a contiguous range of blocks. + /// Give up a contiguous range of blocks: inside an operation, once it + /// succeeds. pub fn free_range( &mut self, io: &dyn BlockIO, start: BlockNum, count: u32, ) -> Result<(), FsError> { - for i in 0..count as u64 { - self.set_free(io, BlockNum::new(start.raw() + i))?; + match &mut self.op { + Some(op) => { + op.given.push((start.raw(), count)); + Ok(()) + } + None => self.give(io, start.raw(), count), } - Ok(()) } /// Initialize bitmap on disk: zero all bitmap blocks, then mark metadata blocks as used. @@ -248,6 +414,10 @@ impl BitmapAllocator { total_blocks, free_blocks: total_blocks - metadata_blocks, next_alloc: metadata_blocks, + shadow: false, + fresh: Runs::default(), + pending: Vec::new(), + op: None, }; // Metadata blocks (superblock, bitmap, journal area), and the backup diff --git a/bcachefs/src/btree.rs b/bcachefs/src/btree.rs index 59e6ba3d8cc..bafb13be04b 100644 --- a/bcachefs/src/btree.rs +++ b/bcachefs/src/btree.rs @@ -365,15 +365,24 @@ fn write_entry(b: &mut [u8; BLOCK_SIZE], offset: usize, key: &Key, value: &[u8]) /// child's subtree, so the answer is the last child whose key is `<= key`, /// defaulting to the first — which covers everything below the second key. fn find_child(children: &[Child], key: &Key) -> Option { - let mut chosen = children.first()?.block; - for child in children { - if child.key <= *key { - chosen = child.block; - } else { - break; - } + children.get(child_index(children, key)).map(|c| c.block) +} + +/// [`find_child`]'s answer as an index into `children`. +fn child_index(children: &[Child], key: &Key) -> usize { + children.iter().take_while(|c| c.key <= *key).count().saturating_sub(1) +} + +/// The block a node the operation in progress changed is written to: its +/// own where [`BitmapAllocator::in_place`] allows, and otherwise a new one, +/// the old given up. +fn own(io: &dyn BlockIO, alloc: &mut BitmapAllocator, block: BlockNum) -> Result { + if alloc.in_place(block) { + return Ok(block); } - Some(chosen) + let moved = alloc.alloc_block(io)?; + alloc.free_range(io, block, 1)?; + Ok(moved) } /// Search the B+ tree for an exact key match. Returns the leaf entry's value. @@ -434,27 +443,48 @@ pub fn search_by_hash( } } -/// Delete an exact key from the B+ tree. Returns the old value if found. +/// Delete an exact key from the B+ tree: the root after it and the old value, +/// or `None` with nothing written when no entry has the key. /// Does not merge underflowing nodes — just removes the entry from the leaf. -pub fn delete(io: &dyn BlockIO, root: BlockNum, key: &Key) -> Result>, FsError> { - let mut block = root; - let mut depth = Depth::ROOT; +pub fn delete( + io: &dyn BlockIO, + alloc: &mut BitmapAllocator, + root: BlockNum, + key: &Key, +) -> Result)>, FsError> { + delete_recursive(io, alloc, root, Depth::ROOT, key) +} - loop { - match Node::read(io, block)? { - Node::Leaf(mut entries) => { - let Some(pos) = entries.iter().position(|e| e.key == *key) else { - return Ok(None); - }; - let old = entries.remove(pos); - Node::Leaf(entries).write(io, block)?; - return Ok(Some(old.value)); - } - Node::Interior { children, .. } => { - let next = find_child(&children, key).ok_or(FsError::CorruptedNode(block))?; - depth = depth.descend(block)?; - block = next; +fn delete_recursive( + io: &dyn BlockIO, + alloc: &mut BitmapAllocator, + block: BlockNum, + depth: Depth, + key: &Key, +) -> Result)>, FsError> { + match Node::read(io, block)? { + Node::Leaf(mut entries) => { + let Some(pos) = entries.iter().position(|e| e.key == *key) else { + return Ok(None); + }; + let old = entries.remove(pos); + let at = own(io, alloc, block)?; + Node::Leaf(entries).write(io, at)?; + Ok(Some((at, old.value))) + } + Node::Interior { level, mut children } => { + let idx = child_index(&children, key); + let child = children.get(idx).ok_or(FsError::CorruptedNode(block))?.block; + let Some((moved, old)) = delete_recursive(io, alloc, child, depth.descend(block)?, key)? else { + return Ok(None); + }; + if moved == child { + return Ok(Some((block, old))); } + children[idx].block = moved; + let at = own(io, alloc, block)?; + Node::Interior { level, children }.write(io, at)?; + Ok(Some((at, old))) } } } @@ -499,7 +529,8 @@ fn walk_recursive( /// Insert a key-value pair into the B+ tree. /// -/// Returns the root block, which changes when the old root was split. +/// Returns the root block, which changes when the old root was split or +/// written somewhere new. pub fn insert( io: &dyn BlockIO, alloc: &mut BitmapAllocator, @@ -508,29 +539,29 @@ pub fn insert( ) -> Result { check_entry_fits(&entry)?; - match insert_recursive(io, alloc, root, Depth::ROOT, entry)? { - InsertResult::Done => Ok(root), - InsertResult::Split(siblings) => { - let level = Node::read(io, root)? - .level() - .checked_add(1) - .ok_or(FsError::CorruptedNode(root))?; - let old_min_key = min_key(io, root, Depth::ROOT)?; - let new_root_block = alloc.alloc_block(io)?; - - let mut children = alloc::vec![Child { key: old_min_key, block: root }]; - children.extend(siblings); - Node::Interior { level, children }.write(io, new_root_block)?; - - Ok(new_root_block) - } + let Placed { at, split } = insert_recursive(io, alloc, root, Depth::ROOT, entry)?; + if split.is_empty() { + return Ok(at); } + let level = Node::read(io, at)? + .level() + .checked_add(1) + .ok_or(FsError::CorruptedNode(at))?; + let old_min_key = min_key(io, at, Depth::ROOT)?; + let new_root_block = alloc.alloc_block(io)?; + + let mut children = alloc::vec![Child { key: old_min_key, block: at }]; + children.extend(split); + Node::Interior { level, children }.write(io, new_root_block)?; + + Ok(new_root_block) } -enum InsertResult { - Done, - /// The node split: these follow it, in key order. - Split(Vec), +/// Where an insert left a node: its block, and the siblings it split off, in +/// key order. +struct Placed { + at: BlockNum, + split: Vec, } fn insert_recursive( @@ -539,7 +570,7 @@ fn insert_recursive( block: BlockNum, depth: Depth, entry: Entry, -) -> Result { +) -> Result { match Node::read(io, block)? { Node::Leaf(mut entries) => { match entries.binary_search_by(|e| e.key.cmp(&entry.key)) { @@ -548,32 +579,24 @@ fn insert_recursive( } write_or_split(io, alloc, block, Node::Leaf(entries)) } - Node::Interior { level, children } => { - let mut idx = 0; - for (i, child) in children.iter().enumerate() { - if child.key <= entry.key { - idx = i; - } else { - break; - } - } + Node::Interior { level, mut children } => { + let idx = child_index(&children, &entry.key); let child_block = children.get(idx).ok_or(FsError::CorruptedNode(block))?.block; let deeper = depth.descend(block)?; - match insert_recursive(io, alloc, child_block, deeper, entry)? { - InsertResult::Done => Ok(InsertResult::Done), - InsertResult::Split(siblings) => { - let mut children = children; - for sibling in siblings { - let pos = match children.binary_search_by(|c| c.key.cmp(&sibling.key)) { - Ok(i) => i + 1, - Err(i) => i, - }; - children.insert(pos, sibling); - } - write_or_split(io, alloc, block, Node::Interior { level, children }) - } + let Placed { at, split } = insert_recursive(io, alloc, child_block, deeper, entry)?; + if at == child_block && split.is_empty() { + return Ok(Placed { at: block, split }); + } + children[idx].block = at; + for sibling in split { + let pos = match children.binary_search_by(|c| c.key.cmp(&sibling.key)) { + Ok(i) => i + 1, + Err(i) => i, + }; + children.insert(pos, sibling); } + write_or_split(io, alloc, block, Node::Interior { level, children }) } } } @@ -583,20 +606,23 @@ fn write_or_split( alloc: &mut BitmapAllocator, block: BlockNum, node: Node, -) -> Result { +) -> Result { + let at = own(io, alloc, block)?; if NODE_HEADER_SIZE + node.payload_size() <= BLOCK_SIZE { - node.write(io, block)?; - return Ok(InsertResult::Done); + node.write(io, at)?; + return Ok(Placed { at, split: Vec::new() }); } - split_node(io, alloc, block, node) + Ok(Placed { at, split: split_node(io, alloc, at, node)? }) } +/// Split `node` across `block` and new siblings, answering the siblings. The +/// blocks it took are the operation's, which gives them back if it fails. fn split_node( io: &dyn BlockIO, alloc: &mut BitmapAllocator, block: BlockNum, node: Node, -) -> Result { +) -> Result, FsError> { match node { Node::Leaf(entries) => { // One entry is not a split problem. Halving by *count* used to @@ -610,28 +636,15 @@ fn split_node( let mut nodes = pack(entries); let first = nodes.remove(0); - let mut blocks = Vec::with_capacity(nodes.len()); - for _ in &nodes { - match alloc.alloc_block(io) { - Ok(sibling) => blocks.push(sibling), - Err(e) => { - for taken in blocks { - alloc.free_range(io, taken, 1)?; - } - return Err(e); - } - } - } - // The siblings first: a failure before `block` is replaced leaves - // the tree as it was and blocks nothing names. let mut children = Vec::with_capacity(nodes.len()); - for (node, sibling) in nodes.into_iter().zip(blocks) { + for node in nodes { + let sibling = alloc.alloc_block(io)?; children.push(Child { key: node[0].key, block: sibling }); Node::Leaf(node).write(io, sibling)?; } Node::Leaf(first).write(io, block)?; - Ok(InsertResult::Split(children)) + Ok(children) } Node::Interior { level, mut children } => { if children.len() < 2 { @@ -649,7 +662,7 @@ fn split_node( Node::Interior { level, children }.write(io, block)?; Node::Interior { level, children: right }.write(io, right_block)?; - Ok(InsertResult::Split(alloc::vec![Child { key: split_key, block: right_block }])) + Ok(alloc::vec![Child { key: split_key, block: right_block }]) } } } @@ -806,9 +819,8 @@ mod tests { ); } - /// A split that finds no block for a later sibling gives back the ones it - /// took for the earlier: nothing names them yet, so nothing else frees - /// them. + /// A split that finds no block for a later sibling fails its operation, + /// which gives back the block it took for the earlier: nothing names it. #[test] fn a_split_short_of_a_sibling_gives_back_the_ones_it_took() { let io = crate::block_io::VecBlockIO::new(16); @@ -821,10 +833,13 @@ mod tests { let mut middle: Vec = (0..40).map(|_| entry(72)).collect(); middle.insert(20, entry(MAX_ENTRY_SIZE - KEY_HEADER_SIZE)); assert_eq!(pack(middle.clone()).len(), 3, "two siblings, and a block for one"); + alloc.begin(); assert!(matches!( split_node(&io, &mut alloc, leaf, Node::Leaf(middle)), Err(FsError::NoSpace { .. }), )); + assert_eq!(alloc.free_blocks, 0, "the first sibling took the last block"); + alloc.fail(&io); assert_eq!(alloc.free_blocks, 1, "the first sibling's block went back"); } diff --git a/bcachefs/src/fs.rs b/bcachefs/src/fs.rs index e83a49d17e3..161112f8879 100644 --- a/bcachefs/src/fs.rs +++ b/bcachefs/src/fs.rs @@ -139,12 +139,10 @@ impl FsError { pub struct ReadOnly; pub struct ReadWrite; -/// A formatted but not yet mounted filesystem. Used for building images (mkfs). -pub struct Formatted { - io: IO, - sb: Superblock, - alloc: BitmapAllocator, -} +/// A formatted but not yet mounted filesystem. Used for building images +/// (mkfs): written in place, since nothing it holds is committed before +/// [`Formatted::into_io`]. +pub struct Formatted(Mounted); /// A mounted filesystem. Mode is ReadOnly or ReadWrite. pub struct Mounted { @@ -412,9 +410,8 @@ fn decode_leaf_value(value: &[u8], volume_blocks: u64) -> Result 0 { - let run = match alloc.alloc_up_to(io, alloc.next_alloc, remaining) { - Ok(run) => run, - Err(err) => return Err(give_back(io, alloc, &extents, err)), - }; + let run = alloc.alloc_up_to(io, alloc.next_alloc, remaining)?; push_extent(&mut extents, run.start.raw(), run.len); let mut buf = BlockBuf::zeroed(); @@ -444,9 +438,7 @@ fn write_data( let len = chunk_end - data_offset; buf.0[..len].copy_from_slice(&data[data_offset..chunk_end]); } - if let Err(err) = io.write(BlockNum::new(run.start.raw() + i), &buf) { - return Err(give_back(io, alloc, &extents, err)); - } + io.write(BlockNum::new(run.start.raw() + i), &buf)?; data_offset += BLOCK_SIZE; } @@ -456,24 +448,6 @@ fn write_data( Ok(extents) } -/// Hand back the runs a failed [`write_data`] had already reserved, and return -/// the failure that stopped it. -/// -/// Best effort by construction: this runs because something has already gone -/// wrong, and a bitmap write that also fails has no better answer to give than -/// the error already in hand. -fn give_back( - io: &dyn BlockIO, - alloc: &mut BitmapAllocator, - extents: &[Extent], - err: FsError, -) -> FsError { - for ext in extents { - let _ = alloc.free_range(io, BlockNum::new(ext.start_block), ext.block_count); - } - err -} - /// Read file data from a list of extents. /// /// **`size` sizes the `Vec` and `size` is a number off the disk**, so it is @@ -575,84 +549,44 @@ impl Formatted { sb.write(&io)?; - Ok(Self { io, sb, alloc }) + Ok(Self(Mounted { io, sb, alloc, _mode: PhantomData })) } /// Name this filesystem, so a role's kernel argument can select it. /// /// A separate act from formatting: a volume nothing names is legal, and - /// nothing here invents a name for one. Persisted by [`Self::sync`], which - /// [`Self::into_io`] runs. + /// nothing here invents a name for one. Persisted by [`Self::into_io`]. pub fn set_uuid(&mut self, uuid: FsUuid) { - self.sb.uuid = uuid; + self.0.sb.uuid = uuid; } /// Create a file on the formatted filesystem (used during mkfs). pub fn create(&mut self, name: &str, data: &[u8], mtime: u64) -> Result<(), FsError> { - if name.is_empty() || name.len() > MAX_NAME_LEN { - return Err(FsError::NameTooLong { len: name.len(), max: MAX_NAME_LEN }); - } - - let extents = write_data(&self.io, &mut self.alloc, data)?; - let value = encode_leaf_value(KeyType::File, name, data.len() as u64, mtime, &extents); - let key = make_key(&self.sb.hash_seed, name, KeyType::File); - let entry = Entry { key, value }; - - self.sb.root_node = btree::insert(&self.io, &mut self.alloc, self.sb.root_node, entry)?; - - Ok(()) + self.0.create(name, data, mtime) } /// Create a symlink on the formatted filesystem. pub fn create_symlink(&mut self, name: &str, target: &str, mtime: u64) -> Result<(), FsError> { - if name.is_empty() || name.len() > MAX_NAME_LEN { - return Err(FsError::NameTooLong { len: name.len(), max: MAX_NAME_LEN }); - } - - let target_bytes = target.as_bytes(); - let extents = write_data(&self.io, &mut self.alloc, target_bytes)?; - let value = encode_leaf_value(KeyType::Symlink, name, target_bytes.len() as u64, mtime, &extents); - let key = make_key(&self.sb.hash_seed, name, KeyType::Symlink); - let entry = Entry { key, value }; - - self.sb.root_node = btree::insert(&self.io, &mut self.alloc, self.sb.root_node, entry)?; - - Ok(()) - } - - /// Finalize the filesystem: write superblock with clean flag. - pub fn sync(&mut self) -> Result<(), FsError> { - self.sb.free_blocks = self.alloc.free_blocks; - self.sb.next_alloc = self.alloc.next_alloc; - self.sb.set_clean(true); - self.sb.write(&self.io)?; - self.io.flush() + self.0.put(name, KeyType::Symlink, target.as_bytes(), mtime) } /// Mount this formatted filesystem for read-write access. pub fn mount(self) -> Mounted { - Mounted { - io: self.io, - sb: self.sb, - alloc: self.alloc, - _mode: PhantomData, - } + let mut fs = self.0; + fs.alloc.shadow = true; + fs } /// Mount this formatted filesystem for read-only access. pub fn mount_readonly(self) -> Mounted { - Mounted { - io: self.io, - sb: self.sb, - alloc: self.alloc, - _mode: PhantomData, - } + let Mounted { io, sb, alloc, .. } = self.0; + Mounted { io, sb, alloc, _mode: PhantomData } } /// Consume and return the underlying IO (for extracting the image bytes). pub fn into_io(mut self) -> Result { - self.sync()?; - Ok(self.io) + self.0.sync()?; + Ok(self.0.io) } } @@ -662,13 +596,7 @@ impl Mounted { /// Open an existing filesystem from disk. pub fn open(io: IO) -> Result, FsError> { let sb = Superblock::read(&io)?; - let alloc = BitmapAllocator { - bitmap_start: sb.bitmap_start, - bitmap_blocks: sb.bitmap_blocks, - total_blocks: sb.block_count, - free_blocks: sb.free_blocks, - next_alloc: sb.next_alloc, - }; + let alloc = BitmapAllocator::open(&sb); Ok(Mounted { io, sb, @@ -803,11 +731,9 @@ impl Mounted { /// Convert back to Formatted state (for testing — insert more files after reading). pub fn into_formatted(self) -> Formatted { - Formatted { - io: self.io, - sb: self.sb, - alloc: self.alloc, - } + let Self { io, sb, mut alloc, .. } = self; + alloc.shadow = false; + Formatted(Mounted { io, sb, alloc, _mode: PhantomData }) } /// Return the extents and file size for a file. @@ -825,6 +751,17 @@ impl Mounted { // --- ReadWrite-only operations --- +/// **Every change is one operation and every sync one commit.** An operation +/// ([`Mounted::atomic`]) takes effect whole or, refused anywhere, leaves the +/// tree and the allocator as they were; it never writes over a block the last +/// commit's tree reaches, but to new blocks up to a new root. A commit +/// ([`Mounted::sync`]) is the primary superblock naming that root landing on +/// the device, after everything the root reaches, and before the backup copy: +/// killed anywhere, the device holds the last commit's tree whole or the new +/// one whole, a torn primary failing its checksum for the backup, which is the +/// last commit still. What a kill costs is the blocks taken since the last +/// commit and those only the old tree reached: marked used, and named by no +/// tree, until a sweep (`issues/a-crash-leaks-the-blocks-of-its-last-commit.md`). impl Mounted { /// Create a file, replacing whatever answered to `name`. pub fn create(&mut self, name: &str, data: &[u8], mtime: u64) -> Result<(), FsError> { @@ -836,6 +773,24 @@ impl Mounted { self.put(name, KeyType::Symlink, target.as_bytes(), 0) } + /// Run `change` as one operation: whole, or refused with the root and + /// every block as they were. + fn atomic(&mut self, change: impl FnOnce(&mut Self) -> Result) -> Result { + let root = self.sb.root_node; + self.alloc.begin(); + match change(self) { + Ok(done) => { + self.alloc.succeed(&self.io); + Ok(done) + } + Err(e) => { + self.sb.root_node = root; + self.alloc.fail(&self.io); + Err(e) + } + } + } + /// Put `name` on the volume, displacing whatever answered to it. /// /// The new entry goes in before the old one comes out, for the reason @@ -860,21 +815,28 @@ impl Mounted { return Err(FsError::NameTooLong { len: name.len(), max: MAX_NAME_LEN }); } - let displaced = match self.find_by_name(name)? { - Some((key, value)) => Some((key, self.decode(&value)?.extents().to_vec())), - None => None, - }; + self.atomic(|fs| { + let displaced = match fs.find_by_name(name)? { + Some((key, value)) => Some((key, fs.decode(&value)?.extents().to_vec())), + None => None, + }; - let extents = write_data(&self.io, &mut self.alloc, data)?; - let value = encode_leaf_value(key_type, name, data.len() as u64, mtime, &extents); - let key = make_key(&self.sb.hash_seed, name, key_type); - self.sb.root_node = btree::insert( - &self.io, &mut self.alloc, - self.sb.root_node, - Entry { key, value }, - )?; + let extents = write_data(&fs.io, &mut fs.alloc, data)?; + let value = encode_leaf_value(key_type, name, data.len() as u64, mtime, &extents); + let key = make_key(&fs.sb.hash_seed, name, key_type); + fs.sb.root_node = btree::insert(&fs.io, &mut fs.alloc, fs.sb.root_node, Entry { key, value })?; - self.retire_displaced(displaced, key) + fs.retire_displaced(displaced, key) + }) + } + + /// Remove `key`'s entry, if the tree has one. + fn remove(&mut self, key: &Key) -> Result { + let Some((root, _)) = btree::delete(&self.io, &mut self.alloc, self.sb.root_node, key)? else { + return Ok(false); + }; + self.sb.root_node = root; + Ok(true) } /// Remove the entry the insert of `new_key` did not replace, and free the @@ -890,26 +852,9 @@ impl Mounted { ) -> Result<(), FsError> { let Some((old_key, old_extents)) = displaced else { return Ok(()) }; if old_key != new_key { - btree::delete(&self.io, self.sb.root_node, &old_key)?; - } - for ext in &old_extents { - self.alloc.free_range(&self.io, BlockNum::new(ext.start_block), ext.block_count)?; + self.remove(&old_key)?; } - Ok(()) - } - - /// Delete a file or symlink by name. Returns true if found and deleted. - pub fn delete(&mut self, name: &str) -> Result { - self.delete_by_name(name) - } - - /// Sync filesystem state to disk. - pub fn sync(&mut self) -> Result<(), FsError> { - self.sb.free_blocks = self.alloc.free_blocks; - self.sb.next_alloc = self.alloc.next_alloc; - self.sb.set_clean(true); - self.sb.write(&self.io)?; - self.io.flush() + self.free_extents(&old_extents) } /// Delete a file/symlink by name, freeing its data blocks. Returns true if found. @@ -921,32 +866,57 @@ impl Mounted { /// caller nothing had happened. It also answers it once for both key /// types, where the old shape fell through from File to Symlink after a /// non-matching removal and could take two entries out in one call. - fn delete_by_name(&mut self, name: &str) -> Result { - let Some((key, value)) = self.find_by_name(name)? else { return Ok(false) }; - let extents = self.decode(&value)?.extents().to_vec(); - - // `find_by_name` reached this key by the descent `btree::delete` is - // about to repeat, so an empty removal is not "no such file" — it is a - // tree that answers two ways. - if btree::delete(&self.io, self.sb.root_node, &key)?.is_none() { - return Err(FsError::CorruptedNode(self.sb.root_node)); - } - for ext in &extents { - self.alloc.free_range(&self.io, BlockNum::new(ext.start_block), ext.block_count)?; + pub fn delete(&mut self, name: &str) -> Result { + self.atomic(|fs| { + let Some((key, value)) = fs.find_by_name(name)? else { return Ok(false) }; + let extents = fs.decode(&value)?.extents().to_vec(); + + // `find_by_name` reached this key by the descent `btree::delete` is + // about to repeat, so an empty removal is not "no such file" — it is a + // tree that answers two ways. + if !fs.remove(&key)? { + return Err(FsError::CorruptedNode(fs.sb.root_node)); + } + fs.free_extents(&extents)?; + Ok(true) + }) + } + + /// Commit the tree as it stands: see this block's header. + pub fn sync(&mut self) -> Result<(), FsError> { + self.io.flush()?; + // What the volume holds free once this commit's own frees are made. + self.sb.free_blocks = self.alloc.free_blocks + self.alloc.pending(); + self.sb.next_alloc = self.alloc.next_alloc; + self.sb.set_clean(true); + // From the first superblock write on, this commit may be what a mount + // finds, even where the write is refused. + self.alloc.seal(); + for copy in self.sb.copies() { + self.sb.write_at(&self.io, copy)?; + self.io.flush()?; } - Ok(true) + self.alloc.committed(&self.io) } /// Rename a file or symlink. - /// - /// The new entry goes in before the old one comes out, so a crash between - /// the two leaves the file under both names rather than under neither. What - /// that ordering costs is that the insert *is* the removal of whatever - /// `new_name` named — same name and same type is the same key, and - /// `btree::insert` replaces on an equal key — so the displaced entry has to - /// be read out of the tree before the insert. Asking for it afterwards, by - /// name, answers with the file that was just renamed and frees its extents. pub fn rename(&mut self, old_name: &str, new_name: &str) -> Result<(), FsError> { + self.rename_all(&[(old_name, new_name)]) + } + + /// Rename every `(old, new)` pair in turn, as one operation: a directory + /// is the names beneath it, and moves whole or not at all. + pub fn rename_all(&mut self, renames: &[(&str, &str)]) -> Result<(), FsError> { + self.atomic(|fs| renames.iter().try_for_each(|&(old, new)| fs.rename_one(old, new))) + } + + /// The new entry goes in before the old one comes out. What that ordering + /// costs is that the insert *is* the removal of whatever `new_name` named — + /// same name and same type is the same key, and `btree::insert` replaces on + /// an equal key — so the displaced entry has to be read out of the tree + /// before the insert. Asking for it afterwards, by name, answers with the + /// file that was just renamed and frees its extents. + fn rename_one(&mut self, old_name: &str, new_name: &str) -> Result<(), FsError> { // Every other name-taking entry point bounds its name; this one did // not, and `user_ptr::MAX_USER_STR` lets 64 KiB of it through. if new_name.is_empty() || new_name.len() > MAX_NAME_LEN { @@ -982,7 +952,7 @@ impl Mounted { // extent list. Nothing to delete when the two names share a key — the // entry under it is the one the insert just wrote. if new_key != old_key { - btree::delete(&self.io, self.sb.root_node, &old_key)?; + self.remove(&old_key)?; } Ok(()) @@ -996,29 +966,24 @@ impl Mounted { size: u64, mtime: u64, ) -> Result<(), FsError> { - let (old_key, old_value) = self.find_by_name(name)? - .ok_or(FsError::NotFound)?; - let leaf = self.decode(&old_value)?; - - let new_value = encode_leaf_value(old_key.key_type, leaf.name(), size, mtime, new_extents); - let new_entry = Entry { key: old_key, value: new_value }; - - // No delete first. The key is unchanged and `btree::insert` replaces on - // an equal key, so the delete bought nothing and cost the file: a - // pre-check for `EntryTooLarge` does not cover `insert`'s other - // rejection, a split with no free block to split into, and that one - // left the entry deleted and never put back. Blocks the caller drops - // from the extent list are the caller's to free, through - // [`Self::free_extents`], after this records the shortened list. - self.sb.root_node = btree::insert( - &self.io, &mut self.alloc, - self.sb.root_node, - new_entry, - )?; - Ok(()) + self.atomic(|fs| { + let (old_key, old_value) = fs.find_by_name(name)? + .ok_or(FsError::NotFound)?; + let leaf = fs.decode(&old_value)?; + + let new_value = encode_leaf_value(old_key.key_type, leaf.name(), size, mtime, new_extents); + let new_entry = Entry { key: old_key, value: new_value }; + + // No delete first: the key is unchanged and `btree::insert` replaces + // on an equal key. Blocks the caller drops from the extent list are + // the caller's to free, through [`Self::free_extents`], after this + // records the shortened list. + fs.sb.root_node = btree::insert(&fs.io, &mut fs.alloc, fs.sb.root_node, new_entry)?; + Ok(()) + }) } - /// Return `extents`' blocks to the allocator. Record first, free second: + /// Give `extents`' blocks back to the allocator. Record first, free second: /// the caller shortens the entry's list before calling this, so a failure /// between the two leaks blocks rather than leaving an entry naming freed /// ones. diff --git a/bcachefs/src/superblock.rs b/bcachefs/src/superblock.rs index 5d78b8e3239..d24ec660b4c 100644 --- a/bcachefs/src/superblock.rs +++ b/bcachefs/src/superblock.rs @@ -266,10 +266,19 @@ impl Superblock { /// Write superblock to both block 0 and the backup at the last block. pub fn write(&self, io: &dyn BlockIO) -> Result<(), FsError> { + self.copies().try_for_each(|copy| self.write_at(io, copy)) + } + + /// Block 0, which [`Self::read`] tries first, then the backup. + pub fn copies(&self) -> impl Iterator { + [BlockNum::new(0), BlockNum::new(self.block_count - 1)].into_iter() + } + + /// Write this superblock to one of its [`Self::copies`]. + pub fn write_at(&self, io: &dyn BlockIO, copy: BlockNum) -> Result<(), FsError> { let mut buf = BlockBuf::zeroed(); self.write_to(&mut buf); - io.write(BlockNum::new(0), &buf)?; - io.write(BlockNum::new(self.block_count - 1), &buf) + io.write(copy, &buf) } } diff --git a/bcachefs/tests/crash.rs b/bcachefs/tests/crash.rs new file mode 100644 index 00000000000..81459971950 --- /dev/null +++ b/bcachefs/tests/crash.rs @@ -0,0 +1,238 @@ +//! A directory rename, changes after it and the sync that commits them, +//! stopped at every block write: the crash-point model of the commit. +//! +//! **A device keeps what a flush covered and any part of what it did not.** +//! The writes since the last flush may land in any order, so each stop is +//! tried twice: every write of the open epoch up to the stop landed, and the +//! stop's write alone did. What the device then holds must mount, keep every +//! name beside the directory, and name the directory whole under exactly one +//! of its two names. + +use std::cell::RefCell; +use std::collections::BTreeMap; + +use bcachefs::{BlockBuf, BlockIO, BlockNum, DeviceError, Formatted, FsError, Mounted, ReadOnly, ReadWrite, TransferError, VecBlockIO}; + +const BLOCKS: u64 = 512; +const BLOCK: usize = 4096; +const STAGED: &str = "staged"; +const INSTALLED: &str = "apps/pkg"; + +/// What the device was asked, in order: a block written, or a flush. +enum Event { + Write(u64, Box<[u8; BLOCK]>), + Flush, +} + +/// The device as the filesystem sees it, every write in place at once, with +/// the order it was asked in kept beside it; and the write numbered `refuse`, +/// refused. +struct Logged { + image: RefCell>, + log: RefCell>, + writes: RefCell, + refuse: Option, +} + +impl Logged { + fn new(image: &[u8], refuse: Option) -> Self { + Self { image: RefCell::new(image.to_vec()), log: RefCell::default(), writes: RefCell::default(), refuse } + } +} + +struct Refused; +impl TransferError for Refused { + fn refused_before_attempt(&self) -> bool { + false + } +} + +impl BlockIO for Logged { + fn read_block(&self, block: BlockNum, buf: &mut BlockBuf) -> Result<(), DeviceError> { + let at = block.raw() as usize * BLOCK; + buf.0.copy_from_slice(&self.image.borrow()[at..at + BLOCK]); + Ok(()) + } + + fn write_block(&self, block: BlockNum, buf: &BlockBuf) -> Result<(), DeviceError> { + let n = self.writes.replace_with(|n| *n + 1); + if Some(n) == self.refuse { + return Err(DeviceError::classify(&Refused)); + } + self.log.borrow_mut().push(Event::Write(block.raw(), Box::new(buf.0))); + let at = block.raw() as usize * BLOCK; + self.image.borrow_mut()[at..at + BLOCK].copy_from_slice(&buf.0); + Ok(()) + } + + fn block_count(&self) -> u64 { + BLOCKS + } + + fn sync(&self) -> Result<(), DeviceError> { + self.log.borrow_mut().push(Event::Flush); + Ok(()) + } +} + +fn file(i: usize) -> String { + if i.is_multiple_of(4) { format!("{STAGED}/sub/f{i}") } else { format!("{STAGED}/f{i}") } +} + +/// A committed volume: names beside the directory, and the directory with +/// files enough to span leaves, a subdirectory and an empty one. +fn staged() -> Vec { + let mut fs = Formatted::format(VecBlockIO::new(BLOCKS)).expect("format"); + for i in 0..80 { + fs.create(&format!("beside{i}"), format!("beside {i}").as_bytes(), 1).expect("beside"); + } + fs.create(&format!("{STAGED}/"), b"", 1).expect("the directory's own entry"); + fs.create(&format!("{STAGED}/empty/"), b"", 1).expect("an empty directory"); + for i in 0..60 { + fs.create(&file(i), format!("file {i} of the package").as_bytes(), 1).expect("staged"); + } + fs.into_io().expect("commit").into_vec() +} + +/// Every name `keep` accepts, with its bytes, or what would not read. +fn names(fs: &Mounted, keep: &dyn Fn(&str) -> bool) -> Result>, String> { + let listed = fs.list(usize::MAX, keep).map_err(|e| format!("the list: {e:?}"))?; + listed + .into_iter() + .map(|(name, _)| fs.read_file(&name).map(|bytes| (name.clone(), bytes)).map_err(|e| format!("{name}: {e:?}"))) + .collect() +} + +/// The directory's own entry and every name beneath it, by what follows `dir`. +fn tree(fs: &Mounted, dir: &str) -> Result>, String> { + let beneath = format!("{dir}/"); + let all = names(fs, &|n| n.starts_with(&beneath))?; + Ok(all.into_iter().map(|(name, bytes)| (name[dir.len()..].to_string(), bytes)).collect()) +} + +fn beside(fs: &Mounted) -> Result>, String> { + names(fs, &|n| n.starts_with("beside")) +} + +/// The rename: every name beneath the directory, and its own. +fn rename(fs: &mut Mounted) -> Result<(), FsError> { + let moves: Vec<(String, String)> = tree(fs, STAGED) + .expect("the directory reads") + .into_keys() + .map(|rest| (format!("{STAGED}{rest}"), format!("{INSTALLED}{rest}"))) + .collect(); + let pairs: Vec<(&str, &str)> = moves.iter().map(|(a, b)| (a.as_str(), b.as_str())).collect(); + fs.rename_all(&pairs) +} + +/// What a volume must hold: the directory's names and those beside it. +struct Want { + tree: BTreeMap>, + beside: BTreeMap>, +} + +impl Want { + fn of(image: &[u8]) -> Self { + let fs = reopened(image.to_vec()).expect("the staged volume mounts"); + Self { tree: tree(&fs, STAGED).unwrap(), beside: beside(&fs).unwrap() } + } + + /// Where the directory is: `Ok(true)` whole under its new name, + /// `Ok(false)` whole under its old, and otherwise what was found. + fn whole_under_one(&self, fs: &Mounted) -> Result { + let kept = beside(fs)?; + if kept != self.beside { + return Err(format!("{} of {} names beside the directory", kept.len(), self.beside.len())); + } + let (old, new) = (tree(fs, STAGED)?, tree(fs, INSTALLED)?); + match (old.is_empty(), new.is_empty()) { + (false, true) if old == self.tree => Ok(false), + (true, false) if new == self.tree => Ok(true), + _ => Err(format!("{} names under {STAGED} and {} under {INSTALLED}", old.len(), new.len())), + } + } + + fn held(&self, image: Vec) -> Result { + reopened(image).and_then(|fs| self.whole_under_one(&fs)) + } +} + +fn reopened(image: Vec) -> Result, String> { + Mounted::<_, ReadOnly>::open(VecBlockIO::from_vec(image)).map_err(|e| format!("unmountable: {e:?}")) +} + +#[test] +fn a_directory_rename_is_whole_under_one_name_wherever_the_device_stops() { + let image = staged(); + let want = Want::of(&image); + assert!(want.tree.len() > 60, "the directory holds {} names", want.tree.len()); + + let mut fs = Mounted::<_, ReadWrite>::open(Logged::new(&image, None)).expect("mount"); + rename(&mut fs).expect("the rename"); + // Changes after it, which take the blocks the rename gave up if they + // are handed out before the commit lands. + for i in 0..40 { + fs.create(&format!("after{i}"), &[7; BLOCK], 2).expect("a change after it"); + } + fs.sync().expect("the commit"); + assert_eq!(want.held(fs.io().image.borrow().clone()), Ok(true)); + let log = fs.io().log.replace(Vec::new()); + + let writes: Vec = (0..log.len()).filter(|&i| matches!(log[i], Event::Write(..))).collect(); + assert!(writes.len() > 10, "the run wrote {} blocks", writes.len()); + let mut torn = Vec::new(); + for (k, &stop) in writes.iter().enumerate() { + let epoch = log[..stop].iter().rposition(|e| matches!(e, Event::Flush)).map_or(0, |f| f + 1); + for (shape, landed) in [("in order", epoch..=stop), ("alone", stop..=stop)] { + let mut held = image.clone(); + for event in log[..epoch].iter().chain(&log[landed]) { + if let Event::Write(block, data) = event { + let at = *block as usize * BLOCK; + held[at..at + BLOCK].copy_from_slice(&data[..]); + } + } + if let Err(why) = want.held(held) { + torn.push(format!("stopped at write {k} of {}, the epoch's writes {shape}: {why}", writes.len())); + } + } + } + assert!(torn.is_empty(), "{} of {} stops tear the volume:\n{}", torn.len(), 2 * writes.len(), torn.join("\n")); +} + +/// The same with the one write numbered `n` refused, the filesystem alive +/// after it: the rename's answer is what it serves, the device holds the +/// directory whole under one name past the refused sync, and the next sync +/// makes the answer what the device holds. +#[test] +fn a_directory_rename_is_whole_under_one_name_whichever_write_is_refused() { + let image = staged(); + let want = Want::of(&image); + let writes = { + let mut fs = Mounted::<_, ReadWrite>::open(Logged::new(&image, None)).expect("mount"); + rename(&mut fs).expect("the rename"); + fs.sync().expect("the commit"); + let n = *fs.io().writes.borrow(); + n + }; + + let mut torn = Vec::new(); + for refuse in 0..writes { + let mut fs = Mounted::<_, ReadWrite>::open(Logged::new(&image, Some(refuse))).expect("mount"); + let renamed = rename(&mut fs).is_ok(); + let served = want.whole_under_one(&fs); + let first = fs.sync(); + // Changes past a refused sync, which take the blocks of the tree it + // may have committed if they are handed out before the next lands. + let changed = (0..40).try_for_each(|i| fs.create(&format!("between{i}"), &[7; 9000], 2)); + let between = want.held(fs.io().image.borrow().clone()); + let second = fs.sync(); + let held = want.held(fs.io().image.borrow().clone()); + if served != Ok(renamed) || changed.is_err() || between.is_err() || second.is_err() || held != Ok(renamed) { + torn.push(format!( + "write {refuse} of {writes} refused: renamed {renamed}, served {served:?}, \ + the sync {first:?}, the changes past it {changed:?}, left {between:?}, the next {second:?} left {held:?}" + )); + } + } + assert!(torn.is_empty(), "{} of {writes} refusals tear the directory:\n{}", torn.len(), torn.join("\n")); +} diff --git a/bcachefs/tests/integration.rs b/bcachefs/tests/integration.rs index 838c7071256..448a883d9a0 100644 --- a/bcachefs/tests/integration.rs +++ b/bcachefs/tests/integration.rs @@ -837,40 +837,39 @@ fn entries_of_mixed_size_survive_node_splits() { // --- A short allocation is not the allocation that was asked for --- +/// The blocks a file's data leaves free for nodes: the crate's `NODE_RESERVE`. +const NODE_RESERVE: u32 = 16; + /// A volume whose free space is nothing but one-block holes, so the allocator -/// can only ever report a run shorter than a multi-block request. +/// can only ever report a run shorter than a multi-block request: every block +/// a file's data may take, taken in one run, every other one given back, and +/// the run left at the end for nodes taken after. /// -/// Returns the surviving files' single data blocks alongside it: a block the -/// allocator did *not* hand out is the ground truth for "this write landed on -/// somebody else's file". -fn one_block_holes(blocks: u64) -> (Mounted, Vec<(String, u64)>) { +/// Returns the blocks still taken alongside it: a block the allocator did +/// *not* hand out is the ground truth for "this write landed on somebody +/// else's file". +fn one_block_holes(blocks: u64) -> (Mounted, Vec) { let mut fs = Formatted::format(VecBlockIO::new(blocks)).expect("format").mount(); - let mut made = Vec::new(); - for i in 0..blocks { - let name = format!("f{i:03}"); - if fs.create(&name, &vec![0xAAu8; 4096], 0).is_err() { - break; - } - made.push(name); - } - assert!(made.len() > 8, "volume too small to fragment: {} files", made.len()); - for (i, name) in made.iter().enumerate() { - if i % 2 == 0 { - assert!(fs.delete(name).expect("delete"), "delete {name}"); - } + let mut run = Vec::new(); + let mut page = 0; + while fs.resolve_or_alloc_block(&mut run, page).is_ok() { + page += 1; } - let survivors = made - .iter() - .enumerate() - .filter(|(i, _)| i % 2 == 1) - .map(|(_, n)| { - let (extents, _) = fs.file_extents(n).expect("file_extents").expect("a survivor kept its extents"); - assert_eq!(extents.len(), 1); - assert_eq!(extents[0].block_count, 1); - (n.clone(), extents[0].start_block) - }) - .collect(); - (fs, survivors) + let taken: Vec = run.iter().flat_map(|e| e.start_block..e.start_block + e.block_count as u64).collect(); + assert!(taken.len() > 8, "volume too small to fragment: {} blocks", taken.len()); + let holes: Vec = + taken.iter().step_by(2).map(|&b| Extent { start_block: b, block_count: 1, _reserved: 0 }).collect(); + fs.free_extents(&holes).expect("give every other block back"); + let mut tail = vec![Extent { start_block: *taken.last().unwrap(), block_count: 1, _reserved: 0 }]; + fs.resolve_or_alloc_block(&mut tail, NODE_RESERVE).expect("the run left for nodes"); + + let sb = bcachefs::Superblock::read(fs.io()).expect("the superblock"); + let mut bitmap = bcachefs::BlockBuf::zeroed(); + bcachefs::BlockIO::read_block(fs.io(), sb.bitmap_start, &mut bitmap).expect("the bitmap"); + let free = |b: u64| bitmap.0[b as usize / 8] & (1 << (b % 8)) == 0; + let run = (0..blocks - 1).find(|&b| free(b) && free(b + 1)); + assert_eq!(run, None, "a free run longer than one block: this proves nothing"); + (fs, taken.into_iter().skip(1).step_by(2).collect()) } #[test] @@ -891,11 +890,7 @@ fn a_sparse_write_resolves_inside_the_blocks_it_reserved() { reserved.contains(&block), "page 3 resolved to block {block}, outside the extents it recorded: {extents:?}", ); - assert!( - !survivors.iter().any(|(_, b)| *b == block), - "page 3 resolved to block {block}, which belongs to {:?}", - survivors.iter().find(|(_, b)| *b == block).map(|(n, _)| n), - ); + assert!(!survivors.contains(&block), "page 3 resolved to block {block}, which was already taken"); } #[test] @@ -923,9 +918,10 @@ fn every_page_of_a_fragmented_file_owns_a_distinct_block() { // --- Write-path ordering: an operation that fails must not have destroyed // what it was asked to replace, nor kept what it took. --- -/// Blocks a 64-block volume has to give: everything but the superblock, the -/// bitmap, the root node and the backup superblock. -const FREE_BLOCKS_64: usize = 60; +/// One-block files a fresh 64-block volume takes: its 60 free blocks, less +/// the 16 a file's data leaves for copied nodes, less the formatted root, +/// which the first change copies and frees only at a commit. +const ONE_BLOCK_FILES_64: usize = 60 - NODE_RESERVE as usize - 1; fn small_volume() -> Mounted { Formatted::format(VecBlockIO::new(64)).expect("format").mount() @@ -934,7 +930,7 @@ fn small_volume() -> Mounted { /// How many one-block files this volume still has room for. fn one_block_files_that_fit(fs: &mut Mounted) -> usize { let mut fitted = 0; - for i in 0..FREE_BLOCKS_64 * 2 { + for i in 0..ONE_BLOCK_FILES_64 * 2 { let name = format!("p{:03}", i); if fs.create(&name, b"x", 0).is_err() { break; @@ -971,7 +967,7 @@ fn a_write_that_runs_out_of_space_gives_back_what_it_took() { let mut fresh = small_volume(); let untouched = one_block_files_that_fit(&mut fresh); assert_eq!( - untouched, FREE_BLOCKS_64, + untouched, ONE_BLOCK_FILES_64, "the baseline is wrong, so the comparison below proves nothing", ); @@ -989,12 +985,16 @@ fn a_write_that_runs_out_of_space_gives_back_what_it_took() { #[test] fn a_metadata_update_that_cannot_be_reinserted_leaves_the_entry_alone() { let mut fs = small_volume(); - // Every free block spent, and the root leaf filled to within one entry's - // growth of a split — so the reinsert has to split and the split has no - // block to split into. - for i in 0..FREE_BLOCKS_64 { + // Every block a file's data may take spent, then the rest on the nodes + // of names with no data, until one more needs a block the volume does + // not have — so the reinsert has no block to write to. + for i in 0..ONE_BLOCK_FILES_64 { fs.create(&format!("f{:02}", i), b"x", 100 + i as u64).expect("fill"); } + let mut names = 0; + while fs.create(&format!("empty{names}"), b"", 0).is_ok() { + names += 1; + } let (extents, _) = fs.file_extents("f00").expect("file_extents").expect("f00 is on the volume"); let grown: Vec = (0..16).map(|_| extents[0]).collect(); diff --git a/issues/a-crash-leaks-the-blocks-of-its-last-commit.md b/issues/a-crash-leaks-the-blocks-of-its-last-commit.md new file mode 100644 index 00000000000..841c988484e --- /dev/null +++ b/issues/a-crash-leaks-the-blocks-of-its-last-commit.md @@ -0,0 +1,27 @@ +--- +status: open +kind: defect +opened: 2026-10-09 +--- + +# A crash leaks the blocks taken since the last commit + +The DATA volume commits by shadow paging (`bcachefs/src/fs.rs`, the read-write +block's header): no block the last commit's tree reaches is written over, and +the allocator's bitmap is written in place. So a server killed or a machine +stopped between two commits leaves every block taken since the last one marked +used in the bitmap and named by no tree, and the blocks only the older tree +reached, which a commit that landed was about to free, the same. Nothing finds +them again: a mount reads the bitmap as the device holds it. + +Measured with `bcachefs/tests/crash.rs`'s run (a 512-block volume, a +directory of 62 names renamed, one 9000-byte file written, then a commit) +stopped after every write before the first superblock write: the volume mounts +as it was, with 234 blocks marked used where it had 190, and its superblock +counting 322 free where the bitmap holds 278. + +## Exit condition + +A mount after any stop holds a bitmap whose used blocks are exactly those the +committed tree reaches and the superblock and bitmap's own, measured by +`bcachefs/tests/crash.rs`'s stops. diff --git a/issues/a-directory-rename-on-data-is-not-atomic.md b/issues/a-directory-rename-on-data-is-not-atomic.md deleted file mode 100644 index 457e55f9211..00000000000 --- a/issues/a-directory-rename-on-data-is-not-atomic.md +++ /dev/null @@ -1,32 +0,0 @@ ---- -status: open -kind: defect -opened: 2026-10-09 ---- - -# A directory rename on DATA is not atomic - -Fileserver's. `userland/fileserver/src/data.rs`'s `rename` of a directory -renames each entry under it one at a time, because the format keys every file -by its whole path and has no rename of a prefix; its own comment says a kill -in the middle leaves the directory in two halves. A refused write does the -same without a kill: a host test of `DataVolume` (scratch, not committed) made -`home/staged/a` (5 bytes) and `home/staged/z` (240 pages on a fragmented -volume), and renamed `home/staged` to a 300-byte name. `rename` answered -`ResourceExhausted` (the format's `EntryTooLarge { size: 4192, max: 4064 }` -for `z`), and the volume then held `/a` and `home/staged/z`: half the -directory under each name, and the call reported as failed. The format keeps -no journal either (`issues/bcachefs-crate-is-not-bcachefs.md`), so even one -entry's rename is only as whole as the sync that writes it. - -**What it blocks.** The package track's stage-then-commit -(`issues/a-package-is-a-directory-under-apps-and-the-installer-is-a-program.md`): -`/system/bin/pkg` stages `/apps/` privately and commits it in one step, -which a rename that can leave half a package under `/apps` is not. - -## Exit condition - -A directory rename on DATA leaves the directory whole under exactly one of its -names whatever write is refused and wherever the server is killed, measured by -a test that refuses every write of the rename in turn and kills the server at -every one. diff --git a/issues/a-full-data-volume-frees-space-only-at-a-commit.md b/issues/a-full-data-volume-frees-space-only-at-a-commit.md new file mode 100644 index 00000000000..afabb11552f --- /dev/null +++ b/issues/a-full-data-volume-frees-space-only-at-a-commit.md @@ -0,0 +1,28 @@ +--- +status: open +kind: defect +opened: 2026-10-09 +--- + +# A full DATA volume frees space only at a commit + +The DATA volume's commit is shadow paging (`bcachefs/src/fs.rs`), so a block +the last committed tree reaches is not free until the next commit lands +(`BitmapAllocator`'s `pending`): an unlink or a truncate of a file a sync +already wrote gives its blocks back only at the next sync, and every change +copies the nodes it touches to new blocks first. On a volume its files filled, +a write that follows an unlink is refused `ResourceExhausted` until fileserver's +next sync, at most `WRITEBACK` later, and a run of changes between two commits +that touches more committed nodes than `NODE_RESERVE` (16) holds is refused the +same way, a delete among them. + +Measured on a 1024-block `DataVolume` filled with 841 one-page files and +synced (a scratch host test, not committed): a one-page write after an unlink +was refused `ResourceExhausted` and accepted after a sync; then 13 unlinks of +every seventh file were accepted and the 14th refused `ResourceExhausted`. + +## Exit condition + +On a volume its files filled, an unlink followed by a write of the blocks it +freed is accepted with no sync between them, and so is a delete of every file +on it, measured by a host test of `fileserver::data`. diff --git a/userland/fileserver/src/data.rs b/userland/fileserver/src/data.rs index 1fdbf82260c..1147cb5ff92 100644 --- a/userland/fileserver/src/data.rs +++ b/userland/fileserver/src/data.rs @@ -20,11 +20,11 @@ //! shrinks is recorded before the blocks past it are freed, so a failure //! between the two leaks blocks rather than leaving an entry naming freed ones. //! -//! **What a kill costs.** The format updates its btree in place and keeps no -//! journal (`issues/bcachefs-crate-is-not-bcachefs.md`): what the disk -//! holds is what the last sync wrote, and a server that dies inside a sync can -//! leave a node half of that sync's. Nothing but a sync writes a dirty block, -//! unless the cache is holding more than it keeps. +//! **What a kill costs.** A sync is the format's commit (`bcachefs`'s +//! `Mounted::sync`): a server killed anywhere leaves the volume as the last +//! sync that finished, or as the one it was inside, whole either way. A +//! directory's rename is one operation of the format's +//! (`Mounted::rename_all`): it moves whole, or is refused with nothing moved. //! //! Every name a client chose is bounded by the format ([`FsError::NameTooLong`]) //! before it reaches the tree. @@ -687,38 +687,42 @@ impl Volume for DataVolume { if to.starts_with(&format!("{from}/")) { return Err(SyscallError::InvalidArgument); } - // One entry at a time: the format has no rename of a prefix, - // so a kill in the middle leaves the directory in two halves, - // every entry under exactly one of its names. + // Every entry beneath, and the directory's own, in one + // operation of the format's: the directory moves whole or not at all. let prefix = format!("{from}/"); let moving: Vec<(String, Kind)> = self .names - .range(prefix.clone()..) - .take_while(|(n, _)| n.starts_with(&prefix)) + .get_key_value(from) + .into_iter() + .chain(self.names.range(prefix.clone()..).take_while(|(n, _)| n.starts_with(&prefix))) .map(|(n, k)| (n.clone(), *k)) .collect(); - for (name, kind) in &moving { - let moved = format!("{to}/{}", &name[prefix.len()..]); + for (name, _) in &moving { if let Some(node) = self.by_path.get(name).copied() { self.persist(node)?; } - let (on_disk_from, on_disk_to) = match kind { - Kind::Dir => (format!("{name}/"), format!("{moved}/")), - _ => (name.clone(), moved.clone()), - }; - mapped("rename", name, self.fs.rename(&on_disk_from, &on_disk_to))?; - self.names.remove(name); - self.names.insert(moved.clone(), *kind); - if let Some(node) = self.by_path.remove(name) { + } + let renames: Vec<(String, String)> = moving + .iter() + .map(|(name, kind)| { + let moved = format!("{to}{}", &name[from.len()..]); + match kind { + Kind::Dir => (format!("{name}/"), format!("{moved}/")), + _ => (name.clone(), moved), + } + }) + .collect(); + let pairs: Vec<(&str, &str)> = renames.iter().map(|(a, b)| (a.as_str(), b.as_str())).collect(); + mapped("rename", from, self.fs.rename_all(&pairs))?; + for (name, kind) in moving { + let moved = format!("{to}{}", &name[from.len()..]); + self.names.remove(&name); + self.names.insert(moved.clone(), kind); + if let Some(node) = self.by_path.remove(&name) { self.open.get_mut(&node).expect("indexed").path = moved.clone(); self.by_path.insert(moved, node); } } - if self.names.get(from) == Some(&Kind::Dir) { - mapped("rename", from, self.fs.rename(&format!("{from}/"), &format!("{to}/")))?; - self.names.remove(from); - self.names.insert(to.to_string(), Kind::Dir); - } Ok(()) } } @@ -903,22 +907,23 @@ mod tests { again } - /// A full volume of one-page files, every other one then deleted: no two - /// free blocks are adjacent, so every run a file is given is one block. + /// A volume whose free blocks are all apart, so every run a file is given + /// is one block: every block a file's data may take, taken in one run, + /// every other one given back, and the run left at the end for nodes + /// (`bcachefs`'s `NODE_RESERVE`, 16) taken after. fn fragmented() -> DataVolume { let mut v = DataVolume::format(Ram::new(1024), &["home"], clock).unwrap(); - let mut made = 0; - while let Ok(n) = v.open(&format!("home/fill/{made}"), CREATE) { - let written = v.write(n, 0, &[9; BLOCK]); - v.close(n).unwrap(); - if written.is_err() { - break; - } - made += 1; - } - for i in (0..made).step_by(2) { - v.unlink(&format!("home/fill/{i}")).unwrap(); + let mut run = Vec::new(); + let mut page = 0; + while v.fs.resolve_or_alloc_block(&mut run, page).is_ok() { + page += 1; } + let taken: Vec = run.iter().flat_map(|e| e.start_block..e.start_block + e.block_count as u64).collect(); + let holes: Vec = + taken.iter().step_by(2).map(|&b| Extent { start_block: b, block_count: 1, _reserved: 0 }).collect(); + v.fs.free_extents(&holes).unwrap(); + let mut tail = vec![Extent { start_block: *taken.last().unwrap(), block_count: 1, _reserved: 0 }]; + v.fs.resolve_or_alloc_block(&mut tail, 16).unwrap(); v } @@ -1092,4 +1097,239 @@ mod tests { v.close(to).unwrap(); assert_eq!(v.lstat(&to_path).unwrap().size, 22, "`to`'s length reached the volume"); } + + /// A disk whose blocks are a map the test can copy, that keeps the first + /// `keep` block writes and loses every later one — the server killed there + /// — or refuses the one numbered `refuse` and keeps the rest. A request of + /// several blocks counts each, so a kill can land inside one. + type Image = BTreeMap>; + + struct Stops { + blocks: u64, + /// Shared, so a test reads what the disk holds while the server runs. + image: Rc>, + writes: usize, + keep: usize, + refuse: Option, + } + + impl Stops { + fn new(blocks: u64) -> Self { + Self { blocks, image: Rc::default(), writes: 0, keep: usize::MAX, refuse: None } + } + + fn from(image: &Image, blocks: u64, keep: usize, refuse: Option) -> Self { + Self { blocks, image: Rc::new(image.clone().into()), writes: 0, keep, refuse } + } + } + + impl Disk for Stops { + fn blocks(&self) -> u64 { + self.blocks + } + + fn read(&mut self, first: u64, out: &mut [u8]) -> Result<(), DiskError> { + for (i, chunk) in out.chunks_exact_mut(BLOCK).enumerate() { + match self.image.borrow().get(&(first + i as u64)) { + Some(block) => chunk.copy_from_slice(&block[..]), + None => chunk.fill(0), + } + } + Ok(()) + } + + fn write(&mut self, first: u64, data: &[u8]) -> Result<(), DiskError> { + for (i, chunk) in data.chunks_exact(BLOCK).enumerate() { + let n = self.writes; + self.writes += 1; + if Some(n) == self.refuse { + return Err(DiskError::Device); + } + if n < self.keep { + self.image.borrow_mut().insert(first + i as u64, Box::new(chunk.try_into().expect("a block"))); + } + } + Ok(()) + } + + fn flush(&mut self) -> Result<(), DiskError> { + Ok(()) + } + } + + fn take(v: DataVolume) -> Stops { + let DataVolume { fs, cache, .. } = v; + drop(fs); + Rc::try_unwrap(cache).ok().expect("one owner").into_disk() + } + + /// Every file under `dir` with its bytes, by its path beneath `dir`; a + /// directory with nothing under it is listed with a trailing `/`. + fn tree(v: &mut DataVolume, dir: &str) -> Option>> { + let listed = v.list(dir).ok()?; + let mut out = BTreeMap::new(); + if listed.is_empty() { + out.insert("/".to_string(), Vec::new()); + } + for (name, meta) in listed { + let path = join(dir, &name); + match meta.kind { + Kind::Dir => { + for (below, bytes) in tree(v, &path)? { + let below = if below == "/" { format!("{name}/") } else { format!("{name}/{below}") }; + out.insert(below, bytes); + } + } + _ => { + let n = v.open(&path, PLAIN).ok()?; + let mut bytes = vec![0u8; meta.size as usize]; + let read = v.read(n, 0, &mut Buf(&mut bytes)); + v.close(n).ok()?; + (read.ok()? == bytes.len()).then_some(())?; + out.insert(name, bytes); + } + } + } + Some(out) + } + + const STAGED: &str = "home/staged"; + const INSTALLED: &str = "apps/pkg"; + + /// Where the directory is: `Ok(true)` whole under its new name and absent + /// under its old, `Ok(false)` the reverse, and otherwise what was found. + fn whole_under_one(v: &mut DataVolume, want: &BTreeMap>) -> Result { + let (old, new) = (tree(v, STAGED), tree(v, INSTALLED)); + match (&old, &new) { + (Some(old), None) if old == want => Ok(false), + (None, Some(new)) if new == want => Ok(true), + _ => Err(format!( + "{} names under {STAGED} and {} under {INSTALLED}", + old.as_ref().map_or(0, |t| t.len()), + new.as_ref().map_or(0, |t| t.len()) + )), + } + } + + /// A staged package: files enough to fill several leaves, a subdirectory, + /// an empty one, and names beside it the rename must not move; synced. + fn staged() -> (Image, BTreeMap>) { + let mut v = DataVolume::format(Stops::new(1024), &["home", "apps"], clock).unwrap(); + for i in 0..80 { + let n = v.open(&format!("home/beside{i}"), CREATE).unwrap(); + v.write(n, 0, format!("beside {i}").as_bytes()).unwrap(); + v.close(n).unwrap(); + } + v.mkdir(STAGED).unwrap(); + v.mkdir(&format!("{STAGED}/empty")).unwrap(); + for i in 0..60 { + let path = if i % 4 == 0 { format!("{STAGED}/sub/f{i}") } else { format!("{STAGED}/f{i}") }; + let n = v.open(&path, CREATE).unwrap(); + v.write(n, 0, format!("file {i} of the package").as_bytes()).unwrap(); + v.close(n).unwrap(); + } + assert_eq!(v.sync(), Ok(Vec::new())); + let want = tree(&mut v, STAGED).unwrap(); + (take(v).image.take(), want) + } + + fn mounted(disk: Stops) -> Result, String> { + match DataVolume::probe(disk, &["home", "apps"], clock) { + Probed::Mounted(v) => Ok(v), + Probed::Unmountable(why) => Err(format!("unmountable: {why}")), + Probed::Foreign => Err("foreign".into()), + } + } + + /// The rename and the sync that makes it durable, against a disk that + /// stops at every block write the two make in turn: what the disk then + /// holds mounts, and names the directory whole under exactly one name. + #[test] + fn a_directory_rename_is_whole_under_one_name_wherever_the_server_is_killed() { + let (image, want) = staged(); + let blocks = 1024; + let mut v = mounted(Stops::from(&image, blocks, usize::MAX, None)).unwrap(); + v.rename(STAGED, INSTALLED).unwrap(); + assert_eq!(v.sync(), Ok(Vec::new())); + assert_eq!(whole_under_one(&mut v, &want), Ok(true)); + let writes = take(v).writes; + assert!(writes > 4, "the rename and its sync wrote {writes} blocks"); + + let mut torn = Vec::new(); + for keep in 0..=writes { + let mut v = mounted(Stops::from(&image, blocks, keep, None)).unwrap(); + let _ = v.rename(STAGED, INSTALLED); + let _ = v.sync(); + let verdict = mounted(take(v)).and_then(|mut again| whole_under_one(&mut again, &want)); + if let Err(why) = verdict { + torn.push(format!("killed after {keep} of {writes} writes: {why}")); + } + } + assert!(torn.is_empty(), "{} of {} kill points tear the directory:\n{}", torn.len(), writes + 1, torn.join("\n")); + } + + /// The same, with the one write numbered `n` refused and the server + /// alive: what it answers and what it then serves agree, the disk holds + /// the directory whole under one name past the refused sync, and the next + /// sync makes the answer what the disk holds. + #[test] + fn a_directory_rename_is_whole_under_one_name_whichever_write_is_refused() { + let (image, want) = staged(); + let blocks = 1024; + let mut v = mounted(Stops::from(&image, blocks, usize::MAX, None)).unwrap(); + v.rename(STAGED, INSTALLED).unwrap(); + assert_eq!(v.sync(), Ok(Vec::new())); + let writes = take(v).writes; + let held = |on_disk: &Image| { + mounted(Stops::from(on_disk, blocks, usize::MAX, None)).and_then(|mut v| whole_under_one(&mut v, &want)) + }; + + let mut torn = Vec::new(); + for refuse in 0..writes { + let disk = Stops::from(&image, blocks, usize::MAX, Some(refuse)); + let on_disk = Rc::clone(&disk.image); + let mut v = mounted(disk).unwrap(); + let renamed = v.rename(STAGED, INSTALLED).is_ok(); + let first = v.sync(); + let between = held(&on_disk.borrow()); + let served = whole_under_one(&mut v, &want); + let second = v.sync(); + let after = held(&on_disk.borrow()); + if served != Ok(renamed) || between.is_err() || second != Ok(Vec::new()) || after != Ok(renamed) { + torn.push(format!( + "write {refuse} of {writes} refused: answered {renamed}, served {served:?}, \ + the sync {first:?} left {between:?}, the next {second:?} left {after:?}" + )); + } + } + assert!(torn.is_empty(), "{} of {writes} refusals tear the directory:\n{}", torn.len(), torn.join("\n")); + } + + /// A rename the format refuses part-way — `z`'s extents fit its own name + /// and not the long one — leaves the directory whole under its old name. + #[test] + fn a_directory_rename_the_format_refuses_part_way_moves_nothing() { + let mut v = fragmented(); + let a = v.open("home/staged/a", CREATE).unwrap(); + v.write(a, 0, b"small").unwrap(); + v.close(a).unwrap(); + let z = v.open("home/staged/z", CREATE).unwrap(); + for page in 0..240u64 { + v.write(z, page * BLOCK as u64, &[1; BLOCK]).unwrap(); + } + v.close(z).unwrap(); + let to = format!("home/{}", "t".repeat(300)); + + assert_eq!(v.rename("home/staged", &to), Err(SyscallError::ResourceExhausted)); + + let names = |v: &mut DataVolume, dir: &str| -> Vec { + v.list(dir).map(|l| l.into_iter().map(|(n, _)| n).collect()).unwrap_or_default() + }; + assert_eq!(names(&mut v, "home/staged"), ["a", "z"]); + assert_eq!(v.lstat(&to), Err(SyscallError::NotFound)); + assert_eq!(v.sync(), Ok(Vec::new())); + let mut v = remount(v); + assert_eq!(names(&mut v, "home/staged"), ["a", "z"]); + assert_eq!(v.lstat(&to), Err(SyscallError::NotFound)); + } } From 0c9c05c6787bcecf7e64302c6789b8dc3f7012ba Mon Sep 17 00:00:00 2001 From: japabu Date: Fri, 9 Oct 2026 20:18:50 +0200 Subject: [PATCH 3/5] A full DATA volume still shrinks, a mount counts its bitmap, and the crash model tears sectors and mounts from the backup The review of round 1 found the volume could wedge: every delete and truncate copies its path through the allocator, NODE_RESERVE was held only against a file's data, so names with no data (an open(CREATE), a mkdir) took the reserve down to nothing and, once a sync had freed what was pending, no delete ever succeeded again. The reserve is now held against every allocation (`Reserve::Keep`) but those of an operation that leaves the tree no larger (`Reserve::Draw`): a delete, and an update whose entry grows no longer, which splits no node. A commit gives back whatever such an operation drew, so the reserve is whole after every sync and no state reachable by changes refuses a delete for longer than the next sync. `a_volume_full_of_empty_names_still_deletes_and_frees` is red on the round-1 source (the shrink of f00: NoSpace) and green here. The second road to the same state: after a stop between two commits the superblock's free count names blocks the bitmap holds taken, so data alone could take the real reserve. A mount now counts the bitmap's free bits (`BitmapAllocator::open`); `a_mount_after_a_stop_counts_the_blocks_its_bitmap_holds` is red on the round-1 source and under a mutation that takes the superblock's count. The crash model (`bcachefs/tests/crash.rs`) now lands the stop's write on a block the layout writes in place (a superblock copy, the bitmap) torn at every 512-byte sector boundary, first sectors or last, and checks every state again with block 0 lost, mounting from the backup. Under it the review's mutation (one flush after both superblock copies, not one between them) stays green, and has to: a superblock's bytes lie in its first sector, so a device that writes a sector whole lands each copy old or new, and no order between the two copies changes what a mount finds. The flush between them is deleted, and `Superblock::write` is main's again. Two mutations show the new shapes see what the old did not: the root's number written past the first sector (red only through a torn backup with block 0 lost), and the backup written before the flush of the nodes it names (red only with block 0 lost). `BitmapAllocator::give` gives up the rest of a run past a bit the device would not clear, where it stopped; the block it could not free is a leak, recorded in issues/a-crash-leaks-the-blocks-of-its-last-commit.md with the frees `succeed` and `fail` drop. The read-write block's contract is narrowed to names, lengths and extents: a page written over in place is not shadowed, filed as issues/a-write-over-a-files-committed-page-is-not-shadowed.md. The reserve's limits, depth among them, are in issues/a-full-data-volume-frees-space-only-at-a-commit.md. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_017cSFvbD35xJ2kGANVdm23C --- bcachefs/src/alloc_bitmap.rs | 88 +++++++++++++------ bcachefs/src/btree.rs | 2 +- bcachefs/src/fs.rs | 68 ++++++++------ bcachefs/src/superblock.rs | 13 +-- bcachefs/tests/crash.rs | 61 +++++++++---- bcachefs/tests/integration.rs | 88 +++++++++++++++++-- ...ash-leaks-the-blocks-of-its-last-commit.md | 19 ++-- ...ata-volume-frees-space-only-at-a-commit.md | 19 ++-- ...-a-files-committed-page-is-not-shadowed.md | 26 ++++++ userland/fileserver/src/data.rs | 12 +-- 10 files changed, 291 insertions(+), 105 deletions(-) create mode 100644 issues/a-write-over-a-files-committed-page-is-not-shadowed.md diff --git a/bcachefs/src/alloc_bitmap.rs b/bcachefs/src/alloc_bitmap.rs index 844e9b3243c..55d1d91c46c 100644 --- a/bcachefs/src/alloc_bitmap.rs +++ b/bcachefs/src/alloc_bitmap.rs @@ -27,13 +27,14 @@ pub struct Run { /// The bitmap is stored on disk starting at `bitmap_start` and spanning /// `bitmap_blocks` blocks. Each bit represents one block: 1 = used, 0 = free. /// -/// **With `shadow` on, no block the last committed tree reaches is written -/// over or handed out again before the next commit lands** (`fs::commit`): a -/// btree node is written in place only when the operation in progress took -/// its block ([`Self::in_place`]), and a block given up is free at once only -/// if no commit has named it since it was taken (`fresh`); any other waits in -/// `pending` for the commit. An operation ([`Self::begin`]) gives up its -/// blocks only when it succeeds, and refused gives back every one it took. +/// **With `shadow` on, no node the last committed tree reaches is written +/// over, and no block it reaches is handed out again, before the next commit +/// lands** (`Mounted::sync`): a btree node is written in place only when the +/// operation in progress took its block ([`Self::in_place`]), and a block +/// given up is free at once only if no commit has named it since it was taken +/// (`fresh`); any other waits in `pending` for the commit. An operation +/// ([`Self::begin`]) gives up its blocks only when it succeeds, and refused +/// gives back every one it took. /// mkfs runs with `shadow` off: nothing it writes is committed until it ends. pub struct BitmapAllocator { pub bitmap_start: BlockNum, @@ -49,15 +50,22 @@ pub struct BitmapAllocator { op: Option, } -/// Blocks a file's data never takes while `shadow` is on, so a volume its -/// files filled can still copy the nodes a delete changes: a rename in a tree -/// four levels deep copies four, splits four and adds a root for its insert, -/// and copies four more for its delete. +/// Blocks only an operation that [`Reserve::Draw`]s may take while `shadow` +/// is on, so a full volume still copies the nodes a delete or a shrink +/// changes, as many as sixteen between two commits. pub const NODE_RESERVE: u64 = 16; +/// Whether an operation may take the blocks [`NODE_RESERVE`] keeps: only one +/// that leaves the tree no larger, so a commit gives back all it drew. +#[derive(Clone, Copy, PartialEq, Eq)] +pub enum Reserve { + Keep, + Draw, +} + /// The operation in progress: what it took, and what it gives up if it succeeds. -#[derive(Default)] struct Op { + reserve: Reserve, taken: Runs, given: Vec<(u64, u32)>, } @@ -93,25 +101,46 @@ impl Runs { } impl BitmapAllocator { - /// The allocator of a mounted volume, as its superblock left it. - pub fn open(sb: &Superblock) -> Self { - Self { + /// The allocator of a mounted volume, its free count read off the bitmap: + /// the superblock's is the last commit's, and a stop after it leaves the + /// bitmap holding fewer. + pub fn open(io: &dyn BlockIO, sb: &Superblock) -> Result { + let mut free_blocks = 0; + let mut buf = BlockBuf::zeroed(); + for i in 0..sb.block_count.div_ceil(BITS_PER_BLOCK) { + io.read(BlockNum::new(sb.bitmap_start.raw() + i), &mut buf)?; + let bits = (sb.block_count - i * BITS_PER_BLOCK).min(BITS_PER_BLOCK); + let (whole, rest) = buf.0[..bits.div_ceil(8) as usize].split_at((bits / 8) as usize); + free_blocks += whole.iter().map(|b| b.count_zeros() as u64).sum::(); + free_blocks += rest.first().map_or(0, |b| (!b & ((1u8 << (bits % 8)) - 1)).count_ones() as u64); + } + Ok(Self { bitmap_start: sb.bitmap_start, bitmap_blocks: sb.bitmap_blocks, total_blocks: sb.block_count, - free_blocks: sb.free_blocks, + free_blocks, next_alloc: sb.next_alloc, shadow: true, fresh: Runs::default(), pending: Vec::new(), op: None, - } + }) } /// Begin one operation on the tree, which [`Self::succeed`] or /// [`Self::fail`] ends. - pub fn begin(&mut self) { - assert!(self.op.replace(Op::default()).is_none(), "an operation began inside another"); + pub fn begin(&mut self, reserve: Reserve) { + let op = Op { reserve, taken: Runs::default(), given: Vec::new() }; + assert!(self.op.replace(op).is_none(), "an operation began inside another"); + } + + /// What an allocation may take now: every free block, but for those + /// [`NODE_RESERVE`] keeps from all but an operation that draws on it. + fn spare(&self) -> u64 { + match !self.shadow || self.op.as_ref().is_some_and(|op| op.reserve == Reserve::Draw) { + true => self.free_blocks, + false => self.free_blocks.saturating_sub(NODE_RESERVE), + } } /// Whether a node at `block` may be written where it is. @@ -167,11 +196,15 @@ impl BitmapAllocator { } /// Give up `count` blocks from `start`: free now if no commit has named - /// them, and otherwise once the next one lands. + /// them, and otherwise once the next one lands. A block whose bit the + /// device would not clear is leaked, and the rest are given up still. fn give(&mut self, io: &dyn BlockIO, start: u64, count: u32) -> Result<(), FsError> { + let mut refused = Ok(()); for block in start..start + count as u64 { if !self.shadow || self.fresh.remove(block) { - self.set_free(io, BlockNum::new(block))?; + if let Err(e) = self.set_free(io, BlockNum::new(block)) { + refused = refused.and(Err(e)); + } } else { match self.pending.last_mut() { Some((at, n)) if *at + *n as u64 == block => *n += 1, @@ -179,7 +212,7 @@ impl BitmapAllocator { } } } - Ok(()) + refused } /// Where a block's bit lives, and which bit of that byte it is. @@ -242,15 +275,12 @@ impl BitmapAllocator { } /// Reserve as much of `wanted` as one contiguous run can cover, scanning - /// from block `from`: a file's data, which leaves [`NODE_RESERVE`] free. + /// from block `from`. /// /// The run is never empty and may be shorter than asked for, so every /// caller has to loop or has to be wrong. pub fn alloc_up_to(&mut self, io: &dyn BlockIO, from: u64, wanted: u32) -> Result { - let spare = match self.shadow { - true => self.free_blocks.saturating_sub(NODE_RESERVE), - false => self.free_blocks, - }; + let spare = self.spare(); if spare == 0 { return Err(FsError::NoSpace { requested: wanted, available: 0 }); } @@ -263,6 +293,10 @@ impl BitmapAllocator { /// Reserve all of `count` or nothing, for callers that cannot place a /// short run. Nothing is marked used unless the whole run is there. pub fn alloc_exact(&mut self, io: &dyn BlockIO, count: u32) -> Result { + let spare = self.spare(); + if spare < count as u64 { + return Err(FsError::NoSpace { requested: count, available: spare }); + } let (start, len) = self.longest_free_run(io, self.next_alloc, count)?; if len < count { return Err(FsError::NoSpace { diff --git a/bcachefs/src/btree.rs b/bcachefs/src/btree.rs index bafb13be04b..5a616feb14d 100644 --- a/bcachefs/src/btree.rs +++ b/bcachefs/src/btree.rs @@ -833,7 +833,7 @@ mod tests { let mut middle: Vec = (0..40).map(|_| entry(72)).collect(); middle.insert(20, entry(MAX_ENTRY_SIZE - KEY_HEADER_SIZE)); assert_eq!(pack(middle.clone()).len(), 3, "two siblings, and a block for one"); - alloc.begin(); + alloc.begin(crate::alloc_bitmap::Reserve::Keep); assert!(matches!( split_node(&io, &mut alloc, leaf, Node::Leaf(middle)), Err(FsError::NoSpace { .. }), diff --git a/bcachefs/src/fs.rs b/bcachefs/src/fs.rs index 161112f8879..7793060e745 100644 --- a/bcachefs/src/fs.rs +++ b/bcachefs/src/fs.rs @@ -4,7 +4,7 @@ use alloc::vec::Vec; use core::marker::PhantomData; use core::ops::ControlFlow; -use crate::alloc_bitmap::BitmapAllocator; +use crate::alloc_bitmap::{BitmapAllocator, Reserve}; use crate::block_io::{BlockBuf, BlockNum, BlockIO, BlockIOExt, DeviceError, BLOCK_SIZE}; use crate::btree::{self, Entry, Key, KeyType, Node}; use crate::superblock::{FsUuid, Superblock}; @@ -596,7 +596,7 @@ impl Mounted { /// Open an existing filesystem from disk. pub fn open(io: IO) -> Result, FsError> { let sb = Superblock::read(&io)?; - let alloc = BitmapAllocator::open(&sb); + let alloc = BitmapAllocator::open(&io, &sb)?; Ok(Mounted { io, sb, @@ -753,15 +753,26 @@ impl Mounted { /// **Every change is one operation and every sync one commit.** An operation /// ([`Mounted::atomic`]) takes effect whole or, refused anywhere, leaves the -/// tree and the allocator as they were; it never writes over a block the last +/// tree and the allocator as they were; it never writes over a node the last /// commit's tree reaches, but to new blocks up to a new root. A commit -/// ([`Mounted::sync`]) is the primary superblock naming that root landing on -/// the device, after everything the root reaches, and before the backup copy: -/// killed anywhere, the device holds the last commit's tree whole or the new -/// one whole, a torn primary failing its checksum for the backup, which is the -/// last commit still. What a kill costs is the blocks taken since the last -/// commit and those only the old tree reached: marked used, and named by no -/// tree, until a sweep (`issues/a-crash-leaks-the-blocks-of-its-last-commit.md`). +/// ([`Mounted::sync`]) is both superblock copies naming that root landing on +/// the device after everything the root reaches. A superblock's bytes lie in +/// its first 512-byte sector, which a device writes whole, so killed anywhere +/// each copy names the last commit's tree whole or the new one whole. +/// +/// That is true of names, and of each entry's length and extents; not of a +/// file's bytes. A page written over where the file already holds it +/// ([`Mounted::resolve_or_alloc_block`]) is written in place, so a kill can +/// leave a file's committed blocks holding pages from both sides of a write +/// (`issues/a-write-over-a-files-committed-page-is-not-shadowed.md`). What a +/// kill costs besides is the blocks taken since the last commit and those only +/// the old tree reached: marked used, and named by no tree, until a sweep +/// (`issues/a-crash-leaks-the-blocks-of-its-last-commit.md`). +/// +/// A delete, and an update whose entry grows no longer, may take the blocks +/// [`crate::alloc_bitmap::NODE_RESERVE`] keeps from every other change, so a +/// full volume still shrinks, by as many committed nodes as the reserve holds +/// between two commits (`issues/a-full-data-volume-frees-space-only-at-a-commit.md`). impl Mounted { /// Create a file, replacing whatever answered to `name`. pub fn create(&mut self, name: &str, data: &[u8], mtime: u64) -> Result<(), FsError> { @@ -775,9 +786,13 @@ impl Mounted { /// Run `change` as one operation: whole, or refused with the root and /// every block as they were. - fn atomic(&mut self, change: impl FnOnce(&mut Self) -> Result) -> Result { + fn atomic( + &mut self, + reserve: Reserve, + change: impl FnOnce(&mut Self) -> Result, + ) -> Result { let root = self.sb.root_node; - self.alloc.begin(); + self.alloc.begin(reserve); match change(self) { Ok(done) => { self.alloc.succeed(&self.io); @@ -815,7 +830,7 @@ impl Mounted { return Err(FsError::NameTooLong { len: name.len(), max: MAX_NAME_LEN }); } - self.atomic(|fs| { + self.atomic(Reserve::Keep, |fs| { let displaced = match fs.find_by_name(name)? { Some((key, value)) => Some((key, fs.decode(&value)?.extents().to_vec())), None => None, @@ -867,7 +882,7 @@ impl Mounted { /// types, where the old shape fell through from File to Symlink after a /// non-matching removal and could take two entries out in one call. pub fn delete(&mut self, name: &str) -> Result { - self.atomic(|fs| { + self.atomic(Reserve::Draw, |fs| { let Some((key, value)) = fs.find_by_name(name)? else { return Ok(false) }; let extents = fs.decode(&value)?.extents().to_vec(); @@ -892,10 +907,8 @@ impl Mounted { // From the first superblock write on, this commit may be what a mount // finds, even where the write is refused. self.alloc.seal(); - for copy in self.sb.copies() { - self.sb.write_at(&self.io, copy)?; - self.io.flush()?; - } + self.sb.write(&self.io)?; + self.io.flush()?; self.alloc.committed(&self.io) } @@ -907,7 +920,7 @@ impl Mounted { /// Rename every `(old, new)` pair in turn, as one operation: a directory /// is the names beneath it, and moves whole or not at all. pub fn rename_all(&mut self, renames: &[(&str, &str)]) -> Result<(), FsError> { - self.atomic(|fs| renames.iter().try_for_each(|&(old, new)| fs.rename_one(old, new))) + self.atomic(Reserve::Keep, |fs| renames.iter().try_for_each(|&(old, new)| fs.rename_one(old, new))) } /// The new entry goes in before the old one comes out. What that ordering @@ -966,14 +979,17 @@ impl Mounted { size: u64, mtime: u64, ) -> Result<(), FsError> { - self.atomic(|fs| { - let (old_key, old_value) = fs.find_by_name(name)? - .ok_or(FsError::NotFound)?; - let leaf = fs.decode(&old_value)?; - - let new_value = encode_leaf_value(old_key.key_type, leaf.name(), size, mtime, new_extents); - let new_entry = Entry { key: old_key, value: new_value }; + let (old_key, old_value) = self.find_by_name(name)?.ok_or(FsError::NotFound)?; + let leaf = self.decode(&old_value)?; + let new_value = encode_leaf_value(old_key.key_type, leaf.name(), size, mtime, new_extents); + // An entry no longer than the one it replaces splits no node. + let reserve = match new_value.len() <= old_value.len() { + true => Reserve::Draw, + false => Reserve::Keep, + }; + let new_entry = Entry { key: old_key, value: new_value }; + self.atomic(reserve, |fs| { // No delete first: the key is unchanged and `btree::insert` replaces // on an equal key. Blocks the caller drops from the extent list are // the caller's to free, through [`Self::free_extents`], after this diff --git a/bcachefs/src/superblock.rs b/bcachefs/src/superblock.rs index d24ec660b4c..5d78b8e3239 100644 --- a/bcachefs/src/superblock.rs +++ b/bcachefs/src/superblock.rs @@ -266,19 +266,10 @@ impl Superblock { /// Write superblock to both block 0 and the backup at the last block. pub fn write(&self, io: &dyn BlockIO) -> Result<(), FsError> { - self.copies().try_for_each(|copy| self.write_at(io, copy)) - } - - /// Block 0, which [`Self::read`] tries first, then the backup. - pub fn copies(&self) -> impl Iterator { - [BlockNum::new(0), BlockNum::new(self.block_count - 1)].into_iter() - } - - /// Write this superblock to one of its [`Self::copies`]. - pub fn write_at(&self, io: &dyn BlockIO, copy: BlockNum) -> Result<(), FsError> { let mut buf = BlockBuf::zeroed(); self.write_to(&mut buf); - io.write(copy, &buf) + io.write(BlockNum::new(0), &buf)?; + io.write(BlockNum::new(self.block_count - 1), &buf) } } diff --git a/bcachefs/tests/crash.rs b/bcachefs/tests/crash.rs index 81459971950..357794bc8f1 100644 --- a/bcachefs/tests/crash.rs +++ b/bcachefs/tests/crash.rs @@ -1,20 +1,23 @@ //! A directory rename, changes after it and the sync that commits them, //! stopped at every block write: the crash-point model of the commit. //! -//! **A device keeps what a flush covered and any part of what it did not.** -//! The writes since the last flush may land in any order, so each stop is -//! tried twice: every write of the open epoch up to the stop landed, and the -//! stop's write alone did. What the device then holds must mount, keep every -//! name beside the directory, and name the directory whole under exactly one -//! of its two names. +//! **A device keeps what a flush covered and any part of what it did not, a +//! 512-byte sector at a time.** The writes since the last flush may land in +//! any order, so each stop is tried with every write of the open epoch before +//! it landed and with none of them; and the stop's write itself whole, or +//! torn at a sector boundary: its first sectors new and the rest old, or its +//! last. What the device then holds must mount, keep every name beside the +//! directory, and name the directory whole under exactly one of its two +//! names; and so must it with block 0 lost, from the backup superblock. use std::cell::RefCell; use std::collections::BTreeMap; -use bcachefs::{BlockBuf, BlockIO, BlockNum, DeviceError, Formatted, FsError, Mounted, ReadOnly, ReadWrite, TransferError, VecBlockIO}; +use bcachefs::{BlockBuf, BlockIO, BlockNum, DeviceError, Formatted, FsError, Mounted, ReadOnly, ReadWrite, Superblock, TransferError, VecBlockIO}; const BLOCKS: u64 = 512; const BLOCK: usize = 4096; +const SECTOR: usize = 512; const STAGED: &str = "staged"; const INSTALLED: &str = "apps/pkg"; @@ -180,23 +183,51 @@ fn a_directory_rename_is_whole_under_one_name_wherever_the_device_stops() { let writes: Vec = (0..log.len()).filter(|&i| matches!(log[i], Event::Write(..))).collect(); assert!(writes.len() > 10, "the run wrote {} blocks", writes.len()); - let mut torn = Vec::new(); + let sb = Superblock::read(&VecBlockIO::from_vec(image.clone())).expect("the staged superblock"); + let in_place: Vec = [0, BLOCKS - 1] + .into_iter() + .chain(sb.bitmap_start.raw()..sb.bitmap_start.raw() + sb.bitmap_blocks) + .collect(); + let sectors = BLOCK / SECTOR; + let parts: Vec<(String, std::ops::Range)> = std::iter::once(("whole".to_string(), 0..sectors)) + .chain((1..sectors).flat_map(|k| [(format!("its first {k} sectors"), 0..k), (format!("its last {k} sectors"), sectors - k..sectors)])) + .collect(); + let (mut torn, mut tried) = (Vec::new(), 0); for (k, &stop) in writes.iter().enumerate() { let epoch = log[..stop].iter().rposition(|e| matches!(e, Event::Flush)).map_or(0, |f| f + 1); - for (shape, landed) in [("in order", epoch..=stop), ("alone", stop..=stop)] { - let mut held = image.clone(); - for event in log[..epoch].iter().chain(&log[landed]) { + let Event::Write(block, data) = &log[stop] else { unreachable!("a stop is a write") }; + for (shape, before) in [("the epoch's writes before it landed", epoch..stop), ("alone", stop..stop)] { + let mut landed = image.clone(); + for event in log[..epoch].iter().chain(&log[before]) { if let Event::Write(block, data) = event { let at = *block as usize * BLOCK; - held[at..at + BLOCK].copy_from_slice(&data[..]); + landed[at..at + BLOCK].copy_from_slice(&data[..]); } } - if let Err(why) = want.held(held) { - torn.push(format!("stopped at write {k} of {}, the epoch's writes {shape}: {why}", writes.len())); + let at = *block as usize * BLOCK; + let old = landed[at..at + BLOCK].to_vec(); + for (part, range) in &parts { + let mut held = landed.clone(); + held[at + range.start * SECTOR..at + range.end * SECTOR] + .copy_from_slice(&data[range.start * SECTOR..range.end * SECTOR]); + // A tear only of a block the layout writes in place, and only + // where it leaves bytes neither the old nor the new did. + let new = &held[at..at + BLOCK]; + if range.len() < sectors && (!in_place.contains(block) || new == &old[..] || new == &data[..]) { + continue; + } + let mut lost = held.clone(); + lost[..BLOCK].fill(0); + for (copy, image) in [("", held), (", block 0 lost", lost)] { + tried += 1; + if let Err(why) = want.held(image) { + torn.push(format!("stopped at write {k} of {} (block {block}), {shape}, {part}{copy}: {why}", writes.len())); + } + } } } } - assert!(torn.is_empty(), "{} of {} stops tear the volume:\n{}", torn.len(), 2 * writes.len(), torn.join("\n")); + assert!(torn.is_empty(), "{} of {tried} stops tear the volume:\n{}", torn.len(), torn.join("\n")); } /// The same with the one write numbered `n` refused, the filesystem alive diff --git a/bcachefs/tests/integration.rs b/bcachefs/tests/integration.rs index 448a883d9a0..88963ef7a5b 100644 --- a/bcachefs/tests/integration.rs +++ b/bcachefs/tests/integration.rs @@ -707,7 +707,7 @@ fn a_rename_carries_the_file_to_the_new_name() { fn a_renamed_file_keeps_every_extent_it_had() { // A file in one run survives a rename that reallocates as readily as one // that carries the extents. Discontiguous runs do not. - let (mut fs, _) = one_block_holes(64); + let (mut fs, _) = one_block_holes(128); let data: Vec = (0..4 * 4096 + 11).map(|i| (i % 251) as u8).collect(); fs.create("frag.bin", &data, 3).expect("create a fragmented file"); let (before, _) = fs.file_extents("frag.bin").expect("file_extents").expect("extents"); @@ -837,13 +837,13 @@ fn entries_of_mixed_size_survive_node_splits() { // --- A short allocation is not the allocation that was asked for --- -/// The blocks a file's data leaves free for nodes: the crate's `NODE_RESERVE`. +/// The blocks every change but a shrink leaves free: the crate's `NODE_RESERVE`. const NODE_RESERVE: u32 = 16; /// A volume whose free space is nothing but one-block holes, so the allocator /// can only ever report a run shorter than a multi-block request: every block /// a file's data may take, taken in one run, every other one given back, and -/// the run left at the end for nodes taken after. +/// the run the reserve kept taken after. /// /// Returns the blocks still taken alongside it: a block the allocator did /// *not* hand out is the ground truth for "this write landed on somebody @@ -919,9 +919,10 @@ fn every_page_of_a_fragmented_file_owns_a_distinct_block() { // what it was asked to replace, nor kept what it took. --- /// One-block files a fresh 64-block volume takes: its 60 free blocks, less -/// the 16 a file's data leaves for copied nodes, less the formatted root, -/// which the first change copies and frees only at a commit. -const ONE_BLOCK_FILES_64: usize = 60 - NODE_RESERVE as usize - 1; +/// the 16 a create leaves free, less the formatted root, which the first +/// change copies and frees only at a commit, less the leaf's copy the last +/// create holds beside the one it replaces. +const ONE_BLOCK_FILES_64: usize = 60 - NODE_RESERVE as usize - 2; fn small_volume() -> Mounted { Formatted::format(VecBlockIO::new(64)).expect("format").mount() @@ -1014,6 +1015,81 @@ fn a_metadata_update_that_cannot_be_reinserted_leaves_the_entry_alone() { assert_eq!(fs.file_mtime("f00").expect("mtime").unwrap_or(0), 100, "the failed update left its mtime behind"); } +/// Fill `fs` with names enough to span leaves, then with one-block files, +/// then with names with none until a create right after a commit is refused: +/// how many files it took. +fn filled_to_the_last_name(fs: &mut Mounted) -> usize { + for i in 0..150 { + fs.create(&format!("spread{i:03}"), b"", 0).expect("a name with no data"); + if i % 50 == 49 { + fs.sync().expect("a commit"); + } + } + let mut files = 0; + while fs.create(&format!("f{files:02}"), b"x", 0).is_ok() { + files += 1; + } + let mut names = 0; + loop { + fs.sync().expect("a commit"); + let before = names; + while fs.create(&format!("empty{names}"), b"", 0).is_ok() { + names += 1; + } + if names == before { + break; + } + } + assert!(files > 4 && names > 0, "{files} files and {names} empty names filled the volume"); + assert!(fs.create("after", b"x", 0).is_err(), "the volume is full"); + files +} + +/// Shrink every one of `files` to nothing, then delete each, on a volume +/// filled to its last name; then, after a commit, take a file again. +fn shrinks_and_deletes_and_takes_again(fs: &mut Mounted, files: usize) { + for i in 0..files { + let name = format!("f{i:02}"); + let (extents, _) = fs.file_extents(&name).expect("file_extents").expect("the file is on the volume"); + fs.update_metadata(&name, &[], 0, 1).expect("a shrink of a full volume's file"); + fs.free_extents(&extents).expect("the shrink's free"); + } + for i in 0..files { + assert!(matches!(fs.delete(&format!("f{i:02}")), Ok(true)), "the delete of f{i:02}"); + } + fs.sync().expect("a commit"); + fs.create("after", b"x", 0).expect("a file in what the deletes gave back"); +} + +/// A volume filled to its last name still shrinks and deletes every file, and +/// after the next commit takes a file again. +#[test] +fn a_volume_full_of_empty_names_still_deletes_and_frees() { + let mut fs = Formatted::format(VecBlockIO::new(128)).expect("format").mount(); + let files = filled_to_the_last_name(&mut fs); + shrinks_and_deletes_and_takes_again(&mut fs, files); +} + +/// A volume stopped between two commits holds more blocks used than its +/// superblock counts free; a mount counts its bitmap, so what the stop leaked +/// is no block a shrink or a delete is promised. +#[test] +fn a_mount_after_a_stop_counts_the_blocks_its_bitmap_holds() { + let mut fs = Formatted::format(VecBlockIO::new(128)).expect("format").mount(); + fs.sync().expect("a commit"); + fs.create("lost", &[1; 20 * 4096], 0).expect("taken and never committed"); + let image = (0..128).flat_map(|b| { + let mut buf = bcachefs::BlockBuf::zeroed(); + bcachefs::BlockIO::read_block(fs.io(), BlockNum::new(b), &mut buf).expect("read"); + buf.as_bytes().to_vec() + }); + let mut fs = Mounted::<_, ReadWrite>::open(VecBlockIO::from_vec(image.collect())).expect("the stopped volume mounts"); + assert!(matches!(fs.read_file("lost"), Err(FsError::NotFound)), "the stop came before the commit"); + + let files = filled_to_the_last_name(&mut fs); + shrinks_and_deletes_and_takes_again(&mut fs, files); +} + // --- The device error channel: a block the device would not give back is not // a block of zeros. --- diff --git a/issues/a-crash-leaks-the-blocks-of-its-last-commit.md b/issues/a-crash-leaks-the-blocks-of-its-last-commit.md index 841c988484e..566ef4537a3 100644 --- a/issues/a-crash-leaks-the-blocks-of-its-last-commit.md +++ b/issues/a-crash-leaks-the-blocks-of-its-last-commit.md @@ -7,21 +7,24 @@ opened: 2026-10-09 # A crash leaks the blocks taken since the last commit The DATA volume commits by shadow paging (`bcachefs/src/fs.rs`, the read-write -block's header): no block the last commit's tree reaches is written over, and +block's header): no node the last commit's tree reaches is written over, and the allocator's bitmap is written in place. So a server killed or a machine stopped between two commits leaves every block taken since the last one marked used in the bitmap and named by no tree, and the blocks only the older tree -reached, which a commit that landed was about to free, the same. Nothing finds -them again: a mount reads the bitmap as the device holds it. +reached, which a commit that landed was about to free, the same. A block whose +bit the device refused to clear when it was given up (`BitmapAllocator::give`, +`succeed`, `fail`) stays marked used the same way, with nothing said. Nothing +finds them again: a mount counts the bitmap as the device holds it +(`BitmapAllocator::open`), so the volume is smaller by every such block. Measured with `bcachefs/tests/crash.rs`'s run (a 512-block volume, a directory of 62 names renamed, one 9000-byte file written, then a commit) stopped after every write before the first superblock write: the volume mounts -as it was, with 234 blocks marked used where it had 190, and its superblock -counting 322 free where the bitmap holds 278. +as it was, with 234 blocks marked used where it had 190. ## Exit condition -A mount after any stop holds a bitmap whose used blocks are exactly those the -committed tree reaches and the superblock and bitmap's own, measured by -`bcachefs/tests/crash.rs`'s stops. +A mount after any stop, and a volume after any refused write, holds a bitmap +whose used blocks are exactly those the committed tree reaches and the +superblock and bitmap's own, measured by `bcachefs/tests/crash.rs`'s stops and +refusals. diff --git a/issues/a-full-data-volume-frees-space-only-at-a-commit.md b/issues/a-full-data-volume-frees-space-only-at-a-commit.md index afabb11552f..5af330624a9 100644 --- a/issues/a-full-data-volume-frees-space-only-at-a-commit.md +++ b/issues/a-full-data-volume-frees-space-only-at-a-commit.md @@ -12,14 +12,21 @@ the last committed tree reaches is not free until the next commit lands already wrote gives its blocks back only at the next sync, and every change copies the nodes it touches to new blocks first. On a volume its files filled, a write that follows an unlink is refused `ResourceExhausted` until fileserver's -next sync, at most `WRITEBACK` later, and a run of changes between two commits -that touches more committed nodes than `NODE_RESERVE` (16) holds is refused the -same way, a delete among them. +next sync, at most `WRITEBACK` later. + +`NODE_RESERVE` (16 blocks) is kept from every change but a delete and an +update whose entry grows no longer, which leave the tree no larger, so a +commit gives back whatever they drew. Its limits: the deletes and shrinks +between two commits on a full volume copy at most 16 committed nodes between +them, and a run that touches more is refused until the next sync; and a tree +deeper than 16 levels could not copy one delete's path at all. Nothing reads +DATA's depth: 16 is a bound picked, not one measured against it. Measured on a 1024-block `DataVolume` filled with 841 one-page files and -synced (a scratch host test, not committed): a one-page write after an unlink -was refused `ResourceExhausted` and accepted after a sync; then 13 unlinks of -every seventh file were accepted and the 14th refused `ResourceExhausted`. +synced (a scratch host test, not committed), before the reserve was kept from +names with no data: a one-page write after an unlink was refused +`ResourceExhausted` and accepted after a sync; then 13 unlinks of every seventh +file were accepted and the 14th refused `ResourceExhausted`. ## Exit condition diff --git a/issues/a-write-over-a-files-committed-page-is-not-shadowed.md b/issues/a-write-over-a-files-committed-page-is-not-shadowed.md new file mode 100644 index 00000000000..d46282fd152 --- /dev/null +++ b/issues/a-write-over-a-files-committed-page-is-not-shadowed.md @@ -0,0 +1,26 @@ +--- +status: open +kind: defect +opened: 2026-10-09 +--- + +# A write over a file's committed page is not shadowed + +DATA's commit (`bcachefs/src/fs.rs`, the read-write block's header) holds +every name, and each entry's length and extents, to one sync or the other, +and not a file's bytes. Fileserver's `write` (`userland/fileserver/src/data.rs`) +resolves a page the file's extents already reach to the block they name +(`Mounted::resolve_or_alloc_block`) and writes the page there, through the +cache, which may write the block out before the next sync. So a kill can leave +a file's committed blocks holding some pages of a write and the rest from +before it, under whichever length the committed entry names. + +Read from the code, not measured: `bcachefs/tests/crash.rs` and fileserver's +kill tests overwrite no page a sync already committed. + +## Exit condition + +A write over a page a sync committed lands in a block no committed entry +names, and a host test that overwrites committed pages and stops the device at +every write of that write and its sync finds each file's bytes as one sync or +the other. diff --git a/userland/fileserver/src/data.rs b/userland/fileserver/src/data.rs index 1147cb5ff92..3fe12928e23 100644 --- a/userland/fileserver/src/data.rs +++ b/userland/fileserver/src/data.rs @@ -21,10 +21,12 @@ //! between the two leaks blocks rather than leaving an entry naming freed ones. //! //! **What a kill costs.** A sync is the format's commit (`bcachefs`'s -//! `Mounted::sync`): a server killed anywhere leaves the volume as the last -//! sync that finished, or as the one it was inside, whole either way. A -//! directory's rename is one operation of the format's -//! (`Mounted::rename_all`): it moves whole, or is refused with nothing moved. +//! `Mounted::sync`): a server killed anywhere leaves every name, and every +//! file's length and extents, as the last sync that finished or as the one it +//! was inside, whole either way; a file's bytes are not so held, since a page +//! written over is written in place. A directory's rename is one operation of +//! the format's (`Mounted::rename_all`): it moves whole, or is refused with +//! nothing moved. //! //! Every name a client chose is bounded by the format ([`FsError::NameTooLong`]) //! before it reaches the tree. @@ -909,7 +911,7 @@ mod tests { /// A volume whose free blocks are all apart, so every run a file is given /// is one block: every block a file's data may take, taken in one run, - /// every other one given back, and the run left at the end for nodes + /// every other one given back, and the run the reserve kept /// (`bcachefs`'s `NODE_RESERVE`, 16) taken after. fn fragmented() -> DataVolume { let mut v = DataVolume::format(Ram::new(1024), &["home"], clock).unwrap(); From 3c280aa999778d7b38721abd28dec170d8bb88a0 Mon Sep 17 00:00:00 2001 From: japabu Date: Fri, 9 Oct 2026 20:26:49 +0200 Subject: [PATCH 4/5] File the btree split that leaves nodes holding a few entries Found while sizing the full-volume test: on main's crate, 300 names with no data on a 1024-block volume, one sync after each, took 97 blocks where a packed tree needs five. `pack` fills each node in key order, so a full leaf splits into a full node and a node of what is left, and hashed keys land in the full one again. Not this branch's to fix. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_017cSFvbD35xJ2kGANVdm23C --- ...plit-leaves-nodes-holding-a-few-entries.md | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+) create mode 100644 issues/a-btree-split-leaves-nodes-holding-a-few-entries.md diff --git a/issues/a-btree-split-leaves-nodes-holding-a-few-entries.md b/issues/a-btree-split-leaves-nodes-holding-a-few-entries.md new file mode 100644 index 00000000000..1add39df1d9 --- /dev/null +++ b/issues/a-btree-split-leaves-nodes-holding-a-few-entries.md @@ -0,0 +1,25 @@ +--- +status: open +kind: defect +opened: 2026-10-09 +--- + +# A btree split leaves nodes holding a few entries + +`bcachefs/src/btree.rs`'s `split_node` packs a leaf that overflows into as +few nodes as hold it (`pack`), each filled in key order: a full node and a +node holding what is left over, often one entry. Keys are hashes of names, so +the next insert lands in the full node more often than not and splits it +again. The tree grows by about a node every few entries where a node holds +dozens. + +Measured with `main`'s crate at 198a9d38e (a scratch binary, not committed): +300 names with no data created on a 1024-block volume, one sync after each. +The first 72 fit one leaf, 4 blocks used; the 228 after them took 93 more +blocks, about 2.5 entries a node. Every DATA create, every `mkdir` and every +`/state/` pays it, and ROOT's mkfs as well. + +## Exit condition + +A host test that creates 300 names one at a time finds the volume's nodes, +leaves and interior, holding on average at least half what a node can. From b028cc83f14d122d6629a2defb7b5d790654f2c4 Mon Sep 17 00:00:00 2001 From: japabu Date: Fri, 9 Oct 2026 21:03:52 +0200 Subject: [PATCH 5/5] Data grown past what a volume spares is tested against the reserve, and only a read-write mount counts the bitmap The reserve's clamp in `alloc_up_to` had no test that could fail: every fill in the suite asks for one block at a time. The new test grows a one-block file, outside any operation, to a page past every block of a volume whose free blocks all lie in one run after it, so the run is contiguous and the entry that records it does not grow. With the clamp it takes what the volume spares and is refused; without it, it takes the reserve too, and the update that records the run is refused NoSpace. `BitmapAllocator::open` read every bitmap block on every mount, the kernel's read-only ROOT mount included, which never reads the free count. The count moves to `count_free`, which only the read-write `open` calls; the read-only mount takes the superblock's count, as it did before this branch, and does no new work and gains no new refusal. The leak issue's measurement is retaken with the run `crash.rs` makes at this head: 40 one-block files after the rename, not one 9000-byte file; 284 blocks used where there were 190. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_017cSFvbD35xJ2kGANVdm23C --- bcachefs/src/alloc_bitmap.rs | 37 +++++++++++-------- bcachefs/src/fs.rs | 29 ++++++++++----- bcachefs/tests/integration.rs | 26 ++++++++++++- ...ash-leaks-the-blocks-of-its-last-commit.md | 8 ++-- 4 files changed, 69 insertions(+), 31 deletions(-) diff --git a/bcachefs/src/alloc_bitmap.rs b/bcachefs/src/alloc_bitmap.rs index 55d1d91c46c..af76e98260c 100644 --- a/bcachefs/src/alloc_bitmap.rs +++ b/bcachefs/src/alloc_bitmap.rs @@ -101,30 +101,35 @@ impl Runs { } impl BitmapAllocator { - /// The allocator of a mounted volume, its free count read off the bitmap: - /// the superblock's is the last commit's, and a stop after it leaves the - /// bitmap holding fewer. - pub fn open(io: &dyn BlockIO, sb: &Superblock) -> Result { - let mut free_blocks = 0; - let mut buf = BlockBuf::zeroed(); - for i in 0..sb.block_count.div_ceil(BITS_PER_BLOCK) { - io.read(BlockNum::new(sb.bitmap_start.raw() + i), &mut buf)?; - let bits = (sb.block_count - i * BITS_PER_BLOCK).min(BITS_PER_BLOCK); - let (whole, rest) = buf.0[..bits.div_ceil(8) as usize].split_at((bits / 8) as usize); - free_blocks += whole.iter().map(|b| b.count_zeros() as u64).sum::(); - free_blocks += rest.first().map_or(0, |b| (!b & ((1u8 << (bits % 8)) - 1)).count_ones() as u64); - } - Ok(Self { + /// The allocator of a mounted volume, its free count the last commit's. + pub fn open(sb: &Superblock) -> Self { + Self { bitmap_start: sb.bitmap_start, bitmap_blocks: sb.bitmap_blocks, total_blocks: sb.block_count, - free_blocks, + free_blocks: sb.free_blocks, next_alloc: sb.next_alloc, shadow: true, fresh: Runs::default(), pending: Vec::new(), op: None, - }) + } + } + + /// Read the free count off the bitmap: a stop after the last commit + /// leaves it holding fewer than the superblock counts. + pub fn count_free(&mut self, io: &dyn BlockIO) -> Result<(), FsError> { + let mut free_blocks = 0; + let mut buf = BlockBuf::zeroed(); + for i in 0..self.total_blocks.div_ceil(BITS_PER_BLOCK) { + io.read(BlockNum::new(self.bitmap_start.raw() + i), &mut buf)?; + let bits = (self.total_blocks - i * BITS_PER_BLOCK).min(BITS_PER_BLOCK); + let (whole, rest) = buf.0[..bits.div_ceil(8) as usize].split_at((bits / 8) as usize); + free_blocks += whole.iter().map(|b| b.count_zeros() as u64).sum::(); + free_blocks += rest.first().map_or(0, |b| (!b & ((1u8 << (bits % 8)) - 1)).count_ones() as u64); + } + self.free_blocks = free_blocks; + Ok(()) } /// Begin one operation on the tree, which [`Self::succeed`] or diff --git a/bcachefs/src/fs.rs b/bcachefs/src/fs.rs index 7793060e745..28355c73d84 100644 --- a/bcachefs/src/fs.rs +++ b/bcachefs/src/fs.rs @@ -592,17 +592,28 @@ impl Formatted { // --- Mounted (read operations, available for both ReadOnly and ReadWrite) --- +impl Mounted { + /// Open an existing filesystem from disk, to read. + pub fn open(io: IO) -> Result { + Self::read_superblock(io) + } +} + +impl Mounted { + /// Open an existing filesystem from disk, to change; a mount that only + /// reads never asks how much is free. + pub fn open(io: IO) -> Result { + let mut fs = Self::read_superblock(io)?; + fs.alloc.count_free(&fs.io)?; + Ok(fs) + } +} + impl Mounted { - /// Open an existing filesystem from disk. - pub fn open(io: IO) -> Result, FsError> { + fn read_superblock(io: IO) -> Result { let sb = Superblock::read(&io)?; - let alloc = BitmapAllocator::open(&io, &sb)?; - Ok(Mounted { - io, - sb, - alloc, - _mode: PhantomData, - }) + let alloc = BitmapAllocator::open(&sb); + Ok(Mounted { io, sb, alloc, _mode: PhantomData }) } /// The device this filesystem was opened over. diff --git a/bcachefs/tests/integration.rs b/bcachefs/tests/integration.rs index 88963ef7a5b..cc4fb2273c1 100644 --- a/bcachefs/tests/integration.rs +++ b/bcachefs/tests/integration.rs @@ -1070,9 +1070,31 @@ fn a_volume_full_of_empty_names_still_deletes_and_frees() { shrinks_and_deletes_and_takes_again(&mut fs, files); } +/// Data grown outside any operation takes no block of the reserve, even +/// where every free block lies in one run after the file's end, so the entry +/// that records the run does not grow and draws on the reserve itself. +#[test] +fn a_file_grown_past_what_the_volume_spares_leaves_the_reserve() { + let mut mkfs = Formatted::format(VecBlockIO::new(128)).expect("format"); + mkfs.create("f00", b"x", 0).expect("a one-block file"); + let mut fs = mkfs.mount(); + let (mut extents, _) = fs.file_extents("f00").expect("file_extents").expect("f00 is on the volume"); + + let grown = fs.resolve_or_alloc_block(&mut extents, 128); + assert!(matches!(grown, Err(FsError::NoSpace { .. })), "a page past every block of the volume: {grown:?}"); + assert_eq!(extents.len(), 1, "the run is not contiguous with the file, so this proves nothing: {extents:?}"); + let blocks = extents[0].block_count as u64; + fs.update_metadata("f00", &extents, blocks * 4096, 1).expect("the run recorded"); + fs.sync().expect("a commit"); + + let free = bcachefs::Superblock::read(fs.io()).expect("the superblock").free_blocks; + assert!(free >= NODE_RESERVE as u64, "the commit holds {free} blocks free, fewer than the reserve"); + shrinks_and_deletes_and_takes_again(&mut fs, 1); +} + /// A volume stopped between two commits holds more blocks used than its -/// superblock counts free; a mount counts its bitmap, so what the stop leaked -/// is no block a shrink or a delete is promised. +/// superblock counts free; a read-write mount counts its bitmap, so what the +/// stop leaked is no block a shrink or a delete is promised. #[test] fn a_mount_after_a_stop_counts_the_blocks_its_bitmap_holds() { let mut fs = Formatted::format(VecBlockIO::new(128)).expect("format").mount(); diff --git a/issues/a-crash-leaks-the-blocks-of-its-last-commit.md b/issues/a-crash-leaks-the-blocks-of-its-last-commit.md index 566ef4537a3..20f7bd96675 100644 --- a/issues/a-crash-leaks-the-blocks-of-its-last-commit.md +++ b/issues/a-crash-leaks-the-blocks-of-its-last-commit.md @@ -14,13 +14,13 @@ used in the bitmap and named by no tree, and the blocks only the older tree reached, which a commit that landed was about to free, the same. A block whose bit the device refused to clear when it was given up (`BitmapAllocator::give`, `succeed`, `fail`) stays marked used the same way, with nothing said. Nothing -finds them again: a mount counts the bitmap as the device holds it -(`BitmapAllocator::open`), so the volume is smaller by every such block. +finds them again: a read-write mount counts the bitmap as the device holds it +(`BitmapAllocator::count_free`), so the volume is smaller by every such block. Measured with `bcachefs/tests/crash.rs`'s run (a 512-block volume, a -directory of 62 names renamed, one 9000-byte file written, then a commit) +directory of 62 names renamed, 40 one-block files written, then a commit) stopped after every write before the first superblock write: the volume mounts -as it was, with 234 blocks marked used where it had 190. +as it was, with 284 blocks marked used where it had 190. ## Exit condition