Repository navigation
Conversation
Verbatim from wt/toyos-pkgrepo (d760336), so the branch that fixes it closes it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017cSFvbD35xJ2kGANVdm23C
… 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017cSFvbD35xJ2kGANVdm23C
Collaborator
Author
|
Mutation and negative-control patches for PR #816, each applied with negative-control-whole-change-reverteddiff --git b/bcachefs/src/alloc_bitmap.rs a/bcachefs/src/alloc_bitmap.rs
index 844e9b324..c6a5db186 100644
--- b/bcachefs/src/alloc_bitmap.rs
+++ a/bcachefs/src/alloc_bitmap.rs
@@ -1,9 +1,5 @@
-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;
@@ -26,162 +22,15 @@ 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<Op>,
-}
-
-/// 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<u64, u64>);
-
-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;
@@ -242,20 +91,13 @@ 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<Run, FsError> {
- 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).min(u32::try_from(spare).unwrap_or(u32::MAX));
+ let wanted = wanted.max(1);
let (start, len) = self.longest_free_run(io, from % self.total_blocks, wanted)?;
self.reserve(io, start, len.min(wanted))
}
@@ -281,10 +123,6 @@ impl BitmapAllocator {
fn reserve(&mut self, io: &dyn BlockIO, start: u64, len: u32) -> Result<Run, FsError> {
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 {
@@ -378,21 +216,17 @@ impl BitmapAllocator {
Ok((start, best_count))
}
- /// Give up a contiguous range of blocks: inside an operation, once it
- /// succeeds.
+ /// Free a contiguous range of blocks.
pub fn free_range(
&mut self,
io: &dyn BlockIO,
start: BlockNum,
count: u32,
) -> Result<(), FsError> {
- match &mut self.op {
- Some(op) => {
- op.given.push((start.raw(), count));
- Ok(())
- }
- None => self.give(io, start.raw(), count),
+ for i in 0..count as u64 {
+ self.set_free(io, BlockNum::new(start.raw() + i))?;
}
+ Ok(())
}
/// Initialize bitmap on disk: zero all bitmap blocks, then mark metadata blocks as used.
@@ -414,10 +248,6 @@ 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 b/bcachefs/src/btree.rs a/bcachefs/src/btree.rs
index bafb13be0..59e6ba3d8 100644
--- b/bcachefs/src/btree.rs
+++ a/bcachefs/src/btree.rs
@@ -365,24 +365,15 @@ 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<BlockNum> {
- 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<BlockNum, FsError> {
- if alloc.in_place(block) {
- return Ok(block);
+ let mut chosen = children.first()?.block;
+ for child in children {
+ if child.key <= *key {
+ chosen = child.block;
+ } else {
+ break;
+ }
}
- let moved = alloc.alloc_block(io)?;
- alloc.free_range(io, block, 1)?;
- Ok(moved)
+ Some(chosen)
}
/// Search the B+ tree for an exact key match. Returns the leaf entry's value.
@@ -443,48 +434,27 @@ pub fn search_by_hash(
}
}
-/// 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.
+/// Delete an exact key from the B+ tree. Returns the old value if found.
/// Does not merge underflowing nodes — just removes the entry from the leaf.
-pub fn delete(
- io: &dyn BlockIO,
- alloc: &mut BitmapAllocator,
- root: BlockNum,
- key: &Key,
-) -> Result<Option<(BlockNum, Vec<u8>)>, FsError> {
- delete_recursive(io, alloc, root, Depth::ROOT, key)
-}
+pub fn delete(io: &dyn BlockIO, root: BlockNum, key: &Key) -> Result<Option<Vec<u8>>, FsError> {
+ let mut block = root;
+ let mut depth = Depth::ROOT;
-fn delete_recursive(
- io: &dyn BlockIO,
- alloc: &mut BitmapAllocator,
- block: BlockNum,
- depth: Depth,
- key: &Key,
-) -> Result<Option<(BlockNum, Vec<u8>)>, 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)));
+ 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;
}
- children[idx].block = moved;
- let at = own(io, alloc, block)?;
- Node::Interior { level, children }.write(io, at)?;
- Ok(Some((at, old)))
}
}
}
@@ -529,8 +499,7 @@ fn walk_recursive(
/// Insert a key-value pair into the B+ tree.
///
-/// Returns the root block, which changes when the old root was split or
-/// written somewhere new.
+/// Returns the root block, which changes when the old root was split.
pub fn insert(
io: &dyn BlockIO,
alloc: &mut BitmapAllocator,
@@ -539,29 +508,29 @@ pub fn insert(
) -> Result<BlockNum, FsError> {
check_entry_fits(&entry)?;
- let Placed { at, split } = insert_recursive(io, alloc, root, Depth::ROOT, entry)?;
- if split.is_empty() {
- return Ok(at);
+ 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 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)
}
-/// Where an insert left a node: its block, and the siblings it split off, in
-/// key order.
-struct Placed {
- at: BlockNum,
- split: Vec<Child>,
+enum InsertResult {
+ Done,
+ /// The node split: these follow it, in key order.
+ Split(Vec<Child>),
}
fn insert_recursive(
@@ -570,7 +539,7 @@ fn insert_recursive(
block: BlockNum,
depth: Depth,
entry: Entry,
-) -> Result<Placed, FsError> {
+) -> Result<InsertResult, FsError> {
match Node::read(io, block)? {
Node::Leaf(mut entries) => {
match entries.binary_search_by(|e| e.key.cmp(&entry.key)) {
@@ -579,24 +548,32 @@ fn insert_recursive(
}
write_or_split(io, alloc, block, Node::Leaf(entries))
}
- Node::Interior { level, mut children } => {
- let idx = child_index(&children, &entry.key);
+ Node::Interior { level, children } => {
+ let mut idx = 0;
+ for (i, child) in children.iter().enumerate() {
+ if child.key <= entry.key {
+ idx = i;
+ } else {
+ break;
+ }
+ }
let child_block = children.get(idx).ok_or(FsError::CorruptedNode(block))?.block;
let deeper = depth.descend(block)?;
- 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);
+ 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 })
+ }
}
- write_or_split(io, alloc, block, Node::Interior { level, children })
}
}
}
@@ -606,23 +583,20 @@ fn write_or_split(
alloc: &mut BitmapAllocator,
block: BlockNum,
node: Node,
-) -> Result<Placed, FsError> {
- let at = own(io, alloc, block)?;
+) -> Result<InsertResult, FsError> {
if NODE_HEADER_SIZE + node.payload_size() <= BLOCK_SIZE {
- node.write(io, at)?;
- return Ok(Placed { at, split: Vec::new() });
+ node.write(io, block)?;
+ return Ok(InsertResult::Done);
}
- Ok(Placed { at, split: split_node(io, alloc, at, node)? })
+ split_node(io, alloc, block, 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<Vec<Child>, FsError> {
+) -> Result<InsertResult, FsError> {
match node {
Node::Leaf(entries) => {
// One entry is not a split problem. Halving by *count* used to
@@ -636,15 +610,28 @@ 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 in nodes {
- let sibling = alloc.alloc_block(io)?;
+ for (node, sibling) in nodes.into_iter().zip(blocks) {
children.push(Child { key: node[0].key, block: sibling });
Node::Leaf(node).write(io, sibling)?;
}
Node::Leaf(first).write(io, block)?;
- Ok(children)
+ Ok(InsertResult::Split(children))
}
Node::Interior { level, mut children } => {
if children.len() < 2 {
@@ -662,7 +649,7 @@ fn split_node(
Node::Interior { level, children }.write(io, block)?;
Node::Interior { level, children: right }.write(io, right_block)?;
- Ok(alloc::vec![Child { key: split_key, block: right_block }])
+ Ok(InsertResult::Split(alloc::vec![Child { key: split_key, block: right_block }]))
}
}
}
@@ -819,8 +806,9 @@ mod tests {
);
}
- /// 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.
+ /// 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.
#[test]
fn a_split_short_of_a_sibling_gives_back_the_ones_it_took() {
let io = crate::block_io::VecBlockIO::new(16);
@@ -833,13 +821,10 @@ mod tests {
let mut middle: Vec<Entry> = (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 b/bcachefs/src/fs.rs a/bcachefs/src/fs.rs
index 161112f88..e83a49d17 100644
--- b/bcachefs/src/fs.rs
+++ a/bcachefs/src/fs.rs
@@ -139,10 +139,12 @@ impl FsError {
pub struct ReadOnly;
pub struct ReadWrite;
-/// 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<IO: BlockIO>(Mounted<IO, ReadWrite>);
+/// A formatted but not yet mounted filesystem. Used for building images (mkfs).
+pub struct Formatted<IO: BlockIO> {
+ io: IO,
+ sb: Superblock,
+ alloc: BitmapAllocator,
+}
/// A mounted filesystem. Mode is ReadOnly or ReadWrite.
pub struct Mounted<IO: BlockIO, Mode = ReadWrite> {
@@ -410,8 +412,9 @@ fn decode_leaf_value(value: &[u8], volume_blocks: u64) -> Result<LeafValue, FsEr
/// Allocate blocks and write `data` into them, returning the extent list.
///
/// The allocator answers with a run that may be shorter than the request, so
-/// covering `data` takes a loop. A run reserved by an earlier turn of that loop
-/// is the operation's, which gives it back if a later turn fails.
+/// covering `data` takes a loop — and a run reserved by an earlier turn of that
+/// loop is a block the bitmap calls taken that no entry names, once a later
+/// turn fails. Every run goes back before the error does.
fn write_data(
io: &dyn BlockIO,
alloc: &mut BitmapAllocator,
@@ -427,7 +430,10 @@ fn write_data(
let mut data_offset = 0usize;
while remaining > 0 {
- let run = alloc.alloc_up_to(io, alloc.next_alloc, remaining)?;
+ 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)),
+ };
push_extent(&mut extents, run.start.raw(), run.len);
let mut buf = BlockBuf::zeroed();
@@ -438,7 +444,9 @@ fn write_data(
let len = chunk_end - data_offset;
buf.0[..len].copy_from_slice(&data[data_offset..chunk_end]);
}
- io.write(BlockNum::new(run.start.raw() + i), &buf)?;
+ if let Err(err) = io.write(BlockNum::new(run.start.raw() + i), &buf) {
+ return Err(give_back(io, alloc, &extents, err));
+ }
data_offset += BLOCK_SIZE;
}
@@ -448,6 +456,24 @@ 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
@@ -549,44 +575,84 @@ impl<IO: BlockIO> Formatted<IO> {
sb.write(&io)?;
- Ok(Self(Mounted { io, sb, alloc, _mode: PhantomData }))
+ Ok(Self { io, sb, alloc })
}
/// 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::into_io`].
+ /// nothing here invents a name for one. Persisted by [`Self::sync`], which
+ /// [`Self::into_io`] runs.
pub fn set_uuid(&mut self, uuid: FsUuid) {
- self.0.sb.uuid = uuid;
+ self.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> {
- self.0.create(name, data, mtime)
+ 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(())
}
/// Create a symlink on the formatted filesystem.
pub fn create_symlink(&mut self, name: &str, target: &str, mtime: u64) -> Result<(), FsError> {
- self.0.put(name, KeyType::Symlink, target.as_bytes(), mtime)
+ 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()
}
/// Mount this formatted filesystem for read-write access.
pub fn mount(self) -> Mounted<IO, ReadWrite> {
- let mut fs = self.0;
- fs.alloc.shadow = true;
- fs
+ Mounted {
+ io: self.io,
+ sb: self.sb,
+ alloc: self.alloc,
+ _mode: PhantomData,
+ }
}
/// Mount this formatted filesystem for read-only access.
pub fn mount_readonly(self) -> Mounted<IO, ReadOnly> {
- let Mounted { io, sb, alloc, .. } = self.0;
- Mounted { io, sb, alloc, _mode: PhantomData }
+ Mounted {
+ io: self.io,
+ sb: self.sb,
+ alloc: self.alloc,
+ _mode: PhantomData,
+ }
}
/// Consume and return the underlying IO (for extracting the image bytes).
pub fn into_io(mut self) -> Result<IO, FsError> {
- self.0.sync()?;
- Ok(self.0.io)
+ self.sync()?;
+ Ok(self.io)
}
}
@@ -596,7 +662,13 @@ impl<IO: BlockIO, Mode> Mounted<IO, Mode> {
/// Open an existing filesystem from disk.
pub fn open(io: IO) -> Result<Mounted<IO, Mode>, FsError> {
let sb = Superblock::read(&io)?;
- let alloc = BitmapAllocator::open(&sb);
+ 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,
+ };
Ok(Mounted {
io,
sb,
@@ -731,9 +803,11 @@ impl<IO: BlockIO, Mode> Mounted<IO, Mode> {
/// Convert back to Formatted state (for testing — insert more files after reading).
pub fn into_formatted(self) -> Formatted<IO> {
- let Self { io, sb, mut alloc, .. } = self;
- alloc.shadow = false;
- Formatted(Mounted { io, sb, alloc, _mode: PhantomData })
+ Formatted {
+ io: self.io,
+ sb: self.sb,
+ alloc: self.alloc,
+ }
}
/// Return the extents and file size for a file.
@@ -751,17 +825,6 @@ impl<IO: BlockIO, Mode> Mounted<IO, Mode> {
// --- 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<IO: BlockIO> Mounted<IO, ReadWrite> {
/// Create a file, replacing whatever answered to `name`.
pub fn create(&mut self, name: &str, data: &[u8], mtime: u64) -> Result<(), FsError> {
@@ -773,24 +836,6 @@ impl<IO: BlockIO> Mounted<IO, ReadWrite> {
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<T>(&mut self, change: impl FnOnce(&mut Self) -> Result<T, FsError>) -> Result<T, FsError> {
- 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
@@ -815,28 +860,21 @@ impl<IO: BlockIO> Mounted<IO, ReadWrite> {
return Err(FsError::NameTooLong { len: name.len(), max: MAX_NAME_LEN });
}
- 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(&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 })?;
+ let displaced = match self.find_by_name(name)? {
+ Some((key, value)) => Some((key, self.decode(&value)?.extents().to_vec())),
+ None => None,
+ };
- fs.retire_displaced(displaced, key)
- })
- }
+ 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 },
+ )?;
- /// Remove `key`'s entry, if the tree has one.
- fn remove(&mut self, key: &Key) -> Result<bool, FsError> {
- 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)
+ self.retire_displaced(displaced, key)
}
/// Remove the entry the insert of `new_key` did not replace, and free the
@@ -852,9 +890,26 @@ impl<IO: BlockIO> Mounted<IO, ReadWrite> {
) -> Result<(), FsError> {
let Some((old_key, old_extents)) = displaced else { return Ok(()) };
if old_key != new_key {
- self.remove(&old_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.free_extents(&old_extents)
+ Ok(())
+ }
+
+ /// Delete a file or symlink by name. Returns true if found and deleted.
+ pub fn delete(&mut self, name: &str) -> Result<bool, FsError> {
+ 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()
}
/// Delete a file/symlink by name, freeing its data blocks. Returns true if found.
@@ -866,57 +921,32 @@ impl<IO: BlockIO> Mounted<IO, ReadWrite> {
/// 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.
- pub fn delete(&mut self, name: &str) -> Result<bool, FsError> {
- 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()?;
+ fn delete_by_name(&mut self, name: &str) -> Result<bool, FsError> {
+ 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));
}
- self.alloc.committed(&self.io)
+ for ext in &extents {
+ self.alloc.free_range(&self.io, BlockNum::new(ext.start_block), ext.block_count)?;
+ }
+ Ok(true)
}
/// 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 {
@@ -952,7 +982,7 @@ impl<IO: BlockIO> Mounted<IO, ReadWrite> {
// 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 {
- self.remove(&old_key)?;
+ btree::delete(&self.io, self.sb.root_node, &old_key)?;
}
Ok(())
@@ -966,24 +996,29 @@ impl<IO: BlockIO> Mounted<IO, ReadWrite> {
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 };
-
- // 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(())
- })
+ 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(())
}
- /// Give `extents`' blocks back to the allocator. Record first, free second:
+ /// Return `extents`' blocks 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 b/bcachefs/src/superblock.rs a/bcachefs/src/superblock.rs
index d24ec660b..5d78b8e32 100644
--- b/bcachefs/src/superblock.rs
+++ a/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<Item = BlockNum> {
- [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)
}
}
--- a/userland/fileserver/src/data.rs 2026-10-09 18:00:09
+++ b/userland/fileserver/src/data.rs 2026-10-09 18:00:09
@@ -687,42 +687,38 @@
if to.starts_with(&format!("{from}/")) {
return Err(SyscallError::InvalidArgument);
}
- // Every entry beneath, and the directory's own, in one
- // operation of the format's: the directory moves whole or not at all.
+ // 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.
let prefix = format!("{from}/");
let moving: Vec<(String, Kind)> = self
.names
- .get_key_value(from)
- .into_iter()
- .chain(self.names.range(prefix.clone()..).take_while(|(n, _)| n.starts_with(&prefix)))
+ .range(prefix.clone()..)
+ .take_while(|(n, _)| n.starts_with(&prefix))
.map(|(n, k)| (n.clone(), *k))
.collect();
- for (name, _) in &moving {
+ for (name, kind) in &moving {
+ let moved = format!("{to}/{}", &name[prefix.len()..]);
if let Some(node) = self.by_path.get(name).copied() {
self.persist(node)?;
}
- }
- 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) {
+ 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) {
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(())
}
}m1-every-node-in-place--- a/bcachefs/src/alloc_bitmap.rs 2026-10-09 17:59:56
+++ b/bcachefs/src/alloc_bitmap.rs 2026-10-09 17:59:56
@@ -116,7 +116,7 @@
/// 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()))
+ true || 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 them2-no-flush-before-the-superblock--- a/bcachefs/src/fs.rs 2026-10-09 17:59:56
+++ b/bcachefs/src/fs.rs 2026-10-09 17:59:56
@@ -884,7 +884,6 @@
/// 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;m3-committed-blocks-free-at-once--- a/bcachefs/src/alloc_bitmap.rs 2026-10-09 17:59:56
+++ b/bcachefs/src/alloc_bitmap.rs 2026-10-09 17:59:56
@@ -170,7 +170,7 @@
/// 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) {
+ if true || self.fresh.remove(block) {
self.set_free(io, BlockNum::new(block))?;
} else {
match self.pending.last_mut() {m4-a-refused-operation-keeps-its-root--- a/bcachefs/src/fs.rs 2026-10-09 17:59:56
+++ b/bcachefs/src/fs.rs 2026-10-09 17:59:56
@@ -784,7 +784,6 @@
Ok(done)
}
Err(e) => {
- self.sb.root_node = root;
self.alloc.fail(&self.io);
Err(e)
}m5-no-seal--- a/bcachefs/src/fs.rs 2026-10-09 17:59:56
+++ b/bcachefs/src/fs.rs 2026-10-09 17:59:56
@@ -891,7 +891,6 @@
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()?;m6-fileserver-renames-entry-by-entry--- a/userland/fileserver/src/data.rs 2026-10-09 18:00:09
+++ b/userland/fileserver/src/data.rs 2026-10-09 18:00:09
@@ -687,42 +687,38 @@
if to.starts_with(&format!("{from}/")) {
return Err(SyscallError::InvalidArgument);
}
- // Every entry beneath, and the directory's own, in one
- // operation of the format's: the directory moves whole or not at all.
+ // 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.
let prefix = format!("{from}/");
let moving: Vec<(String, Kind)> = self
.names
- .get_key_value(from)
- .into_iter()
- .chain(self.names.range(prefix.clone()..).take_while(|(n, _)| n.starts_with(&prefix)))
+ .range(prefix.clone()..)
+ .take_while(|(n, _)| n.starts_with(&prefix))
.map(|(n, k)| (n.clone(), *k))
.collect();
- for (name, _) in &moving {
+ for (name, kind) in &moving {
+ let moved = format!("{to}/{}", &name[prefix.len()..]);
if let Some(node) = self.by_path.get(name).copied() {
self.persist(node)?;
}
- }
- 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) {
+ 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) {
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(())
}
} |
Collaborator
Author
|
Review of #816 at 3adcf42 (round 1). Net BLOCKER
NOTE
SEND BACK |
Collaborator
Author
|
T14 at |
…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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017cSFvbD35xJ2kGANVdm23C
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017cSFvbD35xJ2kGANVdm23C
Collaborator
Author
|
Round 2 evidence for #816 at The runner#!/bin/bash
# Apply each patch at the head, run the crate's and fileserver's tests, restore.
cd <worktree> || exit 1
M=<scratch>/mutations
L=<scratch>/logs
echo "head $(git rev-parse HEAD)"
for n in negative-control-whole-change-reverted m1-every-node-in-place m2-no-flush-before-the-superblock m3-committed-blocks-free-at-once m4-a-refused-operation-keeps-its-root m5-no-seal m6-fileserver-renames-entry-by-entry m7-the-root-past-the-first-sector m8-the-backup-before-the-nodes-flush m9-a-mount-takes-the-superblocks-count m10-a-delete-keeps-the-reserve m11-a-shrink-keeps-the-reserve; do
git apply --check $M/$n.patch || { echo "$n: does not apply"; exit 1; }
git apply $M/$n.patch
cargo test -p bcachefs --no-fail-fast > $L/$n.final.bcachefs.log 2>&1; b=$?
cargo test -p fileserver --lib > $L/$n.final.fileserver.log 2>&1; f=$?
git apply -R $M/$n.patch
echo "$n bcachefs EXIT=$b fileserver EXIT=$f"
done
git status --porcelain --ignore-submodules=noneIts logm1-every-node-in-place--- a/bcachefs/src/alloc_bitmap.rs 2026-10-09 17:59:56
+++ b/bcachefs/src/alloc_bitmap.rs 2026-10-09 17:59:56
@@ -116,7 +116,7 @@
/// 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()))
+ true || 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 them2-no-flush-before-the-superblock--- a/bcachefs/src/fs.rs 2026-10-09 17:59:56
+++ b/bcachefs/src/fs.rs 2026-10-09 17:59:56
@@ -884,7 +884,6 @@
/// 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;m3-committed-blocks-free-at-once--- a/bcachefs/src/alloc_bitmap.rs
+++ b/bcachefs/src/alloc_bitmap.rs
@@ -201,7 +201,7 @@
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) {
+ if true || self.fresh.remove(block) {
if let Err(e) = self.set_free(io, BlockNum::new(block)) {
refused = refused.and(Err(e));
}m4-a-refused-operation-keeps-its-root--- a/bcachefs/src/fs.rs 2026-10-09 17:59:56
+++ b/bcachefs/src/fs.rs 2026-10-09 17:59:56
@@ -784,7 +784,6 @@
Ok(done)
}
Err(e) => {
- self.sb.root_node = root;
self.alloc.fail(&self.io);
Err(e)
}m5-no-seal--- a/bcachefs/src/fs.rs
+++ b/bcachefs/src/fs.rs
@@ -906,7 +906,7 @@
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();
+
self.sb.write(&self.io)?;
self.io.flush()?;
self.alloc.committed(&self.io)m6-fileserver-renames-entry-by-entry--- a/userland/fileserver/src/data.rs 2026-10-09 18:00:09
+++ b/userland/fileserver/src/data.rs 2026-10-09 18:00:09
@@ -687,42 +687,38 @@
if to.starts_with(&format!("{from}/")) {
return Err(SyscallError::InvalidArgument);
}
- // Every entry beneath, and the directory's own, in one
- // operation of the format's: the directory moves whole or not at all.
+ // 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.
let prefix = format!("{from}/");
let moving: Vec<(String, Kind)> = self
.names
- .get_key_value(from)
- .into_iter()
- .chain(self.names.range(prefix.clone()..).take_while(|(n, _)| n.starts_with(&prefix)))
+ .range(prefix.clone()..)
+ .take_while(|(n, _)| n.starts_with(&prefix))
.map(|(n, k)| (n.clone(), *k))
.collect();
- for (name, _) in &moving {
+ for (name, kind) in &moving {
+ let moved = format!("{to}/{}", &name[prefix.len()..]);
if let Some(node) = self.by_path.get(name).copied() {
self.persist(node)?;
}
- }
- 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) {
+ 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) {
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(())
}
}m7-the-root-past-the-first-sector--- a/bcachefs/src/superblock.rs
+++ b/bcachefs/src/superblock.rs
@@ -147,7 +147,7 @@
Ok(Self {
block_count: read_u64(b, 12),
- root_node: BlockNum::new(read_u64(b, 24)),
+ root_node: BlockNum::new(read_u64(b, 600)),
next_alloc: read_u64(b, 36),
free_blocks: read_u64(b, 44),
bitmap_start: BlockNum::new(read_u64(b, 52)),
@@ -172,7 +172,7 @@
write_u64(b, 12, self.block_count);
write_u32(b, 20, BLOCK_SIZE as u32);
- write_u64(b, 24, self.root_node.raw());
+ write_u64(b, 600, self.root_node.raw());
// [32..36] pad — the tree's depth used to live here, and drove three
// recursions against a 128 KiB kernel stack. The descent ends at a
// `Node::Leaf` now, so the disk has no say in how deep it goes.m8-the-backup-before-the-nodes-flush--- a/bcachefs/src/fs.rs
+++ b/bcachefs/src/fs.rs
@@ -899,6 +899,9 @@
/// Commit the tree as it stands: see this block's header.
pub fn sync(&mut self) -> Result<(), FsError> {
+ let mut backup = BlockBuf::zeroed();
+ self.sb.write_to(&mut backup);
+ self.io.write(BlockNum::new(self.sb.block_count - 1), &backup)?;
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();m9-a-mount-takes-the-superblocks-count--- a/bcachefs/src/alloc_bitmap.rs
+++ b/bcachefs/src/alloc_bitmap.rs
@@ -118,7 +118,7 @@
bitmap_start: sb.bitmap_start,
bitmap_blocks: sb.bitmap_blocks,
total_blocks: sb.block_count,
- free_blocks,
+ free_blocks: sb.free_blocks + 0 * free_blocks,
next_alloc: sb.next_alloc,
shadow: true,
fresh: Runs::default(),m10-a-delete-keeps-the-reserve--- a/bcachefs/src/fs.rs
+++ b/bcachefs/src/fs.rs
@@ -882,7 +882,7 @@
/// 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<bool, FsError> {
- self.atomic(Reserve::Draw, |fs| {
+ self.atomic(Reserve::Keep, |fs| {
let Some((key, value)) = fs.find_by_name(name)? else { return Ok(false) };
let extents = fs.decode(&value)?.extents().to_vec();
m11-a-shrink-keeps-the-reserve--- a/bcachefs/src/fs.rs
+++ b/bcachefs/src/fs.rs
@@ -984,7 +984,7 @@
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,
+ true => Reserve::Keep,
false => Reserve::Keep,
};
let new_entry = Entry { key: old_key, value: new_value };The review's mutation, at the round-1 sync with the new crash model (before the flush between the copies was deleted)--- a/bcachefs/src/fs.rs
+++ b/bcachefs/src/fs.rs
@@ -898,6 +898,6 @@
self.alloc.seal();
for copy in self.sb.copies() {
self.sb.write_at(&self.io, copy)?;
- self.io.flush()?;
}
+ self.io.flush()?;
self.alloc.committed(&self.io)
}What m7 and m8 turn red, from their crash logsThe full-volume tests on the round-1 source (
|
Collaborator
Author
|
mkfs differential for #816 at Inputs: The scratch binary (one copy per crate,
|
Japabu
changed the title
Collaborator
Author
|
T14 at |
Collaborator
Author
|
Review of #816 at 3b43b83 (round 2). Net Round-1 BLOCKERs
BLOCKER
NOTE
SEND BACK |
…nd 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017cSFvbD35xJ2kGANVdm23C
Collaborator
Author
Round-3 evidence at b028cc8Paths are scrubbed: Mutation matrix (
|
Collaborator
Author
|
Review of #816 at b028cc8 (round 3). Net Round-2 BLOCKER
Round-2 NOTEs
BLOCKER
NOTE
SEND BACK |
Collaborator
Author
|
T14 at |
Collaborator
Author
|
Reposted by the orchestrator with the T14's model and firmware strings masked; the text is otherwise the reviewer's (original comment 6087785084, deleted because GitHub keeps edit history). Review of #816 at b028cc8 (round 4, head unchanged since round 3). Round-3 BLOCKER
BLOCKERNone. NOTE
LAND AFTER NAMED CHANGES |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
DATA's filesystem commits atomically: every change is one operation that never writes over a node the last commit reaches, a sync is the commit, and a directory rename moves whole or not at all. A full volume still shrinks.
Issue files and #808
issues/a-directory-rename-on-data-is-not-atomic.mdis carried verbatim fromwt/toyos-pkgrepo(d760336) in the first commit and deleted in the second, so over the branch it is a net no-op onmain. Both landing orders need one act on the branch that lands second:mainnever holds the file.wt/toyos-pkgrepo(toyos-update verifies a signed package repository and src/publish.rs writes one through it; pkg install <name> waits on an atomic directory rename on DATA #808) must drop its own copy and the citation of it atissues/a-package-is-a-directory-under-apps-and-the-installer-is-a-program.md:99when it next takesmain.mainhere brings toyos-update verifies a signed package repository and src/publish.rs writes one through it; pkg install <name> waits on an atomic directory rename on DATA #808's copy and its citation in. The merge deletes the file and the citation, since this branch closes it.#808 is in the merge queue and had not landed when this round was pushed (
gh pr view 808: OPEN,mergedAtnull, checked after b028cc8; this round mergedmainat 8e3a813). The second order is the one expected to apply. What the later merge must do: once #808 is onmain, mergemainhere. #808's copy of the issue matches the one this branch carries, so the merge keeps it without a conflict. That merge must thengit rm issues/a-directory-rename-on-data-is-not-atomic.mdand delete its citation inissues/a-package-is-a-directory-under-apps-and-the-installer-is-a-program.md: stage 4's "and the commit by rename, which waits onissues/a-directory-rename-on-data-is-not-atomic.md." becomes "and the commit by rename." It must also search the bare namea-directory-rename-on-data-is-not-atomicacross the tree and find nothing left.Landing order (orchestrator): this branch is not enqueued while #808 is in the merge queue. After #808 lands, this branch merges
main; that merge runsgit rm issues/a-directory-rename-on-data-is-not-atomic.md, deletes the citation in stage 4 ofissues/a-package-is-a-directory-under-apps-and-the-installer-is-a-program.md, leaves a search for the bare name empty, and getshostandguest / suitegreen and a review of the merge alone. If #808 leaves the queue instead and this branch lands first, the deletion is pushed to #808's branch before #808 is enqueued again.What the defect was, measured first
The issue named fileserver's per-entry rename. That was half of it. A host test (
a_directory_rename_is_whole_under_one_name_wherever_the_server_is_killed, below) keeps the first k block writes of a directory rename and its sync, for every k. Run againstmain's code, 39 of 44 stops tore the volume: 2 left the directory half under each name, and 37 left DATA unmountable. The cause wasBadMagicon a root the superblock named that the disk did not hold yet: the cache flushes lowest block first, so block 0's superblock went out before the nodes it names. The format had no commit a kill could not tear, so the fix is in both owners.What changed, per decision
bcachefs (the interim format): a sync is a shadow-paging commit, and no byte of the layout changes.
btree::ownwrites a node in place only if the operation in progress took its block. Otherwise it moves the node to a new block and gives the old one up. Insert and delete (which now returns the root) both go through it.Mounted::atomic). If it is refused anywhere, the root goes back and every block it took is freed. If it succeeds, what it gave up is given up.split_node's andwrite_data's own give-backs are deleted, because the rollback covers them.Mounted::rename_allrenames a list of names as one operation.Superblock::writeismain's again.seal). A commit that is refused after its superblock reached the device therefore keeps both trees until one lands.NODE_RESERVE(16), round 2. The reserve is held against every allocation (Reserve::Keep), a file's data grown outside any operation included (alloc_up_toclamps a run to what the volume spares; tested in round 3), except those of an operation that leaves the tree no larger (Reserve::Draw): a delete, and anupdate_metadatawhose entry grows no longer, which splits no node. A commit gives back whatever such an operation drew. The reserve is therefore whole after every sync, and the round-1 wedge cannot be reached by changes: there, names with no data took the reserve and no delete ever succeeded again. Round 1 held the reserve only against a file's data.BitmapAllocator::count_free, called only byMounted::<_, ReadWrite>::open). Added in round 2; narrowed to read-write in round 3. It no longer trusts the superblock's count, which after a stop between commits names blocks the bitmap holds taken. This was the review's second road to the wedge.openis now one function per mode, and the read-only one takes the superblock's count, as onmain. So the kernel's ROOT mount (kernel/src/rootfs.rs), which never reads the count, reads no bitmap block and gains no refusal.BitmapAllocator::givekeeps giving up a run past a bit the device would not clear, round 2. Before, it stopped there and left the rest of the run neither freed nor pending.Formattedis aMountedwith shadowing off, and its own create, symlink and sync are deleted. mkfs bytes are unchanged (the differential is below).fileserver: a directory rename persists the open files beneath it, hands every entry beneath it and the directory's own marker to
rename_all, and moves its name index only once that answers.The contract, narrowed in round 2 (
bcachefs/src/fs.rs's read-write header,data.rs's module header): a kill leaves every name, and each entry's length and extents, as one sync or the other. A file's bytes are not held that way, because fileserver writes over a page the file already holds in place.Kept, filed:
issues/a-write-over-a-files-committed-page-is-not-shadowed.md(new). The in-place page overwrite. Exit: such a write lands in a block no committed entry names, and a stop test over it finds each file's bytes as one sync or the other.issues/a-crash-leaks-the-blocks-of-its-last-commit.md. A stop leaves blocks marked used that no tree names. Round 3 retook the measurement with the runcrash.rsmakes at this head (the rename, 40 one-block files, the commit, stopped before the first superblock write): 284 used where 190 were. Round 2's number came from an older run that wrote one 9000-byte file. Round 2 adds the freesgive,succeedandfaildrop when the device refuses a bit, and the exit covers refused writes too.issues/a-full-data-volume-frees-space-only-at-a-commit.md. Round 2 adds the reserve's rule and its limits: at most 16 committed nodes copied by the deletes and shrinks between two commits on a full volume, and a tree deeper than 16 cannot delete there at all. Nothing reads DATA's depth, so 16 is a bound picked, not one measured.issues/a-btree-split-leaves-nodes-holding-a-few-entries.md(new, found on the way, not fixed). Onmain's crate, 300 names with no data, synced one by one, took 97 blocks where a packed tree needs five.packsplits a full leaf into a full node plus a remainder.Tests
bcachefs/tests/crash.rsis the crash-point model on the crate, with no cache between it and the device. A directory of 62 names is renamed, 40 one-block files are written after it, then a commit. Every block write is a stop. Each stop is tried with the open flush epoch's earlier writes landed and with none of them. The stop's own write lands whole, or, where it hits a block the layout writes in place (a superblock copy or the bitmap), torn at each 512-byte boundary, first k sectors new or last k, skipping tears that leave the block equal to the old or new bytes. Every state is checked twice: as the device holds it, and with block 0 lost, mounting from the backup (round 2). 589 writes and 2356 states: each must mount, keep the 80 names beside the directory byte for byte, and hold the directory whole under exactly one name. A second test refuses each write in turn with the filesystem alive. The answer must equal what it serves, the device must be whole past the refused sync and past 40 changes after it, and the next sync must make the answer durable.bcachefs/tests/integration.rs, round 2:a_volume_full_of_empty_names_still_deletes_and_freesis the review's test, taken further. It fills a 128-block volume with names enough to span leaves, then one-block files until refused, then empty names until a create right after a sync is refused. It then shrinks every file to nothing, deletes every file with no sync between, syncs, and asserts a one-block create is accepted.a_mount_after_a_stop_counts_the_blocks_its_bitmap_holdsstops a volume after a 20-block create it never committed, remounts the image, then does the same fill, shrinks and deletes.a_file_grown_past_what_the_volume_spares_leaves_the_reserve(round 3, the review's BLOCKER) is mkfs's one-blockf00on a 128-block volume. Every free block lies in one run straight after it, so a run that growsf00is contiguous and the entry recording it does not grow. The test growsf00withresolve_or_alloc_blockto page 128, past every block of the volume, and assertsNoSpace. It asserts the extent list is still one extent (the test's premise), records the run withupdate_metadata, and syncs. It then asserts the superblock's free count is at leastNODE_RESERVEand runsshrinks_and_deletes_and_takes_again. Under the review's patch (m12) the grow takes the reserve too, and the update is refusedNoSpace { requested: 1, available: 0 }: the round-1 wedge, reached through data. The defect the review described is real in the sense that only the clamp stops it. The clamp was already in the code, so no source changes for it.userland/fileserver/src/data.rsruns the same throughDataVolumeand its cache. It kills after every write of a rename plus sync. It refuses every write, checking the disk right after the refused sync and again after the retry. It also covers the issue's own measurement:EntryTooLargeon a 300-byte target now moves nothing.ONE_BLOCK_FILES_64is 42 (60 − 16 − 2), because a create's leaf copies are now held to the reserve too.a_renamed_file_keeps_every_extent_it_hadfragments a 128-block volume, since 64 left its rename no blocks under the reserve.DataVolume::rename, which is fileserver's whole part.main.rsonly forwards the call, and the disks' flush paths are unchanged.Gates at b028cc8
cargo run -- --ci hostcargo run -- --build-onlycargo test(whole guest suite)uptimeload averages 41.33 34.53 30.51 at the start and 28.96 33.03 30.42 at the endcargo test -p bcachefs --no-fail-fastcargo test -p fileserver --libb028cc8 is this round's commit on a merge of
mainat 8e3a813 (#806). The merge touchedtests/common/compile.rsand one issue file, and no crate or fileserver byte.mainhas since taken #807. This branch has not merged it, and the merge queue tests the two together.Checks of high-risk code (filesystem crash consistency)
Every patch was re-run in round 3 at b028cc8, in one script (
mutations.log). Each was checked withgit apply --check, applied, built, run withcargo test -p bcachefs --no-fail-fastand thencargo test -p fileserver --lib, and reverted. The tree's status was empty after. Every exit below is that run's. m1 to m8, m10 and m11 applied unchanged from round 2. m9 is rewritten for the moved count. m12 is new. The negative control is regenerated against the merge base 8e3a813. Patches, runner and output are in the round-3 evidence comment. The "red" column's counts are round 2's readings; round 3 re-measured the exits.bcachefs/srcatorigin/main, fileserver's rename atmain's, this branch's testscrash.rsdoes not compile (rename_allis new); fileserver's threea_directory_rename_*tests are red (39 of 43 refusals tear)…_the_format_refuses_part_way_moves_nothingseal…_the_format_refuses_part_way_moves_nothingcrash.rs, 4 of 2372 states, only torn backup writes with block 0 losta_mount_after_a_stop_counts_the_blocks_its_bitmap_holdsalloc_up_todoes not clamp a run to what the volume spares (round 3, the review's)a_file_grown_past_what_the_volume_spares_leaves_the_reserve: the update is refusedNoSpace, available 0The review's flush mutation was measured on the round-1 sync with this round's crash model, before the flush between the copies was deleted. The mutation is one flush after both superblock copies instead of one between them.
crashexited 0 and fileserver exited 0 (mutations-r2a.log, in the comment). It stays green because the model cannot fail on it: a superblock's bytes past its first 122 are zero in every copy, so a sector-aligned tear of one leaves the old copy or the new one. In the same run, m7 and m8 went red only through the new shapes, so the shapes have teeth. The flush between the copies was then deleted rather than kept untested.The full-volume tests on the round-1 source (
bcachefs/srcat 3adcf42, this round's tests) exit 101.a_volume_full_of_empty_names_still_deletes_and_freesfails on the shrink of f00 (NoSpace, available 0).a_mount_after_a_stop_counts_the_blocks_its_bitmap_holdsfails on the same shrink (NoSpace, available 21: the superblock's count against an empty bitmap). Both are green at the head.Independent oracle for the commit's premise, and for deleting the flush between the copies. The commit rests on a device writing a superblock's first 512-byte sector whole. The NVMe base specification guarantees that. Identify Controller's AWUPF (Atomic Write Unit Power Fail) is a 0's based count of logical blocks the controller writes atomically across a power failure, so every controller guarantees at least one LBA. An LBA is at least 512 bytes. DATA's only device is NVMe: diskserver drives one NVMe controller, under QEMU and on the T14. So a superblock copy lands old or new, and it does so without a flush between the two copies. The crash model's sector tears are that guarantee taken as the failure boundary. Neither the crash model nor AWUPF bounds a device that breaks the specification.
Independent oracle for the rest. The format has no specification and no second implementation (
issues/bcachefs-crate-is-not-bcachefs.md), so the oracle for crash consistency is the crash-point model itself: every write point, out-of-order landing inside a flush epoch, sector tears of in-place blocks, and a mount from the backup, each judged by a fresh mount. For mkfs there is a differential (comment). One scratch binary was built againstorigin/main's crate (198a9d3) and against this branch's (0c9c05c, whose crate 3b43b83 carries unchanged), overkernel/src(231 files, 24 symlinks) and main'sbcachefs/(21 files, 3 symlinks) on 65536 blocks. sha2563383376928e3…cea80039both,cmpexit 0;b4692a62ea0a…592b480cadboth,cmpexit 0.T14
The orchestrator read
boot:testcasesat 3b43b83: judge EXIT=0, 249 passed, 0 failed, 2 boots. Round 3 changes which mounts read the bitmap, and every boot mounts, so it owes a new reading. It is staged at b028cc8 withcargo test --test toyos-build -- --metal --metal-readback <dir> boot:testcases. The command exited 2: the images are staged and the machine was not touched.request.txtcarries each image's sha256 and the judge's command:testcases76ba2071f6cc…2519de2ca,testcases-watchdog8ff7ab7411e7…a9d086c98a29.Unsure
NODE_RESERVE= 16 is picked, not measured against DATA's depth (recorded in the full-volume issue).fileserver: this machine has no DATA partition; /apps, /config, /home and /state are in memory), so the T14 readings ran only boot and ROOT's read-only mount; the guest suite under QEMU is the only device-level run of DATA's read-write path. No timing claim is made.Net lines (
git diff --shortstat origin/main...HEAD, at b028cc8 against 8e3a813): 10 files, +1303 −362. By path:bcachefs/src+495 −279 (a few of thembtree.rstests),bcachefs/tests+412 −45,userland/fileserver/src/data.rs+280 −38 (mostly its tests),issues/+116.🤖 Generated with Claude Code
https://claude.ai/code/session_017cSFvbD35xJ2kGANVdm23C