From c440f3636b40e33042e9ebd9b166a3a985b4a8e5 Mon Sep 17 00:00:00 2001 From: Matt Keeter Date: Tue, 14 Oct 2025 16:13:49 -0400 Subject: [PATCH 1/6] Add `verify` to `ExtentInner` trait --- downstairs/src/extent.rs | 6 +++ downstairs/src/extent_inner_raw.rs | 53 +++++++++++++++++++++++++++ downstairs/src/extent_inner_sqlite.rs | 6 +++ 3 files changed, 65 insertions(+) diff --git a/downstairs/src/extent.rs b/downstairs/src/extent.rs index 7d255bea5..4bba34762 100644 --- a/downstairs/src/extent.rs +++ b/downstairs/src/extent.rs @@ -35,6 +35,7 @@ pub(crate) trait ExtentInner: Send + Sync + Debug { fn gen_number(&self) -> Result; fn flush_number(&self) -> Result; fn dirty(&self) -> Result; + fn validate(&self) -> Result<(), CrucibleError>; /// Performs any metadata updates needed before a flush fn pre_flush( @@ -511,6 +512,11 @@ impl Extent { Ok((gen, flush, dirty)) } + /// Validates the extent data + pub fn validate(&self) -> Result<(), CrucibleError> { + self.inner.validate() + } + /** * Create an extent at the location requested. * Start off with the default meta data. diff --git a/downstairs/src/extent_inner_raw.rs b/downstairs/src/extent_inner_raw.rs index 2ba1ef6ad..79d0fd5b0 100644 --- a/downstairs/src/extent_inner_raw.rs +++ b/downstairs/src/extent_inner_raw.rs @@ -581,6 +581,59 @@ impl ExtentInner for RawInner { r } + fn validate(&self) -> Result<(), CrucibleError> { + let block_size = self.extent_size.block_size_in_bytes() as u64; + for block in 0..self.extent_size.value { + // Read the block data itself: + let mut buf = vec![0; block_size as usize]; + pread_all(self.file.as_fd(), &mut buf, (block_size * block) as i64) + .map_err(|e| { + CrucibleError::IoError(format!( + "extent {}: reading block {block} data failed: {e}", + self.extent_number + )) + })?; + let hash = integrity_hash(&[&buf]); + + // Then, read the slot data and decide if either slot + // (1) is present and + // (2) has a matching hash + let mut matching_slot = None; + let mut empty_slot = None; + for slot in [ContextSlot::A, ContextSlot::B] { + // Read a single context slot, which is by definition contiguous + let mut context = self.layout.read_context_slots_contiguous( + &self.file, block, 1, slot, + )?; + assert_eq!(context.len(), 1); + let context = context.pop().unwrap(); + + if let Some(context) = context { + if context.on_disk_hash == hash { + matching_slot = Some(slot); + } + } else if empty_slot.is_none() { + empty_slot = Some(slot); + } + } + if matching_slot.is_some() { + // great work, everyone + } else if empty_slot.is_some() { + if !buf.iter().all(|v| *v == 0u8) { + return Err(CrucibleError::GenericError(format!( + "block {block} has no matching slot, \ + and has a empty slot but has none-zero data" + ))); + } + } else { + return Err(CrucibleError::GenericError(format!( + "block {block} has both slots mismatched" + ))); + } + } + Ok(()) + } + #[cfg(test)] fn set_dirty_and_block_context( &mut self, diff --git a/downstairs/src/extent_inner_sqlite.rs b/downstairs/src/extent_inner_sqlite.rs index e1d309c25..c9bdec223 100644 --- a/downstairs/src/extent_inner_sqlite.rs +++ b/downstairs/src/extent_inner_sqlite.rs @@ -103,6 +103,12 @@ impl ExtentInner for SqliteInner { Ok(bc) } + fn validate(&self) -> Result<(), CrucibleError> { + Err(CrucibleError::GenericError( + "`validate` is not implemented for Sqlite extent".to_owned(), + )) + } + #[cfg(test)] fn set_dirty_and_block_context( &mut self, From 93518d8e1e9073084af308a42ec136468fb61658 Mon Sep 17 00:00:00 2001 From: Matt Keeter Date: Tue, 14 Oct 2025 17:22:28 -0400 Subject: [PATCH 2/6] Use active_context array to pick slot to check --- downstairs/src/extent_inner_raw.rs | 50 +++++++++++++----------------- 1 file changed, 21 insertions(+), 29 deletions(-) diff --git a/downstairs/src/extent_inner_raw.rs b/downstairs/src/extent_inner_raw.rs index 79d0fd5b0..7c5fba8f7 100644 --- a/downstairs/src/extent_inner_raw.rs +++ b/downstairs/src/extent_inner_raw.rs @@ -595,40 +595,32 @@ impl ExtentInner for RawInner { })?; let hash = integrity_hash(&[&buf]); - // Then, read the slot data and decide if either slot - // (1) is present and - // (2) has a matching hash - let mut matching_slot = None; - let mut empty_slot = None; - for slot in [ContextSlot::A, ContextSlot::B] { - // Read a single context slot, which is by definition contiguous - let mut context = self.layout.read_context_slots_contiguous( - &self.file, block, 1, slot, - )?; - assert_eq!(context.len(), 1); - let context = context.pop().unwrap(); + // Read the active context slot, which is by definition contiguous + let slot = self.active_context[block]; + let mut context = self + .layout + .read_context_slots_contiguous(&self.file, block, 1, slot)?; + assert_eq!(context.len(), 1); + let context = context.pop().unwrap(); - if let Some(context) = context { - if context.on_disk_hash == hash { - matching_slot = Some(slot); - } - } else if empty_slot.is_none() { - empty_slot = Some(slot); - } - } - if matching_slot.is_some() { - // great work, everyone - } else if empty_slot.is_some() { - if !buf.iter().all(|v| *v == 0u8) { + if let Some(context) = context { + if context.on_disk_hash == hash { + // great work, everyone + } else { return Err(CrucibleError::GenericError(format!( - "block {block} has no matching slot, \ - and has a empty slot but has none-zero data" + "block {block} has an active slot with mismatched hash" ))); } } else { - return Err(CrucibleError::GenericError(format!( - "block {block} has both slots mismatched" - ))); + // context slot is empty, hopefully data is as well! + if buf.iter().all(|v| *v == 0u8) { + // great work, everyone + } else { + return Err(CrucibleError::GenericError(format!( + "block {block} has an empty active slot, \ + but contains none-zero data" + ))); + } } } Ok(()) From 773256ee1531582177be3a855b6bbdb11c22feb9 Mon Sep 17 00:00:00 2001 From: Alan Hanson Date: Wed, 15 Oct 2025 00:49:18 +0000 Subject: [PATCH 3/6] Made a verify subcommand to crucible-downstairs directly. Return an error if the verify failed. --- downstairs/src/dump.rs | 27 +++++++++++++++++++++++++++ downstairs/src/lib.rs | 2 +- downstairs/src/main.rs | 11 +++++++++++ 3 files changed, 39 insertions(+), 1 deletion(-) diff --git a/downstairs/src/dump.rs b/downstairs/src/dump.rs index 3d9a89d71..db37b7595 100644 --- a/downstairs/src/dump.rs +++ b/downstairs/src/dump.rs @@ -10,6 +10,33 @@ struct ExtInfo { ei_hm: HashMap, } +pub fn verify_region( + region_dir: PathBuf, + log: Logger, +) -> Result<()> { + + let mut verify_error = false; + let region = Region::open(region_dir, false, true, &log)?; + + for e in ®ion.extents { + let e = match e { + extent::ExtentState::Opened(extent) => extent, + extent::ExtentState::Closed => panic!("dump on closed extent!"), + }; + + if let Err(err) = e.validate() { + println!( + "validation failed for extent {}: {:?}", + e.number, err + ); + verify_error = true; + } + } + if verify_error { + bail!("Region failed to verify"); + } + Ok(()) +} /* * Dump the metadata for one or more region directories. * diff --git a/downstairs/src/lib.rs b/downstairs/src/lib.rs index ce4b14f57..cc546dfbc 100644 --- a/downstairs/src/lib.rs +++ b/downstairs/src/lib.rs @@ -52,7 +52,7 @@ use extent::ExtentState; use region::Region; pub use admin::run_dropshot; -pub use dump::dump_region; +pub use dump::{verify_region, dump_region}; pub use dynamometer::*; pub use stats::{DsCountStat, DsStatOuter}; diff --git a/downstairs/src/main.rs b/downstairs/src/main.rs index 64b49942c..d40f31dd1 100644 --- a/downstairs/src/main.rs +++ b/downstairs/src/main.rs @@ -218,6 +218,12 @@ enum Args { #[clap(long, default_value = "127.0.0.1:4567", action)] bind_addr: SocketAddr, }, + /// Verify region + Verify { + /// Directory containing a region. + #[clap(short, long, value_name = "DIRECTORY", action)] + data: PathBuf, + }, Version, /// Measure an isolated downstairs' disk usage Dynamometer { @@ -465,6 +471,11 @@ async fn main() -> Result<()> { run_dropshot(bind_addr, &log).await } + Args::Verify { + data, + } => { + verify_region(data, log) + } Args::Version => { let info = crucible_common::BuildInfo::default(); println!("Crucible Version: {}", info); From 05dbccad8a154b2aca01cd0132245d39a9a19df3 Mon Sep 17 00:00:00 2001 From: Alan Hanson Date: Wed, 15 Oct 2025 03:01:20 +0000 Subject: [PATCH 4/6] dump verify, now with par_iter --- downstairs/src/dump.rs | 39 ++++++++++++++++++++------------------- downstairs/src/lib.rs | 2 +- downstairs/src/main.rs | 6 +----- 3 files changed, 22 insertions(+), 25 deletions(-) diff --git a/downstairs/src/dump.rs b/downstairs/src/dump.rs index db37b7595..78db92e01 100644 --- a/downstairs/src/dump.rs +++ b/downstairs/src/dump.rs @@ -1,6 +1,7 @@ // Copyright 2021 Oxide Computer Company use super::*; use crate::extent::ExtentMeta; +use rayon::prelude::*; use std::convert::TryInto; use sha2::{Digest, Sha256}; @@ -10,29 +11,29 @@ struct ExtInfo { ei_hm: HashMap, } -pub fn verify_region( - region_dir: PathBuf, - log: Logger, -) -> Result<()> { - - let mut verify_error = false; +pub fn verify_region(region_dir: PathBuf, log: Logger) -> Result<()> { let region = Region::open(region_dir, false, true, &log)?; + let errors: Vec<_> = region + .extents + .par_iter() + .filter_map(|e| { + let extent = match e { + extent::ExtentState::Opened(extent) => extent, + extent::ExtentState::Closed => panic!("dump on closed extent!"), + }; - for e in ®ion.extents { - let e = match e { - extent::ExtentState::Opened(extent) => extent, - extent::ExtentState::Closed => panic!("dump on closed extent!"), - }; + if let Err(err) = extent.validate() { + Some((extent.number, err)) + } else { + None + } + }) + .collect(); - if let Err(err) = e.validate() { - println!( - "validation failed for extent {}: {:?}", - e.number, err - ); - verify_error = true; + if !errors.is_empty() { + for (number, err) in &errors { + println!("validation failed for extent {}: {:?}", number, err); } - } - if verify_error { bail!("Region failed to verify"); } Ok(()) diff --git a/downstairs/src/lib.rs b/downstairs/src/lib.rs index cc546dfbc..e7d692ca3 100644 --- a/downstairs/src/lib.rs +++ b/downstairs/src/lib.rs @@ -52,7 +52,7 @@ use extent::ExtentState; use region::Region; pub use admin::run_dropshot; -pub use dump::{verify_region, dump_region}; +pub use dump::{dump_region, verify_region}; pub use dynamometer::*; pub use stats::{DsCountStat, DsStatOuter}; diff --git a/downstairs/src/main.rs b/downstairs/src/main.rs index d40f31dd1..3b39579fa 100644 --- a/downstairs/src/main.rs +++ b/downstairs/src/main.rs @@ -471,11 +471,7 @@ async fn main() -> Result<()> { run_dropshot(bind_addr, &log).await } - Args::Verify { - data, - } => { - verify_region(data, log) - } + Args::Verify { data } => verify_region(data, log), Args::Version => { let info = crucible_common::BuildInfo::default(); println!("Crucible Version: {}", info); From 3acbe25f4318f37899f92978b887399feed1b81e Mon Sep 17 00:00:00 2001 From: Matt Keeter Date: Wed, 15 Oct 2025 10:29:24 -0400 Subject: [PATCH 5/6] Read all context slots in advance --- downstairs/src/extent_inner_raw.rs | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/downstairs/src/extent_inner_raw.rs b/downstairs/src/extent_inner_raw.rs index 7c5fba8f7..4a60b48a6 100644 --- a/downstairs/src/extent_inner_raw.rs +++ b/downstairs/src/extent_inner_raw.rs @@ -583,6 +583,20 @@ impl ExtentInner for RawInner { fn validate(&self) -> Result<(), CrucibleError> { let block_size = self.extent_size.block_size_in_bytes() as u64; + + let ctx_a = self.layout.read_context_slots_contiguous( + &self.file, + 0, + self.layout.block_count(), + ContextSlot::A, + )?; + let ctx_b = self.layout.read_context_slots_contiguous( + &self.file, + 0, + self.layout.block_count(), + ContextSlot::B, + )?; + for block in 0..self.extent_size.value { // Read the block data itself: let mut buf = vec![0; block_size as usize]; @@ -596,12 +610,10 @@ impl ExtentInner for RawInner { let hash = integrity_hash(&[&buf]); // Read the active context slot, which is by definition contiguous - let slot = self.active_context[block]; - let mut context = self - .layout - .read_context_slots_contiguous(&self.file, block, 1, slot)?; - assert_eq!(context.len(), 1); - let context = context.pop().unwrap(); + let context = match self.active_context[block] { + ContextSlot::A => &ctx_a, + ContextSlot::B => &ctx_b, + }[block as usize]; if let Some(context) = context { if context.on_disk_hash == hash { From 13b1577b584ca0dbd09b897d333d5c2b128ce008 Mon Sep 17 00:00:00 2001 From: Matt Keeter Date: Wed, 15 Oct 2025 10:55:34 -0400 Subject: [PATCH 6/6] Read blocks in bulk --- downstairs/src/extent_inner_raw.rs | 83 ++++++++++++++++++------------ 1 file changed, 50 insertions(+), 33 deletions(-) diff --git a/downstairs/src/extent_inner_raw.rs b/downstairs/src/extent_inner_raw.rs index 4a60b48a6..715630ae4 100644 --- a/downstairs/src/extent_inner_raw.rs +++ b/downstairs/src/extent_inner_raw.rs @@ -582,8 +582,9 @@ impl ExtentInner for RawInner { } fn validate(&self) -> Result<(), CrucibleError> { - let block_size = self.extent_size.block_size_in_bytes() as u64; + let block_size = self.extent_size.block_size_in_bytes() as usize; + // Read context data to local arrays let ctx_a = self.layout.read_context_slots_contiguous( &self.file, 0, @@ -597,41 +598,57 @@ impl ExtentInner for RawInner { ContextSlot::B, )?; - for block in 0..self.extent_size.value { - // Read the block data itself: - let mut buf = vec![0; block_size as usize]; - pread_all(self.file.as_fd(), &mut buf, (block_size * block) as i64) - .map_err(|e| { - CrucibleError::IoError(format!( - "extent {}: reading block {block} data failed: {e}", - self.extent_number - )) - })?; - let hash = integrity_hash(&[&buf]); + // Read blocks in bulk, 128 KiB at a time + let nblocks = 128 * 1024 / block_size; + let mut buf = vec![0; block_size * nblocks]; + for start_block in (0..self.extent_size.value).step_by(nblocks) { + let num_blocks = + ((self.extent_size.value - start_block) as usize).min(nblocks); - // Read the active context slot, which is by definition contiguous - let context = match self.active_context[block] { - ContextSlot::A => &ctx_a, - ContextSlot::B => &ctx_b, - }[block as usize]; + // Read the block data itself: + buf.resize(num_blocks * block_size, 0u8); + pread_all( + self.file.as_fd(), + &mut buf, + block_size as i64 * start_block as i64, + ) + .map_err(|e| { + CrucibleError::IoError(format!( + "extent {}: reading block {start_block} data failed: {e}", + self.extent_number + )) + })?; - if let Some(context) = context { - if context.on_disk_hash == hash { - // great work, everyone - } else { - return Err(CrucibleError::GenericError(format!( - "block {block} has an active slot with mismatched hash" - ))); - } - } else { - // context slot is empty, hopefully data is as well! - if buf.iter().all(|v| *v == 0u8) { - // great work, everyone + // Hash and check individual blocks against context slots + for (i, data) in buf.chunks_exact(block_size).enumerate() { + let block = start_block as usize + i; + let hash = integrity_hash(&[data]); + + // Pick out the active context slot + let context = match self.active_context[block as u64] { + ContextSlot::A => &ctx_a, + ContextSlot::B => &ctx_b, + }[block]; + + if let Some(context) = context { + if context.on_disk_hash == hash { + // great work, everyone + } else { + return Err(CrucibleError::GenericError(format!( + "block {block} has an active slot \ + with mismatched hash" + ))); + } } else { - return Err(CrucibleError::GenericError(format!( - "block {block} has an empty active slot, \ - but contains none-zero data" - ))); + // context slot is empty, hopefully data is as well! + if data.iter().all(|v| *v == 0u8) { + // great work, everyone + } else { + return Err(CrucibleError::GenericError(format!( + "block {block} has an empty active slot, \ + but contains non-zero data", + ))); + } } } }