From e1628f9004f518050b790bb3ba092d80c0af0766 Mon Sep 17 00:00:00 2001 From: Ming-Hung Tsai Date: Wed, 21 Apr 2021 14:36:16 +0800 Subject: [PATCH] [cache_check (rust)] Add more checks - Check array indices continuity - Support ignore_non_fatal - Report blocknr of IoErrors - Report array block indeices --- src/bin/cache_check.rs | 6 ++++++ src/cache/check.rs | 11 +++++----- src/pdata/array.rs | 23 ++++++++++++++++++--- src/pdata/array_walker.rs | 42 +++++++++++++++++++-------------------- 4 files changed, 52 insertions(+), 30 deletions(-) diff --git a/src/bin/cache_check.rs b/src/bin/cache_check.rs index f142f39..452a495 100644 --- a/src/bin/cache_check.rs +++ b/src/bin/cache_check.rs @@ -44,6 +44,11 @@ fn main() { .long("skip-discards") .value_name("SKIP_DISCARDS"), ) + .arg( + Arg::with_name("IGNORE_NON_FATAL") + .help("Only return a non-zero exit code if a fatal error is found.") + .long("ignore-non-fatal-errors"), + ) .arg( Arg::with_name("QUIET") .help("Suppress output messages, return only exit code.") @@ -70,6 +75,7 @@ fn main() { skip_mappings: matches.is_present("SKIP_MAPPINGS"), skip_hints: matches.is_present("SKIP_HINTS"), skip_discards: matches.is_present("SKIP_DISCARDS"), + ignore_non_fatal: matches.is_present("IGNORE_NON_FATAL"), report, }; diff --git a/src/cache/check.rs b/src/cache/check.rs index b91504f..71a59ae 100644 --- a/src/cache/check.rs +++ b/src/cache/check.rs @@ -195,7 +195,7 @@ impl ArrayVisitor for HintChecker { //------------------------------------------ -// TODO: ignore_non_fatal, clear_needs_check, auto_repair +// TODO: clear_needs_check, auto_repair pub struct CacheCheckOptions<'a> { pub dev: &'a Path, pub async_io: bool, @@ -203,6 +203,7 @@ pub struct CacheCheckOptions<'a> { pub skip_mappings: bool, pub skip_hints: bool, pub skip_discards: bool, + pub ignore_non_fatal: bool, pub report: Arc, } @@ -257,7 +258,7 @@ pub fn check(opts: CacheCheckOptions) -> anyhow::Result<()> { // TODO: factor out into check_mappings() if !opts.skip_mappings { - let w = ArrayWalker::new(engine.clone(), false); + let w = ArrayWalker::new(engine.clone(), opts.ignore_non_fatal); match sb.version { 1 => { let mut c = format1::MappingChecker::new(nr_origin_blocks); @@ -271,7 +272,7 @@ pub fn check(opts: CacheCheckOptions) -> anyhow::Result<()> { engine.clone(), sb.dirty_root.unwrap(), sb.cache_blocks as usize, - false, + opts.ignore_non_fatal, ); if err.is_some() { ctx.report.fatal(&format!("{}", err.unwrap())); @@ -291,7 +292,7 @@ pub fn check(opts: CacheCheckOptions) -> anyhow::Result<()> { if sb.policy_hint_size != 4 { return Err(anyhow!("cache_check only supports policy hint size of 4")); } - let w = ArrayWalker::new(engine.clone(), false); + let w = ArrayWalker::new(engine.clone(), opts.ignore_non_fatal); let mut c = HintChecker::new(); if let Err(e) = w.walk(&mut c, sb.hint_root) { ctx.report.fatal(&format!("{}", e)); @@ -305,7 +306,7 @@ pub fn check(opts: CacheCheckOptions) -> anyhow::Result<()> { engine.clone(), sb.discard_root, sb.cache_blocks as usize, - false, + opts.ignore_non_fatal, ); if err.is_some() { ctx.report.fatal(&format!("{}", err.unwrap())); diff --git a/src/pdata/array.rs b/src/pdata/array.rs index 788d239..b2cfbaf 100644 --- a/src/pdata/array.rs +++ b/src/pdata/array.rs @@ -53,8 +53,8 @@ pub struct ArrayBlock { #[derive(Error, Clone, Debug)] pub enum ArrayError { - //#[error("io_error")] - IoError, + //#[error("io_error {0}")] + IoError(u64), //#[error("block error: {0}")] BlockError(String), @@ -62,6 +62,9 @@ pub enum ArrayError { //#[error("value error: {0}")] ValueError(String), + //#[error("index: {0:?}")] + IndexContext(u64, Box), + //#[error("aggregate: {0:?}")] Aggregate(Vec), @@ -75,9 +78,13 @@ pub enum ArrayError { impl fmt::Display for ArrayError { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match self { - ArrayError::IoError => write!(f, "io error"), + ArrayError::IoError(b) => write!(f, "io error {}", b), ArrayError::BlockError(msg) => write!(f, "block error: {}", msg), ArrayError::ValueError(msg) => write!(f, "value error: {}", msg), + ArrayError::IndexContext(idx, e) => { + write!(f, "{}, effecting index {}", e, idx)?; + Ok(()) + } ArrayError::Aggregate(errs) => { for e in errs { write!(f, "{}", e)? @@ -90,6 +97,10 @@ impl fmt::Display for ArrayError { } } +pub fn io_err(path: &[u64], blocknr: u64) -> ArrayError { + ArrayError::Path(path.to_vec(), Box::new(ArrayError::IoError(blocknr))) +} + pub fn array_block_err(path: &[u64], msg: &str) -> ArrayError { ArrayError::Path( path.to_vec(), @@ -105,6 +116,12 @@ pub fn aggregate_error(errs: Vec) -> ArrayError { ArrayError::Aggregate(errs) } +impl ArrayError { + pub fn index_context(self, index: u64) -> ArrayError { + ArrayError::IndexContext(index, Box::new(self)) + } +} + pub type Result = std::result::Result; //------------------------------------------ diff --git a/src/pdata/array_walker.rs b/src/pdata/array_walker.rs index cd9d5e1..34df482 100644 --- a/src/pdata/array_walker.rs +++ b/src/pdata/array_walker.rs @@ -40,39 +40,48 @@ impl<'a, V: Unpack + Copy> BlockValueVisitor<'a, V> { } impl<'a, V: Unpack + Copy> NodeVisitor for BlockValueVisitor<'a, V> { - // FIXME: wrap ArrayError into BTreeError, rather than mapping to value_err? fn visit( &self, path: &[u64], - _kr: &KeyRange, + kr: &KeyRange, _h: &NodeHeader, keys: &[u64], values: &[u64], ) -> btree::Result<()> { let mut path = path.to_vec(); - let mut errs: Vec = Vec::new(); - // TODO: check index continuity + // The ordering of array indices had been verified in unpack_node(), + // thus checking the upper bound implies key continuity among siblings. + if *keys.first().unwrap() + keys.len() as u64 != *keys.last().unwrap() + 1 { + return Err(btree::value_err(format!("gaps in array indicies"))); + } + if let Some(end) = kr.end { + if *keys.last().unwrap() + 1 != end { + return Err(btree::value_err(format!("gaps or overlaps in array indicies"))); + } + } + + // FIXME: will the returned blocks be reordered? match self.engine.read_many(values) { Err(_) => { // IO completely failed on all the child blocks - // FIXME: count read errors on its parent (BTreeError::IoError) or on its location - // (ArrayError::IoError)? - for (_i, _b) in values.iter().enumerate() { - errs.push(btree::io_err(&path)); // FIXME: add key_context + for (i, b) in values.iter().enumerate() { + // TODO: report indices of array entries based on the type size + let mut array_errs = self.array_errs.lock().unwrap(); + array_errs.push(array::io_err(&path, *b).index_context(keys[i])); } } Ok(rblocks) => { for (i, rb) in rblocks.into_iter().enumerate() { match rb { Err(_) => { - errs.push(btree::io_err(&path)); // FIXME: add key_context + let mut array_errs = self.array_errs.lock().unwrap(); + array_errs.push(array::io_err(&path, values[i]).index_context(keys[i])); }, Ok(b) => { path.push(b.loc); match unpack_array_block::(&path, b.get_data()) { Ok(array_block) => { - // FIXME: will the returned blocks be reordered? if let Err(e) = self.array_visitor.visit(keys[i], array_block) { self.array_errs.lock().unwrap().push(e); } @@ -88,18 +97,7 @@ impl<'a, V: Unpack + Copy> NodeVisitor for BlockValueVisitor<'a, V> { } } - // FIXME: duplicate to BTreeWalker::build_aggregrate() - match errs.len() { - 0 => Ok(()), - 1 => { - let e = errs[0].clone(); - Err(e) - } - _ => { - let e = btree::aggregate_error(errs); - Err(e) - } - } + Ok(()) } fn visit_again(&self, _path: &[u64], _b: u64) -> btree::Result<()> {