bcachefs: a split of a tree's first child keeps its siblings after it

lib/bcachefs/src/btree.rs

@@ -143,7 +143,9 @@ pub fn value_fits(value_len: usize) -> bool {
     disk_size(value_len) <= MAX_ENTRY_SIZE
 }
 
-/// A child pointer: the minimum key of the subtree, and the block it lives in.
+/// A child pointer: its separator, and the block the subtree lives in. Every
+/// key of the subtree is at least the separator and below the next child's;
+/// the first child's is never read ([`child_index`]).
 #[derive(Debug, Clone, Copy)]
 pub struct Child {
     pub key: Key,
@@ -361,16 +363,17 @@ fn write_entry(b: &mut [u8; BLOCK_SIZE], offset: usize, key: &Key, value: &[u8])
 
 /// The child to descend into for `key`.
 ///
-/// Interior children are sorted and each key is the minimum key of that
-/// 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.
+/// The children after the first are in order of their separators, so the
+/// answer is the last of them whose separator is `<= key`, or the first child
+/// for a key below them all. The first child's own key goes unread: it is
+/// below every key routed to it only until an insert makes a new least 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)
+    children.iter().skip(1).take_while(|c| c.key <= *key).count()
 }
 
 /// The block a node the operation in progress changed is written to: its
@@ -547,10 +550,10 @@ pub fn insert(
         .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 }];
+    // A first child's key is never read (`child_index`).
+    let mut children = alloc::vec![Child { key: Key::ZERO, block: at }];
     children.extend(split);
     Node::Interior { level, children }.write(io, new_root_block)?;
 
@@ -589,13 +592,8 @@ fn insert_recursive(
                 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);
-            }
+            // The siblings hold the keys of `idx` past those it kept, in order.
+            children.splice(idx + 1..idx + 1, split);
             write_or_split(io, alloc, block, Node::Interior { level, children })
         }
     }
@@ -685,17 +683,6 @@ fn pack(entries: Vec<Entry>) -> Vec<Vec<Entry>> {
     nodes
 }
 
