From c56b599f8df8303191a004d2b55df6d1634540e6 Mon Sep 17 00:00:00 2001 From: Matt Keeter Date: Fri, 14 Mar 2025 10:16:14 -0400 Subject: [PATCH 1/3] Make ImpactedBlocks absolute --- .../proptest-regressions/impacted_blocks.txt | 8 + common/src/impacted_blocks.rs | 500 ++++++------------ upstairs/proptest-regressions/active_jobs.txt | 8 + upstairs/src/active_jobs.rs | 165 ++---- upstairs/src/deferred.rs | 2 +- upstairs/src/downstairs.rs | 375 ++++--------- upstairs/src/test.rs | 53 +- upstairs/src/upstairs.rs | 22 +- 8 files changed, 334 insertions(+), 799 deletions(-) create mode 100644 common/proptest-regressions/impacted_blocks.txt create mode 100644 upstairs/proptest-regressions/active_jobs.txt diff --git a/common/proptest-regressions/impacted_blocks.txt b/common/proptest-regressions/impacted_blocks.txt new file mode 100644 index 000000000..29adff553 --- /dev/null +++ b/common/proptest-regressions/impacted_blocks.txt @@ -0,0 +1,8 @@ +# Seeds for failure cases proptest has generated in the past. It is +# automatically read and these particular cases re-run before any +# novel cases are generated. +# +# It is recommended to check this file in to source control so that +# everyone who runs the test benefits from these saved cases. +cc a0eeb2a4e85b48b2bc28fd4353784f7f4a587cd07b624761558a41e2cc981e6c # shrinks to input = _IblocksNewPanicsForFlippedPolarityArgs { start_block: 198292286474641, end_block: 198292286474884 } +cc 71508540f37d62051cb8623d470e89dd6af7571763083f694fcf728e43550bfe # shrinks to input = _IblocksBlocksIteratesOverAllBlocksArgs { extent_count: 2, extent_size: 76, start_eid: 0, start_block: 56, end_eid: 1, end_block: 63 } diff --git a/common/src/impacted_blocks.rs b/common/src/impacted_blocks.rs index 88d9d9f2d..5fef1afee 100644 --- a/common/src/impacted_blocks.rs +++ b/common/src/impacted_blocks.rs @@ -13,43 +13,34 @@ pub struct ImpactedAddr { /// Store a list of impacted blocks for later job dependency calculation #[derive(Copy, Clone, Debug, PartialEq, Eq)] pub enum ImpactedBlocks { - // Note that there is no extent size stored here. This is intentional! If - // we stored an extent size we'd need to do all sorts of messy checks - // when comparing two ImpactedBlocks ranges to make sure their extent - // sizes match. Instead, if something wants a list of blocks, they tell - // us how big they think an extent is, and we give them what they want. /// No blocks are impacted Empty, /// First impacted block and last impacted block (inclusive!) - InclusiveRange(ImpactedAddr, ImpactedAddr), + InclusiveRange(BlockIndex, BlockIndex), } -/// An iteration over the blocks in an ImpactedBlocks range +/// An iteration over the blocks in an [`ImpactedBlocks`] range pub struct ImpactedBlockIter { - extent_size: u64, - active_range: Option<(ImpactedAddr, ImpactedAddr)>, + active_range: Option<(BlockIndex, BlockIndex)>, } impl Iterator for ImpactedBlockIter { - type Item = (ExtentId, BlockOffset); + type Item = BlockIndex; #[inline] fn next(&mut self) -> Option { match &mut self.active_range { None => None, Some((first, last)) => { - let result = (first.extent_id, first.block); + let result = *first; // If next == last, then the iterator is now done. Otherwise, // increment next. if first == last { self.active_range = None - } else if first.block.0 == self.extent_size - 1 { - first.block.0 = 0; - first.extent_id += 1; } else { - first.block.0 += 1; + first.0 += 1; } Some(result) @@ -69,8 +60,7 @@ impl Iterator for ImpactedBlockIter { where Self: Sized, { - self.active_range - .map(|(_, last)| (last.extent_id, last.block)) + self.active_range.map(|(_, last)| last) } } @@ -78,11 +68,7 @@ impl ExactSizeIterator for ImpactedBlockIter { fn len(&self) -> usize { match self.active_range { None => 0, - Some((fst, lst)) => { - let extents = (lst.extent_id - fst.extent_id) as u64; - (extents * self.extent_size + lst.block.0 - fst.block.0 + 1) - as usize - } + Some((fst, lst)) => (lst.0 + 1 - fst.0) as usize, } } } @@ -93,17 +79,14 @@ impl DoubleEndedIterator for ImpactedBlockIter { match &mut self.active_range { None => None, Some((first, last)) => { - let result = (last.extent_id, last.block); + let result = *last; // If first == last, then the iterator is now done. Otherwise, // increment first. if first == last { self.active_range = None - } else if last.block.0 == 0 { - last.block.0 = self.extent_size - 1; - last.extent_id -= 1; } else { - last.block.0 -= 1; + last.0 -= 1; } Some(result) @@ -117,10 +100,7 @@ impl FusedIterator for ImpactedBlockIter {} impl ImpactedBlocks { /// Create a new ImpactedBlocks range from first and last impacted blocks /// (inclusive). Panics if last_impacted < first_impacted. - pub fn new( - first_impacted: ImpactedAddr, - last_impacted: ImpactedAddr, - ) -> Self { + pub fn new(first_impacted: BlockIndex, last_impacted: BlockIndex) -> Self { assert!(last_impacted >= first_impacted); ImpactedBlocks::InclusiveRange(first_impacted, last_impacted) } @@ -130,39 +110,27 @@ impl ImpactedBlocks { } /// Returns the first impacted address - pub fn start(&self) -> Option { + pub fn start(&self) -> Option { match self { ImpactedBlocks::InclusiveRange(a, _) => Some(*a), ImpactedBlocks::Empty => None, } } - /// Create a new ImpactedBlocks range starting at a given offset, and - /// stretching n_blocks further into the extent. Panics an error if - /// `extent_size` is 0. Returns ImpactedBlocks::Empty if n_blocks is 0. - pub fn from_offset( - extent_size: u64, - first_impacted: ImpactedAddr, - n_blocks: u64, - ) -> Self { - assert!(extent_size > 0); - + /// Create a new [`ImpactedBlocks`] range starting at a given block, and + /// stretching `n_blocks` further into the extent. Returns + /// [`ImpactedBlocks::Empty`] if `n_blocks` is 0. + pub fn from_offset(start_block: BlockIndex, n_blocks: u64) -> Self { if n_blocks == 0 { - return ImpactedBlocks::Empty; - } - - // So because we're inclusive, if we have 1 block then the first - // impacted should be the same as the last impacted. If we have - // 2 blocks, it's +1. etc. - let ending_block = n_blocks + first_impacted.block.0 - 1; - - let last_impacted = ImpactedAddr { - extent_id: first_impacted.extent_id - + (ending_block / extent_size) as u32, - block: BlockOffset(ending_block % extent_size), - }; + ImpactedBlocks::Empty + } else { + // So because we're inclusive, if we have 1 block then the first + // impacted should be the same as the last impacted. If we have + // 2 blocks, it's +1. etc. + let ending_block = BlockIndex(n_blocks + start_block.0 - 1); - ImpactedBlocks::InclusiveRange(first_impacted, last_impacted) + ImpactedBlocks::InclusiveRange(start_block, ending_block) + } } pub fn is_empty(&self) -> bool { @@ -170,31 +138,33 @@ impl ImpactedBlocks { } /// Return a range of impacted extents - pub fn extents(&self) -> Option> { + pub fn extents( + &self, + ddef: &RegionDefinition, + ) -> Option> { + // XXX return a range of `ExtentId` once `std::iter::Step` is stable + let blocks_per_extent = ddef.extent_size().value; match self { ImpactedBlocks::Empty => None, /* empty range */ - ImpactedBlocks::InclusiveRange(fst, lst) => { - Some(fst.extent_id.0..=lst.extent_id.0) - } + ImpactedBlocks::InclusiveRange(fst, lst) => Some( + ((fst.0 / blocks_per_extent) as u32) + ..=((lst.0 / blocks_per_extent) as u32), + ), } } - pub fn blocks(&self, ddef: &RegionDefinition) -> ImpactedBlockIter { - let extent_size = ddef.extent_size().value; + pub fn blocks(&self) -> ImpactedBlockIter { let active_range = match self { ImpactedBlocks::Empty => None, ImpactedBlocks::InclusiveRange(fst, lst) => Some((*fst, *lst)), }; - ImpactedBlockIter { - extent_size, - active_range, - } + ImpactedBlockIter { active_range } } /// Returns the number of impacted blocks - pub fn len(&self, ddef: &RegionDefinition) -> usize { - self.blocks(ddef).len() + pub fn len(&self) -> usize { + self.blocks().len() } } @@ -213,7 +183,7 @@ impl ImpactedBlocks { /// The example offset and length above spans 7 blocks over two extents (where /// the numbers at the bottom of the diagram are block numbers). /// -/// Return an ImpactedBlocks object that stores the affected region as a range +/// Return an [`ImpactedBlocks`] object that stores the affected region as a range /// of [`ImpactedAddr`] values (which are `(ExtentId, BlockOffset)` tuples) pub fn extent_from_offset( ddef: &RegionDefinition, @@ -235,28 +205,19 @@ pub fn extent_from_offset( <= ddef.extent_count() as u128 * extent_size as u128 ); - let fst = ImpactedAddr { - extent_id: ExtentId((offset.0 / extent_size) as u32), - block: BlockOffset(offset.0 % extent_size), - }; - - ImpactedBlocks::from_offset(extent_size, fst, num_blocks) + ImpactedBlocks::from_offset(offset, num_blocks) } +/// Converts an extent to a range of impacted blocks pub fn extent_to_impacted_blocks( ddef: &RegionDefinition, eid: ExtentId, ) -> ImpactedBlocks { assert!(eid.0 < ddef.extent_count()); - let one = ImpactedAddr { - extent_id: eid, - block: BlockOffset(0), - }; - let two = ImpactedAddr { - extent_id: eid, - block: BlockOffset(ddef.extent_size().value - 1), - }; - ImpactedBlocks::InclusiveRange(one, two) + let extent_size = ddef.extent_size().value; + let start = BlockIndex(extent_size * u64::from(eid.0)); + let end = BlockIndex(start.0 + extent_size - 1); + ImpactedBlocks::InclusiveRange(start, end) } #[cfg(test)] @@ -267,10 +228,6 @@ mod test { use std::panic::UnwindSafe; use test_strategy::{proptest, Arbitrary}; - fn extent_tuple(eid: u32, offset: u64) -> (ExtentId, BlockOffset) { - (ExtentId(eid), BlockOffset(offset)) - } - fn basic_region_definition( extent_size: u32, extent_count: u32, @@ -291,23 +248,23 @@ mod test { // First assert_eq!( extent_to_impacted_blocks(ddef, ExtentId(0)) - .blocks(ddef) + .blocks() .collect::>(), - vec![extent_tuple(0, 0), extent_tuple(0, 1)], + vec![BlockIndex(0), BlockIndex(1)], ); // Somewhere in the middle assert_eq!( extent_to_impacted_blocks(ddef, ExtentId(3)) - .blocks(ddef) + .blocks() .collect::>(), - vec![extent_tuple(3, 0), extent_tuple(3, 1)], + vec![BlockIndex(6), BlockIndex(7)], ); // Last assert_eq!( extent_to_impacted_blocks(ddef, ExtentId(9)) - .blocks(ddef) + .blocks() .collect::>(), - vec![extent_tuple(9, 0), extent_tuple(9, 1)], + vec![BlockIndex(18), BlockIndex(19)], ); } @@ -319,52 +276,52 @@ mod test { assert_eq!( extent_to_impacted_blocks(ddef, ExtentId(0)) - .blocks(ddef) + .blocks() .collect::>(), vec![ - extent_tuple(0, 0), - extent_tuple(0, 1), - extent_tuple(0, 2), - extent_tuple(0, 3), - extent_tuple(0, 4), - extent_tuple(0, 5), - extent_tuple(0, 6), - extent_tuple(0, 7), - extent_tuple(0, 8), + BlockIndex(0), + BlockIndex(1), + BlockIndex(2), + BlockIndex(3), + BlockIndex(4), + BlockIndex(5), + BlockIndex(6), + BlockIndex(7), + BlockIndex(8), ], ); // Somewhere in the middle assert_eq!( extent_to_impacted_blocks(ddef, ExtentId(2)) - .blocks(ddef) + .blocks() .collect::>(), vec![ - extent_tuple(2, 0), - extent_tuple(2, 1), - extent_tuple(2, 2), - extent_tuple(2, 3), - extent_tuple(2, 4), - extent_tuple(2, 5), - extent_tuple(2, 6), - extent_tuple(2, 7), - extent_tuple(2, 8), + BlockIndex(18), + BlockIndex(19), + BlockIndex(20), + BlockIndex(21), + BlockIndex(22), + BlockIndex(23), + BlockIndex(24), + BlockIndex(25), + BlockIndex(26), ], ); // Last assert_eq!( extent_to_impacted_blocks(ddef, ExtentId(4)) - .blocks(ddef) + .blocks() .collect::>(), vec![ - extent_tuple(4, 0), - extent_tuple(4, 1), - extent_tuple(4, 2), - extent_tuple(4, 3), - extent_tuple(4, 4), - extent_tuple(4, 5), - extent_tuple(4, 6), - extent_tuple(4, 7), - extent_tuple(4, 8), + BlockIndex(36), + BlockIndex(37), + BlockIndex(38), + BlockIndex(39), + BlockIndex(40), + BlockIndex(41), + BlockIndex(42), + BlockIndex(43), + BlockIndex(44), ], ); } @@ -378,78 +335,63 @@ mod test { // Test block size, less than extent size assert_eq!( extent_from_offset(ddef, BlockIndex(0), 1) - .blocks(ddef) + .blocks() .collect::>(), - vec![extent_tuple(0, 0)], + vec![BlockIndex(0)], ); // Test greater than block size, less than extent size assert_eq!( extent_from_offset(ddef, BlockIndex(0), 2) - .blocks(ddef) + .blocks() .collect::>(), - vec![extent_tuple(0, 0), extent_tuple(0, 1)], + vec![BlockIndex(0), BlockIndex(1)], ); // Test greater than extent size assert_eq!( extent_from_offset(ddef, BlockIndex(0), 4) - .blocks(ddef) + .blocks() .collect::>(), - vec![ - extent_tuple(0, 0), - extent_tuple(0, 1), - extent_tuple(1, 0), - extent_tuple(1, 1), - ], + vec![BlockIndex(0), BlockIndex(1), BlockIndex(2), BlockIndex(3)], ); // Test offsets assert_eq!( extent_from_offset(ddef, BlockIndex(1), 4) - .blocks(ddef) + .blocks() .collect::>(), - vec![ - extent_tuple(0, 1), - extent_tuple(1, 0), - extent_tuple(1, 1), - extent_tuple(2, 0), - ], + vec![BlockIndex(1), BlockIndex(2), BlockIndex(3), BlockIndex(4)], ); assert_eq!( extent_from_offset(ddef, BlockIndex(2), 4) - .blocks(ddef) + .blocks() .collect::>(), - vec![ - extent_tuple(1, 0), - extent_tuple(1, 1), - extent_tuple(2, 0), - extent_tuple(2, 1), - ], + vec![BlockIndex(2), BlockIndex(3), BlockIndex(4), BlockIndex(5)], ); assert_eq!( extent_from_offset(ddef, BlockIndex(2), 16) - .blocks(ddef) + .blocks() .collect::>(), vec![ - extent_tuple(1, 0), - extent_tuple(1, 1), - extent_tuple(2, 0), - extent_tuple(2, 1), - extent_tuple(3, 0), - extent_tuple(3, 1), - extent_tuple(4, 0), - extent_tuple(4, 1), - extent_tuple(5, 0), - extent_tuple(5, 1), - extent_tuple(6, 0), - extent_tuple(6, 1), - extent_tuple(7, 0), - extent_tuple(7, 1), - extent_tuple(8, 0), - extent_tuple(8, 1), + BlockIndex(2), + BlockIndex(3), + BlockIndex(4), + BlockIndex(5), + BlockIndex(6), + BlockIndex(7), + BlockIndex(8), + BlockIndex(9), + BlockIndex(10), + BlockIndex(11), + BlockIndex(12), + BlockIndex(13), + BlockIndex(14), + BlockIndex(15), + BlockIndex(16), + BlockIndex(17), ], ); } @@ -464,9 +406,9 @@ mod test { BlockIndex(2), // offset 1, // num_blocks ) - .blocks(ddef) + .blocks() .collect::>(), - vec![extent_tuple(1, 0)] + vec![BlockIndex(2)] ); assert_eq!( @@ -475,9 +417,9 @@ mod test { BlockIndex(2), // offset 2, // num_blocks ) - .blocks(ddef) + .blocks() .collect::>(), - vec![extent_tuple(1, 0), extent_tuple(1, 1)] + vec![BlockIndex(2), BlockIndex(3)] ); assert_eq!( @@ -486,27 +428,9 @@ mod test { BlockIndex(2), // offset 3, // num_blocks ) - .blocks(ddef) + .blocks() .collect::>(), - vec![extent_tuple(1, 0), extent_tuple(1, 1), extent_tuple(2, 0)] - ); - } - - #[test] - #[should_panic] - /// If we create an impacted range where the last block comes before the - /// first block, that range should be empty. - fn test_new_range_panics_when_last_extent_before_first() { - // Extent id ordering works - let _ = ImpactedBlocks::new( - ImpactedAddr { - extent_id: ExtentId(1), - block: BlockOffset(0), - }, - ImpactedAddr { - extent_id: ExtentId(0), - block: BlockOffset(0), - }, + vec![BlockIndex(2), BlockIndex(3), BlockIndex(4)] ); } @@ -516,16 +440,7 @@ mod test { /// first block, that range should be empty. fn test_new_range_panics_when_last_block_before_first() { // Block ordering works - let _ = ImpactedBlocks::new( - ImpactedAddr { - extent_id: ExtentId(0), - block: BlockOffset(1), - }, - ImpactedAddr { - extent_id: ExtentId(0), - block: BlockOffset(0), - }, - ); + let _ = ImpactedBlocks::new(BlockIndex(1), BlockIndex(0)); } #[test] @@ -536,83 +451,37 @@ mod test { const EXTENT_SIZE: u64 = 512; // Test that extent-aligned creation works - let fst = ImpactedAddr { - extent_id: ExtentId(2), - block: BlockOffset(20), - }; - let control = ImpactedBlocks::new( - fst, - ImpactedAddr { - extent_id: ExtentId(4), - block: BlockOffset(19), - }, - ); - assert_eq!( - control, - ImpactedBlocks::from_offset(EXTENT_SIZE, fst, EXTENT_SIZE * 2) - ); + let fst = BlockIndex(EXTENT_SIZE * 2 + 20); + let control = + ImpactedBlocks::new(fst, BlockIndex(EXTENT_SIZE * 4 + 19)); + assert_eq!(control, ImpactedBlocks::from_offset(fst, EXTENT_SIZE * 2)); // Single block within a single extent - let fst = ImpactedAddr { - extent_id: ExtentId(2), - block: BlockOffset(20), - }; - let control = ImpactedBlocks::new( - fst, - ImpactedAddr { - extent_id: ExtentId(2), - block: BlockOffset(20), - }, - ); - assert_eq!(control, ImpactedBlocks::from_offset(EXTENT_SIZE, fst, 1)); + let fst = BlockIndex(EXTENT_SIZE * 2 + 20); + let control = + ImpactedBlocks::new(fst, BlockIndex(EXTENT_SIZE * 2 + 20)); + assert_eq!(control, ImpactedBlocks::from_offset(fst, 1)); // Ending on the end of an extent should work - let fst = ImpactedAddr { - extent_id: ExtentId(2), - block: BlockOffset(20), - }; - let control = ImpactedBlocks::new( - fst, - ImpactedAddr { - extent_id: ExtentId(2), - block: BlockOffset(EXTENT_SIZE - 1), - }, - ); + let fst = BlockIndex(EXTENT_SIZE * 2 + 20); + let lst = BlockIndex(EXTENT_SIZE * 2 + EXTENT_SIZE - 1); + let control = ImpactedBlocks::new(fst, lst); assert_eq!( control, - ImpactedBlocks::from_offset( - EXTENT_SIZE, - fst, - EXTENT_SIZE - fst.block.0, - ) + ImpactedBlocks::from_offset(fst, lst.0 - fst.0 + 1) ); // Ending on the start of an extent should work - let fst = ImpactedAddr { - extent_id: ExtentId(2), - block: BlockOffset(20), - }; - let control = ImpactedBlocks::new( - fst, - ImpactedAddr { - extent_id: ExtentId(3), - block: BlockOffset(0), - }, - ); + let fst = BlockIndex(EXTENT_SIZE * 2 + 20); + let lst = BlockIndex(EXTENT_SIZE * 3); + let control = ImpactedBlocks::new(fst, lst); assert_eq!( control, - ImpactedBlocks::from_offset( - EXTENT_SIZE, - fst, - EXTENT_SIZE - fst.block.0 + 1, - ) + ImpactedBlocks::from_offset(fst, lst.0 - fst.0 + 1) ); // 0-length should be empty - assert_eq!( - ImpactedBlocks::from_offset(EXTENT_SIZE, fst, 0), - ImpactedBlocks::Empty - ); + assert_eq!(ImpactedBlocks::from_offset(fst, 0), ImpactedBlocks::Empty); } // Proptest time @@ -713,7 +582,16 @@ mod test { ), }; - ImpactedBlocks::InclusiveRange(left_addr, right_addr) + let left_block = BlockIndex( + u64::from(left_addr.extent_id.0) * extent_size as u64 + + left_addr.block.0, + ); + let right_block = BlockIndex( + u64::from(right_addr.extent_id.0) * extent_size as u64 + + right_addr.block.0, + ); + + ImpactedBlocks::InclusiveRange(left_block, right_block) } } } @@ -750,62 +628,29 @@ mod test { #[proptest] fn iblocks_from_offset_is_empty_for_zero_blocks( - #[strategy(1..=u64::MAX)] extent_size: u64, + #[strategy(1..=u32::MAX as u64)] extent_size: u64, start_eid: u32, #[strategy(0..#extent_size)] start_block: u64, ) { prop_assert_eq!( ImpactedBlocks::from_offset( - extent_size, - ImpactedAddr { - extent_id: ExtentId(start_eid), - block: BlockOffset(start_block), - }, + BlockIndex(u64::from(start_eid) * extent_size + start_block), 0 ), ImpactedBlocks::Empty ); } - #[proptest] - fn iblocks_from_offset_with_zero_extent_size_panics( - start_eid: u32, - start_block: u64, - n_blocks: u64, - ) { - prop_should_panic(|| { - ImpactedBlocks::from_offset( - 0, - ImpactedAddr { - extent_id: ExtentId(start_eid), - block: BlockOffset(start_block), - }, - n_blocks, - ) - })?; - } - #[proptest] /// Make sure that when the right address is less than the left address, /// ImpactedBlocks::new() panics fn iblocks_new_panics_for_flipped_polarity( - #[strategy(0..=u32::MAX - 1)] start_eid: u32, #[strategy(0..=u64::MAX - 1)] start_block: u64, - - #[strategy(#start_eid + 1 ..= u32::MAX)] end_eid: u32, #[strategy(#start_block + 1 ..= u64::MAX)] end_block: u64, ) { - let start_addr = ImpactedAddr { - extent_id: ExtentId(start_eid), - block: BlockOffset(start_block), - }; - - let end_addr = ImpactedAddr { - extent_id: ExtentId(end_eid), - block: BlockOffset(end_block), - }; - - prop_should_panic(|| ImpactedBlocks::new(end_addr, start_addr))?; + prop_should_panic(|| { + ImpactedBlocks::new(BlockIndex(end_block), BlockIndex(start_block)) + })?; } #[proptest] @@ -825,14 +670,8 @@ mod test { // Set up our iblocks let iblocks = ImpactedBlocks::new( - ImpactedAddr { - extent_id: start_eid, - block: BlockOffset(start_block), - }, - ImpactedAddr { - extent_id: end_eid, - block: BlockOffset(end_block), - }, + BlockIndex(u64::from(start_eid.0) * extent_size + start_block), + BlockIndex(u64::from(end_eid.0) * extent_size + end_block), ); let mut ddef = RegionDefinition::default(); @@ -842,23 +681,19 @@ mod test { // Generate reference data let mut expected_addresses = Vec::new(); - let mut cur_eid = start_eid; - let mut cur_block = start_block; + let mut cur_block = u64::from(start_eid.0) * extent_size + start_block; + let end_block = u64::from(end_eid.0) * extent_size + end_block; loop { - expected_addresses.push((cur_eid, BlockOffset(cur_block))); - if cur_eid == end_eid && cur_block == end_block { + expected_addresses.push(BlockIndex(cur_block)); + if cur_block == end_block { break; } cur_block += 1; - if cur_block == extent_size { - cur_block = 0; - cur_eid += 1; - } } prop_assert_eq!( - iblocks.blocks(&ddef).collect::>(), + iblocks.blocks().collect::>(), expected_addresses ); } @@ -874,11 +709,8 @@ mod test { ) { let (ddef, iblocks) = region_and_iblocks; - let (first_eid, first_block) = iblocks.blocks(&ddef).next().unwrap(); - let first = BlockIndex( - first_block.0 + first_eid.0 as u64 * ddef.extent_size().value, - ); - let num_blocks = iblocks.len(&ddef) as u64; + let first = iblocks.blocks().next().unwrap(); + let num_blocks = iblocks.len() as u64; prop_assert_eq!(extent_from_offset(&ddef, first, num_blocks), iblocks); } @@ -917,24 +749,4 @@ mod test { prop_should_panic(|| extent_from_offset(&ddef, first, num_blocks))?; } - - #[proptest] - fn iblocks_extents_returns_correct_extents( - eid_a: u32, - block_a: u64, - - #[strategy(#eid_a..=u32::MAX)] eid_b: u32, - #[strategy(#block_a..=u64::MAX)] block_b: u64, - ) { - let addr_a = ImpactedAddr { - extent_id: ExtentId(eid_a), - block: BlockOffset(block_a), - }; - let addr_b = ImpactedAddr { - extent_id: ExtentId(eid_b), - block: BlockOffset(block_b), - }; - let iblocks = ImpactedBlocks::new(addr_a, addr_b); - prop_assert_eq!(iblocks.extents(), Some(eid_a..=eid_b)); - } } diff --git a/upstairs/proptest-regressions/active_jobs.txt b/upstairs/proptest-regressions/active_jobs.txt new file mode 100644 index 000000000..4b1ba0ad9 --- /dev/null +++ b/upstairs/proptest-regressions/active_jobs.txt @@ -0,0 +1,8 @@ +# Seeds for failure cases proptest has generated in the past. It is +# automatically read and these particular cases re-run before any +# novel cases are generated. +# +# It is recommended to check this file in to source control so that +# everyone who runs the test benefits from these saved cases. +cc 124286ecc9750bc4c0f7f95be4cec889b7f1a29a8a3cd5714cc834e9144f518e # shrinks to a = InclusiveRange(BlockIndex(64), BlockIndex(64)), a_type = true, b = Empty, b_type = false, c = Empty, c_type = false, remove = 1001, read = InclusiveRange(BlockIndex(0), BlockIndex(64)), read_type = false +cc 86445fd7899695fd2fc64ef55b3acc14a34f466893469135a4119f94dea74940 # shrinks to a = Empty, a_type = false, b = InclusiveRange(BlockIndex(29), BlockIndex(29)), b_type = true, c = Empty, c_type = false, read = InclusiveRange(BlockIndex(0), BlockIndex(29)), read_type = false diff --git a/upstairs/src/active_jobs.rs b/upstairs/src/active_jobs.rs index 6e21c3e56..c105e638d 100644 --- a/upstairs/src/active_jobs.rs +++ b/upstairs/src/active_jobs.rs @@ -1,11 +1,9 @@ // Copyright 2023 Oxide Computer Company -use crucible_common::{BlockOffset, ExtentId}; +use crucible_common::{BlockIndex, ExtentId, RegionDefinition}; use crucible_protocol::JobId; -use crate::{ - DownstairsIO, ExtentRepairIDs, IOop, ImpactedAddr, ImpactedBlocks, -}; +use crate::{DownstairsIO, ExtentRepairIDs, IOop, ImpactedBlocks}; use std::collections::{BTreeMap, BTreeSet}; /// `ActiveJobs` tracks active jobs (and associated metadata) by job ID @@ -119,16 +117,8 @@ impl ActiveJobs { } pub fn deps_for_flush(&mut self, flush_id: JobId) -> Vec { - let blocks = ImpactedBlocks::InclusiveRange( - ImpactedAddr { - extent_id: ExtentId(0), - block: BlockOffset(0), - }, - ImpactedAddr { - extent_id: ExtentId(u32::MAX), - block: BlockOffset(u64::MAX), - }, - ); + let blocks = + ImpactedBlocks::InclusiveRange(BlockIndex(0), BlockIndex(u64::MAX)); let dep = self.block_to_active.check_range(blocks, true); self.block_to_active.insert_range(blocks, flush_id, true); dep @@ -173,16 +163,14 @@ impl ActiveJobs { &mut self, repair_ids: ExtentRepairIDs, extent: ExtentId, + ddef: &RegionDefinition, ) -> Vec { + let blocks_per_extent = ddef.extent_size().value; let blocks = ImpactedBlocks::InclusiveRange( - ImpactedAddr { - extent_id: extent, - block: BlockOffset(0), - }, - ImpactedAddr { - extent_id: extent, - block: BlockOffset(u64::MAX), - }, + BlockIndex(u64::from(extent.0) * blocks_per_extent), + BlockIndex( + u64::from(extent.0) * blocks_per_extent + blocks_per_extent - 1, + ), ); let dep = self.block_to_active.check_range(blocks, true); self.block_to_active @@ -191,7 +179,7 @@ impl ActiveJobs { } #[cfg(test)] - pub fn get_extents_for(&self, job: JobId) -> ImpactedBlocks { + pub fn get_blocks_for(&self, job: JobId) -> ImpactedBlocks { *self.block_to_active.job_to_range.get(&job).unwrap() } } @@ -217,19 +205,8 @@ struct BlockMap { /// /// The data structure must maintain the invariant that block ranges never /// overlap. - addr_to_jobs: BTreeMap, + addr_to_jobs: BTreeMap, job_to_range: BTreeMap, - - /// If we know the number of blocks in an extent, it's stored here - /// - /// This is used for optimization: for example, if there are 8 blocks per - /// extent, then `{ extent: 0, block: 8 }` represents the same block as - /// `{ extent: 1, block: 0 }`. - /// - /// If we _don't_ know blocks per extent, this still works: the former is - /// lexicographically ordered below the latter. However, there could be - /// zero-length ranges in the range map. - blocks_per_extent: Option, } impl BlockMap { @@ -246,15 +223,12 @@ impl BlockMap { fn blocks_to_range( r: ImpactedBlocks, - ) -> Option> { + ) -> Option> { match r { ImpactedBlocks::Empty => None, - ImpactedBlocks::InclusiveRange(first, last) => Some( - first..ImpactedAddr { - extent_id: last.extent_id, - block: BlockOffset(last.block.0.saturating_add(1)), - }, - ), + ImpactedBlocks::InclusiveRange(first, last) => { + Some(first..BlockIndex(last.0.saturating_add(1))) + } } } @@ -312,7 +286,7 @@ impl BlockMap { self.merge_adjacent_sections(r); } - fn insert_splits(&mut self, r: std::ops::Range) { + fn insert_splits(&mut self, r: std::ops::Range) { // Okay, this is a tricky one. The incoming range `r` can overlap with // existing ranges in a variety of ways. // @@ -360,14 +334,14 @@ impl BlockMap { /// /// If such a range exists, returns its starting address (i.e. the key to /// look it up in [`self.addr_to_jobs`]. - fn find_split_location(&self, i: ImpactedAddr) -> Option { + fn find_split_location(&self, i: BlockIndex) -> Option { match self.addr_to_jobs.range(..i).next_back() { Some((start, (end, _))) if i < *end => Some(*start), _ => None, } } - fn merge_adjacent_sections(&mut self, r: std::ops::Range) { + fn merge_adjacent_sections(&mut self, r: std::ops::Range) { // Pick a start position that's right below our modified range if // possible; otherwise, pick the first value in the map (or return if // the entire map is empty). @@ -394,11 +368,7 @@ impl BlockMap { } _ => { // Remove blocks which are pathologically empty - if (pos.block.0 == u64::MAX - || Some(pos.block.0) == self.blocks_per_extent) - && end.block.0 == 0 - && end.extent_id == pos.extent_id + 1 - { + if end.0 == pos.0 { self.addr_to_jobs.remove(&pos).unwrap(); } // Shuffle along @@ -418,9 +388,8 @@ impl BlockMap { fn iter_overlapping( &self, - r: std::ops::Range, - ) -> impl Iterator - { + r: std::ops::Range, + ) -> impl Iterator { let start = self .addr_to_jobs .range(..=r.start) @@ -460,10 +429,7 @@ impl BlockMap { .range_mut(pos..) .next() .map(|(start, _)| *start) - .unwrap_or(ImpactedAddr { - extent_id: ExtentId(u32::MAX), - block: BlockOffset(u64::MAX), - }); + .unwrap_or(BlockIndex(u64::MAX)); if next_start == pos { // Remove ourself from the existing range let (next_end, v) = @@ -605,7 +571,6 @@ impl DependencySet { #[cfg(test)] mod test { use super::*; - use crate::{Block, RegionDefinition}; use proptest::prelude::*; /// The Oracle is similar to a [`BlockMap`], but doesn't do range stuff @@ -614,23 +579,15 @@ mod test { /// This is much less efficient, but provides a ground-truth oracle for /// property-based testing. struct Oracle { - addr_to_jobs: BTreeMap, + addr_to_jobs: BTreeMap, job_to_range: BTreeMap, - ddef: RegionDefinition, } impl Oracle { fn new() -> Self { - let mut ddef = RegionDefinition::default(); - ddef.set_extent_size(Block { - value: BLOCKS_PER_EXTENT, - shift: crucible_common::MIN_SHIFT, // unused - }); - ddef.set_block_size(crucible_common::MIN_BLOCK_SIZE as u64); Self { addr_to_jobs: BTreeMap::new(), job_to_range: BTreeMap::new(), - ddef, } } fn insert_range( @@ -639,12 +596,8 @@ mod test { job: JobId, blocking: bool, ) { - for (i, b) in r.blocks(&self.ddef) { - let addr = ImpactedAddr { - extent_id: i, - block: b, - }; - let v = self.addr_to_jobs.entry(addr).or_default(); + for b in r.blocks() { + let v = self.addr_to_jobs.entry(b).or_default(); if blocking { v.insert_blocking(job) } else { @@ -655,12 +608,8 @@ mod test { } fn check_range(&self, r: ImpactedBlocks, blocking: bool) -> Vec { let mut out = BTreeSet::new(); - for (i, b) in r.blocks(&self.ddef) { - let addr = ImpactedAddr { - extent_id: i, - block: b, - }; - if let Some(s) = self.addr_to_jobs.get(&addr) { + for b in r.blocks() { + if let Some(s) = self.addr_to_jobs.get(&b) { out.extend(s.iter_jobs(blocking)); } } @@ -668,42 +617,22 @@ mod test { } fn remove_job(&mut self, job: JobId) { let r = self.job_to_range.remove(&job).unwrap(); - for (i, b) in r.blocks(&self.ddef) { - let addr = ImpactedAddr { - extent_id: i, - block: b, - }; - if let Some(s) = self.addr_to_jobs.get_mut(&addr) { + for b in r.blocks() { + if let Some(s) = self.addr_to_jobs.get_mut(&b) { s.remove(job); } } } } - // Pick an extent size that plays nice with `block_strat` - // - // This should be small enough that `block_strat` generates `ImpactedBlocks` - // that span multiple extents, but is otherwise arbitrary - const BLOCKS_PER_EXTENT: u64 = 8; - fn block_strat() -> impl Strategy { (0u64..100, 0u64..100).prop_map(|(start, len)| { if len == 0 { ImpactedBlocks::Empty } else { ImpactedBlocks::InclusiveRange( - ImpactedAddr { - extent_id: ExtentId((start / BLOCKS_PER_EXTENT) as u32), - block: BlockOffset(start % BLOCKS_PER_EXTENT), - }, - ImpactedAddr { - extent_id: ExtentId( - ((start + len - 1) / BLOCKS_PER_EXTENT) as u32, - ), - block: BlockOffset( - (start + len - 1) % BLOCKS_PER_EXTENT, - ), - }, + BlockIndex(start), + BlockIndex(start + len - 1), ) } }) @@ -717,10 +646,7 @@ mod test { c in block_strat(), c_type in any::(), read in block_strat(), read_type in any::(), ) { - let mut dut = BlockMap { - blocks_per_extent:Some(BLOCKS_PER_EXTENT), - ..BlockMap::default() - }; + let mut dut = BlockMap::default(); dut.self_check(); dut.insert_range(a, JobId(1000), a_type); dut.self_check(); @@ -747,10 +673,7 @@ mod test { remove in 1000u64..1003, read in block_strat(), read_type in any::(), ) { - let mut dut = BlockMap { - blocks_per_extent:Some(BLOCKS_PER_EXTENT), - ..BlockMap::default() - }; + let mut dut = BlockMap::default(); dut.insert_range(a, JobId(1000), a_type); dut.insert_range(b, JobId(1001), b_type); dut.insert_range(c, JobId(1002), c_type); @@ -775,10 +698,7 @@ mod test { a in block_strat(), a_type in any::(), read in block_strat(), read_type in any::(), ) { - let mut dut = BlockMap { - blocks_per_extent:Some(BLOCKS_PER_EXTENT), - ..BlockMap::default() - }; + let mut dut = BlockMap::default(); dut.insert_range(a, JobId(1000), a_type); dut.self_check(); dut.remove_job(JobId(1000)); @@ -803,11 +723,7 @@ mod test { order in Just([JobId(1000), JobId(1001), JobId(1002)]).prop_shuffle(), read in block_strat(), read_type in any::(), ) { - let mut dut = BlockMap { - blocks_per_extent:Some(BLOCKS_PER_EXTENT), - ..BlockMap::default() - }; - dut.blocks_per_extent = Some(BLOCKS_PER_EXTENT); + let mut dut = BlockMap::default(); dut.insert_range(a, JobId(1000), a_type); dut.insert_range(b, JobId(1001), b_type); dut.insert_range(c, JobId(1002), c_type); @@ -861,14 +777,7 @@ mod test { b in block_strat(), b_type in any::(), ) { let flush_range = ImpactedBlocks::InclusiveRange( - ImpactedAddr { - extent_id: ExtentId(0), - block: BlockOffset(0), - }, - ImpactedAddr { - extent_id: ExtentId(u32::MAX), - block: BlockOffset(u64::MAX), - }, + BlockIndex(0), BlockIndex(u64::MAX), ); let mut dut = BlockMap::default(); diff --git a/upstairs/src/deferred.rs b/upstairs/src/deferred.rs index 301575a64..9b0235375 100644 --- a/upstairs/src/deferred.rs +++ b/upstairs/src/deferred.rs @@ -139,7 +139,7 @@ pub(crate) struct EncryptedWrite { impl DeferredWrite { pub fn run(mut self) -> EncryptedWrite { - let num_blocks = self.impacted_blocks.blocks(&self.ddef).len(); + let num_blocks = self.impacted_blocks.blocks().len(); let mut blocks = Vec::with_capacity(num_blocks); let block_size = self.ddef.block_size() as usize; diff --git a/upstairs/src/downstairs.rs b/upstairs/src/downstairs.rs index 78430f721..4a00f3d11 100644 --- a/upstairs/src/downstairs.rs +++ b/upstairs/src/downstairs.rs @@ -26,10 +26,7 @@ use crate::{ RawWrite, ReconcileIO, ReconciliationId, RegionDefinition, ReplaceResult, SnapshotDetails, WorkSummary, }; -use crucible_common::{ - impacted_blocks::ImpactedAddr, BlockIndex, BlockOffset, ExtentId, - NegotiationError, -}; +use crucible_common::{BlockIndex, BlockOffset, ExtentId, NegotiationError}; use crucible_protocol::WriteHeader; use ringbuffer::RingBuffer; @@ -1505,16 +1502,7 @@ impl Downstairs { #[cfg(test)] fn create_and_enqueue_generic_read_eob(&mut self) -> JobId { - let iblocks = ImpactedBlocks::new( - ImpactedAddr { - extent_id: ExtentId(0), - block: BlockOffset(7), - }, - ImpactedAddr { - extent_id: ExtentId(0), - block: BlockOffset(7), - }, - ); + let iblocks = ImpactedBlocks::new(BlockIndex(7), BlockIndex(7)); let ds_id = self.next_id(); let dependencies = self.ds_active.deps_for_read(ds_id, iblocks); @@ -1545,16 +1533,7 @@ impl Downstairs { hash: 0, }], }; - let iblocks = ImpactedBlocks::new( - ImpactedAddr { - extent_id: ExtentId(0), - block: BlockOffset(7), - }, - ImpactedAddr { - extent_id: ExtentId(0), - block: BlockOffset(7), - }, - ); + let iblocks = ImpactedBlocks::new(BlockIndex(7), BlockIndex(7)); self.create_and_enqueue_write_eob( iblocks, request, @@ -1585,23 +1564,26 @@ impl Downstairs { debug!(self.log, "IO Write {} has deps {:?}", ds_id, dependencies); // TODO: can anyone actually give us an empty write? - let start = blocks.start().unwrap_or(ImpactedAddr { - extent_id: ExtentId(0), - block: BlockOffset(0), - }); + let start = blocks.start().unwrap_or(BlockIndex(0)); + + // XXX change IOop to take `BlockIndex` instead? + let extent_size = self.ddef.unwrap().extent_size().value; + let start_eid = ExtentId((start.0 / extent_size) as u32); + let start_offset = BlockOffset(start.0 % extent_size); + let awrite = if is_write_unwritten { IOop::WriteUnwritten { dependencies, - start_eid: start.extent_id, - start_offset: start.block, + start_eid, + start_offset, data: write.data.freeze(), blocks: write.blocks, } } else { IOop::Write { dependencies, - start_eid: start.extent_id, - start_offset: start.block, + start_eid, + start_offset, data: write.data.freeze(), blocks: write.blocks, } @@ -1676,7 +1658,8 @@ impl Downstairs { }; info!(self.log, "Create new job ids for {}: {repair_ids:?}", eid); - let deps = self.ds_active.deps_for_repair(repair_ids, eid); + let ddef = self.ddef.as_ref().unwrap(); + let deps = self.ds_active.deps_for_repair(repair_ids, eid, ddef); (repair_ids, deps) } } @@ -2096,9 +2079,10 @@ impl Downstairs { let Some(eur) = self.get_extent_under_repair() else { return; }; + let ddef = self.ddef.as_ref().unwrap(); let mut future_repair = false; for eid in impacted_blocks - .extents() + .extents(ddef) .into_iter() .flatten() .map(ExtentId) @@ -2148,7 +2132,8 @@ impl Downstairs { noop_id, reopen_id, }; - let deps = self.ds_active.deps_for_repair(repair_ids, eid); + let ddef = self.ddef.as_ref().unwrap(); + let deps = self.ds_active.deps_for_repair(repair_ids, eid, ddef); info!( self.log, "reserving repair IDs for {eid}: {repair_ids:?}; got dep {deps:?}" @@ -2182,16 +2167,19 @@ impl Downstairs { debug!(self.log, "IO Read {} has deps {:?}", ds_id, dependencies); // TODO: can anyone actually give us an empty write? - let start = blocks.start().unwrap_or(ImpactedAddr { - extent_id: ExtentId(0), - block: BlockOffset(0), - }); + let start = blocks.start().unwrap_or(BlockIndex(0)); let ddef = self.ddef.unwrap(); + + // XXX change IOop to take `BlockIndex` instead? + let extent_size = ddef.extent_size().value; + let start_eid = ExtentId((start.0 / extent_size) as u32); + let start_offset = BlockOffset(start.0 % extent_size); + let aread = IOop::Read { dependencies, - start_eid: start.extent_id, - start_offset: start.block, - count: blocks.blocks(&ddef).len() as u64, + start_eid, + start_offset, + count: blocks.blocks().len() as u64, block_size: ddef.block_size(), }; @@ -3514,13 +3502,26 @@ impl Downstairs { self.ds_active.keys().cloned().collect() } + /// Return the block range covered by the given job + /// + /// # Panics + /// If the job is not stored in our `Downstairs` + #[cfg(test)] + pub fn get_blocks_for(&self, ds_id: JobId) -> ImpactedBlocks { + self.ds_active.get_blocks_for(ds_id) + } + /// Return the extent range covered by the given job /// /// # Panics /// If the job is not stored in our `Downstairs` #[cfg(test)] - pub fn get_extents_for(&self, ds_id: JobId) -> ImpactedBlocks { - self.ds_active.get_extents_for(ds_id) + pub fn get_extents_for( + &self, + ds_id: JobId, + ) -> std::ops::RangeInclusive { + let ddef = self.ddef.as_ref().unwrap(); + self.get_blocks_for(ds_id).extents(ddef).unwrap() } #[cfg(test)] @@ -3545,7 +3546,8 @@ impl Downstairs { /// Submit a write to this downstairs. Use when you don't care about what /// the data you're writing is, and only care about getting some write-jobs - /// enqueued. The write will be to a single extent, as specified by eid + /// enqueued. The write will be to a single block, as specified by `eid` and + /// `block` #[cfg(test)] fn submit_test_write_block( &mut self, @@ -3553,16 +3555,9 @@ impl Downstairs { block: BlockOffset, is_write_unwritten: bool, ) -> JobId { - let blocks = ImpactedBlocks::new( - ImpactedAddr { - extent_id: eid, - block, - }, - ImpactedAddr { - extent_id: eid, - block, - }, - ); + let extent_size = self.ddef.unwrap().extent_size().value; + let block = BlockIndex(u64::from(eid.0) * extent_size + block.0); + let blocks = ImpactedBlocks::new(block, block); // Extent size doesn't matter as long as it can contain our write self.submit_test_write(blocks, is_write_unwritten) @@ -3570,7 +3565,8 @@ impl Downstairs { /// Submit a write to this downstairs. Use when you don't care about what /// the data you're writing is, and only care about getting some write-jobs - /// enqueued. The write will be to a single extent, as specified by eid + /// enqueued. The write will be to a range of blocks, as specified by + /// `blocks` #[cfg(test)] fn submit_test_write( &mut self, @@ -3580,10 +3576,9 @@ impl Downstairs { use bytes::BytesMut; use crucible_protocol::BlockContext; - let ddef = self.ddef.unwrap(); let write_blocks: Vec<_> = blocks - .blocks(&ddef) - .map(|(_eid, _b)| BlockContext { + .blocks() + .map(|_b| BlockContext { hash: crucible_common::integrity_hash(&[&vec![0xff; 512]]), encryption_context: None, }) @@ -3602,32 +3597,26 @@ impl Downstairs { ) } - #[cfg(test)] /// Submit a read to this downstairs. Use when you don't care about what /// the data you're read is, and only care about getting some read-jobs - /// enqueued. The read will be to a single extent, as specified by eid + /// enqueued. The read will be to a single block, as specified by `eid` and + /// `block` + #[cfg(test)] fn submit_read_block( &mut self, eid: ExtentId, block: BlockOffset, ) -> JobId { - let blocks = ImpactedBlocks::new( - ImpactedAddr { - extent_id: eid, - block, - }, - ImpactedAddr { - extent_id: eid, - block, - }, - ); + let extent_size = self.ddef.unwrap().extent_size().value; + let block = BlockIndex(u64::from(eid.0) * extent_size + block.0); + let blocks = ImpactedBlocks::new(block, block); self.submit_dummy_read(blocks) } #[cfg(test)] fn submit_dummy_read(&mut self, blocks: ImpactedBlocks) -> JobId { let ddef = self.ddef.as_ref().unwrap(); - let block_count = blocks.blocks(ddef).len(); + let block_count = blocks.blocks().len(); let buf = Buffer::new(block_count, ddef.block_size() as usize); self.submit_read(blocks, buf, BlockRes::dummy(), IOLimitGuard::dummy()) } @@ -4015,13 +4004,12 @@ pub(crate) mod test { live_repair::ExtentInfo, upstairs::UpstairsState, BlockOpWaiter, BlockRes, ClientId, CrucibleError, DsState, ExtentFix, - ExtentRepairIDs, IOState, IOop, ImpactedAddr, ImpactedBlocks, JobId, - RawReadResponse, ReconcileIO, ReconcileIOState, ReconciliationId, - SnapshotDetails, + ExtentRepairIDs, IOState, IOop, ImpactedBlocks, JobId, RawReadResponse, + ReconcileIO, ReconcileIOState, ReconciliationId, SnapshotDetails, }; use bytes::BytesMut; - use crucible_common::{BlockOffset, ExtentId}; + use crucible_common::{BlockIndex, BlockOffset, ExtentId}; use crucible_protocol::{Message, ReadBlockContext}; use ringbuffer::RingBuffer; @@ -8701,16 +8689,7 @@ pub(crate) mod test { let mut ds = Downstairs::repair_test_one_repair(); let write_id = ds.submit_test_write( - ImpactedBlocks::new( - ImpactedAddr { - extent_id: ExtentId(0), - block: BlockOffset(1), - }, - ImpactedAddr { - extent_id: ExtentId(1), - block: BlockOffset(0), - }, - ), + ImpactedBlocks::new(BlockIndex(1), BlockIndex(3)), false, ); @@ -8737,16 +8716,7 @@ pub(crate) mod test { let mut ds = Downstairs::repair_test_one_repair(); let write_id = ds.submit_test_write( - ImpactedBlocks::new( - ImpactedAddr { - extent_id: ExtentId(0), - block: BlockOffset(2), - }, - ImpactedAddr { - extent_id: ExtentId(1), - block: BlockOffset(1), - }, - ), + ImpactedBlocks::new(BlockIndex(2), BlockIndex(4)), false, ); @@ -8774,14 +8744,8 @@ pub(crate) mod test { let mut ds = Downstairs::repair_test_one_repair(); let read_id = ds.submit_dummy_read(ImpactedBlocks::new( - ImpactedAddr { - extent_id: ExtentId(0), - block: BlockOffset(1), - }, - ImpactedAddr { - extent_id: ExtentId(1), - block: BlockOffset(0), - }, + BlockIndex(1), + BlockIndex(3), )); let repair_ids = create_and_enqueue_repair_ops(&mut ds, ExtentId(1)); @@ -8807,14 +8771,8 @@ pub(crate) mod test { let mut ds = Downstairs::repair_test_one_repair(); let read_id = ds.submit_dummy_read(ImpactedBlocks::new( - ImpactedAddr { - extent_id: ExtentId(0), - block: BlockOffset(2), - }, - ImpactedAddr { - extent_id: ExtentId(1), - block: BlockOffset(1), - }, + BlockIndex(2), + BlockIndex(4), )); let repair_ids = create_and_enqueue_repair_ops(&mut ds, ExtentId(0)); @@ -8846,16 +8804,7 @@ pub(crate) mod test { let mut ds = Downstairs::repair_test_one_repair(); let write_id = ds.submit_test_write( - ImpactedBlocks::new( - ImpactedAddr { - extent_id: ExtentId(0), - block: BlockOffset(2), - }, - ImpactedAddr { - extent_id: ExtentId(1), - block: BlockOffset(0), - }, - ), + ImpactedBlocks::new(BlockIndex(2), BlockIndex(3)), false, ); @@ -8883,14 +8832,8 @@ pub(crate) mod test { let mut ds = Downstairs::repair_test_one_repair(); let read_id = ds.submit_dummy_read(ImpactedBlocks::new( - ImpactedAddr { - extent_id: ExtentId(0), - block: BlockOffset(0), - }, - ImpactedAddr { - extent_id: ExtentId(2), - block: BlockOffset(2), - }, + BlockIndex(0), + BlockIndex(8), )); let repair_ids = create_and_enqueue_repair_ops(&mut ds, ExtentId(1)); @@ -8954,16 +8897,7 @@ pub(crate) mod test { // A write of blocks 2,3,4 which spans extent 0 and extent 1. let write_id = ds.submit_test_write( - ImpactedBlocks::new( - ImpactedAddr { - extent_id: ExtentId(0), - block: BlockOffset(2), - }, - ImpactedAddr { - extent_id: ExtentId(1), - block: BlockOffset(1), - }, - ), + ImpactedBlocks::new(BlockIndex(2), BlockIndex(4)), false, ); @@ -8994,14 +8928,8 @@ pub(crate) mod test { let repair_ids = create_and_enqueue_repair_ops(&mut ds, ExtentId(1)); let read_id = ds.submit_dummy_read(ImpactedBlocks::new( - ImpactedAddr { - extent_id: ExtentId(0), - block: BlockOffset(1), - }, - ImpactedAddr { - extent_id: ExtentId(1), - block: BlockOffset(0), - }, + BlockIndex(1), + BlockIndex(3), )); assert_eq!(ds.ds_active.len(), 5); @@ -9028,27 +8956,12 @@ pub(crate) mod test { let mut ds = Downstairs::repair_test_one_repair(); let read_id = ds.submit_dummy_read(ImpactedBlocks::new( - ImpactedAddr { - extent_id: ExtentId(0), - block: BlockOffset(0), - }, - ImpactedAddr { - extent_id: ExtentId(1), - block: BlockOffset(0), - }, + BlockIndex(0), + BlockIndex(3), )); let write_id = ds.submit_test_write( - ImpactedBlocks::new( - ImpactedAddr { - extent_id: ExtentId(1), - block: BlockOffset(2), - }, - ImpactedAddr { - extent_id: ExtentId(2), - block: BlockOffset(2), - }, - ), + ImpactedBlocks::new(BlockIndex(5), BlockIndex(8)), false, ); @@ -9151,16 +9064,7 @@ pub(crate) mod test { // A write of blocks 2,3,4 which spans the extent. let write_id = ds.submit_test_write( - ImpactedBlocks::new( - ImpactedAddr { - extent_id: ExtentId(0), - block: BlockOffset(2), - }, - ImpactedAddr { - extent_id: ExtentId(1), - block: BlockOffset(1), - }, - ), + ImpactedBlocks::new(BlockIndex(2), BlockIndex(4)), false, ); @@ -9227,16 +9131,7 @@ pub(crate) mod test { // A write of blocks 2,3,4 which spans extents. let write_id = ds.submit_test_write( - ImpactedBlocks::new( - ImpactedAddr { - extent_id: ExtentId(0), - block: BlockOffset(2), - }, - ImpactedAddr { - extent_id: ExtentId(1), - block: BlockOffset(1), - }, - ), + ImpactedBlocks::new(BlockIndex(2), BlockIndex(4)), false, ); assert_eq!(ds.ds_active.len(), 1); @@ -9293,14 +9188,8 @@ pub(crate) mod test { // A read of blocks 2,3,4 which spans extents. let read_id = ds.submit_dummy_read(ImpactedBlocks::new( - ImpactedAddr { - extent_id: ExtentId(0), - block: BlockOffset(2), - }, - ImpactedAddr { - extent_id: ExtentId(1), - block: BlockOffset(1), - }, + BlockIndex(2), + BlockIndex(4), )); // Now enqueue the repair on extent 1, it should populate one of the @@ -9570,42 +9459,15 @@ pub(crate) mod test { // Now, put three IOs on the queue ds.submit_test_write( - ImpactedBlocks::new( - ImpactedAddr { - extent_id: ExtentId(0), - block: BlockOffset(1), - }, - ImpactedAddr { - extent_id: ExtentId(1), - block: BlockOffset(0), - }, - ), + ImpactedBlocks::new(BlockIndex(1), BlockIndex(3)), false, ); ds.submit_test_write( - ImpactedBlocks::new( - ImpactedAddr { - extent_id: ExtentId(0), - block: BlockOffset(2), - }, - ImpactedAddr { - extent_id: ExtentId(1), - block: BlockOffset(1), - }, - ), + ImpactedBlocks::new(BlockIndex(2), BlockIndex(4)), false, ); ds.submit_test_write( - ImpactedBlocks::new( - ImpactedAddr { - extent_id: ExtentId(1), - block: BlockOffset(0), - }, - ImpactedAddr { - extent_id: ExtentId(1), - block: BlockOffset(2), - }, - ), + ImpactedBlocks::new(BlockIndex(3), BlockIndex(5)), false, ); @@ -9637,16 +9499,7 @@ pub(crate) mod test { // Create a write on extent 1 (not yet repaired) ds.submit_test_write( - ImpactedBlocks::new( - ImpactedAddr { - extent_id: ExtentId(1), - block: BlockOffset(0), - }, - ImpactedAddr { - extent_id: ExtentId(1), - block: BlockOffset(2), - }, - ), + ImpactedBlocks::new(BlockIndex(3), BlockIndex(5)), false, ); @@ -9658,16 +9511,7 @@ pub(crate) mod test { // IO submitted so far, and will also require creation of // space for future repair work. ds.submit_test_write( - ImpactedBlocks::new( - ImpactedAddr { - extent_id: ExtentId(0), - block: BlockOffset(1), - }, - ImpactedAddr { - extent_id: ExtentId(1), - block: BlockOffset(0), - }, - ), + ImpactedBlocks::new(BlockIndex(1), BlockIndex(3)), false, ); @@ -9885,16 +9729,7 @@ pub(crate) mod test { // A write of blocks 2,3,4 which spans extents 0-1. let write_id1 = ds.submit_test_write( - ImpactedBlocks::new( - ImpactedAddr { - extent_id: ExtentId(0), - block: BlockOffset(2), - }, - ImpactedAddr { - extent_id: ExtentId(1), - block: BlockOffset(1), - }, - ), + ImpactedBlocks::new(BlockIndex(2), BlockIndex(4)), false, ); @@ -9980,46 +9815,22 @@ pub(crate) mod test { // A write of blocks 3,4,5,6 which spans extents 1-2. let write_id1 = ds.submit_test_write( - ImpactedBlocks::new( - ImpactedAddr { - extent_id: ExtentId(1), - block: BlockOffset(0), - }, - ImpactedAddr { - extent_id: ExtentId(2), - block: BlockOffset(0), - }, - ), + ImpactedBlocks::new(BlockIndex(3), BlockIndex(6)), false, ); // A write of block 2-3, which overlaps the previous write and should // also trigger a repair. let write_id2 = ds.submit_test_write( - ImpactedBlocks::new( - ImpactedAddr { - extent_id: ExtentId(0), - block: BlockOffset(2), - }, - ImpactedAddr { - extent_id: ExtentId(1), - block: BlockOffset(0), - }, - ), + ImpactedBlocks::new(BlockIndex(2), BlockIndex(3)), false, ); // A read of block 5-7, which overlaps the previous repair and should // also force waiting on a new repair. let read_id = ds.submit_dummy_read(ImpactedBlocks::new( - ImpactedAddr { - extent_id: ExtentId(1), - block: BlockOffset(2), - }, - ImpactedAddr { - extent_id: ExtentId(2), - block: BlockOffset(1), - }, + BlockIndex(5), + BlockIndex(7), )); assert_eq!(ds.ds_active.len(), 3); diff --git a/upstairs/src/test.rs b/upstairs/src/test.rs index d046fbdc2..f43a9a0fb 100644 --- a/upstairs/src/test.rs +++ b/upstairs/src/test.rs @@ -59,10 +59,6 @@ pub(crate) mod up_test { build_logger() } - fn extent_tuple(eid: u32, offset: u64) -> (ExtentId, BlockOffset) { - (ExtentId(eid), BlockOffset(offset)) - } - #[test] fn test_iospan() { let span = IOSpan::new(512, 1024, 512); @@ -131,10 +127,10 @@ pub(crate) mod up_test { up: &Upstairs, offset: BlockIndex, num_blocks: u64, - ) -> Vec<(ExtentId, BlockOffset)> { + ) -> Vec { let ddef = up.get_region_definition(); extent_from_offset(&ddef, offset, num_blocks) - .blocks(&ddef) + .blocks() .collect() } @@ -143,19 +139,19 @@ pub(crate) mod up_test { let up = make_upstairs(); for i in 0..100 { - let exv = vec![extent_tuple(0, i)]; + let exv = vec![BlockIndex(i)]; assert_eq!(up_efo(&up, BlockIndex(i), 1), exv); } for i in 0..100 { - let exv = vec![extent_tuple(1, i)]; + let exv = vec![BlockIndex(100 + i)]; assert_eq!(up_efo(&up, BlockIndex(100 + i), 1), exv); } - let exv = vec![extent_tuple(2, 0)]; + let exv = vec![BlockIndex(200)]; assert_eq!(up_efo(&up, BlockIndex(200), 1), exv); - let exv = vec![extent_tuple(9, 99)]; + let exv = vec![BlockIndex(999)]; assert_eq!(up_efo(&up, BlockIndex(999), 1), exv); } @@ -164,25 +160,25 @@ pub(crate) mod up_test { let up = make_upstairs(); for i in 0..99 { - let exv = vec![extent_tuple(0, i), extent_tuple(0, i + 1)]; + let exv = vec![BlockIndex(i), BlockIndex(i + 1)]; assert_eq!(up_efo(&up, BlockIndex(i), 2), exv); } - let exv = vec![extent_tuple(0, 99), extent_tuple(1, 0)]; + let exv = vec![BlockIndex(99), BlockIndex(100)]; assert_eq!(up_efo(&up, BlockIndex(99), 2), exv); for i in 0..99 { - let exv = vec![extent_tuple(1, i)]; + let exv = vec![BlockIndex(100 + i)]; assert_eq!(up_efo(&up, BlockIndex(100 + i), 1), exv); } - let exv = vec![extent_tuple(1, 99), extent_tuple(2, 0)]; + let exv = vec![BlockIndex(199), BlockIndex(200)]; assert_eq!(up_efo(&up, BlockIndex(199), 2), exv); - let exv = vec![extent_tuple(2, 0), extent_tuple(2, 1)]; + let exv = vec![BlockIndex(200), BlockIndex(201)]; assert_eq!(up_efo(&up, BlockIndex(200), 2), exv); - let exv = vec![extent_tuple(9, 98), extent_tuple(9, 99)]; + let exv = vec![BlockIndex(998), BlockIndex(999)]; assert_eq!(up_efo(&up, BlockIndex(998), 2), exv); } @@ -198,15 +194,15 @@ pub(crate) mod up_test { */ assert_eq!( up_efo(&up, BlockIndex(99), 2), - vec![extent_tuple(0, 99), extent_tuple(1, 0)], + vec![BlockIndex(99), BlockIndex(100)], ); assert_eq!( up_efo(&up, BlockIndex(98), 4), vec![ - extent_tuple(0, 98), - extent_tuple(0, 99), - extent_tuple(1, 0), - extent_tuple(1, 1), + BlockIndex(98), + BlockIndex(99), + BlockIndex(100), + BlockIndex(101) ], ); @@ -214,18 +210,9 @@ pub(crate) mod up_test { * Largest buffer at different offsets */ for offset in 0..100 { - let expected: Vec<_> = (0..100) - .map(|i| { - extent_tuple( - (offset + i) / 100, - u64::from((offset + i) % 100), - ) - }) - .collect(); - assert_eq!( - up_efo(&up, BlockIndex(u64::from(offset)), 100), - expected - ); + let expected: Vec<_> = + (0..100).map(|i| BlockIndex(offset + i)).collect(); + assert_eq!(up_efo(&up, BlockIndex(offset), 100), expected); } } diff --git a/upstairs/src/upstairs.rs b/upstairs/src/upstairs.rs index 620f11deb..0cf6274c2 100644 --- a/upstairs/src/upstairs.rs +++ b/upstairs/src/upstairs.rs @@ -3375,9 +3375,9 @@ pub(crate) mod test { assert_eq!(jobs.len(), 3); // confirm which extents are impacted (in case make_upstairs changes) - assert_eq!(ds.get_extents_for(jobs[0]).extents().unwrap().count(), 1); - assert_eq!(ds.get_extents_for(jobs[1]).extents().unwrap().count(), 2); - assert_eq!(ds.get_extents_for(jobs[2]).extents().unwrap().count(), 1); + assert_eq!(ds.get_extents_for(jobs[0]).count(), 1); + assert_eq!(ds.get_extents_for(jobs[1]).count(), 2); + assert_eq!(ds.get_extents_for(jobs[2]).count(), 1); assert_ne!(ds.get_extents_for(jobs[0]), ds.get_extents_for(jobs[2])); // confirm deps @@ -3438,11 +3438,11 @@ pub(crate) mod test { assert_eq!(jobs.len(), 5); // confirm which extents are impacted (in case make_upstairs changes) - assert_eq!(ds.get_extents_for(jobs[0]).extents().unwrap().count(), 1); - assert_eq!(ds.get_extents_for(jobs[1]).extents().unwrap().count(), 2); - assert_eq!(ds.get_extents_for(jobs[2]).extents().unwrap().count(), 1); - assert_eq!(ds.get_extents_for(jobs[3]).extents().unwrap().count(), 2); - assert_eq!(ds.get_extents_for(jobs[4]).extents().unwrap().count(), 1); + assert_eq!(ds.get_extents_for(jobs[0]).count(), 1); + assert_eq!(ds.get_extents_for(jobs[1]).count(), 2); + assert_eq!(ds.get_extents_for(jobs[2]).count(), 1); + assert_eq!(ds.get_extents_for(jobs[3]).count(), 2); + assert_eq!(ds.get_extents_for(jobs[4]).count(), 1); assert_ne!(ds.get_extents_for(jobs[0]), ds.get_extents_for(jobs[2])); assert_ne!(ds.get_extents_for(jobs[4]), ds.get_extents_for(jobs[2])); @@ -3494,9 +3494,9 @@ pub(crate) mod test { assert_eq!(jobs.len(), 3); // confirm which extents are impacted (in case make_upstairs changes) - assert_eq!(ds.get_extents_for(jobs[0]).extents().unwrap().count(), 1); - assert_eq!(ds.get_extents_for(jobs[1]).extents().unwrap().count(), 1); - assert_eq!(ds.get_extents_for(jobs[2]).extents().unwrap().count(), 2); + assert_eq!(ds.get_extents_for(jobs[0]).count(), 1); + assert_eq!(ds.get_extents_for(jobs[1]).count(), 1); + assert_eq!(ds.get_extents_for(jobs[2]).count(), 2); assert_ne!(ds.get_extents_for(jobs[0]), ds.get_extents_for(jobs[1])); From b40358e7c8b2937d4cb858fbd6d0dcdfd9cd7fa5 Mon Sep 17 00:00:00 2001 From: Matt Keeter Date: Tue, 18 Mar 2025 12:06:35 -0400 Subject: [PATCH 2/3] Return an iterator, because that's how it's used --- common/src/impacted_blocks.rs | 10 ++++++---- upstairs/src/downstairs.rs | 16 ++++------------ upstairs/src/upstairs.rs | 22 +++++++++++----------- 3 files changed, 21 insertions(+), 27 deletions(-) diff --git a/common/src/impacted_blocks.rs b/common/src/impacted_blocks.rs index 5fef1afee..4b1422d3f 100644 --- a/common/src/impacted_blocks.rs +++ b/common/src/impacted_blocks.rs @@ -2,7 +2,7 @@ use super::*; -use std::{iter::FusedIterator, ops::RangeInclusive}; +use std::iter::FusedIterator; #[derive(Copy, Clone, Debug, PartialEq, Eq, PartialOrd, Ord)] pub struct ImpactedAddr { @@ -137,12 +137,11 @@ impl ImpactedBlocks { self == &ImpactedBlocks::Empty } - /// Return a range of impacted extents + /// Return an iterator over impacted extents pub fn extents( &self, ddef: &RegionDefinition, - ) -> Option> { - // XXX return a range of `ExtentId` once `std::iter::Step` is stable + ) -> impl Iterator { let blocks_per_extent = ddef.extent_size().value; match self { ImpactedBlocks::Empty => None, /* empty range */ @@ -151,6 +150,9 @@ impl ImpactedBlocks { ..=((lst.0 / blocks_per_extent) as u32), ), } + .into_iter() + .flatten() + .map(ExtentId) } pub fn blocks(&self) -> ImpactedBlockIter { diff --git a/upstairs/src/downstairs.rs b/upstairs/src/downstairs.rs index 4a00f3d11..98c1be678 100644 --- a/upstairs/src/downstairs.rs +++ b/upstairs/src/downstairs.rs @@ -2081,12 +2081,7 @@ impl Downstairs { }; let ddef = self.ddef.as_ref().unwrap(); let mut future_repair = false; - for eid in impacted_blocks - .extents(ddef) - .into_iter() - .flatten() - .map(ExtentId) - { + for eid in impacted_blocks.extents(ddef) { if eid == *eur.start() { future_repair = true; } else if eid > *eur.start() && (eid <= *eur.end() || future_repair) @@ -3511,17 +3506,14 @@ impl Downstairs { self.ds_active.get_blocks_for(ds_id) } - /// Return the extent range covered by the given job + /// Returns a list of extents covered by the given job /// /// # Panics /// If the job is not stored in our `Downstairs` #[cfg(test)] - pub fn get_extents_for( - &self, - ds_id: JobId, - ) -> std::ops::RangeInclusive { + pub fn get_extents_for(&self, ds_id: JobId) -> Vec { let ddef = self.ddef.as_ref().unwrap(); - self.get_blocks_for(ds_id).extents(ddef).unwrap() + self.get_blocks_for(ds_id).extents(ddef).collect() } #[cfg(test)] diff --git a/upstairs/src/upstairs.rs b/upstairs/src/upstairs.rs index 0cf6274c2..458ecfd0f 100644 --- a/upstairs/src/upstairs.rs +++ b/upstairs/src/upstairs.rs @@ -3375,9 +3375,9 @@ pub(crate) mod test { assert_eq!(jobs.len(), 3); // confirm which extents are impacted (in case make_upstairs changes) - assert_eq!(ds.get_extents_for(jobs[0]).count(), 1); - assert_eq!(ds.get_extents_for(jobs[1]).count(), 2); - assert_eq!(ds.get_extents_for(jobs[2]).count(), 1); + assert_eq!(ds.get_extents_for(jobs[0]).len(), 1); + assert_eq!(ds.get_extents_for(jobs[1]).len(), 2); + assert_eq!(ds.get_extents_for(jobs[2]).len(), 1); assert_ne!(ds.get_extents_for(jobs[0]), ds.get_extents_for(jobs[2])); // confirm deps @@ -3438,11 +3438,11 @@ pub(crate) mod test { assert_eq!(jobs.len(), 5); // confirm which extents are impacted (in case make_upstairs changes) - assert_eq!(ds.get_extents_for(jobs[0]).count(), 1); - assert_eq!(ds.get_extents_for(jobs[1]).count(), 2); - assert_eq!(ds.get_extents_for(jobs[2]).count(), 1); - assert_eq!(ds.get_extents_for(jobs[3]).count(), 2); - assert_eq!(ds.get_extents_for(jobs[4]).count(), 1); + assert_eq!(ds.get_extents_for(jobs[0]).len(), 1); + assert_eq!(ds.get_extents_for(jobs[1]).len(), 2); + assert_eq!(ds.get_extents_for(jobs[2]).len(), 1); + assert_eq!(ds.get_extents_for(jobs[3]).len(), 2); + assert_eq!(ds.get_extents_for(jobs[4]).len(), 1); assert_ne!(ds.get_extents_for(jobs[0]), ds.get_extents_for(jobs[2])); assert_ne!(ds.get_extents_for(jobs[4]), ds.get_extents_for(jobs[2])); @@ -3494,9 +3494,9 @@ pub(crate) mod test { assert_eq!(jobs.len(), 3); // confirm which extents are impacted (in case make_upstairs changes) - assert_eq!(ds.get_extents_for(jobs[0]).count(), 1); - assert_eq!(ds.get_extents_for(jobs[1]).count(), 1); - assert_eq!(ds.get_extents_for(jobs[2]).count(), 2); + assert_eq!(ds.get_extents_for(jobs[0]).len(), 1); + assert_eq!(ds.get_extents_for(jobs[1]).len(), 1); + assert_eq!(ds.get_extents_for(jobs[2]).len(), 2); assert_ne!(ds.get_extents_for(jobs[0]), ds.get_extents_for(jobs[1])); From 4db001cb61ae18d0e289ad285a5b02d948a4816c Mon Sep 17 00:00:00 2001 From: Matt Keeter Date: Mon, 24 Mar 2025 15:12:19 -0400 Subject: [PATCH 3/3] Fix manual extent_to_impacted_blocks implementation --- upstairs/src/active_jobs.rs | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/upstairs/src/active_jobs.rs b/upstairs/src/active_jobs.rs index c105e638d..bd9189b18 100644 --- a/upstairs/src/active_jobs.rs +++ b/upstairs/src/active_jobs.rs @@ -3,7 +3,10 @@ use crucible_common::{BlockIndex, ExtentId, RegionDefinition}; use crucible_protocol::JobId; -use crate::{DownstairsIO, ExtentRepairIDs, IOop, ImpactedBlocks}; +use crate::{ + extent_to_impacted_blocks, DownstairsIO, ExtentRepairIDs, IOop, + ImpactedBlocks, +}; use std::collections::{BTreeMap, BTreeSet}; /// `ActiveJobs` tracks active jobs (and associated metadata) by job ID @@ -165,13 +168,7 @@ impl ActiveJobs { extent: ExtentId, ddef: &RegionDefinition, ) -> Vec { - let blocks_per_extent = ddef.extent_size().value; - let blocks = ImpactedBlocks::InclusiveRange( - BlockIndex(u64::from(extent.0) * blocks_per_extent), - BlockIndex( - u64::from(extent.0) * blocks_per_extent + blocks_per_extent - 1, - ), - ); + let blocks = extent_to_impacted_blocks(ddef, extent); let dep = self.block_to_active.check_range(blocks, true); self.block_to_active .insert_range(blocks, repair_ids.reopen_id, true);