-/// Find the minimum key in a subtree.
-fn min_key(io: &dyn BlockIO, block: BlockNum, depth: Depth) -> Result<Key, FsError> {
-    match Node::read(io, block)? {
-        Node::Leaf(entries) => Ok(entries.first().map_or(Key::ZERO, |e| e.key)),
-        Node::Interior { children, .. } => {
-            let first = children.first().ok_or(FsError::CorruptedNode(block))?.block;
-            min_key(io, first, depth.descend(block)?)
-        }
-    }
-}
-
 #[cfg(test)]
 mod tests {
     use super::*;
@@ -843,6 +830,125 @@ mod tests {
         assert_eq!(alloc.free_blocks, 1, "the first sibling's block went back");
     }
 
+    /// Keys below every key the tree holds go down its leftmost spine, into
+    /// child 0 of every node on it. When that child splits, its siblings take
+    /// keys below the one child 0 was split off at, and every key the tree
+    /// holds is still found by its own search afterwards.
+    #[test]
+    fn keys_below_the_tree_s_least_split_its_first_child_and_every_key_is_still_found() {
+        let io = crate::block_io::VecBlockIO::new(4096);
+        let mut alloc = BitmapAllocator::format(&io, BlockNum::new(0), 1, 4096, 1).unwrap();
+        let mut root = alloc.alloc_block(&io).unwrap();
+        Node::Leaf(Vec::new()).write(&io, root).unwrap();
+        let key = |name_hash: u64| Key { name_hash, name_hash_hi: 0, key_type: KeyType::File };
+        // Above, then ever further below, so the second run splits child 0.
+        let keys: Vec<u64> = (0..400).map(|i| 1_000_000 + i).chain((0..800).map(|i| 999_999 - i)).collect();
+        for &k in &keys {
+            root = insert(&io, &mut alloc, root, Entry { key: key(k), value: vec![k as u8; 200] }).unwrap();
+        }
+        assert!(Node::read(&io, root).unwrap().level() >= 1, "the tree never grew past a leaf");
+        let lost: Vec<u64> = keys.iter().copied().filter(|&k| search(&io, root, &key(k)).unwrap().is_none()).collect();
+        assert!(lost.is_empty(), "{} of {} keys are not found, {:?} first", lost.len(), keys.len(), lost.first());
+    }
+
+    /// xorshift64*: the model test's whole source of choice, so one seed is
+    /// one run.
+    struct Rng(u64);
+
+    impl Rng {
+        fn next(&mut self) -> u64 {
+            self.0 ^= self.0 >> 12;
+            self.0 ^= self.0 << 25;
+            self.0 ^= self.0 >> 27;
+            self.0.wrapping_mul(0x2545_F491_4F6C_DD1D)
+        }
+
+        fn below(&mut self, n: u64) -> u64 {
+            self.next() % n
+        }
+    }
+
+    /// The tree against a plain map of what each operation made it: new keys,
+    /// replacements whose values change size, deletes, a run of ever lower
+    /// keys down the leftmost spine, and values so large a leaf packs three
+    /// ways. Every key the map holds is found with its value, every key it
+    /// deleted is not, and a walk yields exactly the map's keys, strictly
+    /// ascending, which holds the whole tree's order and not only the keys a
+    /// search probes.
+    #[test]
+    fn the_tree_agrees_with_a_map_through_inserts_replacements_and_deletes() {
+        let io = crate::block_io::VecBlockIO::new(1 << 15);
+        let mut alloc = BitmapAllocator::format(&io, BlockNum::new(0), 1, 1 << 15, 1).unwrap();
+        let mut root = alloc.alloc_block(&io).unwrap();
+        Node::Leaf(Vec::new()).write(&io, root).unwrap();
+        let mut rng = Rng(0x9E37_79B9_7F4A_7C15);
+        let mut model: alloc::collections::BTreeMap<Key, Vec<u8>> = alloc::collections::BTreeMap::new();
+        let mut gone: Vec<Key> = Vec::new();
+        let big = MAX_ENTRY_SIZE - KEY_HEADER_SIZE;
+        let value = |rng: &mut Rng| {
+            let len = match rng.below(50) {
+                0 => big,
+                1..=4 => 1000 + rng.below(1000) as usize,
+                _ => 8 + rng.below(200) as usize,
+            };
+            vec![rng.next() as u8; len]
+        };
+        let key = |name_hash: u64, hi: u64| Key { name_hash, name_hash_hi: hi, key_type: KeyType::File };
+
+        let check = |root: BlockNum, model: &alloc::collections::BTreeMap<Key, Vec<u8>>, gone: &[Key], at: usize| {
+            for (k, v) in model {
+                assert_eq!(search(&io, root, k).unwrap().as_ref(), Some(v), "op {at}: {k:?} is not found as written");
+            }
+            for k in gone.iter().filter(|k| !model.contains_key(k)) {
+                assert_eq!(search(&io, root, k).unwrap(), None, "op {at}: deleted {k:?} is found");
+            }
+            let mut walked = Vec::new();
+            let walk = for_each_live(&io, root, &mut |e| {
+                walked.push(e.key);
+                Ok(ControlFlow::Continue(()))
+            });
+            assert!(walk.unwrap().is_continue(), "op {at}: the walk did not end");
+            assert!(walked.windows(2).all(|w| w[0] < w[1]), "op {at}: the walk is not strictly ascending");
+            assert!(walked.iter().eq(model.keys()), "op {at}: the walk names other keys than the map");
+        };
+
+        for op in 0..6000 {
+            let roll = rng.below(10);
+            if (2000..2600).contains(&op) {
+                // Below the least key the tree holds, each time.
+                let least = model.keys().next().map_or(u64::MAX, |k| k.name_hash);
+                let k = key(least.saturating_sub(1 + rng.below(3)), rng.next());
+                let v = value(&mut rng);
+                root = insert(&io, &mut alloc, root, Entry { key: k, value: v.clone() }).unwrap();
+                model.insert(k, v);
+            } else if roll < 6 || model.is_empty() {
+                let k = key(rng.next(), rng.next());
+                let v = value(&mut rng);
+                root = insert(&io, &mut alloc, root, Entry { key: k, value: v.clone() }).unwrap();
+                model.insert(k, v);
+            } else {
+                let k = *model.keys().nth(rng.below(model.len() as u64) as usize).unwrap();
+                if roll < 8 {
+                    let v = value(&mut rng);
+                    root = insert(&io, &mut alloc, root, Entry { key: k, value: v.clone() }).unwrap();
+                    model.insert(k, v);
+                } else {
+                    let (at, old) = delete(&io, &mut alloc, root, &k)
+                        .unwrap()
+                        .unwrap_or_else(|| panic!("op {op}: the tree has no {k:?}, which the map holds, to delete"));
+                    assert_eq!(Some(old), model.remove(&k));
+                    root = at;
+                    gone.push(k);
+                }
+            }
+            if op % 500 == 499 {
+                check(root, &model, &gone, op);
+            }
+        }
+        check(root, &model, &gone, 6000);
+        assert!(Node::read(&io, root).unwrap().level() >= 2, "the tree never reached a second interior level");
+    }
+
     #[test]
     fn a_descent_gives_up_before_it_runs_out_of_stack() {
         let mut depth = Depth::ROOT;

lib/bcachefs/src/superblock.rs

@@ -5,7 +5,7 @@ use crate::crc32c::crc32c;
 use crate::fs::FsError;
 
 pub const MAGIC: [u8; 4] = *b"BCFS";
-pub const VERSION: u32 = 1;
+pub const VERSION: u32 = 2;
 
 /// The designation stamp: the *only* thing that authorises ToyOS to destroy
 /// what is on a block device.