From 74fcb9d505be4d015dac7ba98d4f6a8e4f5643cd Mon Sep 17 00:00:00 2001 From: Ming-Hung Tsai Date: Thu, 25 Feb 2021 18:14:01 +0800 Subject: [PATCH 01/24] [cache (rust)] Fix data types --- src/cache/xml.rs | 8 ++++---- tests/common/cache_xml_generator.rs | 10 +++++----- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/cache/xml.rs b/src/cache/xml.rs index d39b057..2cf2e12 100644 --- a/src/cache/xml.rs +++ b/src/cache/xml.rs @@ -11,22 +11,22 @@ use quick_xml::Writer; #[derive(Clone)] pub struct Superblock { pub uuid: String, - pub block_size: u64, - pub nr_cache_blocks: u64, + pub block_size: u32, + pub nr_cache_blocks: u32, pub policy: String, pub hint_width: u32, } #[derive(Clone)] pub struct Map { - pub cblock: u64, + pub cblock: u32, pub oblock: u64, pub dirty: bool, } #[derive(Clone)] pub struct Hint { - pub cblock: u64, + pub cblock: u32, pub data: Vec, } diff --git a/tests/common/cache_xml_generator.rs b/tests/common/cache_xml_generator.rs index d9e558e..68672e2 100644 --- a/tests/common/cache_xml_generator.rs +++ b/tests/common/cache_xml_generator.rs @@ -24,8 +24,8 @@ pub fn write_xml(path: &Path, g: &mut dyn XmlGen) -> Result<()> { } pub struct CacheGen { - block_size: u64, - nr_cache_blocks: u64, + block_size: u32, + nr_cache_blocks: u32, nr_origin_blocks: u64, percent_resident: u8, percent_dirty: u8, @@ -33,8 +33,8 @@ pub struct CacheGen { impl CacheGen { pub fn new( - block_size: u64, - nr_cache_blocks: u64, + block_size: u32, + nr_cache_blocks: u32, nr_origin_blocks: u64, percent_resident: u8, percent_dirty: u8, @@ -67,7 +67,7 @@ impl XmlGen for CacheGen { v.mappings_b()?; { - let nr_resident = (self.nr_cache_blocks * 100 as u64) / (self.percent_resident as u64); + let nr_resident = (self.nr_cache_blocks * 100 as u32) / (self.percent_resident as u32); let mut used = HashSet::new(); for n in 0..nr_resident { let mut oblock = 0u64; From fde0e0e2b801a0a7e2cbef02bb54c6afe91f43a8 Mon Sep 17 00:00:00 2001 From: Ming-Hung Tsai Date: Fri, 26 Feb 2021 15:14:50 +0800 Subject: [PATCH 02/24] [cache (rust)] Add Mapping::is_dirty() --- src/cache/mapping.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/cache/mapping.rs b/src/cache/mapping.rs index c18325a..7ee8768 100644 --- a/src/cache/mapping.rs +++ b/src/cache/mapping.rs @@ -24,6 +24,10 @@ impl Mapping { pub fn is_valid(&self) -> bool { return (self.flags & MappingFlags::Valid as u32) != 0; } + + pub fn is_dirty(&self) -> bool { + return (self.flags & MappingFlags::Dirty as u32) != 0; + } } From eb3d181f953bfc5d350f5e5a694b4d372ceacd7e Mon Sep 17 00:00:00 2001 From: Ming-Hung Tsai Date: Thu, 25 Feb 2021 18:14:44 +0800 Subject: [PATCH 03/24] [cache_dump (rust)] First draft of cache_dump --- src/bin/cache_dump.rs | 41 +++++++++++ src/cache/dump.rs | 157 ++++++++++++++++++++++++++++++++++++++++++ src/cache/mod.rs | 1 + 3 files changed, 199 insertions(+) create mode 100644 src/bin/cache_dump.rs create mode 100644 src/cache/dump.rs diff --git a/src/bin/cache_dump.rs b/src/bin/cache_dump.rs new file mode 100644 index 0000000..4d4bce7 --- /dev/null +++ b/src/bin/cache_dump.rs @@ -0,0 +1,41 @@ +extern crate clap; +extern crate thinp; + +use clap::{App, Arg}; +use std::path::Path; +use thinp::cache::dump::{dump, CacheDumpOptions}; + +//------------------------------------------ + +fn main() { + let parser = App::new("cache_check") + .version(thinp::version::TOOLS_VERSION) + .arg( + Arg::with_name("INPUT") + .help("Specify the input device to check") + .required(true) + .index(1), + ) + .arg( + Arg::with_name("REPAIR") + .help("") + .long("repair") + .value_name("REPAIR"), + ); + + let matches = parser.get_matches(); + let input_file = Path::new(matches.value_of("INPUT").unwrap()); + + let opts = CacheDumpOptions { + dev: &input_file, + async_io: false, + repair: matches.is_present("REPAIR"), + }; + + if let Err(reason) = dump(opts) { + eprintln!("{}", reason); + std::process::exit(1); + } +} + +//------------------------------------------ diff --git a/src/cache/dump.rs b/src/cache/dump.rs new file mode 100644 index 0000000..566673f --- /dev/null +++ b/src/cache/dump.rs @@ -0,0 +1,157 @@ +use std::marker::PhantomData; +use std::path::Path; +use std::sync::{Arc, Mutex}; + +use crate::io_engine::{AsyncIoEngine, IoEngine, SyncIoEngine}; +use crate::cache::hint::Hint; +use crate::cache::mapping::Mapping; +use crate::cache::superblock::*; +use crate::cache::xml::{self, MetadataVisitor}; +use crate::pdata::array_walker::*; + +//------------------------------------------ + +const MAX_CONCURRENT_IO: u32 = 1024; + +//------------------------------------------ + +// TODO: pull out MetadataVisitor from the xml crate? +struct MappingEmitter { + emitter: Arc>, +} + +impl MappingEmitter { + pub fn new(emitter: Arc>) -> MappingEmitter { + MappingEmitter { + emitter: emitter, + } + } +} + +impl ArrayBlockVisitor for MappingEmitter { + fn visit(&self, index: u64, m: Mapping) -> anyhow::Result<()> { + if m.oblock == 0 { + return Ok(()); + } + + // TODO: eliminate xml::Map? + let m = xml::Map { + cblock: index as u32, + oblock: m.oblock, + dirty: m.is_dirty(), + }; + + let mut emitter = self.emitter.lock().unwrap(); + emitter.mapping(&m)?; + + Ok(()) + } +} + +//----------------------------------------- + +struct HintEmitter { + emitter: Arc>, + _not_used: PhantomData, +} + +impl HintEmitter { + pub fn new(emitter: Arc>) -> HintEmitter { + HintEmitter { + emitter: emitter, + _not_used: PhantomData, + } + } +} + +impl ArrayBlockVisitor> for HintEmitter { + fn visit(&self, index: u64, hint: Hint) -> anyhow::Result<()> { + // TODO: skip invalid blocks + + let h = xml::Hint { + cblock: index as u32, + data: hint.hint.to_vec(), + }; + + let mut emitter = self.emitter.lock().unwrap(); + emitter.hint(&h)?; + + Ok(()) + } +} + +//------------------------------------------ + +pub struct CacheDumpOptions<'a> { + pub dev: &'a Path, + pub async_io: bool, + pub repair: bool, +} + +// TODO: add report +struct Context { + engine: Arc, +} + +fn mk_context(opts: &CacheDumpOptions) -> anyhow::Result { + let engine: Arc; + + if opts.async_io { + engine = Arc::new(AsyncIoEngine::new( + opts.dev, + MAX_CONCURRENT_IO, + false, + )?); + } else { + let nr_threads = std::cmp::max(8, num_cpus::get() * 2); + engine = Arc::new(SyncIoEngine::new(opts.dev, nr_threads, false)?); + } + + Ok(Context { + engine, + }) +} + +fn dump_metadata(ctx: &Context, sb: &Superblock, _repair: bool) -> anyhow::Result<()>{ + let engine = &ctx.engine; + + let out = Arc::new(Mutex::new(xml::XmlWriter::new(std::io::stdout()))); + let xml_sb = xml::Superblock { + uuid: "".to_string(), + block_size: sb.data_block_size, + nr_cache_blocks: sb.cache_blocks, + policy: std::str::from_utf8(&sb.policy_name[..])?.to_string(), + hint_width: sb.policy_hint_size, + }; + out.lock().unwrap().superblock_b(&xml_sb)?; + + out.lock().unwrap().mappings_b()?; + let w = ArrayWalker::new(engine.clone(), false); + let emitter = Box::new(MappingEmitter::new(out.clone())); + w.walk(emitter, sb.mapping_root)?; + out.lock().unwrap().mappings_e()?; + + out.lock().unwrap().hints_b()?; + type Width = typenum::U4; // FIXME: align with sb.policy_hint_size + let emitter = Box::new(HintEmitter::::new(out.clone())); + w.walk(emitter, sb.hint_root)?; + out.lock().unwrap().hints_e()?; + + // FIXME: walk discards + //out.lock().unwrap().discards_b()?; + //out.lock().unwrap().discards_e()?; + + out.lock().unwrap().superblock_e()?; + + Ok(()) +} + +pub fn dump(opts: CacheDumpOptions) -> anyhow::Result<()> { + let ctx = mk_context(&opts)?; + let engine = &ctx.engine; + let sb = read_superblock(engine.as_ref(), SUPERBLOCK_LOCATION)?; + + dump_metadata(&ctx, &sb, opts.repair) +} + +//------------------------------------------ diff --git a/src/cache/mod.rs b/src/cache/mod.rs index a120ce6..5aa61fa 100644 --- a/src/cache/mod.rs +++ b/src/cache/mod.rs @@ -1,4 +1,5 @@ pub mod check; +pub mod dump; pub mod hint; pub mod mapping; pub mod superblock; From 3cf0dba469e785b61e0d71795b6be4a84a942438 Mon Sep 17 00:00:00 2001 From: Joe Thornber Date: Wed, 3 Mar 2021 09:47:42 +0000 Subject: [PATCH 04/24] [cache_dump] fix how needs_check flag is checked --- src/cache/superblock.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cache/superblock.rs b/src/cache/superblock.rs index e98a3e9..5f7fb8f 100644 --- a/src/cache/superblock.rs +++ b/src/cache/superblock.rs @@ -98,7 +98,7 @@ fn unpack(data: &[u8]) -> IResult<&[u8], Superblock> { i, Superblock { flags: SuperblockFlags { - needs_check: (flags | 0x1) != 0, + needs_check: (flags & 0x1) != 0, }, block, version, From fdcc09c27ef0bd062a750398e35039dd4b52faa2 Mon Sep 17 00:00:00 2001 From: Joe Thornber Date: Wed, 3 Mar 2021 09:48:15 +0000 Subject: [PATCH 05/24] [cache_dump] Squash some clippy warnings --- src/cache/check.rs | 4 ++-- src/cache/dump.rs | 4 ++-- src/cache/mapping.rs | 4 ++-- src/pdata/array_block.rs | 2 +- 4 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/cache/check.rs b/src/cache/check.rs index 92895e1..5f9d295 100644 --- a/src/cache/check.rs +++ b/src/cache/check.rs @@ -35,7 +35,7 @@ impl CheckMappingVisitor { fn seen_oblock(&self, b: u64) -> bool { let seen_oblocks = self.seen_oblocks.lock().unwrap(); - return seen_oblocks.contains(&b); + seen_oblocks.contains(&b) } fn record_oblock(&self, b: u64) { @@ -46,7 +46,7 @@ impl CheckMappingVisitor { // FIXME: is it possible to validate flags at an early phase? // e.g., move to ctor of Mapping? fn has_unknown_flags(&self, m: &Mapping) -> bool { - return (m.flags & self.allowed_flags) != 0; + (m.flags & self.allowed_flags) != 0 } } diff --git a/src/cache/dump.rs b/src/cache/dump.rs index 566673f..fc2714a 100644 --- a/src/cache/dump.rs +++ b/src/cache/dump.rs @@ -23,7 +23,7 @@ struct MappingEmitter { impl MappingEmitter { pub fn new(emitter: Arc>) -> MappingEmitter { MappingEmitter { - emitter: emitter, + emitter, } } } @@ -58,7 +58,7 @@ struct HintEmitter { impl HintEmitter { pub fn new(emitter: Arc>) -> HintEmitter { HintEmitter { - emitter: emitter, + emitter, _not_used: PhantomData, } } diff --git a/src/cache/mapping.rs b/src/cache/mapping.rs index 7ee8768..4919ce5 100644 --- a/src/cache/mapping.rs +++ b/src/cache/mapping.rs @@ -22,11 +22,11 @@ pub struct Mapping { impl Mapping { pub fn is_valid(&self) -> bool { - return (self.flags & MappingFlags::Valid as u32) != 0; + (self.flags & MappingFlags::Valid as u32) != 0 } pub fn is_dirty(&self) -> bool { - return (self.flags & MappingFlags::Dirty as u32) != 0; + (self.flags & MappingFlags::Dirty as u32) != 0 } } diff --git a/src/pdata/array_block.rs b/src/pdata/array_block.rs index 0cf7069..ad52bba 100644 --- a/src/pdata/array_block.rs +++ b/src/pdata/array_block.rs @@ -50,7 +50,7 @@ impl ArrayBlock { } } -fn convert_result<'a, V>(r: IResult<&'a [u8], V>) -> Result<(&'a [u8], V)> { +fn convert_result(r: IResult<&[u8], V>) -> Result<(&[u8], V)> { r.map_err(|_| anyhow!("parse error")) } From e6c6275aea7dc3af8b63d3b7be1d493869f91eb4 Mon Sep 17 00:00:00 2001 From: Joe Thornber Date: Wed, 3 Mar 2021 10:27:57 +0000 Subject: [PATCH 06/24] Code review of src/pdata/array* --- src/pdata/array.rs | 39 --------------------------------- src/pdata/array_block.rs | 20 +++++------------ src/pdata/array_walker.rs | 46 +++++++++++++++------------------------ src/pdata/mod.rs | 1 - 4 files changed, 22 insertions(+), 84 deletions(-) delete mode 100644 src/pdata/array.rs diff --git a/src/pdata/array.rs b/src/pdata/array.rs deleted file mode 100644 index d8299c2..0000000 --- a/src/pdata/array.rs +++ /dev/null @@ -1,39 +0,0 @@ -use nom::{number::complete::*, IResult}; -use std::fmt; - -use crate::pdata::unpack::*; - -//------------------------------------------ - -// TODO: build a data structure representating an array? - -// FIXME: rename this struct -pub struct ArrayBlockEntry { - pub block: u64, -} - -impl Unpack for ArrayBlockEntry { - fn disk_size() -> u32 { - 8 - } - - fn unpack(i: &[u8]) -> IResult<&[u8], ArrayBlockEntry> { - let (i, n) = le_u64(i)?; - let block = n; - - Ok(( - i, - ArrayBlockEntry { - block, - } - )) - } -} - -impl fmt::Display for ArrayBlockEntry { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - write!(f, "{}", self.block) - } -} - -//------------------------------------------ diff --git a/src/pdata/array_block.rs b/src/pdata/array_block.rs index ad52bba..b082d92 100644 --- a/src/pdata/array_block.rs +++ b/src/pdata/array_block.rs @@ -33,7 +33,7 @@ impl Unpack for ArrayBlockHeader { max_entries, nr_entries, value_size, - blocknr + blocknr, }, )) } @@ -44,27 +44,17 @@ pub struct ArrayBlock { pub values: Vec, } -impl ArrayBlock { - pub fn get_header(&self) -> &ArrayBlockHeader { - &self.header - } -} - fn convert_result(r: IResult<&[u8], V>) -> Result<(&[u8], V)> { r.map_err(|_| anyhow!("parse error")) } -pub fn unpack_array_block( - data: &[u8], -) -> Result> { +pub fn unpack_array_block(data: &[u8]) -> Result> { // TODO: collect errors - let (i, header) = ArrayBlockHeader::unpack(data).map_err(|_e| anyhow!("Couldn't parse header"))?; + let (i, header) = + ArrayBlockHeader::unpack(data).map_err(|_e| anyhow!("Couldn't parse header"))?; let (_i, values) = convert_result(count(V::unpack, header.nr_entries as usize)(i))?; - Ok(ArrayBlock { - header, - values, - }) + Ok(ArrayBlock { header, values }) } //------------------------------------------ diff --git a/src/pdata/array_walker.rs b/src/pdata/array_walker.rs index e8287b1..a8d5c57 100644 --- a/src/pdata/array_walker.rs +++ b/src/pdata/array_walker.rs @@ -1,7 +1,6 @@ use std::sync::Arc; use crate::io_engine::*; -use crate::pdata::array::*; use crate::pdata::array_block::*; use crate::pdata::btree::*; use crate::pdata::btree_walker::*; @@ -11,16 +10,12 @@ use crate::pdata::unpack::*; pub struct ArrayWalker { engine: Arc, - ignore_non_fatal: bool + ignore_non_fatal: bool, } // FIXME: define another Result type for array visiting? pub trait ArrayBlockVisitor { - fn visit( - &self, - index: u64, - v: V, - ) -> anyhow::Result<()>; + fn visit(&self, index: u64, v: V) -> anyhow::Result<()>; } struct BlockValueVisitor { @@ -29,9 +24,9 @@ struct BlockValueVisitor { } impl BlockValueVisitor { - pub fn new>( + pub fn new( e: Arc, - v: Box, + v: Box>, ) -> BlockValueVisitor { BlockValueVisitor { engine: e, @@ -39,19 +34,17 @@ impl BlockValueVisitor { } } - pub fn visit_array_block( - &self, - index: u64, - array_block: ArrayBlock, - ) { - let begin = index * u64::from(array_block.header.nr_entries); + pub fn visit_array_block(&self, index: u64, array_block: ArrayBlock) { + let begin = index * array_block.header.max_entries as u64; for i in 0..array_block.header.nr_entries { - self.array_block_visitor.visit(begin + u64::from(i), array_block.values[i as usize]).unwrap(); + self.array_block_visitor + .visit(begin + i as u64, array_block.values[i as usize]) + .unwrap(); } } } -impl NodeVisitor for BlockValueVisitor { +impl NodeVisitor for BlockValueVisitor { // FIXME: return errors fn visit( &self, @@ -59,23 +52,20 @@ impl NodeVisitor for BlockValueVisitor { _kr: &KeyRange, _h: &NodeHeader, keys: &[u64], - values: &[ArrayBlockEntry], + values: &[u64], ) -> Result<()> { - for n in 0..keys.len() { - let index = keys[n]; - let b = self.engine.read(values[n].block).map_err(|_| io_err(path))?; + for (n, index) in keys.iter().enumerate() { + let b = self.engine.read(values[n]).map_err(|_| io_err(path))?; let array_block = unpack_array_block::(b.get_data()).map_err(|_| io_err(path))?; - self.visit_array_block(index, array_block); + self.visit_array_block(*index, array_block); } Ok(()) } - // FIXME: stub fn visit_again(&self, _path: &[u64], _b: u64) -> Result<()> { Ok(()) } - // FIXME: stub fn end_walk(&self) -> Result<()> { Ok(()) } @@ -90,15 +80,13 @@ impl ArrayWalker { r } - // FIXME: pass the visitor by reference? // FIXME: redefine the Result type for array visiting? - pub fn walk(&self, visitor: Box, root: u64) -> Result<()> + pub fn walk(&self, visitor: Box>, root: u64) -> Result<()> where - BV: 'static + ArrayBlockVisitor, - V: Unpack, + V: Unpack + Copy, { let w = BTreeWalker::new(self.engine.clone(), self.ignore_non_fatal); - let mut path = Vec::new(); // FIXME: eliminate this line? + let mut path = Vec::new(); path.push(0); let v = BlockValueVisitor::::new(self.engine.clone(), visitor); w.walk(&mut path, &v, root) diff --git a/src/pdata/mod.rs b/src/pdata/mod.rs index 370d244..93f8811 100644 --- a/src/pdata/mod.rs +++ b/src/pdata/mod.rs @@ -1,4 +1,3 @@ -pub mod array; pub mod array_block; pub mod array_walker; pub mod btree; From 7e869bb8e088018da97093934410cc0942939e04 Mon Sep 17 00:00:00 2001 From: Joe Thornber Date: Thu, 4 Mar 2021 11:13:08 +0000 Subject: [PATCH 07/24] Code review of cache_dump/check Added support for metadata format2 to cache_dump. Nothing tested. --- Cargo.toml | 1 - src/cache/check.rs | 49 +++----- src/cache/dump.rs | 252 ++++++++++++++++++++++++++++---------- src/cache/hint.rs | 16 +-- src/cache/mapping.rs | 3 +- src/cache/superblock.rs | 2 +- src/pdata/array_walker.rs | 16 +-- 7 files changed, 222 insertions(+), 117 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index cc558c5..b47c1b6 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -33,7 +33,6 @@ threadpool = "1.8" thiserror = "1.0" tui = "0.10" termion = "1.5" -typenum = "1.12.0" [dev-dependencies] json = "0.12" diff --git a/src/cache/check.rs b/src/cache/check.rs index 5f9d295..f5e24c5 100644 --- a/src/cache/check.rs +++ b/src/cache/check.rs @@ -1,13 +1,12 @@ use anyhow::anyhow; -use std::marker::PhantomData; +use std::collections::*; use std::path::Path; use std::sync::{Arc, Mutex}; -use std::collections::*; -use crate::io_engine::{AsyncIoEngine, IoEngine, SyncIoEngine}; use crate::cache::hint::*; use crate::cache::mapping::*; use crate::cache::superblock::*; +use crate::io_engine::{AsyncIoEngine, IoEngine, SyncIoEngine}; use crate::pdata::array_walker::*; //------------------------------------------ @@ -50,7 +49,7 @@ impl CheckMappingVisitor { } } -impl ArrayBlockVisitor for CheckMappingVisitor { +impl ArrayVisitor for CheckMappingVisitor { fn visit(&self, _index: u64, m: Mapping) -> anyhow::Result<()> { if !m.is_valid() { return Ok(()); @@ -72,20 +71,16 @@ impl ArrayBlockVisitor for CheckMappingVisitor { //------------------------------------------ -struct CheckHintVisitor { - _not_used: PhantomData, -} +struct CheckHintVisitor {} -impl CheckHintVisitor { - fn new() -> CheckHintVisitor { - CheckHintVisitor { - _not_used: PhantomData, - } +impl CheckHintVisitor { + fn new() -> CheckHintVisitor { + CheckHintVisitor {} } } -impl ArrayBlockVisitor> for CheckHintVisitor { - fn visit(&self, _index: u64, _hint: Hint) -> anyhow::Result<()> { +impl ArrayVisitor for CheckHintVisitor { + fn visit(&self, _index: u64, _hint: Hint) -> anyhow::Result<()> { // TODO: check hints Ok(()) } @@ -112,19 +107,13 @@ fn mk_context(opts: &CacheCheckOptions) -> anyhow::Result { let engine: Arc; if opts.async_io { - engine = Arc::new(AsyncIoEngine::new( - opts.dev, - MAX_CONCURRENT_IO, - false, - )?); + engine = Arc::new(AsyncIoEngine::new(opts.dev, MAX_CONCURRENT_IO, false)?); } else { let nr_threads = std::cmp::max(8, num_cpus::get() * 2); engine = Arc::new(SyncIoEngine::new(opts.dev, nr_threads, false)?); } - Ok(Context { - engine, - }) + Ok(Context { engine }) } pub fn check(opts: CacheCheckOptions) -> anyhow::Result<()> { @@ -141,19 +130,21 @@ 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 c = Box::new(CheckMappingVisitor::new(sb.version)); - w.walk(c, sb.mapping_root)?; + let mut c = CheckMappingVisitor::new(sb.version); + w.walk(&mut c, sb.mapping_root)?; if sb.version >= 2 { - // TODO: check dirty bitset + // TODO: check dirty bitset } } - if !opts.skip_hints && sb.hint_root != 0 { + if !opts.skip_hints && sb.hint_root != 0 && sb.policy_hint_size != 0 { + 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); - type Width = typenum::U4; // FIXME: check sb.policy_hint_size - let c = Box::new(CheckHintVisitor::::new()); - w.walk(c, sb.hint_root)?; + let mut c = CheckHintVisitor::new(); + w.walk(&mut c, sb.hint_root)?; } if !opts.skip_discards { diff --git a/src/cache/dump.rs b/src/cache/dump.rs index fc2714a..eacda2a 100644 --- a/src/cache/dump.rs +++ b/src/cache/dump.rs @@ -1,12 +1,13 @@ -use std::marker::PhantomData; +use anyhow::{anyhow, Result}; +use fixedbitset::FixedBitSet; use std::path::Path; use std::sync::{Arc, Mutex}; -use crate::io_engine::{AsyncIoEngine, IoEngine, SyncIoEngine}; use crate::cache::hint::Hint; use crate::cache::mapping::Mapping; use crate::cache::superblock::*; use crate::cache::xml::{self, MetadataVisitor}; +use crate::io_engine::{AsyncIoEngine, IoEngine, SyncIoEngine}; use crate::pdata::array_walker::*; //------------------------------------------ @@ -15,66 +16,167 @@ const MAX_CONCURRENT_IO: u32 = 1024; //------------------------------------------ -// TODO: pull out MetadataVisitor from the xml crate? -struct MappingEmitter { - emitter: Arc>, -} +mod format1 { + use super::*; -impl MappingEmitter { - pub fn new(emitter: Arc>) -> MappingEmitter { - MappingEmitter { - emitter, + struct Inner<'a> { + visitor: &'a mut dyn MetadataVisitor, + valid_mappings: FixedBitSet, + } + + pub struct MappingEmitter<'a> { + inner: Mutex>, + } + + impl<'a> MappingEmitter<'a> { + pub fn new(nr_entries: usize, visitor: &'a mut dyn MetadataVisitor) -> MappingEmitter<'a> { + MappingEmitter { + inner: Mutex::new(Inner { + visitor, + valid_mappings: FixedBitSet::with_capacity(nr_entries), + }), + } + } + + pub fn get_valid(self) -> FixedBitSet { + let inner = self.inner.into_inner().unwrap(); + inner.valid_mappings + } + } + + impl<'a> ArrayVisitor for MappingEmitter<'a> { + fn visit(&self, index: u64, m: Mapping) -> Result<()> { + if m.is_valid() { + let m = xml::Map { + cblock: index as u32, + oblock: m.oblock, + dirty: m.is_dirty(), + }; + + let mut inner = self.inner.lock().unwrap(); + inner.valid_mappings.set(index as usize, true); + inner.visitor.mapping(&m)?; + } + + Ok(()) } } } -impl ArrayBlockVisitor for MappingEmitter { - fn visit(&self, index: u64, m: Mapping) -> anyhow::Result<()> { - if m.oblock == 0 { - return Ok(()); +//------------------------------------------ + +mod format2 { + use super::*; + + //------------------- + // Dirty bitset visitor + pub struct DirtyVisitor { + nr_entries: usize, + bits: Mutex, + } + + impl DirtyVisitor { + pub fn new(nr_entries: usize) -> Self { + DirtyVisitor { + nr_entries, + bits: Mutex::new(FixedBitSet::with_capacity(nr_entries)), + } } - // TODO: eliminate xml::Map? - let m = xml::Map { - cblock: index as u32, - oblock: m.oblock, - dirty: m.is_dirty(), - }; + pub fn get_bits(self) -> FixedBitSet { + self.bits.into_inner().unwrap() + } + } - let mut emitter = self.emitter.lock().unwrap(); - emitter.mapping(&m)?; + impl ArrayVisitor for DirtyVisitor { + fn visit(&self, index: u64, bits: u64) -> Result<()> { + for i in 0..64u64 { + if (index + i) >= self.nr_entries as u64 { + break; + } - Ok(()) + self.bits.lock().unwrap().set((index + i) as usize, bits & (1 << i) != 0); + } + Ok(()) + } + } + + //------------------- + // Mapping visitor + + struct Inner<'a> { + visitor: &'a mut dyn MetadataVisitor, + dirty_bits: FixedBitSet, + valid_mappings: FixedBitSet, + } + + pub struct MappingEmitter<'a> { + inner: Mutex>, + } + + impl<'a> MappingEmitter<'a> { + pub fn new(nr_entries: usize, dirty_bits: FixedBitSet, visitor: &'a mut dyn MetadataVisitor) -> MappingEmitter<'a> { + MappingEmitter { + inner: Mutex::new(Inner { + visitor, + dirty_bits, + valid_mappings: FixedBitSet::with_capacity(nr_entries), + }), + } + } + + pub fn get_valid(self) -> FixedBitSet { + let inner = self.inner.into_inner().unwrap(); + inner.valid_mappings + } + } + + impl<'a> ArrayVisitor for MappingEmitter<'a> { + fn visit(&self, index: u64, m: Mapping) -> Result<()> { + if m.is_valid() { + let mut inner = self.inner.lock().unwrap(); + let dirty = inner.dirty_bits.contains(index as usize); + let m = xml::Map { + cblock: index as u32, + oblock: m.oblock, + dirty, + }; + + inner.valid_mappings.set(index as usize, true); + inner.visitor.mapping(&m)?; + } + + Ok(()) + } } } //----------------------------------------- -struct HintEmitter { - emitter: Arc>, - _not_used: PhantomData, +struct HintEmitter<'a> { + emitter: Mutex<&'a mut dyn MetadataVisitor>, + valid_mappings: FixedBitSet, } -impl HintEmitter { - pub fn new(emitter: Arc>) -> HintEmitter { +impl<'a> HintEmitter<'a> { + pub fn new(emitter: &'a mut dyn MetadataVisitor, valid_mappings: FixedBitSet) -> HintEmitter { HintEmitter { - emitter, - _not_used: PhantomData, + emitter: Mutex::new(emitter), + valid_mappings, } } } -impl ArrayBlockVisitor> for HintEmitter { - fn visit(&self, index: u64, hint: Hint) -> anyhow::Result<()> { - // TODO: skip invalid blocks +impl<'a> ArrayVisitor for HintEmitter<'a> { + fn visit(&self, index: u64, hint: Hint) -> anyhow::Result<()> { + if self.valid_mappings.contains(index as usize) { + let h = xml::Hint { + cblock: index as u32, + data: hint.hint.to_vec(), + }; - let h = xml::Hint { - cblock: index as u32, - data: hint.hint.to_vec(), - }; - - let mut emitter = self.emitter.lock().unwrap(); - emitter.hint(&h)?; + self.emitter.lock().unwrap().hint(&h)?; + } Ok(()) } @@ -88,7 +190,6 @@ pub struct CacheDumpOptions<'a> { pub repair: bool, } -// TODO: add report struct Context { engine: Arc, } @@ -97,25 +198,19 @@ fn mk_context(opts: &CacheDumpOptions) -> anyhow::Result { let engine: Arc; if opts.async_io { - engine = Arc::new(AsyncIoEngine::new( - opts.dev, - MAX_CONCURRENT_IO, - false, - )?); + engine = Arc::new(AsyncIoEngine::new(opts.dev, MAX_CONCURRENT_IO, false)?); } else { let nr_threads = std::cmp::max(8, num_cpus::get() * 2); engine = Arc::new(SyncIoEngine::new(opts.dev, nr_threads, false)?); } - Ok(Context { - engine, - }) + Ok(Context { engine }) } -fn dump_metadata(ctx: &Context, sb: &Superblock, _repair: bool) -> anyhow::Result<()>{ +fn dump_metadata(ctx: &Context, sb: &Superblock, _repair: bool) -> anyhow::Result<()> { let engine = &ctx.engine; - let out = Arc::new(Mutex::new(xml::XmlWriter::new(std::io::stdout()))); + let mut out = xml::XmlWriter::new(std::io::stdout()); let xml_sb = xml::Superblock { uuid: "".to_string(), block_size: sb.data_block_size, @@ -123,25 +218,50 @@ fn dump_metadata(ctx: &Context, sb: &Superblock, _repair: bool) -> anyhow::Resul policy: std::str::from_utf8(&sb.policy_name[..])?.to_string(), hint_width: sb.policy_hint_size, }; - out.lock().unwrap().superblock_b(&xml_sb)?; + out.superblock_b(&xml_sb)?; - out.lock().unwrap().mappings_b()?; - let w = ArrayWalker::new(engine.clone(), false); - let emitter = Box::new(MappingEmitter::new(out.clone())); - w.walk(emitter, sb.mapping_root)?; - out.lock().unwrap().mappings_e()?; + out.mappings_b()?; + let valid_mappings = match sb.version { + 1 => { + let w = ArrayWalker::new(engine.clone(), false); + let mut emitter = format1::MappingEmitter::new(sb.cache_blocks as usize, &mut out); + w.walk(&mut emitter, sb.mapping_root)?; + emitter.get_valid() + } + 2 => { + // We need to walk the dirty bitset first. + let w = ArrayWalker::new(engine.clone(), false); + let mut v = format2::DirtyVisitor::new(sb.cache_blocks as usize); - out.lock().unwrap().hints_b()?; - type Width = typenum::U4; // FIXME: align with sb.policy_hint_size - let emitter = Box::new(HintEmitter::::new(out.clone())); - w.walk(emitter, sb.hint_root)?; - out.lock().unwrap().hints_e()?; + if let Some(root) = sb.dirty_root { + w.walk(&mut v, root)?; + } else { + // FIXME: is there a way this can legally happen? eg, + // a crash of a freshly created cache? + return Err(anyhow!("format 2 selected, but no dirty bitset present")); + } + let dirty_bits = v.get_bits(); - // FIXME: walk discards - //out.lock().unwrap().discards_b()?; - //out.lock().unwrap().discards_e()?; + let w = ArrayWalker::new(engine.clone(), false); + let mut emitter = format2::MappingEmitter::new(sb.cache_blocks as usize, dirty_bits, &mut out); + w.walk(&mut emitter, sb.mapping_root)?; + emitter.get_valid() + } + v => { + return Err(anyhow!("unsupported metadata version: {}", v)); + } + }; + out.mappings_e()?; - out.lock().unwrap().superblock_e()?; + out.hints_b()?; + { + let w = ArrayWalker::new(engine.clone(), false); + let mut emitter = HintEmitter::new(&mut out, valid_mappings); + w.walk(&mut emitter, sb.hint_root)?; + } + out.hints_e()?; + + out.superblock_e()?; Ok(()) } diff --git a/src/cache/hint.rs b/src/cache/hint.rs index ef0d83f..b066645 100644 --- a/src/cache/hint.rs +++ b/src/cache/hint.rs @@ -1,5 +1,4 @@ use nom::IResult; -use std::marker::PhantomData; use std::convert::TryInto; use crate::pdata::unpack::*; @@ -7,24 +6,21 @@ use crate::pdata::unpack::*; //------------------------------------------ #[derive(Clone, Copy)] -pub struct Hint { - pub hint: [u8; 4], // FIXME: support various hint sizes - _not_used: PhantomData, +pub struct Hint { + pub hint: [u8; 4], } -impl Unpack for Hint { +impl Unpack for Hint { fn disk_size() -> u32 { - Width::to_u32() + 4 } - // FIXME: support different width - fn unpack(i: &[u8]) -> IResult<&[u8], Hint> { - let size = Width::to_usize(); + fn unpack(i: &[u8]) -> IResult<&[u8], Hint> { + let size = 4; Ok(( &i[size..], Hint { hint: i[0..size].try_into().unwrap(), - _not_used: PhantomData, }, )) } diff --git a/src/cache/mapping.rs b/src/cache/mapping.rs index 4919ce5..497bc48 100644 --- a/src/cache/mapping.rs +++ b/src/cache/mapping.rs @@ -1,5 +1,5 @@ -use nom::IResult; use nom::number::complete::*; +use nom::IResult; use crate::pdata::unpack::*; @@ -30,7 +30,6 @@ impl Mapping { } } - impl Unpack for Mapping { fn disk_size() -> u32 { 8 diff --git a/src/cache/superblock.rs b/src/cache/superblock.rs index 5f7fb8f..a71140b 100644 --- a/src/cache/superblock.rs +++ b/src/cache/superblock.rs @@ -1,7 +1,7 @@ use anyhow::{anyhow, Result}; +use nom::{bytes::complete::*, number::complete::*, IResult}; use crate::io_engine::*; -use nom::{bytes::complete::*, number::complete::*, IResult}; //------------------------------------------ diff --git a/src/pdata/array_walker.rs b/src/pdata/array_walker.rs index a8d5c57..672259e 100644 --- a/src/pdata/array_walker.rs +++ b/src/pdata/array_walker.rs @@ -14,20 +14,20 @@ pub struct ArrayWalker { } // FIXME: define another Result type for array visiting? -pub trait ArrayBlockVisitor { +pub trait ArrayVisitor { fn visit(&self, index: u64, v: V) -> anyhow::Result<()>; } -struct BlockValueVisitor { +struct BlockValueVisitor<'a, V> { engine: Arc, - array_block_visitor: Box>, + array_block_visitor: &'a mut dyn ArrayVisitor, } -impl BlockValueVisitor { +impl<'a, V: Unpack + Copy> BlockValueVisitor<'a, V> { pub fn new( e: Arc, - v: Box>, - ) -> BlockValueVisitor { + v: &'a mut dyn ArrayVisitor, + ) -> BlockValueVisitor<'a, V> { BlockValueVisitor { engine: e, array_block_visitor: v, @@ -44,7 +44,7 @@ impl BlockValueVisitor { } } -impl NodeVisitor for BlockValueVisitor { +impl<'a, V: Unpack + Copy> NodeVisitor for BlockValueVisitor<'a, V> { // FIXME: return errors fn visit( &self, @@ -81,7 +81,7 @@ impl ArrayWalker { } // FIXME: redefine the Result type for array visiting? - pub fn walk(&self, visitor: Box>, root: u64) -> Result<()> + pub fn walk(&self, visitor: &mut dyn ArrayVisitor, root: u64) -> Result<()> where V: Unpack + Copy, { From d0675dd7bf53cd46be7d1e2eb83d79b136d99ccb Mon Sep 17 00:00:00 2001 From: Joe Thornber Date: Thu, 4 Mar 2021 11:19:04 +0000 Subject: [PATCH 08/24] Add missing header. So many errors from one issue --- dbg-lib/output_formatter.h | 1 + 1 file changed, 1 insertion(+) diff --git a/dbg-lib/output_formatter.h b/dbg-lib/output_formatter.h index 2cc4bc5..a21a92c 100644 --- a/dbg-lib/output_formatter.h +++ b/dbg-lib/output_formatter.h @@ -6,6 +6,7 @@ #include #include +#include #include #include From 860b3ca7d2ebf4219becdc4eb9c6ef98cb066b90 Mon Sep 17 00:00:00 2001 From: Ming-Hung Tsai Date: Tue, 30 Mar 2021 23:43:40 +0800 Subject: [PATCH 09/24] [cache_check (rust)] Add more checks - Check the version-1 dirty flag - Check mappings against the origin device size, if the cache is clean - Check superblock fields --- src/bin/cache_dump.rs | 2 +- src/cache/check.rs | 95 +++++++++++++++++++++++++++++------------ src/cache/mapping.rs | 3 +- src/cache/superblock.rs | 4 +- 4 files changed, 74 insertions(+), 30 deletions(-) diff --git a/src/bin/cache_dump.rs b/src/bin/cache_dump.rs index 4d4bce7..edf8b4c 100644 --- a/src/bin/cache_dump.rs +++ b/src/bin/cache_dump.rs @@ -8,7 +8,7 @@ use thinp::cache::dump::{dump, CacheDumpOptions}; //------------------------------------------ fn main() { - let parser = App::new("cache_check") + let parser = App::new("cache_dump") .version(thinp::version::TOOLS_VERSION) .arg( Arg::with_name("INPUT") diff --git a/src/cache/check.rs b/src/cache/check.rs index f5e24c5..6c4d356 100644 --- a/src/cache/check.rs +++ b/src/cache/check.rs @@ -16,54 +16,79 @@ const MAX_CONCURRENT_IO: u32 = 1024; //------------------------------------------ struct CheckMappingVisitor { + metadata_version: u32, allowed_flags: u32, + nr_origin_blocks: u64, seen_oblocks: Arc>>, + //dirty_iterator: Option, } impl CheckMappingVisitor { - fn new(metadata_version: u32) -> CheckMappingVisitor { + fn new(metadata_version: u32, nr_origin_blocks: Option, dirty_root: Option) -> CheckMappingVisitor { let mut flags: u32 = MappingFlags::Valid as u32; + //let dirty_iterator; if metadata_version == 1 { flags |= MappingFlags::Dirty as u32; + //dirty_iterator = None; + } else { + let _b = dirty_root.expect("dirty bitset unavailable"); + //dirty_iterator = Some(BitsetIterator::new(b)); } + CheckMappingVisitor { + metadata_version, allowed_flags: flags, + nr_origin_blocks: if let Some(n) = nr_origin_blocks {n} else {MAX_ORIGIN_BLOCKS}, seen_oblocks: Arc::new(Mutex::new(BTreeSet::new())), + //dirty_iterator, } } - fn seen_oblock(&self, b: u64) -> bool { - let seen_oblocks = self.seen_oblocks.lock().unwrap(); - seen_oblocks.contains(&b) + // TODO: move to ctor of Mapping? + fn check_flags(&self, m: &Mapping) -> anyhow::Result<()> { + if (m.flags & !self.allowed_flags) != 0 { + return Err(anyhow!("unknown flags in mapping")); + } + + if !m.is_valid() { + if self.metadata_version == 1 { + if m.is_dirty() { + return Err(anyhow!("dirty bit found on an unmapped block")); + } + }/*else if dirty_iterator.expect("dirty bitset unavailable").next() { + return Err(anyhow!("dirty bit found on an unmapped block")); + }*/ + } + + Ok(()) } - fn record_oblock(&self, b: u64) { + fn check_oblock(&self, m: &Mapping) -> anyhow::Result<()> { + if !m.is_valid() { + if m.oblock > 0 { + return Err(anyhow!("invalid mapped block")); + } + return Ok(()); + } + + if m.oblock >= self.nr_origin_blocks { + return Err(anyhow!("mapping beyond end of the origin device")); + } + let mut seen_oblocks = self.seen_oblocks.lock().unwrap(); - seen_oblocks.insert(b); - } + if seen_oblocks.contains(&m.oblock) { + return Err(anyhow!("origin block already mapped")); + } + seen_oblocks.insert(m.oblock); - // FIXME: is it possible to validate flags at an early phase? - // e.g., move to ctor of Mapping? - fn has_unknown_flags(&self, m: &Mapping) -> bool { - (m.flags & self.allowed_flags) != 0 + Ok(()) } } impl ArrayVisitor for CheckMappingVisitor { fn visit(&self, _index: u64, m: Mapping) -> anyhow::Result<()> { - if !m.is_valid() { - return Ok(()); - } - - if self.seen_oblock(m.oblock) { - return Err(anyhow!("origin block already mapped")); - } - - self.record_oblock(m.oblock); - - if !self.has_unknown_flags(&m) { - return Err(anyhow!("unknown flags in mapping")); - } + self.check_flags(&m)?; + self.check_oblock(&m)?; Ok(()) } @@ -71,11 +96,11 @@ impl ArrayVisitor for CheckMappingVisitor { //------------------------------------------ -struct CheckHintVisitor {} +struct CheckHintVisitor; impl CheckHintVisitor { fn new() -> CheckHintVisitor { - CheckHintVisitor {} + CheckHintVisitor } } @@ -116,21 +141,37 @@ fn mk_context(opts: &CacheCheckOptions) -> anyhow::Result { Ok(Context { engine }) } +fn check_superblock(sb: &Superblock) -> anyhow::Result<()> { + if sb.version >= 2 && sb.dirty_root == None { + return Err(anyhow!("dirty bitset not found")); + } + Ok(()) +} + pub fn check(opts: CacheCheckOptions) -> anyhow::Result<()> { let ctx = mk_context(&opts)?; let engine = &ctx.engine; let sb = read_superblock(engine.as_ref(), SUPERBLOCK_LOCATION)?; + check_superblock(&sb)?; if opts.sb_only { return Ok(()); } + let nr_origin_blocks; + if sb.flags.clean_shutdown { + let origin_sectors = sb.discard_block_size * sb.discard_nr_blocks; + nr_origin_blocks = Some(origin_sectors / sb.data_block_size as u64); + } else { + nr_origin_blocks = None; + } + // TODO: factor out into check_mappings() if !opts.skip_mappings { let w = ArrayWalker::new(engine.clone(), false); - let mut c = CheckMappingVisitor::new(sb.version); + let mut c = CheckMappingVisitor::new(sb.version, nr_origin_blocks, sb.dirty_root); w.walk(&mut c, sb.mapping_root)?; if sb.version >= 2 { diff --git a/src/cache/mapping.rs b/src/cache/mapping.rs index 497bc48..ac5a036 100644 --- a/src/cache/mapping.rs +++ b/src/cache/mapping.rs @@ -5,7 +5,8 @@ use crate::pdata::unpack::*; //------------------------------------------ -static FLAGS_MASK: u64 = (1 << 16) - 1; +pub const MAX_ORIGIN_BLOCKS: u64 = 1 << 48; +const FLAGS_MASK: u64 = (1 << 16) - 1; //------------------------------------------ diff --git a/src/cache/superblock.rs b/src/cache/superblock.rs index a71140b..c944439 100644 --- a/src/cache/superblock.rs +++ b/src/cache/superblock.rs @@ -14,6 +14,7 @@ const SPACE_MAP_ROOT_SIZE: usize = 128; #[derive(Debug, Clone)] pub struct SuperblockFlags { + pub clean_shutdown: bool, pub needs_check: bool, } @@ -98,7 +99,8 @@ fn unpack(data: &[u8]) -> IResult<&[u8], Superblock> { i, Superblock { flags: SuperblockFlags { - needs_check: (flags & 0x1) != 0, + clean_shutdown: (flags & 0x1) != 0, + needs_check: (flags & 0x2) != 0, }, block, version, From ae630f1fd8688d3dc3a080230d25db4a3ba03725 Mon Sep 17 00:00:00 2001 From: Ming-Hung Tsai Date: Thu, 8 Apr 2021 01:25:48 +0800 Subject: [PATCH 10/24] [btree_walker] Fix error returning --- src/pdata/btree_walker.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/pdata/btree_walker.rs b/src/pdata/btree_walker.rs index 3e9c2d3..28babc0 100644 --- a/src/pdata/btree_walker.rs +++ b/src/pdata/btree_walker.rs @@ -379,13 +379,13 @@ where } } Ok(rblocks) => { - let errs = Arc::new(Mutex::new(Vec::new())); + let child_errs = Arc::new(Mutex::new(Vec::new())); for (i, rb) in rblocks.into_iter().enumerate() { match rb { Err(_) => { let e = io_err(path).keys_context(&filtered_krs[i]); - let mut errs = errs.lock().unwrap(); + let mut errs = child_errs.lock().unwrap(); errs.push(e.clone()); w.set_fail(blocks[i], e); } @@ -393,7 +393,7 @@ where let w = w.clone(); let visitor = visitor.clone(); let kr = filtered_krs[i].clone(); - let errs = errs.clone(); + let errs = child_errs.clone(); let mut path = path.clone(); pool.execute(move || { @@ -410,6 +410,8 @@ where } pool.join(); + let mut child_errs = Arc::try_unwrap(child_errs).unwrap().into_inner().unwrap(); + errs.append(&mut child_errs); } } From ace9c1d1e3a3d5e24737059ba67faa920ffaf2cf Mon Sep 17 00:00:00 2001 From: Ming-Hung Tsai Date: Sun, 11 Apr 2021 00:43:12 +0800 Subject: [PATCH 11/24] [cache_check (rust)] Add more checks - check version-2 dirty bitset size and consistency - check discard bitset size --- Cargo.lock | 7 -- src/cache/check.rs | 212 +++++++++++++++++++++++-------------- src/pdata/bitset_walker.rs | 51 +++++++++ src/pdata/mod.rs | 1 + 4 files changed, 187 insertions(+), 84 deletions(-) create mode 100644 src/pdata/bitset_walker.rs diff --git a/Cargo.lock b/Cargo.lock index 7f8f53f..e4ed583 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -810,7 +810,6 @@ dependencies = [ "thiserror", "threadpool", "tui", - "typenum", ] [[package]] @@ -864,12 +863,6 @@ dependencies = [ "unicode-width", ] -[[package]] -name = "typenum" -version = "1.12.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "373c8a200f9e67a0c95e62a4f52fbf80c23b4381c05a17845531982fa99e6b33" - [[package]] name = "unicode-segmentation" version = "1.6.0" diff --git a/src/cache/check.rs b/src/cache/check.rs index 6c4d356..261840b 100644 --- a/src/cache/check.rs +++ b/src/cache/check.rs @@ -1,5 +1,6 @@ use anyhow::anyhow; -use std::collections::*; +use fixedbitset::FixedBitSet; +use std::collections::BTreeSet; use std::path::Path; use std::sync::{Arc, Mutex}; @@ -8,6 +9,7 @@ use crate::cache::mapping::*; use crate::cache::superblock::*; use crate::io_engine::{AsyncIoEngine, IoEngine, SyncIoEngine}; use crate::pdata::array_walker::*; +use crate::pdata::bitset_walker::*; //------------------------------------------ @@ -15,96 +17,138 @@ const MAX_CONCURRENT_IO: u32 = 1024; //------------------------------------------ -struct CheckMappingVisitor { - metadata_version: u32, - allowed_flags: u32, - nr_origin_blocks: u64, - seen_oblocks: Arc>>, - //dirty_iterator: Option, -} +mod format1 { + use super::*; -impl CheckMappingVisitor { - fn new(metadata_version: u32, nr_origin_blocks: Option, dirty_root: Option) -> CheckMappingVisitor { - let mut flags: u32 = MappingFlags::Valid as u32; - //let dirty_iterator; - if metadata_version == 1 { - flags |= MappingFlags::Dirty as u32; - //dirty_iterator = None; - } else { - let _b = dirty_root.expect("dirty bitset unavailable"); - //dirty_iterator = Some(BitsetIterator::new(b)); - } - - CheckMappingVisitor { - metadata_version, - allowed_flags: flags, - nr_origin_blocks: if let Some(n) = nr_origin_blocks {n} else {MAX_ORIGIN_BLOCKS}, - seen_oblocks: Arc::new(Mutex::new(BTreeSet::new())), - //dirty_iterator, - } + pub struct MappingChecker { + nr_origin_blocks: u64, + seen_oblocks: Mutex>, } - // TODO: move to ctor of Mapping? - fn check_flags(&self, m: &Mapping) -> anyhow::Result<()> { - if (m.flags & !self.allowed_flags) != 0 { - return Err(anyhow!("unknown flags in mapping")); - } - - if !m.is_valid() { - if self.metadata_version == 1 { - if m.is_dirty() { - return Err(anyhow!("dirty bit found on an unmapped block")); - } - }/*else if dirty_iterator.expect("dirty bitset unavailable").next() { - return Err(anyhow!("dirty bit found on an unmapped block")); - }*/ - } - - Ok(()) - } - - fn check_oblock(&self, m: &Mapping) -> anyhow::Result<()> { - if !m.is_valid() { - if m.oblock > 0 { - return Err(anyhow!("invalid mapped block")); + impl MappingChecker { + pub fn new(nr_origin_blocks: Option) -> MappingChecker { + MappingChecker { + nr_origin_blocks: if let Some(n) = nr_origin_blocks {n} else {MAX_ORIGIN_BLOCKS}, + seen_oblocks: Mutex::new(BTreeSet::new()), } - return Ok(()); } - if m.oblock >= self.nr_origin_blocks { - return Err(anyhow!("mapping beyond end of the origin device")); + fn check_flags(&self, m: &Mapping) -> anyhow::Result<()> { + if (m.flags & !(MappingFlags::Valid as u32 | MappingFlags::Dirty as u32)) != 0 { + return Err(anyhow!("unknown flags in mapping: {}", m.flags)); + } + if !m.is_valid() && m.is_dirty() { + return Err(anyhow!("dirty bit found on an unmapped block")); + } + Ok(()) } - let mut seen_oblocks = self.seen_oblocks.lock().unwrap(); - if seen_oblocks.contains(&m.oblock) { - return Err(anyhow!("origin block already mapped")); - } - seen_oblocks.insert(m.oblock); + fn check_oblock(&self, m: &Mapping) -> anyhow::Result<()> { + if !m.is_valid() { + if m.oblock > 0 { + return Err(anyhow!("invalid mapped block")); + } + return Ok(()); + } + if m.oblock >= self.nr_origin_blocks { + return Err(anyhow!("mapping beyond end of the origin device")); + } - Ok(()) + let mut seen_oblocks = self.seen_oblocks.lock().unwrap(); + if seen_oblocks.contains(&m.oblock) { + return Err(anyhow!("origin block already mapped")); + } + seen_oblocks.insert(m.oblock); + + Ok(()) + } + } + + impl ArrayVisitor for MappingChecker { + fn visit(&self, _index: u64, m: Mapping) -> anyhow::Result<()> { + self.check_flags(&m)?; + self.check_oblock(&m)?; + + Ok(()) + } } } -impl ArrayVisitor for CheckMappingVisitor { - fn visit(&self, _index: u64, m: Mapping) -> anyhow::Result<()> { - self.check_flags(&m)?; - self.check_oblock(&m)?; +mod format2 { + use super::*; - Ok(()) + pub struct MappingChecker { + nr_origin_blocks: u64, + inner: Mutex, + } + + struct Inner { + seen_oblocks: BTreeSet, + dirty_bits: FixedBitSet, + } + + impl MappingChecker { + pub fn new(nr_origin_blocks: Option, dirty_bits: FixedBitSet) -> MappingChecker { + MappingChecker { + nr_origin_blocks: if let Some(n) = nr_origin_blocks {n} else {MAX_ORIGIN_BLOCKS}, + inner: Mutex::new(Inner { + seen_oblocks: BTreeSet::new(), + dirty_bits, + }), + } + } + + fn check_flags(&self, m: &Mapping, dirty_bit: bool) -> anyhow::Result<()> { + if (m.flags & !(MappingFlags::Valid as u32)) != 0 { + return Err(anyhow!("unknown flags in mapping: {}", m.flags)); + } + if !m.is_valid() && dirty_bit { + return Err(anyhow!("dirty bit found on an unmapped block")); + } + Ok(()) + } + + fn check_oblock(&self, m: &Mapping, seen_oblocks: &mut BTreeSet) -> anyhow::Result<()> { + if !m.is_valid() { + if m.oblock > 0 { + return Err(anyhow!("invalid mapped block")); + } + return Ok(()); + } + if m.oblock >= self.nr_origin_blocks { + return Err(anyhow!("mapping beyond end of the origin device")); + } + if seen_oblocks.contains(&m.oblock) { + return Err(anyhow!("origin block already mapped")); + } + seen_oblocks.insert(m.oblock); + + Ok(()) + } + } + + impl ArrayVisitor for MappingChecker { + fn visit(&self, index: u64, m: Mapping) -> anyhow::Result<()> { + let mut inner = self.inner.lock().unwrap(); + self.check_flags(&m, inner.dirty_bits.contains(index as usize))?; + self.check_oblock(&m, &mut inner.seen_oblocks)?; + + Ok(()) + } } } //------------------------------------------ -struct CheckHintVisitor; +struct HintChecker; -impl CheckHintVisitor { - fn new() -> CheckHintVisitor { - CheckHintVisitor +impl HintChecker { + fn new() -> HintChecker { + HintChecker } } -impl ArrayVisitor for CheckHintVisitor { +impl ArrayVisitor for HintChecker { fn visit(&self, _index: u64, _hint: Hint) -> anyhow::Result<()> { // TODO: check hints Ok(()) @@ -171,11 +215,22 @@ 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 mut c = CheckMappingVisitor::new(sb.version, nr_origin_blocks, sb.dirty_root); - w.walk(&mut c, sb.mapping_root)?; - - if sb.version >= 2 { - // TODO: check dirty bitset + match sb.version { + 1 => { + let mut c = format1::MappingChecker::new(nr_origin_blocks); + w.walk(&mut c, sb.mapping_root)?; + } + 2 => { + // FIXME: possibly size truncation on 32-bit targets + let mut dirty_bits = FixedBitSet::with_capacity(sb.cache_blocks as usize); + // TODO: allow ignore_none_fatal + read_bitset(engine.clone(), sb.dirty_root.unwrap(), false, &mut dirty_bits)?; + let mut c = format2::MappingChecker::new(nr_origin_blocks, dirty_bits); + w.walk(&mut c, sb.mapping_root)?; + } + v => { + return Err(anyhow!("unsupported metadata version {}", v)); + } } } @@ -184,12 +239,15 @@ pub fn check(opts: CacheCheckOptions) -> anyhow::Result<()> { return Err(anyhow!("cache_check only supports policy hint size of 4")); } let w = ArrayWalker::new(engine.clone(), false); - let mut c = CheckHintVisitor::new(); + let mut c = HintChecker::new(); w.walk(&mut c, sb.hint_root)?; } - if !opts.skip_discards { - // TODO: check discard bitset + // The discard bitset might not be available if the cache has never been suspended, + // e.g., a crash of freshly created cache. + if !opts.skip_discards && sb.discard_root != 0 { + let mut discard_bits = FixedBitSet::with_capacity(sb.discard_nr_blocks as usize); + read_bitset(engine.clone(), sb.discard_root, false, &mut discard_bits)?; } Ok(()) diff --git a/src/pdata/bitset_walker.rs b/src/pdata/bitset_walker.rs new file mode 100644 index 0000000..89b118d --- /dev/null +++ b/src/pdata/bitset_walker.rs @@ -0,0 +1,51 @@ +use anyhow::{anyhow, Result}; +use fixedbitset::FixedBitSet; +use std::sync::{Arc, Mutex}; + +use crate::io_engine::IoEngine; +use crate::pdata::array_walker::{ArrayVisitor, ArrayWalker}; +use crate::pdata::btree::{self}; + +struct BitsetVisitor<'a> { + nr_entries: u64, + bits: Mutex<&'a mut FixedBitSet>, +} + +impl<'a> BitsetVisitor<'a> { + pub fn new(bitset: &'a mut FixedBitSet) -> Self { + BitsetVisitor { + nr_entries: bitset.len() as u64, + bits: Mutex::new(bitset), + } + } +} + +impl<'a> ArrayVisitor for BitsetVisitor<'a> { + fn visit(&self, index: u64, bits: u64) -> Result<()> { + let begin: u64 = index << 6; + if begin > self.nr_entries { + return Err(anyhow!("bitset size exceeds expectation")); + } + + let end: u64 = std::cmp::min(begin + 64, self.nr_entries); + let mut mask = 1; + for i in begin..end { + self.bits.lock().unwrap().set(i as usize, bits & mask != 0); + mask <<= 1; + } + Ok(()) + } +} + +// TODO: remap errors +// TODO: multi-threaded is possible +pub fn read_bitset( + engine: Arc, + root: u64, + ignore_none_fatal: bool, + bitset: &mut FixedBitSet, +)-> btree::Result<()> { + let w = ArrayWalker::new(engine.clone(), ignore_none_fatal); + let mut v = BitsetVisitor::new(bitset); + w.walk(&mut v, root) +} diff --git a/src/pdata/mod.rs b/src/pdata/mod.rs index 93f8811..9d13c33 100644 --- a/src/pdata/mod.rs +++ b/src/pdata/mod.rs @@ -1,5 +1,6 @@ pub mod array_block; pub mod array_walker; +pub mod bitset_walker; pub mod btree; pub mod btree_builder; pub mod btree_merge; From 9b4a0607ea18f9b313558fa18cb60c37f0e919ec Mon Sep 17 00:00:00 2001 From: Ming-Hung Tsai Date: Tue, 13 Apr 2021 13:39:14 +0800 Subject: [PATCH 12/24] [cache_check (rust)] Detect structural errors on arrays - Define error types for arrays - Propagate low-level errors to tools - Rename array_block.rs to array.rs --- src/cache/check.rs | 35 +++++----- src/cache/dump.rs | 21 +++--- src/checksum.rs | 4 ++ src/pack/node_encode.rs | 4 ++ src/pack/toplevel.rs | 1 + src/pdata/array.rs | 133 +++++++++++++++++++++++++++++++++++++ src/pdata/array_block.rs | 60 ----------------- src/pdata/array_walker.rs | 92 +++++++++++++++++++------ src/pdata/bitset_walker.rs | 9 ++- src/pdata/mod.rs | 2 +- 10 files changed, 249 insertions(+), 112 deletions(-) create mode 100644 src/pdata/array.rs delete mode 100644 src/pdata/array_block.rs diff --git a/src/cache/check.rs b/src/cache/check.rs index 261840b..01882e2 100644 --- a/src/cache/check.rs +++ b/src/cache/check.rs @@ -8,6 +8,7 @@ use crate::cache::hint::*; use crate::cache::mapping::*; use crate::cache::superblock::*; use crate::io_engine::{AsyncIoEngine, IoEngine, SyncIoEngine}; +use crate::pdata::array; use crate::pdata::array_walker::*; use crate::pdata::bitset_walker::*; @@ -33,30 +34,30 @@ mod format1 { } } - fn check_flags(&self, m: &Mapping) -> anyhow::Result<()> { + fn check_flags(&self, m: &Mapping) -> array::Result<()> { if (m.flags & !(MappingFlags::Valid as u32 | MappingFlags::Dirty as u32)) != 0 { - return Err(anyhow!("unknown flags in mapping: {}", m.flags)); + return Err(array::value_err(format!("unknown flags in mapping: {}", m.flags))); } if !m.is_valid() && m.is_dirty() { - return Err(anyhow!("dirty bit found on an unmapped block")); + return Err(array::value_err(format!("dirty bit found on an unmapped block"))); } Ok(()) } - fn check_oblock(&self, m: &Mapping) -> anyhow::Result<()> { + fn check_oblock(&self, m: &Mapping) -> array::Result<()> { if !m.is_valid() { if m.oblock > 0 { - return Err(anyhow!("invalid mapped block")); + return Err(array::value_err(format!("invalid block is mapped"))); } return Ok(()); } if m.oblock >= self.nr_origin_blocks { - return Err(anyhow!("mapping beyond end of the origin device")); + return Err(array::value_err(format!("mapping beyond end of the origin device"))); } let mut seen_oblocks = self.seen_oblocks.lock().unwrap(); if seen_oblocks.contains(&m.oblock) { - return Err(anyhow!("origin block already mapped")); + return Err(array::value_err(format!("origin block already mapped"))); } seen_oblocks.insert(m.oblock); @@ -65,7 +66,7 @@ mod format1 { } impl ArrayVisitor for MappingChecker { - fn visit(&self, _index: u64, m: Mapping) -> anyhow::Result<()> { + fn visit(&self, _index: u64, m: Mapping) -> array::Result<()> { self.check_flags(&m)?; self.check_oblock(&m)?; @@ -98,28 +99,28 @@ mod format2 { } } - fn check_flags(&self, m: &Mapping, dirty_bit: bool) -> anyhow::Result<()> { + fn check_flags(&self, m: &Mapping, dirty_bit: bool) -> array::Result<()> { if (m.flags & !(MappingFlags::Valid as u32)) != 0 { - return Err(anyhow!("unknown flags in mapping: {}", m.flags)); + return Err(array::value_err(format!("unknown flags in mapping: {}", m.flags))); } if !m.is_valid() && dirty_bit { - return Err(anyhow!("dirty bit found on an unmapped block")); + return Err(array::value_err(format!("dirty bit found on an unmapped block"))); } Ok(()) } - fn check_oblock(&self, m: &Mapping, seen_oblocks: &mut BTreeSet) -> anyhow::Result<()> { + fn check_oblock(&self, m: &Mapping, seen_oblocks: &mut BTreeSet) -> array::Result<()> { if !m.is_valid() { if m.oblock > 0 { - return Err(anyhow!("invalid mapped block")); + return Err(array::value_err(format!("invalid mapped block"))); } return Ok(()); } if m.oblock >= self.nr_origin_blocks { - return Err(anyhow!("mapping beyond end of the origin device")); + return Err(array::value_err(format!("mapping beyond end of the origin device"))); } if seen_oblocks.contains(&m.oblock) { - return Err(anyhow!("origin block already mapped")); + return Err(array::value_err(format!("origin block already mapped"))); } seen_oblocks.insert(m.oblock); @@ -128,7 +129,7 @@ mod format2 { } impl ArrayVisitor for MappingChecker { - fn visit(&self, index: u64, m: Mapping) -> anyhow::Result<()> { + fn visit(&self, index: u64, m: Mapping) -> array::Result<()> { let mut inner = self.inner.lock().unwrap(); self.check_flags(&m, inner.dirty_bits.contains(index as usize))?; self.check_oblock(&m, &mut inner.seen_oblocks)?; @@ -149,7 +150,7 @@ impl HintChecker { } impl ArrayVisitor for HintChecker { - fn visit(&self, _index: u64, _hint: Hint) -> anyhow::Result<()> { + fn visit(&self, _index: u64, _hint: Hint) -> array::Result<()> { // TODO: check hints Ok(()) } diff --git a/src/cache/dump.rs b/src/cache/dump.rs index eacda2a..9a21eed 100644 --- a/src/cache/dump.rs +++ b/src/cache/dump.rs @@ -1,4 +1,4 @@ -use anyhow::{anyhow, Result}; +use anyhow::anyhow; use fixedbitset::FixedBitSet; use std::path::Path; use std::sync::{Arc, Mutex}; @@ -8,6 +8,7 @@ use crate::cache::mapping::Mapping; use crate::cache::superblock::*; use crate::cache::xml::{self, MetadataVisitor}; use crate::io_engine::{AsyncIoEngine, IoEngine, SyncIoEngine}; +use crate::pdata::array; use crate::pdata::array_walker::*; //------------------------------------------ @@ -45,7 +46,7 @@ mod format1 { } impl<'a> ArrayVisitor for MappingEmitter<'a> { - fn visit(&self, index: u64, m: Mapping) -> Result<()> { + fn visit(&self, index: u64, m: Mapping) -> array::Result<()> { if m.is_valid() { let m = xml::Map { cblock: index as u32, @@ -55,7 +56,7 @@ mod format1 { let mut inner = self.inner.lock().unwrap(); inner.valid_mappings.set(index as usize, true); - inner.visitor.mapping(&m)?; + inner.visitor.mapping(&m).map_err(|e| array::value_err(format!("{}", e)))?; } Ok(()) @@ -89,7 +90,7 @@ mod format2 { } impl ArrayVisitor for DirtyVisitor { - fn visit(&self, index: u64, bits: u64) -> Result<()> { + fn visit(&self, index: u64, bits: u64) -> array::Result<()> { for i in 0..64u64 { if (index + i) >= self.nr_entries as u64 { break; @@ -132,7 +133,7 @@ mod format2 { } impl<'a> ArrayVisitor for MappingEmitter<'a> { - fn visit(&self, index: u64, m: Mapping) -> Result<()> { + fn visit(&self, index: u64, m: Mapping) -> array::Result<()> { if m.is_valid() { let mut inner = self.inner.lock().unwrap(); let dirty = inner.dirty_bits.contains(index as usize); @@ -143,7 +144,7 @@ mod format2 { }; inner.valid_mappings.set(index as usize, true); - inner.visitor.mapping(&m)?; + inner.visitor.mapping(&m).map_err(|e| array::value_err(format!("{}", e)))?; } Ok(()) @@ -168,14 +169,18 @@ impl<'a> HintEmitter<'a> { } impl<'a> ArrayVisitor for HintEmitter<'a> { - fn visit(&self, index: u64, hint: Hint) -> anyhow::Result<()> { + fn visit(&self, index: u64, hint: Hint) -> array::Result<()> { if self.valid_mappings.contains(index as usize) { let h = xml::Hint { cblock: index as u32, data: hint.hint.to_vec(), }; - self.emitter.lock().unwrap().hint(&h)?; + self.emitter + .lock() + .unwrap() + .hint(&h) + .map_err(|e| array::value_err(format!("{}", e)))?; } Ok(()) diff --git a/src/checksum.rs b/src/checksum.rs index 0ffbd47..9ffbd70 100644 --- a/src/checksum.rs +++ b/src/checksum.rs @@ -11,6 +11,7 @@ const SUPERBLOCK_CSUM_XOR: u32 = 160774; const BITMAP_CSUM_XOR: u32 = 240779; const INDEX_CSUM_XOR: u32 = 160478; const BTREE_CSUM_XOR: u32 = 121107; +const ARRAY_CSUM_XOR: u32 = 595846735; fn checksum(buf: &[u8]) -> u32 { crc32c(&buf[4..]) ^ 0xffffffff @@ -22,6 +23,7 @@ pub enum BT { NODE, INDEX, BITMAP, + ARRAY, UNKNOWN, } @@ -41,6 +43,7 @@ pub fn metadata_block_type(buf: &[u8]) -> BT { BTREE_CSUM_XOR => BT::NODE, BITMAP_CSUM_XOR => BT::BITMAP, INDEX_CSUM_XOR => BT::INDEX, + ARRAY_CSUM_XOR => BT::ARRAY, _ => BT::UNKNOWN, } } @@ -56,6 +59,7 @@ pub fn write_checksum(buf: &mut [u8], kind: BT) -> Result<()> { NODE => BTREE_CSUM_XOR, BITMAP => BITMAP_CSUM_XOR, INDEX => INDEX_CSUM_XOR, + ARRAY => ARRAY_CSUM_XOR, UNKNOWN => {return Err(anyhow!("Invalid block type"));} }; diff --git a/src/pack/node_encode.rs b/src/pack/node_encode.rs index 466fd40..d146ca3 100644 --- a/src/pack/node_encode.rs +++ b/src/pack/node_encode.rs @@ -114,4 +114,8 @@ pub fn pack_index(w: &mut W, bytes: &[u8]) -> PResult<()> { io_to_pr(pack_literal(w, bytes)) } +pub fn pack_array(w: &mut W, bytes: &[u8]) -> PResult<()> { + io_to_pr(pack_literal(w, bytes)) +} + //------------------------------------- diff --git a/src/pack/toplevel.rs b/src/pack/toplevel.rs index 449de91..98b4dcf 100644 --- a/src/pack/toplevel.rs +++ b/src/pack/toplevel.rs @@ -209,6 +209,7 @@ fn pack_block(w: &mut W, kind: BT, buf: &[u8]) -> Result<()> { BT::NODE => pack_btree_node(w, buf).context("unable to pack btree node")?, BT::INDEX => pack_index(w, buf).context("unable to pack space map index")?, BT::BITMAP => pack_bitmap(w, buf).context("unable to pack space map bitmap")?, + BT::ARRAY => pack_array(w, buf).context("unable to pack array block")?, BT::UNKNOWN => return Err(anyhow!("asked to pack an unknown block type")), } diff --git a/src/pdata/array.rs b/src/pdata/array.rs new file mode 100644 index 0000000..68a04b3 --- /dev/null +++ b/src/pdata/array.rs @@ -0,0 +1,133 @@ +use nom::{multi::count, number::complete::*, IResult}; +use thiserror::Error; + +use crate::io_engine::BLOCK_SIZE; +use crate::pdata::btree; +use crate::pdata::unpack::Unpack; + +//------------------------------------------ + +const ARRAY_BLOCK_HEADER_SIZE: u32 = 24; + +pub struct ArrayBlockHeader { + pub csum: u32, + pub max_entries: u32, + pub nr_entries: u32, + pub value_size: u32, + pub blocknr: u64, +} + +impl Unpack for ArrayBlockHeader { + fn disk_size() -> u32 { + ARRAY_BLOCK_HEADER_SIZE + } + + fn unpack(data: &[u8]) -> IResult<&[u8], ArrayBlockHeader> { + let (i, csum) = le_u32(data)?; + let (i, max_entries) = le_u32(i)?; + let (i, nr_entries) = le_u32(i)?; + let (i, value_size) = le_u32(i)?; + let (i, blocknr) = le_u64(i)?; + + Ok(( + i, + ArrayBlockHeader { + csum, + max_entries, + nr_entries, + value_size, + blocknr, + }, + )) + } +} + +pub struct ArrayBlock { + pub header: ArrayBlockHeader, + pub values: Vec, +} + +//------------------------------------------ + +#[derive(Error, Clone, Debug)] +pub enum ArrayError { + #[error("io_error")] + IoError, + + #[error("block error: {0}")] + BlockError(String), + + #[error("value error: {0}")] + ValueError(String), + + #[error("aggregate: {0:?}")] + Aggregate(Vec), + + #[error("{0:?}, {1}")] + Path(Vec, Box), + + #[error(transparent)] + BTreeError(#[from] btree::BTreeError), +} + +pub fn array_block_err(path: &[u64], msg: &str) -> ArrayError { + ArrayError::Path( + path.to_vec(), + Box::new(ArrayError::BlockError(msg.to_string())), + ) +} + +pub fn value_err(msg: String) -> ArrayError { + ArrayError::ValueError(msg) +} + +pub fn aggregate_error(errs: Vec) -> ArrayError { + ArrayError::Aggregate(errs) +} + +pub type Result = std::result::Result; + +//------------------------------------------ + +fn convert_result<'a, V>(path: &[u64], r: IResult<&'a [u8], V>) -> Result<(&'a [u8], V)> { + r.map_err(|_| array_block_err(path, "parse error")) +} + +pub fn unpack_array_block(path: &[u64], data: &[u8]) -> Result> { + // TODO: collect errors + let (i, header) = + ArrayBlockHeader::unpack(data).map_err(|_| array_block_err( + path, + "Couldn't parse array block header" + ))?; + + // check value_size + if header.value_size != V::disk_size() { + return Err(array_block_err( + path, + &format!( + "value_size mismatch: expected {}, was {}", + V::disk_size(), + header.value_size + ) + )); + } + + // check max_entries + if header.value_size * header.max_entries + ARRAY_BLOCK_HEADER_SIZE > BLOCK_SIZE as u32 { + return Err(array_block_err( + path, + &format!("max_entries is too large ({})", header.max_entries) + )); + } + + // TODO: check nr_entries < max_entries + + // TODO: check block_nr + + let (_i, values) = convert_result(path, count(V::unpack, header.nr_entries as usize)(i))?; + + Ok(ArrayBlock { header, values }) +} + +//------------------------------------------ diff --git a/src/pdata/array_block.rs b/src/pdata/array_block.rs deleted file mode 100644 index b082d92..0000000 --- a/src/pdata/array_block.rs +++ /dev/null @@ -1,60 +0,0 @@ -use anyhow::anyhow; -use anyhow::Result; -use nom::{multi::count, number::complete::*, IResult}; - -use crate::pdata::unpack::Unpack; - -//------------------------------------------ - -pub struct ArrayBlockHeader { - pub csum: u32, - pub max_entries: u32, - pub nr_entries: u32, - pub value_size: u32, - pub blocknr: u64, -} - -impl Unpack for ArrayBlockHeader { - fn disk_size() -> u32 { - 24 - } - - fn unpack(data: &[u8]) -> IResult<&[u8], ArrayBlockHeader> { - let (i, csum) = le_u32(data)?; - let (i, max_entries) = le_u32(i)?; - let (i, nr_entries) = le_u32(i)?; - let (i, value_size) = le_u32(i)?; - let (i, blocknr) = le_u64(i)?; - - Ok(( - i, - ArrayBlockHeader { - csum, - max_entries, - nr_entries, - value_size, - blocknr, - }, - )) - } -} - -pub struct ArrayBlock { - pub header: ArrayBlockHeader, - pub values: Vec, -} - -fn convert_result(r: IResult<&[u8], V>) -> Result<(&[u8], V)> { - r.map_err(|_| anyhow!("parse error")) -} - -pub fn unpack_array_block(data: &[u8]) -> Result> { - // TODO: collect errors - let (i, header) = - ArrayBlockHeader::unpack(data).map_err(|_e| anyhow!("Couldn't parse header"))?; - let (_i, values) = convert_result(count(V::unpack, header.nr_entries as usize)(i))?; - - Ok(ArrayBlock { header, values }) -} - -//------------------------------------------ diff --git a/src/pdata/array_walker.rs b/src/pdata/array_walker.rs index 672259e..e418402 100644 --- a/src/pdata/array_walker.rs +++ b/src/pdata/array_walker.rs @@ -1,8 +1,9 @@ use std::sync::Arc; +use crate::checksum; use crate::io_engine::*; -use crate::pdata::array_block::*; -use crate::pdata::btree::*; +use crate::pdata::array::{self, *}; +use crate::pdata::btree::{self, *}; use crate::pdata::btree_walker::*; use crate::pdata::unpack::*; @@ -15,12 +16,12 @@ pub struct ArrayWalker { // FIXME: define another Result type for array visiting? pub trait ArrayVisitor { - fn visit(&self, index: u64, v: V) -> anyhow::Result<()>; + fn visit(&self, index: u64, v: V) -> array::Result<()>; } struct BlockValueVisitor<'a, V> { engine: Arc, - array_block_visitor: &'a mut dyn ArrayVisitor, + array_visitor: &'a mut dyn ArrayVisitor, } impl<'a, V: Unpack + Copy> BlockValueVisitor<'a, V> { @@ -30,22 +31,37 @@ impl<'a, V: Unpack + Copy> BlockValueVisitor<'a, V> { ) -> BlockValueVisitor<'a, V> { BlockValueVisitor { engine: e, - array_block_visitor: v, + array_visitor: v, } } - pub fn visit_array_block(&self, index: u64, array_block: ArrayBlock) { + pub fn visit_array_block(&self, index: u64, array_block: ArrayBlock) -> array::Result<()>{ + let mut errs: Vec = Vec::new(); + let begin = index * array_block.header.max_entries as u64; for i in 0..array_block.header.nr_entries { - self.array_block_visitor - .visit(begin + i as u64, array_block.values[i as usize]) - .unwrap(); + if let Err(e) = self.array_visitor.visit(begin + i as u64, array_block.values[i as usize]) { + errs.push(e); // TODO: add path or keys context? + } + } + + // FIXME: duplicate to BTreeWalker::build_aggregrate() + match errs.len() { + 0 => Ok(()), + 1 => { + let e = errs[0].clone(); + Err(e) + } + _ => { + let e = array::aggregate_error(errs); + Err(e) + } } } } impl<'a, V: Unpack + Copy> NodeVisitor for BlockValueVisitor<'a, V> { - // FIXME: return errors + // FIXME: wrap ArrayError into BTreeError, rather than mapping to value_err? fn visit( &self, path: &[u64], @@ -53,20 +69,54 @@ impl<'a, V: Unpack + Copy> NodeVisitor for BlockValueVisitor<'a, V> { _h: &NodeHeader, keys: &[u64], values: &[u64], - ) -> Result<()> { + ) -> btree::Result<()> { + let mut path = path.to_vec(); + let mut errs: Vec = Vec::new(); + for (n, index) in keys.iter().enumerate() { - let b = self.engine.read(values[n]).map_err(|_| io_err(path))?; - let array_block = unpack_array_block::(b.get_data()).map_err(|_| io_err(path))?; - self.visit_array_block(*index, array_block); + let b = self.engine.read(values[n]).map_err(|_| io_err(&path))?; + + // FIXME: move to unpack_array_block? + let bt = checksum::metadata_block_type(b.get_data()); + if bt != checksum::BT::ARRAY { + errs.push(btree::value_err( + format!("checksum failed for array block {}, {:?}", b.loc, bt) + )); + } + + path.push(values[n]); + match unpack_array_block::(&path, b.get_data()) { + Ok(array_block) => { + if let Err(e) = self.visit_array_block(*index, array_block) { + errs.push(btree::value_err(format!("{}", e))); + } + }, + Err(e) => { + errs.push(btree::value_err(format!("{}", e))); + } + } + path.pop(); } + + // 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) + } + } + } + + fn visit_again(&self, _path: &[u64], _b: u64) -> btree::Result<()> { Ok(()) } - fn visit_again(&self, _path: &[u64], _b: u64) -> Result<()> { - Ok(()) - } - - fn end_walk(&self) -> Result<()> { + fn end_walk(&self) -> btree::Result<()> { Ok(()) } } @@ -81,7 +131,7 @@ impl ArrayWalker { } // FIXME: redefine the Result type for array visiting? - pub fn walk(&self, visitor: &mut dyn ArrayVisitor, root: u64) -> Result<()> + pub fn walk(&self, visitor: &mut dyn ArrayVisitor, root: u64) -> array::Result<()> where V: Unpack + Copy, { @@ -89,7 +139,7 @@ impl ArrayWalker { let mut path = Vec::new(); path.push(0); let v = BlockValueVisitor::::new(self.engine.clone(), visitor); - w.walk(&mut path, &v, root) + w.walk(&mut path, &v, root).map_err(|e| ArrayError::BTreeError(e)) } } diff --git a/src/pdata/bitset_walker.rs b/src/pdata/bitset_walker.rs index 89b118d..a75bae9 100644 --- a/src/pdata/bitset_walker.rs +++ b/src/pdata/bitset_walker.rs @@ -1,10 +1,9 @@ -use anyhow::{anyhow, Result}; use fixedbitset::FixedBitSet; use std::sync::{Arc, Mutex}; use crate::io_engine::IoEngine; +use crate::pdata::array; use crate::pdata::array_walker::{ArrayVisitor, ArrayWalker}; -use crate::pdata::btree::{self}; struct BitsetVisitor<'a> { nr_entries: u64, @@ -21,10 +20,10 @@ impl<'a> BitsetVisitor<'a> { } impl<'a> ArrayVisitor for BitsetVisitor<'a> { - fn visit(&self, index: u64, bits: u64) -> Result<()> { + fn visit(&self, index: u64, bits: u64) -> array::Result<()> { let begin: u64 = index << 6; if begin > self.nr_entries { - return Err(anyhow!("bitset size exceeds expectation")); + return Err(array::value_err("bitset size exceeds expectation".to_string())); } let end: u64 = std::cmp::min(begin + 64, self.nr_entries); @@ -44,7 +43,7 @@ pub fn read_bitset( root: u64, ignore_none_fatal: bool, bitset: &mut FixedBitSet, -)-> btree::Result<()> { +)-> array::Result<()> { let w = ArrayWalker::new(engine.clone(), ignore_none_fatal); let mut v = BitsetVisitor::new(bitset); w.walk(&mut v, root) diff --git a/src/pdata/mod.rs b/src/pdata/mod.rs index 9d13c33..ef04f6b 100644 --- a/src/pdata/mod.rs +++ b/src/pdata/mod.rs @@ -1,4 +1,4 @@ -pub mod array_block; +pub mod array; pub mod array_walker; pub mod bitset_walker; pub mod btree; From 95dee9f66d69c56785954c221064dbd91013002b Mon Sep 17 00:00:00 2001 From: Ming-Hung Tsai Date: Thu, 15 Apr 2021 19:39:31 +0800 Subject: [PATCH 13/24] [cache_check (rust)] Do not remap ArrayErrors to keep the error context --- src/pdata/array.rs | 10 +++++++- src/pdata/array_walker.rs | 54 ++++++++++++++++++++++++--------------- 2 files changed, 43 insertions(+), 21 deletions(-) diff --git a/src/pdata/array.rs b/src/pdata/array.rs index 68a04b3..4a977f6 100644 --- a/src/pdata/array.rs +++ b/src/pdata/array.rs @@ -1,6 +1,7 @@ use nom::{multi::count, number::complete::*, IResult}; use thiserror::Error; +use crate::checksum; use crate::io_engine::BLOCK_SIZE; use crate::pdata::btree; use crate::pdata::unpack::Unpack; @@ -94,7 +95,14 @@ fn convert_result<'a, V>(path: &[u64], r: IResult<&'a [u8], V>) -> Result<(&'a [ } pub fn unpack_array_block(path: &[u64], data: &[u8]) -> Result> { - // TODO: collect errors + let bt = checksum::metadata_block_type(data); + if bt != checksum::BT::ARRAY { + return Err(array_block_err( + path, + &format!("checksum failed for array block {}, {:?}", path.last().unwrap(), bt) + )); + } + let (i, header) = ArrayBlockHeader::unpack(data).map_err(|_| array_block_err( path, diff --git a/src/pdata/array_walker.rs b/src/pdata/array_walker.rs index e418402..fe0dcda 100644 --- a/src/pdata/array_walker.rs +++ b/src/pdata/array_walker.rs @@ -1,6 +1,5 @@ -use std::sync::Arc; +use std::sync::{Arc, Mutex}; -use crate::checksum; use crate::io_engine::*; use crate::pdata::array::{self, *}; use crate::pdata::btree::{self, *}; @@ -14,14 +13,17 @@ pub struct ArrayWalker { ignore_non_fatal: bool, } -// FIXME: define another Result type for array visiting? pub trait ArrayVisitor { fn visit(&self, index: u64, v: V) -> array::Result<()>; } +//------------------------------------------ + +// FIXME: Eliminate this structure by impl NodeVisitor for ArrayWalker? struct BlockValueVisitor<'a, V> { engine: Arc, array_visitor: &'a mut dyn ArrayVisitor, + array_errs: Mutex>, } impl<'a, V: Unpack + Copy> BlockValueVisitor<'a, V> { @@ -32,10 +34,11 @@ impl<'a, V: Unpack + Copy> BlockValueVisitor<'a, V> { BlockValueVisitor { engine: e, array_visitor: v, + array_errs: Mutex::new(Vec::new()), } } - pub fn visit_array_block(&self, index: u64, array_block: ArrayBlock) -> array::Result<()>{ + fn visit_array_block(&self, index: u64, array_block: ArrayBlock) -> array::Result<()>{ let mut errs: Vec = Vec::new(); let begin = index * array_block.header.max_entries as u64; @@ -49,12 +52,10 @@ impl<'a, V: Unpack + Copy> BlockValueVisitor<'a, V> { match errs.len() { 0 => Ok(()), 1 => { - let e = errs[0].clone(); - Err(e) + Err(errs[0].clone()) } _ => { - let e = array::aggregate_error(errs); - Err(e) + Err(array::aggregate_error(errs)) } } } @@ -73,26 +74,27 @@ impl<'a, V: Unpack + Copy> NodeVisitor for BlockValueVisitor<'a, V> { let mut path = path.to_vec(); let mut errs: Vec = Vec::new(); + // TODO: check index continuity + // FIXME: use IoEngine::read_many() for (n, index) in keys.iter().enumerate() { - let b = self.engine.read(values[n]).map_err(|_| io_err(&path))?; - - // FIXME: move to unpack_array_block? - let bt = checksum::metadata_block_type(b.get_data()); - if bt != checksum::BT::ARRAY { - errs.push(btree::value_err( - format!("checksum failed for array block {}, {:?}", b.loc, bt) - )); + // FIXME: count read errors on its parent (BTreeError::IoError) or on its location + // (ArrayError::IoError)? + let b = self.engine.read(values[n]).map_err(|_| btree::io_err(&path)); + if let Err(e) = b { + errs.push(e); + continue; } + let b = b.unwrap(); path.push(values[n]); match unpack_array_block::(&path, b.get_data()) { Ok(array_block) => { if let Err(e) = self.visit_array_block(*index, array_block) { - errs.push(btree::value_err(format!("{}", e))); + self.array_errs.lock().unwrap().push(e); } }, Err(e) => { - errs.push(btree::value_err(format!("{}", e))); + self.array_errs.lock().unwrap().push(e); } } path.pop(); @@ -121,6 +123,8 @@ impl<'a, V: Unpack + Copy> NodeVisitor for BlockValueVisitor<'a, V> { } } +//------------------------------------------ + impl ArrayWalker { pub fn new(engine: Arc, ignore_non_fatal: bool) -> ArrayWalker { let r: ArrayWalker = ArrayWalker { @@ -130,7 +134,6 @@ impl ArrayWalker { r } - // FIXME: redefine the Result type for array visiting? pub fn walk(&self, visitor: &mut dyn ArrayVisitor, root: u64) -> array::Result<()> where V: Unpack + Copy, @@ -139,7 +142,18 @@ impl ArrayWalker { let mut path = Vec::new(); path.push(0); let v = BlockValueVisitor::::new(self.engine.clone(), visitor); - w.walk(&mut path, &v, root).map_err(|e| ArrayError::BTreeError(e)) + let btree_err = w.walk(&mut path, &v, root).map_err(|e| ArrayError::BTreeError(e)); + + let mut array_errs = v.array_errs.into_inner().unwrap(); + if let Err(e) = btree_err { + array_errs.push(e); + } + + match array_errs.len() { + 0 => Ok(()), + 1 => Err(array_errs[0].clone()), + _ => Err(ArrayError::Aggregate(array_errs)), + } } } From 1964015d81d69bec0239854923e14bd2362f0a1d Mon Sep 17 00:00:00 2001 From: Ming-Hung Tsai Date: Fri, 16 Apr 2021 17:52:54 +0800 Subject: [PATCH 14/24] [array_walker] Handle the whole array block at once That gives the visitor more controls over the data processing and locking, and also improves the performance by 10-15%. --- src/cache/check.rs | 61 +++++++++++++++++++++++++++++-------- src/cache/dump.rs | 62 ++++++++++++++++++++++++++------------ src/pdata/array_walker.rs | 26 ++-------------- src/pdata/bitset_walker.rs | 29 ++++++++++-------- 4 files changed, 111 insertions(+), 67 deletions(-) diff --git a/src/cache/check.rs b/src/cache/check.rs index 01882e2..ac05336 100644 --- a/src/cache/check.rs +++ b/src/cache/check.rs @@ -8,7 +8,7 @@ use crate::cache::hint::*; use crate::cache::mapping::*; use crate::cache::superblock::*; use crate::io_engine::{AsyncIoEngine, IoEngine, SyncIoEngine}; -use crate::pdata::array; +use crate::pdata::array::{self, ArrayBlock, ArrayError}; use crate::pdata::array_walker::*; use crate::pdata::bitset_walker::*; @@ -54,23 +54,40 @@ mod format1 { if m.oblock >= self.nr_origin_blocks { return Err(array::value_err(format!("mapping beyond end of the origin device"))); } - let mut seen_oblocks = self.seen_oblocks.lock().unwrap(); if seen_oblocks.contains(&m.oblock) { return Err(array::value_err(format!("origin block already mapped"))); } seen_oblocks.insert(m.oblock); - Ok(()) } } impl ArrayVisitor for MappingChecker { - fn visit(&self, _index: u64, m: Mapping) -> array::Result<()> { - self.check_flags(&m)?; - self.check_oblock(&m)?; + fn visit(&self, _index: u64, b: ArrayBlock) -> array::Result<()> { + let mut errs: Vec = Vec::new(); - Ok(()) + for i in 0..b.header.nr_entries as usize { + let m = b.values[i]; + + if let Err(e) = self.check_flags(&m) { + errs.push(e); + } + if let Err(e) = self.check_oblock(&m) { + errs.push(e); + } + } + + // FIXME: duplicate to BTreeWalker::build_aggregrate() + match errs.len() { + 0 => Ok(()), + 1 => { + Err(errs[0].clone()) + } + _ => { + Err(array::aggregate_error(errs)) + } + } } } } @@ -129,12 +146,32 @@ mod format2 { } impl ArrayVisitor for MappingChecker { - fn visit(&self, index: u64, m: Mapping) -> array::Result<()> { + fn visit(&self, index: u64, b: ArrayBlock) -> array::Result<()> { let mut inner = self.inner.lock().unwrap(); - self.check_flags(&m, inner.dirty_bits.contains(index as usize))?; - self.check_oblock(&m, &mut inner.seen_oblocks)?; + let mut errs: Vec = Vec::new(); - Ok(()) + let begin = index as usize * b.header.max_entries as usize; + for i in 0..b.header.nr_entries { + let m = b.values[i as usize]; + + if let Err(e) = self.check_flags(&m, inner.dirty_bits.contains(begin + i as usize)) { + errs.push(e); + } + if let Err(e) = self.check_oblock(&m, &mut inner.seen_oblocks) { + errs.push(e); + } + } + + // FIXME: duplicate to BTreeWalker::build_aggregrate() + match errs.len() { + 0 => Ok(()), + 1 => { + Err(errs[0].clone()) + } + _ => { + Err(array::aggregate_error(errs)) + } + } } } } @@ -150,7 +187,7 @@ impl HintChecker { } impl ArrayVisitor for HintChecker { - fn visit(&self, _index: u64, _hint: Hint) -> array::Result<()> { + fn visit(&self, _index: u64, _b: ArrayBlock) -> array::Result<()> { // TODO: check hints Ok(()) } diff --git a/src/cache/dump.rs b/src/cache/dump.rs index 9a21eed..235ef8a 100644 --- a/src/cache/dump.rs +++ b/src/cache/dump.rs @@ -8,7 +8,7 @@ use crate::cache::mapping::Mapping; use crate::cache::superblock::*; use crate::cache::xml::{self, MetadataVisitor}; use crate::io_engine::{AsyncIoEngine, IoEngine, SyncIoEngine}; -use crate::pdata::array; +use crate::pdata::array::{self, ArrayBlock}; use crate::pdata::array_walker::*; //------------------------------------------ @@ -46,12 +46,17 @@ mod format1 { } impl<'a> ArrayVisitor for MappingEmitter<'a> { - fn visit(&self, index: u64, m: Mapping) -> array::Result<()> { - if m.is_valid() { + fn visit(&self, index: u64, b: ArrayBlock) -> array::Result<()> { + for i in 0..b.header.nr_entries as usize { + let map = b.values[i]; + if !map.is_valid() { + continue; + } + let m = xml::Map { cblock: index as u32, - oblock: m.oblock, - dirty: m.is_dirty(), + oblock: map.oblock, + dirty: map.is_dirty(), }; let mut inner = self.inner.lock().unwrap(); @@ -79,7 +84,7 @@ mod format2 { impl DirtyVisitor { pub fn new(nr_entries: usize) -> Self { DirtyVisitor { - nr_entries, + nr_entries, // number of bits bits: Mutex::new(FixedBitSet::with_capacity(nr_entries)), } } @@ -90,13 +95,19 @@ mod format2 { } impl ArrayVisitor for DirtyVisitor { - fn visit(&self, index: u64, bits: u64) -> array::Result<()> { - for i in 0..64u64 { - if (index + i) >= self.nr_entries as u64 { - break; - } + fn visit(&self, index: u64, b: ArrayBlock) -> array::Result<()> { + let mut pos = index as usize * (b.header.max_entries as usize) << 6; + for i in 0..b.header.nr_entries as usize { + let bits = b.values[i]; - self.bits.lock().unwrap().set((index + i) as usize, bits & (1 << i) != 0); + for bi in 0..64u64 { + if pos >= self.nr_entries { + break; + } + + self.bits.lock().unwrap().set(pos, bits & (1 << bi) != 0); + pos += 1; + } } Ok(()) } @@ -133,20 +144,25 @@ mod format2 { } impl<'a> ArrayVisitor for MappingEmitter<'a> { - fn visit(&self, index: u64, m: Mapping) -> array::Result<()> { - if m.is_valid() { + fn visit(&self, index: u64, b: ArrayBlock) -> array::Result<()> { + for i in 0..b.header.nr_entries as usize { + let map = b.values[i]; + + if !map.is_valid() { + continue; + } + let mut inner = self.inner.lock().unwrap(); let dirty = inner.dirty_bits.contains(index as usize); let m = xml::Map { cblock: index as u32, - oblock: m.oblock, + oblock: map.oblock, dirty, }; inner.valid_mappings.set(index as usize, true); inner.visitor.mapping(&m).map_err(|e| array::value_err(format!("{}", e)))?; } - Ok(()) } } @@ -169,10 +185,16 @@ impl<'a> HintEmitter<'a> { } impl<'a> ArrayVisitor for HintEmitter<'a> { - fn visit(&self, index: u64, hint: Hint) -> array::Result<()> { - if self.valid_mappings.contains(index as usize) { + fn visit(&self, index: u64, b: ArrayBlock) -> array::Result<()> { + let mut cblock = index as u32 * b.header.max_entries; + for i in 0..b.header.nr_entries as usize { + if !self.valid_mappings.contains(cblock as usize) { + continue; + } + + let hint = b.values[i]; let h = xml::Hint { - cblock: index as u32, + cblock, data: hint.hint.to_vec(), }; @@ -181,6 +203,8 @@ impl<'a> ArrayVisitor for HintEmitter<'a> { .unwrap() .hint(&h) .map_err(|e| array::value_err(format!("{}", e)))?; + + cblock += 1; } Ok(()) diff --git a/src/pdata/array_walker.rs b/src/pdata/array_walker.rs index fe0dcda..a9f033b 100644 --- a/src/pdata/array_walker.rs +++ b/src/pdata/array_walker.rs @@ -14,7 +14,7 @@ pub struct ArrayWalker { } pub trait ArrayVisitor { - fn visit(&self, index: u64, v: V) -> array::Result<()>; + fn visit(&self, index: u64, b: ArrayBlock) -> array::Result<()>; } //------------------------------------------ @@ -37,28 +37,6 @@ impl<'a, V: Unpack + Copy> BlockValueVisitor<'a, V> { array_errs: Mutex::new(Vec::new()), } } - - fn visit_array_block(&self, index: u64, array_block: ArrayBlock) -> array::Result<()>{ - let mut errs: Vec = Vec::new(); - - let begin = index * array_block.header.max_entries as u64; - for i in 0..array_block.header.nr_entries { - if let Err(e) = self.array_visitor.visit(begin + i as u64, array_block.values[i as usize]) { - errs.push(e); // TODO: add path or keys context? - } - } - - // FIXME: duplicate to BTreeWalker::build_aggregrate() - match errs.len() { - 0 => Ok(()), - 1 => { - Err(errs[0].clone()) - } - _ => { - Err(array::aggregate_error(errs)) - } - } - } } impl<'a, V: Unpack + Copy> NodeVisitor for BlockValueVisitor<'a, V> { @@ -89,7 +67,7 @@ impl<'a, V: Unpack + Copy> NodeVisitor for BlockValueVisitor<'a, V> { path.push(values[n]); match unpack_array_block::(&path, b.get_data()) { Ok(array_block) => { - if let Err(e) = self.visit_array_block(*index, array_block) { + if let Err(e) = self.array_visitor.visit(*index, array_block) { self.array_errs.lock().unwrap().push(e); } }, diff --git a/src/pdata/bitset_walker.rs b/src/pdata/bitset_walker.rs index a75bae9..7cc7b7f 100644 --- a/src/pdata/bitset_walker.rs +++ b/src/pdata/bitset_walker.rs @@ -2,7 +2,7 @@ use fixedbitset::FixedBitSet; use std::sync::{Arc, Mutex}; use crate::io_engine::IoEngine; -use crate::pdata::array; +use crate::pdata::array::{self, ArrayBlock}; use crate::pdata::array_walker::{ArrayVisitor, ArrayWalker}; struct BitsetVisitor<'a> { @@ -20,23 +20,28 @@ impl<'a> BitsetVisitor<'a> { } impl<'a> ArrayVisitor for BitsetVisitor<'a> { - fn visit(&self, index: u64, bits: u64) -> array::Result<()> { - let begin: u64 = index << 6; - if begin > self.nr_entries { - return Err(array::value_err("bitset size exceeds expectation".to_string())); - } + fn visit(&self, index: u64, b: ArrayBlock) -> array::Result<()> { + let mut begin = index as usize * (b.header.max_entries as usize) << 6; - let end: u64 = std::cmp::min(begin + 64, self.nr_entries); - let mut mask = 1; - for i in begin..end { - self.bits.lock().unwrap().set(i as usize, bits & mask != 0); - mask <<= 1; + for i in 0..b.header.nr_entries as usize { + if begin > self.nr_entries as usize { + return Err(array::value_err("bitset size exceeds expectation".to_string())); + } + + let end: usize = std::cmp::min(begin + 64, self.nr_entries as usize); + let mut mask = 1; + let bits = b.values[i]; + + for bi in begin..end { + self.bits.lock().unwrap().set(bi, bits & mask != 0); + mask <<= 1; + } + begin += 64; } Ok(()) } } -// TODO: remap errors // TODO: multi-threaded is possible pub fn read_bitset( engine: Arc, From c17559791fa7ca1f2252abf72c13fab238a6e323 Mon Sep 17 00:00:00 2001 From: Ming-Hung Tsai Date: Fri, 16 Apr 2021 21:44:45 +0800 Subject: [PATCH 15/24] [bitset] Rename bitset module --- src/cache/check.rs | 2 +- src/pdata/{bitset_walker.rs => bitset.rs} | 0 src/pdata/mod.rs | 2 +- 3 files changed, 2 insertions(+), 2 deletions(-) rename src/pdata/{bitset_walker.rs => bitset.rs} (100%) diff --git a/src/cache/check.rs b/src/cache/check.rs index ac05336..01df923 100644 --- a/src/cache/check.rs +++ b/src/cache/check.rs @@ -10,7 +10,7 @@ use crate::cache::superblock::*; use crate::io_engine::{AsyncIoEngine, IoEngine, SyncIoEngine}; use crate::pdata::array::{self, ArrayBlock, ArrayError}; use crate::pdata::array_walker::*; -use crate::pdata::bitset_walker::*; +use crate::pdata::bitset::*; //------------------------------------------ diff --git a/src/pdata/bitset_walker.rs b/src/pdata/bitset.rs similarity index 100% rename from src/pdata/bitset_walker.rs rename to src/pdata/bitset.rs diff --git a/src/pdata/mod.rs b/src/pdata/mod.rs index ef04f6b..50f571c 100644 --- a/src/pdata/mod.rs +++ b/src/pdata/mod.rs @@ -1,6 +1,6 @@ pub mod array; pub mod array_walker; -pub mod bitset_walker; +pub mod bitset; pub mod btree; pub mod btree_builder; pub mod btree_merge; From 3279d8381ba8a9034d15ac5c16a6df7dd5b466af Mon Sep 17 00:00:00 2001 From: Ming-Hung Tsai Date: Sat, 17 Apr 2021 00:05:08 +0800 Subject: [PATCH 16/24] [array_walker] Read multiple array blocks at once --- src/pdata/array_walker.rs | 52 +++++++++++++++++++++++---------------- 1 file changed, 31 insertions(+), 21 deletions(-) diff --git a/src/pdata/array_walker.rs b/src/pdata/array_walker.rs index a9f033b..cd9d5e1 100644 --- a/src/pdata/array_walker.rs +++ b/src/pdata/array_walker.rs @@ -53,29 +53,39 @@ impl<'a, V: Unpack + Copy> NodeVisitor for BlockValueVisitor<'a, V> { let mut errs: Vec = Vec::new(); // TODO: check index continuity - // FIXME: use IoEngine::read_many() - for (n, index) in keys.iter().enumerate() { - // FIXME: count read errors on its parent (BTreeError::IoError) or on its location - // (ArrayError::IoError)? - let b = self.engine.read(values[n]).map_err(|_| btree::io_err(&path)); - if let Err(e) = b { - errs.push(e); - continue; - } - let b = b.unwrap(); - - path.push(values[n]); - match unpack_array_block::(&path, b.get_data()) { - Ok(array_block) => { - if let Err(e) = self.array_visitor.visit(*index, array_block) { - self.array_errs.lock().unwrap().push(e); - } - }, - Err(e) => { - self.array_errs.lock().unwrap().push(e); + 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 + } + } + Ok(rblocks) => { + for (i, rb) in rblocks.into_iter().enumerate() { + match rb { + Err(_) => { + errs.push(btree::io_err(&path)); // FIXME: add key_context + }, + 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); + } + }, + Err(e) => { + self.array_errs.lock().unwrap().push(e); + } + } + path.pop(); + }, + } } } - path.pop(); } // FIXME: duplicate to BTreeWalker::build_aggregrate() From 239ff7dfa172b04b8d2bf629ca2dc67b4d4ff1ac Mon Sep 17 00:00:00 2001 From: Ming-Hung Tsai Date: Tue, 20 Apr 2021 16:07:25 +0800 Subject: [PATCH 17/24] [cache_check (rust)] Add more checks - Report errors - Support reading partially broken bitset - The output is a bitmap of 2-bit entries, indicating availability of bits --- src/bin/cache_check.rs | 20 ++++++++++++++ src/cache/check.rs | 54 +++++++++++++++++++++++++++----------- src/pdata/array.rs | 29 +++++++++++++++++---- src/pdata/bitset.rs | 59 ++++++++++++++++++++++++++++++++---------- 4 files changed, 129 insertions(+), 33 deletions(-) diff --git a/src/bin/cache_check.rs b/src/bin/cache_check.rs index 4fed20c..f142f39 100644 --- a/src/bin/cache_check.rs +++ b/src/bin/cache_check.rs @@ -1,8 +1,12 @@ extern crate clap; extern crate thinp; +use atty::Stream; use clap::{App, Arg}; use std::path::Path; +use std::sync::Arc; + +use thinp::report::*; use thinp::cache::check::{check, CacheCheckOptions}; //------------------------------------------ @@ -39,11 +43,26 @@ fn main() { .help("Don't check the discard bitset") .long("skip-discards") .value_name("SKIP_DISCARDS"), + ) + .arg( + Arg::with_name("QUIET") + .help("Suppress output messages, return only exit code.") + .short("q") + .long("quiet"), ); let matches = parser.get_matches(); let input_file = Path::new(matches.value_of("INPUT").unwrap()); + let report; + if matches.is_present("QUIET") { + report = std::sync::Arc::new(mk_quiet_report()); + } else if atty::is(Stream::Stdout) { + report = std::sync::Arc::new(mk_progress_bar_report()); + } else { + report = Arc::new(mk_simple_report()); + } + let opts = CacheCheckOptions { dev: &input_file, async_io: false, @@ -51,6 +70,7 @@ fn main() { skip_mappings: matches.is_present("SKIP_MAPPINGS"), skip_hints: matches.is_present("SKIP_HINTS"), skip_discards: matches.is_present("SKIP_DISCARDS"), + report, }; if let Err(reason) = check(opts) { diff --git a/src/cache/check.rs b/src/cache/check.rs index 01df923..b91504f 100644 --- a/src/cache/check.rs +++ b/src/cache/check.rs @@ -1,5 +1,4 @@ use anyhow::anyhow; -use fixedbitset::FixedBitSet; use std::collections::BTreeSet; use std::path::Path; use std::sync::{Arc, Mutex}; @@ -11,6 +10,7 @@ use crate::io_engine::{AsyncIoEngine, IoEngine, SyncIoEngine}; use crate::pdata::array::{self, ArrayBlock, ArrayError}; use crate::pdata::array_walker::*; use crate::pdata::bitset::*; +use crate::report::*; //------------------------------------------ @@ -102,11 +102,11 @@ mod format2 { struct Inner { seen_oblocks: BTreeSet, - dirty_bits: FixedBitSet, + dirty_bits: CheckedBitSet, } impl MappingChecker { - pub fn new(nr_origin_blocks: Option, dirty_bits: FixedBitSet) -> MappingChecker { + pub fn new(nr_origin_blocks: Option, dirty_bits: CheckedBitSet) -> MappingChecker { MappingChecker { nr_origin_blocks: if let Some(n) = nr_origin_blocks {n} else {MAX_ORIGIN_BLOCKS}, inner: Mutex::new(Inner { @@ -116,11 +116,11 @@ mod format2 { } } - fn check_flags(&self, m: &Mapping, dirty_bit: bool) -> array::Result<()> { + fn check_flags(&self, m: &Mapping, dirty_bit: Option) -> array::Result<()> { if (m.flags & !(MappingFlags::Valid as u32)) != 0 { return Err(array::value_err(format!("unknown flags in mapping: {}", m.flags))); } - if !m.is_valid() && dirty_bit { + if !m.is_valid() && dirty_bit.is_some() && dirty_bit.unwrap() { return Err(array::value_err(format!("dirty bit found on an unmapped block"))); } Ok(()) @@ -203,10 +203,12 @@ pub struct CacheCheckOptions<'a> { pub skip_mappings: bool, pub skip_hints: bool, pub skip_discards: bool, + pub report: Arc, } -// TODO: thread pool, report +// TODO: thread pool struct Context { + report: Arc, engine: Arc, } @@ -220,7 +222,10 @@ fn mk_context(opts: &CacheCheckOptions) -> anyhow::Result { engine = Arc::new(SyncIoEngine::new(opts.dev, nr_threads, false)?); } - Ok(Context { engine }) + Ok(Context { + report: opts.report.clone(), + engine, + }) } fn check_superblock(sb: &Superblock) -> anyhow::Result<()> { @@ -256,15 +261,25 @@ pub fn check(opts: CacheCheckOptions) -> anyhow::Result<()> { match sb.version { 1 => { let mut c = format1::MappingChecker::new(nr_origin_blocks); - w.walk(&mut c, sb.mapping_root)?; + if let Err(e) = w.walk(&mut c, sb.mapping_root) { + ctx.report.fatal(&format!("{}", e)); + } } 2 => { - // FIXME: possibly size truncation on 32-bit targets - let mut dirty_bits = FixedBitSet::with_capacity(sb.cache_blocks as usize); // TODO: allow ignore_none_fatal - read_bitset(engine.clone(), sb.dirty_root.unwrap(), false, &mut dirty_bits)?; + let (dirty_bits, err) = read_bitset( + engine.clone(), + sb.dirty_root.unwrap(), + sb.cache_blocks as usize, + false, + ); + if err.is_some() { + ctx.report.fatal(&format!("{}", err.unwrap())); + } let mut c = format2::MappingChecker::new(nr_origin_blocks, dirty_bits); - w.walk(&mut c, sb.mapping_root)?; + if let Err(e) = w.walk(&mut c, sb.mapping_root) { + ctx.report.fatal(&format!("{}", e)); + } } v => { return Err(anyhow!("unsupported metadata version {}", v)); @@ -278,14 +293,23 @@ pub fn check(opts: CacheCheckOptions) -> anyhow::Result<()> { } let w = ArrayWalker::new(engine.clone(), false); let mut c = HintChecker::new(); - w.walk(&mut c, sb.hint_root)?; + if let Err(e) = w.walk(&mut c, sb.hint_root) { + ctx.report.fatal(&format!("{}", e)); + } } // The discard bitset might not be available if the cache has never been suspended, // e.g., a crash of freshly created cache. if !opts.skip_discards && sb.discard_root != 0 { - let mut discard_bits = FixedBitSet::with_capacity(sb.discard_nr_blocks as usize); - read_bitset(engine.clone(), sb.discard_root, false, &mut discard_bits)?; + let (discard_bits, err) = read_bitset( + engine.clone(), + sb.discard_root, + sb.cache_blocks as usize, + false, + ); + if err.is_some() { + ctx.report.fatal(&format!("{}", err.unwrap())); + } } Ok(()) diff --git a/src/pdata/array.rs b/src/pdata/array.rs index 4a977f6..788d239 100644 --- a/src/pdata/array.rs +++ b/src/pdata/array.rs @@ -1,4 +1,5 @@ use nom::{multi::count, number::complete::*, IResult}; +use std::fmt; use thiserror::Error; use crate::checksum; @@ -52,25 +53,43 @@ pub struct ArrayBlock { #[derive(Error, Clone, Debug)] pub enum ArrayError { - #[error("io_error")] + //#[error("io_error")] IoError, - #[error("block error: {0}")] + //#[error("block error: {0}")] BlockError(String), - #[error("value error: {0}")] + //#[error("value error: {0}")] ValueError(String), - #[error("aggregate: {0:?}")] + //#[error("aggregate: {0:?}")] Aggregate(Vec), - #[error("{0:?}, {1}")] + //#[error("{0:?}, {1}")] Path(Vec, Box), #[error(transparent)] BTreeError(#[from] btree::BTreeError), } +impl fmt::Display for ArrayError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + ArrayError::IoError => write!(f, "io error"), + ArrayError::BlockError(msg) => write!(f, "block error: {}", msg), + ArrayError::ValueError(msg) => write!(f, "value error: {}", msg), + ArrayError::Aggregate(errs) => { + for e in errs { + write!(f, "{}", e)? + } + Ok(()) + } + ArrayError::Path(path, e) => write!(f, "{} {}", e, btree::encode_node_path(path)), + ArrayError::BTreeError(e) => write!(f, "{}", e), + } + } +} + pub fn array_block_err(path: &[u64], msg: &str) -> ArrayError { ArrayError::Path( path.to_vec(), diff --git a/src/pdata/bitset.rs b/src/pdata/bitset.rs index 7cc7b7f..013d9ec 100644 --- a/src/pdata/bitset.rs +++ b/src/pdata/bitset.rs @@ -5,21 +5,49 @@ use crate::io_engine::IoEngine; use crate::pdata::array::{self, ArrayBlock}; use crate::pdata::array_walker::{ArrayVisitor, ArrayWalker}; -struct BitsetVisitor<'a> { - nr_entries: u64, - bits: Mutex<&'a mut FixedBitSet>, +pub struct CheckedBitSet { + bits: FixedBitSet, } -impl<'a> BitsetVisitor<'a> { - pub fn new(bitset: &'a mut FixedBitSet) -> Self { - BitsetVisitor { - nr_entries: bitset.len() as u64, - bits: Mutex::new(bitset), +impl CheckedBitSet { + pub fn with_capacity(bits: usize) -> CheckedBitSet { + CheckedBitSet { + bits: FixedBitSet::with_capacity(bits << 1), } } + + pub fn set(&mut self, bit: usize, enabled: bool) { + self.bits.set(bit << 1, true); + self.bits.set((bit << 1) + 1, enabled); + } + + pub fn contains(&self, bit: usize) -> Option { + if !self.bits.contains(bit << 1) { + return None; + } + Some(self.bits.contains((bit << 1) + 1)) + } } -impl<'a> ArrayVisitor for BitsetVisitor<'a> { +struct BitsetVisitor { + nr_entries: usize, + bits: Mutex, +} + +impl BitsetVisitor { + pub fn new(nr_entries: usize) -> Self { + BitsetVisitor { + nr_entries, + bits: Mutex::new(CheckedBitSet::with_capacity(nr_entries)), + } + } + + pub fn get_bitset(self) -> CheckedBitSet { + self.bits.into_inner().unwrap() + } +} + +impl ArrayVisitor for BitsetVisitor { fn visit(&self, index: u64, b: ArrayBlock) -> array::Result<()> { let mut begin = index as usize * (b.header.max_entries as usize) << 6; @@ -46,10 +74,15 @@ impl<'a> ArrayVisitor for BitsetVisitor<'a> { pub fn read_bitset( engine: Arc, root: u64, + nr_entries: usize, ignore_none_fatal: bool, - bitset: &mut FixedBitSet, -)-> array::Result<()> { +)-> (CheckedBitSet, Option) { let w = ArrayWalker::new(engine.clone(), ignore_none_fatal); - let mut v = BitsetVisitor::new(bitset); - w.walk(&mut v, root) + let mut v = BitsetVisitor::new(nr_entries); + let err = w.walk(&mut v, root); + let e = match err { + Ok(()) => None, + Err(e) => Some(e), + }; + return (v.get_bitset(), e); } From e1628f9004f518050b790bb3ba092d80c0af0766 Mon Sep 17 00:00:00 2001 From: Ming-Hung Tsai Date: Wed, 21 Apr 2021 14:36:16 +0800 Subject: [PATCH 18/24] [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<()> { From 636d50a38dc978347edaaef723ede2c6369e376d Mon Sep 17 00:00:00 2001 From: Ming-Hung Tsai Date: Thu, 22 Apr 2021 23:17:20 +0800 Subject: [PATCH 19/24] [thin_check (rust)] Pull out space map checking routines --- src/pdata/btree_walker.rs | 48 +++++ src/pdata/mod.rs | 1 + src/pdata/space_map_checker.rs | 308 +++++++++++++++++++++++++++++++++ src/thin/check.rs | 269 ++-------------------------- 4 files changed, 374 insertions(+), 252 deletions(-) create mode 100644 src/pdata/space_map_checker.rs diff --git a/src/pdata/btree_walker.rs b/src/pdata/btree_walker.rs index 28babc0..3de7c85 100644 --- a/src/pdata/btree_walker.rs +++ b/src/pdata/btree_walker.rs @@ -567,3 +567,51 @@ pub fn btree_to_map_with_path( } //------------------------------------------ + +struct NoopVisitor { + dummy: std::marker::PhantomData, +} + +impl NoopVisitor { + pub fn new() -> NoopVisitor { + NoopVisitor { + dummy: std::marker::PhantomData, + } + } +} + +impl NodeVisitor for NoopVisitor { + fn visit( + &self, + _path: &[u64], + _kr: &KeyRange, + _header: &NodeHeader, + _keys: &[u64], + _values: &[V], + ) -> Result<()> { + Ok(()) + } + + //fn visit_again(&self, _path: &[u64], _b: u64) -> Result<()> { + fn visit_again(&self, _path: &[u64], _b: u64) -> Result<()> { + Ok(()) + } + + fn end_walk(&self) -> Result<()> { + Ok(()) + } +} + +pub fn count_btree_blocks( + engine: Arc, + path: &mut Vec, + root: u64, + metadata_sm: ASpaceMap, + ignore_non_fatal: bool, +) -> Result<()> { + let w = BTreeWalker::new_with_sm(engine, metadata_sm, ignore_non_fatal)?; + let v = NoopVisitor::::new(); + w.walk(path, &v, root) +} + +//------------------------------------------ diff --git a/src/pdata/mod.rs b/src/pdata/mod.rs index 50f571c..fb219c4 100644 --- a/src/pdata/mod.rs +++ b/src/pdata/mod.rs @@ -7,5 +7,6 @@ pub mod btree_merge; pub mod btree_leaf_walker; pub mod btree_walker; pub mod space_map; +pub mod space_map_checker; pub mod unpack; diff --git a/src/pdata/space_map_checker.rs b/src/pdata/space_map_checker.rs new file mode 100644 index 0000000..3a114bd --- /dev/null +++ b/src/pdata/space_map_checker.rs @@ -0,0 +1,308 @@ +use anyhow::{anyhow, Result}; +use std::io::Cursor; +use std::sync::Arc; + +use crate::checksum; +use crate::io_engine::IoEngine; +use crate::pdata::btree::{self, *}; +use crate::pdata::btree_walker::*; +use crate::pdata::space_map::*; +use crate::pdata::unpack::*; +use crate::report::Report; + +//------------------------------------------ + +pub struct BitmapLeak { + blocknr: u64, // blocknr for the first entry in the bitmap + loc: u64, // location of the bitmap +} + +//------------------------------------------ + +struct OverflowChecker<'a> { + kind: &'a str, + sm: &'a dyn SpaceMap, +} + +impl<'a> OverflowChecker<'a> { + fn new(kind: &'a str, sm: &'a dyn SpaceMap) -> OverflowChecker<'a> { + OverflowChecker { kind, sm } + } +} + +impl<'a> NodeVisitor for OverflowChecker<'a> { + fn visit( + &self, + _path: &[u64], + _kr: &KeyRange, + _h: &NodeHeader, + keys: &[u64], + values: &[u32], + ) -> btree::Result<()> { + for n in 0..keys.len() { + let k = keys[n]; + let v = values[n]; + let expected = self.sm.get(k).unwrap(); + if expected != v { + return Err(value_err(format!("Bad reference count for {} block {}. Expected {}, but space map contains {}.", + self.kind, k, expected, v))); + } + } + + Ok(()) + } + + fn visit_again(&self, _path: &[u64], _b: u64) -> btree::Result<()> { + Ok(()) + } + + fn end_walk(&self) -> btree::Result<()> { + Ok(()) + } +} + +//------------------------------------------ + +fn inc_entries(sm: &ASpaceMap, entries: &[IndexEntry]) -> Result<()> { + let mut sm = sm.lock().unwrap(); + for ie in entries { + // FIXME: checksumming bitmaps? + sm.inc(ie.blocknr, 1)?; + } + Ok(()) +} + +// Compare the refernece counts in bitmaps against the expected values +// +// `sm` - The in-core space map of expected reference counts +fn check_low_ref_counts<'a>( + engine: Arc, + report: Arc, + kind: &'a str, + entries: Vec, + sm: ASpaceMap, +) -> Result> { + // gathering bitmap blocknr + let mut blocks = Vec::with_capacity(entries.len()); + for i in &entries { + blocks.push(i.blocknr); + } + + // read bitmap blocks + // FIXME: we should do this in batches + let blocks = engine.read_many(&blocks)?; + + // compare ref-counts in bitmap blocks + let mut leaks = 0; + let mut blocknr = 0; + let mut bitmap_leaks = Vec::new(); + let sm = sm.lock().unwrap(); + let nr_blocks = sm.get_nr_blocks()?; + for b in blocks.iter().take(entries.len()) { + match b { + Err(_e) => { + return Err(anyhow!("Unable to read bitmap block")); + } + Ok(b) => { + if checksum::metadata_block_type(&b.get_data()) != checksum::BT::BITMAP { + report.fatal(&format!( + "Index entry points to block ({}) that isn't a bitmap", + b.loc + )); + + // FIXME: revert the ref-count at b.loc? + } + + let bitmap = unpack::(b.get_data())?; + let first_blocknr = blocknr; + let mut contains_leak = false; + for e in bitmap.entries.iter() { + if blocknr >= nr_blocks { + break; + } + + match e { + BitmapEntry::Small(actual) => { + let expected = sm.get(blocknr)?; + if *actual == 1 && expected == 0 { + leaks += 1; + contains_leak = true; + } else if *actual != expected as u8 { + report.fatal(&format!("Bad reference count for {} block {}. Expected {}, but space map contains {}.", + kind, blocknr, expected, actual)); + } + } + BitmapEntry::Overflow => { + let expected = sm.get(blocknr)?; + if expected < 3 { + report.fatal(&format!("Bad reference count for {} block {}. Expected {}, but space map says it's >= 3.", + kind, blocknr, expected)); + } + } + } + blocknr += 1; + } + if contains_leak { + bitmap_leaks.push(BitmapLeak { + blocknr: first_blocknr, + loc: b.loc, + }); + } + } + } + } + + if leaks > 0 { + report.non_fatal(&format!("{} {} blocks have leaked.", leaks, kind)); + } + + Ok(bitmap_leaks) +} + +fn gather_disk_index_entries( + engine: Arc, + bitmap_root: u64, + metadata_sm: ASpaceMap, + ignore_non_fatal: bool, +) -> Result> { + let entries_map = btree_to_map_with_sm::( + &mut vec![0], + engine, + metadata_sm.clone(), + ignore_non_fatal, + bitmap_root, + )?; + + let entries: Vec = entries_map.values().cloned().collect(); + inc_entries(&metadata_sm, &entries[0..])?; + + Ok(entries) +} + +fn gather_metadata_index_entries( + engine: Arc, + bitmap_root: u64, + metadata_sm: ASpaceMap, +) -> Result> { + let b = engine.read(bitmap_root)?; + let entries = unpack::(b.get_data())?.indexes; + + // Filter out unused entries with block 0 + let entries: Vec = entries + .iter() + .take_while(|e| e.blocknr != 0) + .cloned() + .collect(); + + metadata_sm.lock().unwrap().inc(bitmap_root, 1)?; + inc_entries(&metadata_sm, &entries[0..])?; + + Ok(entries) +} + +//------------------------------------------ + +// This checks the space map and returns any leak blocks for auto-repair to process. +// +// `disk_sm` - The in-core space map of expected data block ref-counts +// `metadata_sm` - The in-core space for storing ref-counts of verified blocks +pub fn check_disk_space_map( + engine: Arc, + report: Arc, + root: SMRoot, + disk_sm: ASpaceMap, + metadata_sm: ASpaceMap, + ignore_non_fatal: bool, +) -> Result> { + let entries = gather_disk_index_entries(engine.clone(), root.bitmap_root, metadata_sm.clone(), ignore_non_fatal)?; + + // check overflow ref-counts + { + let sm = disk_sm.lock().unwrap(); + let v = OverflowChecker::new("data", &*sm); + let w = BTreeWalker::new_with_sm(engine.clone(), metadata_sm.clone(), false)?; + w.walk(&mut vec![0], &v, root.ref_count_root)?; + } + + // check low ref-counts in bitmaps + check_low_ref_counts(engine, report, "data", entries, disk_sm) +} + +// This checks the space map and returns any leak blocks for auto-repair to process. +// +// `metadata_sm`: The in-core space map of expected metadata block ref-counts +pub fn check_metadata_space_map( + engine: Arc, + report: Arc, + root: SMRoot, + metadata_sm: ASpaceMap, + ignore_non_fatal: bool, +) -> Result> { + count_btree_blocks::(engine.clone(), &mut vec![0], root.ref_count_root, metadata_sm.clone(), false)?; + let entries = gather_metadata_index_entries(engine.clone(), root.bitmap_root, metadata_sm.clone())?; + + // check overflow ref-counts + { + let sm = metadata_sm.lock().unwrap(); + let v = OverflowChecker::new("metadata", &*sm); + let w = BTreeWalker::new(engine.clone(), ignore_non_fatal); + w.walk(&mut vec![0], &v, root.ref_count_root)?; + } + + // check low ref-counts in bitmaps + check_low_ref_counts(engine, report, "metadata", entries, metadata_sm) +} + +// This assumes the only errors in the space map are leaks. Entries should just be +// those that contain leaks. +pub fn repair_space_map( + engine: Arc, + entries: Vec, + sm: ASpaceMap +) -> Result<()> { + let sm = sm.lock().unwrap(); + + let mut blocks = Vec::with_capacity(entries.len()); + for i in &entries { + blocks.push(i.loc); + } + + // FIXME: we should do this in batches + let rblocks = engine.read_many(&blocks[0..])?; + let mut write_blocks = Vec::new(); + + for (i, rb) in rblocks.into_iter().enumerate() { + if let Ok(b) = rb { + let be = &entries[i]; + let mut blocknr = be.blocknr; + let mut bitmap = unpack::(b.get_data())?; + for e in bitmap.entries.iter_mut() { + if blocknr >= sm.get_nr_blocks()? { + break; + } + + if let BitmapEntry::Small(actual) = e { + let expected = sm.get(blocknr)?; + if *actual == 1 && expected == 0 { + *e = BitmapEntry::Small(0); + } + } + + blocknr += 1; + } + + let mut out = Cursor::new(b.get_data()); + bitmap.pack(&mut out)?; + checksum::write_checksum(b.get_data(), checksum::BT::BITMAP)?; + + write_blocks.push(b); + } else { + return Err(anyhow!("Unable to reread bitmap blocks for repair")); + } + } + + engine.write_many(&write_blocks[0..])?; + Ok(()) +} + +//------------------------------------------ diff --git a/src/thin/check.rs b/src/thin/check.rs index cb25257..8701388 100644 --- a/src/thin/check.rs +++ b/src/thin/check.rs @@ -1,17 +1,16 @@ use anyhow::{anyhow, Result}; use std::collections::BTreeMap; -use std::io::Cursor; use std::path::Path; use std::sync::atomic::{AtomicBool, Ordering}; use std::sync::{Arc, Mutex}; use std::thread::{self, JoinHandle}; use threadpool::ThreadPool; -use crate::checksum; use crate::io_engine::{AsyncIoEngine, IoEngine, SyncIoEngine}; use crate::pdata::btree::{self, *}; use crate::pdata::btree_walker::*; use crate::pdata::space_map::*; +use crate::pdata::space_map_checker::*; use crate::pdata::unpack::*; use crate::report::*; use crate::thin::block_time::*; @@ -72,211 +71,6 @@ impl NodeVisitor for BottomLevelVisitor { //------------------------------------------ -struct OverflowChecker<'a> { - data_sm: &'a dyn SpaceMap, -} - -impl<'a> OverflowChecker<'a> { - fn new(data_sm: &'a dyn SpaceMap) -> OverflowChecker<'a> { - OverflowChecker { data_sm } - } -} - -impl<'a> NodeVisitor for OverflowChecker<'a> { - fn visit( - &self, - _path: &[u64], - _kr: &KeyRange, - _h: &NodeHeader, - keys: &[u64], - values: &[u32], - ) -> btree::Result<()> { - for n in 0..keys.len() { - let k = keys[n]; - let v = values[n]; - let expected = self.data_sm.get(k).unwrap(); - if expected != v { - return Err(value_err(format!("Bad reference count for data block {}. Expected {}, but space map contains {}.", - k, expected, v))); - } - } - - Ok(()) - } - - fn visit_again(&self, _path: &[u64], _b: u64) -> btree::Result<()> { - Ok(()) - } - - fn end_walk(&self) -> btree::Result<()> { - Ok(()) - } -} - -//------------------------------------------ - -struct BitmapLeak { - blocknr: u64, // blocknr for the first entry in the bitmap - loc: u64, // location of the bitmap -} - -// This checks the space map and returns any leak blocks for auto-repair to process. -fn check_space_map( - path: &mut Vec, - ctx: &Context, - kind: &str, - entries: Vec, - metadata_sm: Option, - sm: ASpaceMap, - root: SMRoot, -) -> Result> { - let report = ctx.report.clone(); - let engine = ctx.engine.clone(); - - let sm = sm.lock().unwrap(); - - // overflow btree - { - let v = OverflowChecker::new(&*sm); - let w; - if metadata_sm.is_none() { - w = BTreeWalker::new(engine.clone(), false); - } else { - w = BTreeWalker::new_with_sm(engine.clone(), metadata_sm.unwrap().clone(), false)?; - } - w.walk(path, &v, root.ref_count_root)?; - } - - let mut blocks = Vec::with_capacity(entries.len()); - for i in &entries { - blocks.push(i.blocknr); - } - - // FIXME: we should do this in batches - let blocks = engine.read_many(&blocks)?; - - let mut leaks = 0; - let mut blocknr = 0; - let mut bitmap_leaks = Vec::new(); - for b in blocks.iter().take(entries.len()) { - match b { - Err(_e) => { - return Err(anyhow!("Unable to read bitmap block")); - } - Ok(b) => { - if checksum::metadata_block_type(&b.get_data()) != checksum::BT::BITMAP { - report.fatal(&format!( - "Index entry points to block ({}) that isn't a bitmap", - b.loc - )); - } - - let bitmap = unpack::(b.get_data())?; - let first_blocknr = blocknr; - let mut contains_leak = false; - for e in bitmap.entries.iter() { - if blocknr >= root.nr_blocks { - break; - } - - match e { - BitmapEntry::Small(actual) => { - let expected = sm.get(blocknr)?; - if *actual == 1 && expected == 0 { - leaks += 1; - contains_leak = true; - } else if *actual != expected as u8 { - report.fatal(&format!("Bad reference count for {} block {}. Expected {}, but space map contains {}.", - kind, blocknr, expected, actual)); - } - } - BitmapEntry::Overflow => { - let expected = sm.get(blocknr)?; - if expected < 3 { - report.fatal(&format!("Bad reference count for {} block {}. Expected {}, but space map says it's >= 3.", - kind, blocknr, expected)); - } - } - } - blocknr += 1; - } - if contains_leak { - bitmap_leaks.push(BitmapLeak { - blocknr: first_blocknr, - loc: b.loc, - }); - } - } - } - } - - if leaks > 0 { - report.non_fatal(&format!("{} {} blocks have leaked.", leaks, kind)); - } - - Ok(bitmap_leaks) -} - -// This assumes the only errors in the space map are leaks. Entries should just be -// those that contain leaks. -fn repair_space_map(ctx: &Context, entries: Vec, sm: ASpaceMap) -> Result<()> { - let engine = ctx.engine.clone(); - - let sm = sm.lock().unwrap(); - - let mut blocks = Vec::with_capacity(entries.len()); - for i in &entries { - blocks.push(i.loc); - } - - // FIXME: we should do this in batches - let rblocks = engine.read_many(&blocks[0..])?; - let mut write_blocks = Vec::new(); - - for (i, rb) in rblocks.into_iter().enumerate() { - if let Ok(b) = rb { - let be = &entries[i]; - let mut blocknr = be.blocknr; - let mut bitmap = unpack::(b.get_data())?; - for e in bitmap.entries.iter_mut() { - if blocknr >= sm.get_nr_blocks()? { - break; - } - - if let BitmapEntry::Small(actual) = e { - let expected = sm.get(blocknr)?; - if *actual == 1 && expected == 0 { - *e = BitmapEntry::Small(0); - } - } - - blocknr += 1; - } - - let mut out = Cursor::new(b.get_data()); - bitmap.pack(&mut out)?; - checksum::write_checksum(b.get_data(), checksum::BT::BITMAP)?; - - write_blocks.push(b); - } else { - return Err(anyhow!("Unable to reread bitmap blocks for repair")); - } - } - - engine.write_many(&write_blocks[0..])?; - Ok(()) -} - -//------------------------------------------ - -fn inc_entries(sm: &ASpaceMap, entries: &[IndexEntry]) -> Result<()> { - let mut sm = sm.lock().unwrap(); - for ie in entries { - sm.inc(ie.blocknr, 1)?; - } - Ok(()) -} - fn inc_superblock(sm: &ASpaceMap) -> Result<()> { let mut sm = sm.lock().unwrap(); sm.inc(SUPERBLOCK_LOCATION, 1)?; @@ -506,30 +300,22 @@ pub fn check(opts: ThinCheckOptions) -> Result<()> { check_mapping_bottom_level(&ctx, &metadata_sm, &data_sm, &roots)?; bail_out(&ctx, "mapping tree")?; + //----------------------------------------- + report.set_sub_title("data space map"); let root = unpack::(&sb.data_sm_root[0..])?; - - let entries = btree_to_map_with_sm::( - &mut path, + let data_leaks = check_disk_space_map( engine.clone(), + report.clone(), + root, + data_sm.clone(), metadata_sm.clone(), opts.ignore_non_fatal, - root.bitmap_root, - )?; - let entries: Vec = entries.values().cloned().collect(); - inc_entries(&metadata_sm, &entries[0..])?; - - let data_leaks = check_space_map( - &mut path, - &ctx, - "data", - entries, - Some(metadata_sm.clone()), - data_sm.clone(), - root, )?; bail_out(&ctx, "data space map")?; + //----------------------------------------- + report.set_sub_title("metadata space map"); let root = unpack::(&sb.metadata_sm_root[0..])?; report.info(&format!( @@ -537,49 +323,28 @@ pub fn check(opts: ThinCheckOptions) -> Result<()> { root.nr_blocks - root.nr_allocated )); - let b = engine.read(root.bitmap_root)?; - metadata_sm.lock().unwrap().inc(root.bitmap_root, 1)?; - let entries = unpack::(b.get_data())?.indexes; - - // Unused entries will point to block 0 - let entries: Vec = entries - .iter() - .take_while(|e| e.blocknr != 0) - .cloned() - .collect(); - inc_entries(&metadata_sm, &entries[0..])?; - - // We call this for the side effect of incrementing the ref counts - // for the metadata that holds the tree. - let _counts = btree_to_map_with_sm::( - &mut path, + // Now the counts should be correct and we can check it. + let metadata_leaks = check_metadata_space_map( engine.clone(), + report.clone(), + root, metadata_sm.clone(), opts.ignore_non_fatal, - root.ref_count_root, )?; - // Now the counts should be correct and we can check it. - let metadata_leaks = check_space_map( - &mut path, - &ctx, - "metadata", - entries, - None, - metadata_sm.clone(), - root, - )?; bail_out(&ctx, "metadata space map")?; + //----------------------------------------- + if opts.auto_repair { if !data_leaks.is_empty() { ctx.report.info("Repairing data leaks."); - repair_space_map(&ctx, data_leaks, data_sm.clone())?; + repair_space_map(ctx.engine.clone(), data_leaks, data_sm.clone())?; } if !metadata_leaks.is_empty() { ctx.report.info("Repairing metadata leaks."); - repair_space_map(&ctx, metadata_leaks, metadata_sm.clone())?; + repair_space_map(ctx.engine.clone(), metadata_leaks, metadata_sm.clone())?; } } From cf4b937ade63a9dd1e7a0d9e46347f5696ce5761 Mon Sep 17 00:00:00 2001 From: Ming-Hung Tsai Date: Wed, 21 Apr 2021 23:36:10 +0800 Subject: [PATCH 20/24] [cache_check (rust)] Check space map counts - Support space map checking and auto-repair --- src/bin/cache_check.rs | 6 ++++++ src/cache/check.rs | 45 +++++++++++++++++++++++++++++++++------ src/pdata/array_walker.rs | 34 +++++++++++++++++++++++++++-- src/pdata/bitset.rs | 21 +++++++++++++++++- 4 files changed, 96 insertions(+), 10 deletions(-) diff --git a/src/bin/cache_check.rs b/src/bin/cache_check.rs index 452a495..2c70358 100644 --- a/src/bin/cache_check.rs +++ b/src/bin/cache_check.rs @@ -49,6 +49,11 @@ fn main() { .help("Only return a non-zero exit code if a fatal error is found.") .long("ignore-non-fatal-errors"), ) + .arg( + Arg::with_name("AUTO_REPAIR") + .help("Auto repair trivial issues.") + .long("auto-repair"), + ) .arg( Arg::with_name("QUIET") .help("Suppress output messages, return only exit code.") @@ -76,6 +81,7 @@ fn main() { skip_hints: matches.is_present("SKIP_HINTS"), skip_discards: matches.is_present("SKIP_DISCARDS"), ignore_non_fatal: matches.is_present("IGNORE_NON_FATAL"), + auto_repair: matches.is_present("AUTO_REPAIR"), report, }; diff --git a/src/cache/check.rs b/src/cache/check.rs index 71a59ae..2fce400 100644 --- a/src/cache/check.rs +++ b/src/cache/check.rs @@ -10,6 +10,9 @@ use crate::io_engine::{AsyncIoEngine, IoEngine, SyncIoEngine}; use crate::pdata::array::{self, ArrayBlock, ArrayError}; use crate::pdata::array_walker::*; use crate::pdata::bitset::*; +use crate::pdata::space_map::*; +use crate::pdata::space_map_checker::*; +use crate::pdata::unpack::unpack; use crate::report::*; //------------------------------------------ @@ -18,6 +21,14 @@ const MAX_CONCURRENT_IO: u32 = 1024; //------------------------------------------ +fn inc_superblock(sm: &ASpaceMap) -> anyhow::Result<()> { + let mut sm = sm.lock().unwrap(); + sm.inc(SUPERBLOCK_LOCATION, 1)?; + Ok(()) +} + +//------------------------------------------ + mod format1 { use super::*; @@ -204,6 +215,7 @@ pub struct CacheCheckOptions<'a> { pub skip_hints: bool, pub skip_discards: bool, pub ignore_non_fatal: bool, + pub auto_repair: bool, pub report: Arc, } @@ -240,6 +252,8 @@ pub fn check(opts: CacheCheckOptions) -> anyhow::Result<()> { let ctx = mk_context(&opts)?; let engine = &ctx.engine; + let metadata_sm = core_sm(engine.get_nr_blocks(), u8::MAX as u32); + inc_superblock(&metadata_sm)?; let sb = read_superblock(engine.as_ref(), SUPERBLOCK_LOCATION)?; check_superblock(&sb)?; @@ -258,7 +272,7 @@ pub fn check(opts: CacheCheckOptions) -> anyhow::Result<()> { // TODO: factor out into check_mappings() if !opts.skip_mappings { - let w = ArrayWalker::new(engine.clone(), opts.ignore_non_fatal); + let w = ArrayWalker::new_with_sm(engine.clone(), metadata_sm.clone(), opts.ignore_non_fatal)?; match sb.version { 1 => { let mut c = format1::MappingChecker::new(nr_origin_blocks); @@ -267,13 +281,13 @@ pub fn check(opts: CacheCheckOptions) -> anyhow::Result<()> { } } 2 => { - // TODO: allow ignore_none_fatal - let (dirty_bits, err) = read_bitset( + let (dirty_bits, err) = read_bitset_with_sm( engine.clone(), sb.dirty_root.unwrap(), sb.cache_blocks as usize, + metadata_sm.clone(), opts.ignore_non_fatal, - ); + )?; if err.is_some() { ctx.report.fatal(&format!("{}", err.unwrap())); } @@ -292,7 +306,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(), opts.ignore_non_fatal); + let w = ArrayWalker::new_with_sm(engine.clone(), metadata_sm.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)); @@ -302,17 +316,34 @@ pub fn check(opts: CacheCheckOptions) -> anyhow::Result<()> { // The discard bitset might not be available if the cache has never been suspended, // e.g., a crash of freshly created cache. if !opts.skip_discards && sb.discard_root != 0 { - let (discard_bits, err) = read_bitset( + let (_discard_bits, err) = read_bitset_with_sm( engine.clone(), sb.discard_root, sb.cache_blocks as usize, + metadata_sm.clone(), opts.ignore_non_fatal, - ); + )?; if err.is_some() { ctx.report.fatal(&format!("{}", err.unwrap())); } } + let root = unpack::(&sb.metadata_sm_root[0..])?; + let metadata_leaks = check_metadata_space_map( + engine.clone(), + ctx.report.clone(), + root, + metadata_sm.clone(), + opts.ignore_non_fatal, + )?; + + if opts.auto_repair { + if !metadata_leaks.is_empty() { + ctx.report.info("Repairing metadata leaks."); + repair_space_map(ctx.engine.clone(), metadata_leaks, metadata_sm.clone())?; + } + } + Ok(()) } diff --git a/src/pdata/array_walker.rs b/src/pdata/array_walker.rs index 34df482..f8d029f 100644 --- a/src/pdata/array_walker.rs +++ b/src/pdata/array_walker.rs @@ -4,12 +4,14 @@ use crate::io_engine::*; use crate::pdata::array::{self, *}; use crate::pdata::btree::{self, *}; use crate::pdata::btree_walker::*; +use crate::pdata::space_map::*; use crate::pdata::unpack::*; //------------------------------------------ pub struct ArrayWalker { engine: Arc, + sm: Arc>, ignore_non_fatal: bool, } @@ -23,17 +25,20 @@ pub trait ArrayVisitor { struct BlockValueVisitor<'a, V> { engine: Arc, array_visitor: &'a mut dyn ArrayVisitor, + sm: Arc>, array_errs: Mutex>, } impl<'a, V: Unpack + Copy> BlockValueVisitor<'a, V> { pub fn new( e: Arc, + sm: Arc>, v: &'a mut dyn ArrayVisitor, ) -> BlockValueVisitor<'a, V> { BlockValueVisitor { engine: e, array_visitor: v, + sm: sm, array_errs: Mutex::new(Vec::new()), } } @@ -85,6 +90,8 @@ impl<'a, V: Unpack + Copy> NodeVisitor for BlockValueVisitor<'a, V> { if let Err(e) = self.array_visitor.visit(keys[i], array_block) { self.array_errs.lock().unwrap().push(e); } + let mut sm = self.sm.lock().unwrap(); + sm.inc(b.loc, 1).unwrap(); }, Err(e) => { self.array_errs.lock().unwrap().push(e); @@ -113,21 +120,44 @@ impl<'a, V: Unpack + Copy> NodeVisitor for BlockValueVisitor<'a, V> { impl ArrayWalker { pub fn new(engine: Arc, ignore_non_fatal: bool) -> ArrayWalker { + let nr_blocks = engine.get_nr_blocks() as u64; let r: ArrayWalker = ArrayWalker { engine, + sm: Arc::new(Mutex::new(RestrictedSpaceMap::new(nr_blocks))), ignore_non_fatal, }; r } + pub fn new_with_sm( + engine: Arc, + sm: Arc>, + ignore_non_fatal: bool, + ) -> array::Result { + { + let sm = sm.lock().unwrap(); + assert_eq!(sm.get_nr_blocks().unwrap(), engine.get_nr_blocks()); + } + + Ok(ArrayWalker { + engine, + sm, + ignore_non_fatal, + }) + } + pub fn walk(&self, visitor: &mut dyn ArrayVisitor, root: u64) -> array::Result<()> where V: Unpack + Copy, { - let w = BTreeWalker::new(self.engine.clone(), self.ignore_non_fatal); + let w = BTreeWalker::new_with_sm( + self.engine.clone(), + self.sm.clone(), + self.ignore_non_fatal + )?; let mut path = Vec::new(); path.push(0); - let v = BlockValueVisitor::::new(self.engine.clone(), visitor); + let v = BlockValueVisitor::::new(self.engine.clone(), self.sm.clone(), visitor); let btree_err = w.walk(&mut path, &v, root).map_err(|e| ArrayError::BTreeError(e)); let mut array_errs = v.array_errs.into_inner().unwrap(); diff --git a/src/pdata/bitset.rs b/src/pdata/bitset.rs index 013d9ec..82bf633 100644 --- a/src/pdata/bitset.rs +++ b/src/pdata/bitset.rs @@ -4,6 +4,7 @@ use std::sync::{Arc, Mutex}; use crate::io_engine::IoEngine; use crate::pdata::array::{self, ArrayBlock}; use crate::pdata::array_walker::{ArrayVisitor, ArrayWalker}; +use crate::pdata::space_map::*; pub struct CheckedBitSet { bits: FixedBitSet, @@ -77,7 +78,7 @@ pub fn read_bitset( nr_entries: usize, ignore_none_fatal: bool, )-> (CheckedBitSet, Option) { - let w = ArrayWalker::new(engine.clone(), ignore_none_fatal); + let w = ArrayWalker::new(engine, ignore_none_fatal); let mut v = BitsetVisitor::new(nr_entries); let err = w.walk(&mut v, root); let e = match err { @@ -86,3 +87,21 @@ pub fn read_bitset( }; return (v.get_bitset(), e); } + +// TODO: multi-threaded is possible +pub fn read_bitset_with_sm( + engine: Arc, + root: u64, + nr_entries: usize, + sm: Arc>, + ignore_none_fatal: bool, +)-> array::Result<(CheckedBitSet, Option)> { + let w = ArrayWalker::new_with_sm(engine, sm, ignore_none_fatal)?; + let mut v = BitsetVisitor::new(nr_entries); + let err = w.walk(&mut v, root); + let e = match err { + Ok(()) => None, + Err(e) => Some(e), + }; + return Ok((v.get_bitset(), e)); +} From 0553a78c041cfed4ab5d4ceb90916781c5a175bc Mon Sep 17 00:00:00 2001 From: Ming-Hung Tsai Date: Tue, 11 May 2021 17:18:21 +0800 Subject: [PATCH 21/24] Add pre-commit hooks --- .pre-commit-config.yaml | 13 +++++++++++++ 1 file changed, 13 insertions(+) create mode 100644 .pre-commit-config.yaml diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml new file mode 100644 index 0000000..b423e50 --- /dev/null +++ b/.pre-commit-config.yaml @@ -0,0 +1,13 @@ +repos: +- repo: https://github.com/pre-commit/pre-commit-hooks + rev: v3.4.0 + hooks: + - id: check-merge-conflict + - id: end-of-file-fixer + - id: mixed-line-ending + - id: trailing-whitespace +- repo: https://github.com/doublify/pre-commit-rust + rev: master + hooks: + - id: fmt + - id: clippy From 965fbb6e8fe2a2a9cba1584d3d860379b6204f51 Mon Sep 17 00:00:00 2001 From: Ming-Hung Tsai Date: Tue, 11 May 2021 20:41:30 +0800 Subject: [PATCH 22/24] [all] Apply cargo fmt, and fix clippy warnings --- src/bin/cache_check.rs | 2 +- src/bin/thin_check.rs | 11 +++-- src/bin/thin_explore.rs | 18 ++++---- src/cache/check.rs | 79 ++++++++++++++++++++-------------- src/cache/dump.rs | 21 ++++++--- src/cache/hint.rs | 1 - src/checksum.rs | 5 ++- src/pdata/array.rs | 17 ++++---- src/pdata/array_walker.rs | 26 ++++++----- src/pdata/bitset.rs | 14 +++--- src/pdata/btree.rs | 5 ++- src/pdata/btree_builder.rs | 14 +++--- src/pdata/btree_walker.rs | 5 +-- src/pdata/space_map_checker.rs | 30 +++++++++---- src/pdata/space_map_disk.rs | 2 +- src/thin/check.rs | 21 ++++----- src/thin/dump.rs | 9 ++-- src/thin/restore.rs | 4 +- src/thin/superblock.rs | 6 +-- 19 files changed, 166 insertions(+), 124 deletions(-) diff --git a/src/bin/cache_check.rs b/src/bin/cache_check.rs index 2c70358..94355dc 100644 --- a/src/bin/cache_check.rs +++ b/src/bin/cache_check.rs @@ -6,8 +6,8 @@ use clap::{App, Arg}; use std::path::Path; use std::sync::Arc; -use thinp::report::*; use thinp::cache::check::{check, CacheCheckOptions}; +use thinp::report::*; //------------------------------------------ diff --git a/src/bin/thin_check.rs b/src/bin/thin_check.rs index b607c58..307b439 100644 --- a/src/bin/thin_check.rs +++ b/src/bin/thin_check.rs @@ -106,14 +106,19 @@ fn main() { let engine: Arc; if matches.is_present("ASYNC_IO") { - engine = Arc::new(AsyncIoEngine::new(&input_file, MAX_CONCURRENT_IO, false).expect("unable to open input file")); + engine = Arc::new( + AsyncIoEngine::new(&input_file, MAX_CONCURRENT_IO, false) + .expect("unable to open input file"), + ); } else { let nr_threads = std::cmp::max(8, num_cpus::get() * 2); - engine = Arc::new(SyncIoEngine::new(&input_file, nr_threads, false).expect("unable to open input file")); + engine = Arc::new( + SyncIoEngine::new(&input_file, nr_threads, false).expect("unable to open input file"), + ); } let opts = ThinCheckOptions { - engine: engine, + engine, sb_only: matches.is_present("SB_ONLY"), skip_mappings: matches.is_present("SKIP_MAPPINGS"), ignore_non_fatal: matches.is_present("IGNORE_NON_FATAL"), diff --git a/src/bin/thin_explore.rs b/src/bin/thin_explore.rs index 356cea6..c4918fd 100644 --- a/src/bin/thin_explore.rs +++ b/src/bin/thin_explore.rs @@ -75,15 +75,13 @@ impl Events { let ignore_exit_key = ignore_exit_key.clone(); thread::spawn(move || { let stdin = io::stdin(); - for evt in stdin.keys() { - if let Ok(key) = evt { - if let Err(err) = tx.send(Event::Input(key)) { - eprintln!("{}", err); - return; - } - if !ignore_exit_key.load(Ordering::Relaxed) && key == config.exit_key { - return; - } + for key in stdin.keys().flatten() { + if let Err(err) = tx.send(Event::Input(key)) { + eprintln!("{}", err); + return; + } + if !ignore_exit_key.load(Ordering::Relaxed) && key == config.exit_key { + return; } } }) @@ -91,8 +89,8 @@ impl Events { Events { rx, - ignore_exit_key, input_handle, + ignore_exit_key, } } diff --git a/src/cache/check.rs b/src/cache/check.rs index d8b5caa..15b129a 100644 --- a/src/cache/check.rs +++ b/src/cache/check.rs @@ -41,17 +41,26 @@ mod format1 { impl MappingChecker { pub fn new(nr_origin_blocks: Option) -> MappingChecker { MappingChecker { - nr_origin_blocks: if let Some(n) = nr_origin_blocks {n} else {MAX_ORIGIN_BLOCKS}, + nr_origin_blocks: if let Some(n) = nr_origin_blocks { + n + } else { + MAX_ORIGIN_BLOCKS + }, seen_oblocks: Mutex::new(BTreeSet::new()), } } fn check_flags(&self, m: &Mapping) -> array::Result<()> { if (m.flags & !(MappingFlags::Valid as u32 | MappingFlags::Dirty as u32)) != 0 { - return Err(array::value_err(format!("unknown flags in mapping: {}", m.flags))); + return Err(array::value_err(format!( + "unknown flags in mapping: {}", + m.flags + ))); } if !m.is_valid() && m.is_dirty() { - return Err(array::value_err(format!("dirty bit found on an unmapped block"))); + return Err(array::value_err( + "dirty bit found on an unmapped block".to_string(), + )); } Ok(()) } @@ -59,16 +68,18 @@ mod format1 { fn check_oblock(&self, m: &Mapping) -> array::Result<()> { if !m.is_valid() { if m.oblock > 0 { - return Err(array::value_err(format!("invalid block is mapped"))); + return Err(array::value_err("invalid block is mapped".to_string())); } return Ok(()); } if m.oblock >= self.nr_origin_blocks { - return Err(array::value_err(format!("mapping beyond end of the origin device"))); + return Err(array::value_err( + "mapping beyond end of the origin device".to_string(), + )); } let mut seen_oblocks = self.seen_oblocks.lock().unwrap(); if seen_oblocks.contains(&m.oblock) { - return Err(array::value_err(format!("origin block already mapped"))); + return Err(array::value_err("origin block already mapped".to_string())); } seen_oblocks.insert(m.oblock); Ok(()) @@ -93,12 +104,8 @@ mod format1 { // FIXME: duplicate to BTreeWalker::build_aggregrate() match errs.len() { 0 => Ok(()), - 1 => { - Err(errs[0].clone()) - } - _ => { - Err(array::aggregate_error(errs)) - } + 1 => Err(errs[0].clone()), + _ => Err(array::aggregate_error(errs)), } } } @@ -120,7 +127,11 @@ mod format2 { impl MappingChecker { pub fn new(nr_origin_blocks: Option, dirty_bits: CheckedBitSet) -> MappingChecker { MappingChecker { - nr_origin_blocks: if let Some(n) = nr_origin_blocks {n} else {MAX_ORIGIN_BLOCKS}, + nr_origin_blocks: if let Some(n) = nr_origin_blocks { + n + } else { + MAX_ORIGIN_BLOCKS + }, inner: Mutex::new(Inner { seen_oblocks: BTreeSet::new(), dirty_bits, @@ -130,10 +141,15 @@ mod format2 { fn check_flags(&self, m: &Mapping, dirty_bit: Option) -> array::Result<()> { if (m.flags & !(MappingFlags::Valid as u32)) != 0 { - return Err(array::value_err(format!("unknown flags in mapping: {}", m.flags))); + return Err(array::value_err(format!( + "unknown flags in mapping: {}", + m.flags + ))); } if !m.is_valid() && dirty_bit.is_some() && dirty_bit.unwrap() { - return Err(array::value_err(format!("dirty bit found on an unmapped block"))); + return Err(array::value_err( + "dirty bit found on an unmapped block".to_string(), + )); } Ok(()) } @@ -141,15 +157,17 @@ mod format2 { fn check_oblock(&self, m: &Mapping, seen_oblocks: &mut BTreeSet) -> array::Result<()> { if !m.is_valid() { if m.oblock > 0 { - return Err(array::value_err(format!("invalid mapped block"))); + return Err(array::value_err("invalid mapped block".to_string())); } return Ok(()); } if m.oblock >= self.nr_origin_blocks { - return Err(array::value_err(format!("mapping beyond end of the origin device"))); + return Err(array::value_err( + "mapping beyond end of the origin device".to_string(), + )); } if seen_oblocks.contains(&m.oblock) { - return Err(array::value_err(format!("origin block already mapped"))); + return Err(array::value_err("origin block already mapped".to_string())); } seen_oblocks.insert(m.oblock); @@ -166,7 +184,8 @@ mod format2 { for i in 0..b.header.nr_entries { let m = b.values[i as usize]; - if let Err(e) = self.check_flags(&m, inner.dirty_bits.contains(begin + i as usize)) { + if let Err(e) = self.check_flags(&m, inner.dirty_bits.contains(begin + i as usize)) + { errs.push(e); } if let Err(e) = self.check_oblock(&m, &mut inner.seen_oblocks) { @@ -177,12 +196,8 @@ mod format2 { // FIXME: duplicate to BTreeWalker::build_aggregrate() match errs.len() { 0 => Ok(()), - 1 => { - Err(errs[0].clone()) - } - _ => { - Err(array::aggregate_error(errs)) - } + 1 => Err(errs[0].clone()), + _ => Err(array::aggregate_error(errs)), } } } @@ -273,7 +288,8 @@ pub fn check(opts: CacheCheckOptions) -> anyhow::Result<()> { // TODO: factor out into check_mappings() if !opts.skip_mappings { - let w = ArrayWalker::new_with_sm(engine.clone(), metadata_sm.clone(), opts.ignore_non_fatal)?; + let w = + ArrayWalker::new_with_sm(engine.clone(), metadata_sm.clone(), opts.ignore_non_fatal)?; match sb.version { 1 => { let mut c = format1::MappingChecker::new(nr_origin_blocks); @@ -307,7 +323,8 @@ 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_with_sm(engine.clone(), metadata_sm.clone(), opts.ignore_non_fatal)?; + let w = + ArrayWalker::new_with_sm(engine.clone(), metadata_sm.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)); @@ -338,11 +355,9 @@ pub fn check(opts: CacheCheckOptions) -> anyhow::Result<()> { opts.ignore_non_fatal, )?; - if opts.auto_repair { - if !metadata_leaks.is_empty() { - ctx.report.info("Repairing metadata leaks."); - repair_space_map(ctx.engine.clone(), metadata_leaks, metadata_sm.clone())?; - } + if opts.auto_repair && !metadata_leaks.is_empty() { + ctx.report.info("Repairing metadata leaks."); + repair_space_map(ctx.engine.clone(), metadata_leaks, metadata_sm.clone())?; } Ok(()) diff --git a/src/cache/dump.rs b/src/cache/dump.rs index 235ef8a..7a88ceb 100644 --- a/src/cache/dump.rs +++ b/src/cache/dump.rs @@ -61,7 +61,10 @@ mod format1 { let mut inner = self.inner.lock().unwrap(); inner.valid_mappings.set(index as usize, true); - inner.visitor.mapping(&m).map_err(|e| array::value_err(format!("{}", e)))?; + inner + .visitor + .mapping(&m) + .map_err(|e| array::value_err(format!("{}", e)))?; } Ok(()) @@ -96,7 +99,7 @@ mod format2 { impl ArrayVisitor for DirtyVisitor { fn visit(&self, index: u64, b: ArrayBlock) -> array::Result<()> { - let mut pos = index as usize * (b.header.max_entries as usize) << 6; + let mut pos = (index as usize * (b.header.max_entries as usize)) << 6; for i in 0..b.header.nr_entries as usize { let bits = b.values[i]; @@ -127,7 +130,11 @@ mod format2 { } impl<'a> MappingEmitter<'a> { - pub fn new(nr_entries: usize, dirty_bits: FixedBitSet, visitor: &'a mut dyn MetadataVisitor) -> MappingEmitter<'a> { + pub fn new( + nr_entries: usize, + dirty_bits: FixedBitSet, + visitor: &'a mut dyn MetadataVisitor, + ) -> MappingEmitter<'a> { MappingEmitter { inner: Mutex::new(Inner { visitor, @@ -161,7 +168,10 @@ mod format2 { }; inner.valid_mappings.set(index as usize, true); - inner.visitor.mapping(&m).map_err(|e| array::value_err(format!("{}", e)))?; + inner + .visitor + .mapping(&m) + .map_err(|e| array::value_err(format!("{}", e)))?; } Ok(()) } @@ -272,7 +282,8 @@ fn dump_metadata(ctx: &Context, sb: &Superblock, _repair: bool) -> anyhow::Resul let dirty_bits = v.get_bits(); let w = ArrayWalker::new(engine.clone(), false); - let mut emitter = format2::MappingEmitter::new(sb.cache_blocks as usize, dirty_bits, &mut out); + let mut emitter = + format2::MappingEmitter::new(sb.cache_blocks as usize, dirty_bits, &mut out); w.walk(&mut emitter, sb.mapping_root)?; emitter.get_valid() } diff --git a/src/cache/hint.rs b/src/cache/hint.rs index 2a9ff9b..b066645 100644 --- a/src/cache/hint.rs +++ b/src/cache/hint.rs @@ -1,6 +1,5 @@ use nom::IResult; use std::convert::TryInto; -use std::marker::PhantomData; use crate::pdata::unpack::*; diff --git a/src/checksum.rs b/src/checksum.rs index 8c39f2c..01d8063 100644 --- a/src/checksum.rs +++ b/src/checksum.rs @@ -17,6 +17,7 @@ fn checksum(buf: &[u8]) -> u32 { } #[derive(Debug, PartialEq)] +#[allow(clippy::upper_case_acronyms)] pub enum BT { SUPERBLOCK, NODE, @@ -59,7 +60,9 @@ pub fn write_checksum(buf: &mut [u8], kind: BT) -> Result<()> { BITMAP => BITMAP_CSUM_XOR, INDEX => INDEX_CSUM_XOR, ARRAY => ARRAY_CSUM_XOR, - UNKNOWN => {return Err(anyhow!("Invalid block type"));} + UNKNOWN => { + return Err(anyhow!("Invalid block type")); + } }; let csum = checksum(buf) ^ salt; diff --git a/src/pdata/array.rs b/src/pdata/array.rs index b2cfbaf..f459d1f 100644 --- a/src/pdata/array.rs +++ b/src/pdata/array.rs @@ -135,15 +135,16 @@ pub fn unpack_array_block(path: &[u64], data: &[u8]) -> Result(path: &[u64], data: &[u8]) -> Result(path: &[u64], data: &[u8]) -> Result BLOCK_SIZE as u32 { return Err(array_block_err( path, - &format!("max_entries is too large ({})", header.max_entries) + &format!("max_entries is too large ({})", header.max_entries), )); } diff --git a/src/pdata/array_walker.rs b/src/pdata/array_walker.rs index f8d029f..de5f43d 100644 --- a/src/pdata/array_walker.rs +++ b/src/pdata/array_walker.rs @@ -38,7 +38,7 @@ impl<'a, V: Unpack + Copy> BlockValueVisitor<'a, V> { BlockValueVisitor { engine: e, array_visitor: v, - sm: sm, + sm, array_errs: Mutex::new(Vec::new()), } } @@ -58,11 +58,13 @@ impl<'a, V: Unpack + Copy> NodeVisitor for BlockValueVisitor<'a, V> { // 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"))); + return Err(btree::value_err("gaps in array indicies".to_string())); } if let Some(end) = kr.end { if *keys.last().unwrap() + 1 != end { - return Err(btree::value_err(format!("gaps or overlaps in array indicies"))); + return Err(btree::value_err( + "gaps or overlaps in array indicies".to_string(), + )); } } @@ -82,7 +84,7 @@ impl<'a, V: Unpack + Copy> NodeVisitor for BlockValueVisitor<'a, V> { Err(_) => { 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()) { @@ -92,13 +94,13 @@ impl<'a, V: Unpack + Copy> NodeVisitor for BlockValueVisitor<'a, V> { } let mut sm = self.sm.lock().unwrap(); sm.inc(b.loc, 1).unwrap(); - }, + } Err(e) => { self.array_errs.lock().unwrap().push(e); } } path.pop(); - }, + } } } } @@ -150,15 +152,11 @@ impl ArrayWalker { where V: Unpack + Copy, { - let w = BTreeWalker::new_with_sm( - self.engine.clone(), - self.sm.clone(), - self.ignore_non_fatal - )?; - let mut path = Vec::new(); - path.push(0); + let w = + BTreeWalker::new_with_sm(self.engine.clone(), self.sm.clone(), self.ignore_non_fatal)?; + let mut path = vec![0]; let v = BlockValueVisitor::::new(self.engine.clone(), self.sm.clone(), visitor); - let btree_err = w.walk(&mut path, &v, root).map_err(|e| ArrayError::BTreeError(e)); + let btree_err = w.walk(&mut path, &v, root).map_err(ArrayError::BTreeError); let mut array_errs = v.array_errs.into_inner().unwrap(); if let Err(e) = btree_err { diff --git a/src/pdata/bitset.rs b/src/pdata/bitset.rs index 82bf633..ef23ea8 100644 --- a/src/pdata/bitset.rs +++ b/src/pdata/bitset.rs @@ -50,11 +50,13 @@ impl BitsetVisitor { impl ArrayVisitor for BitsetVisitor { fn visit(&self, index: u64, b: ArrayBlock) -> array::Result<()> { - let mut begin = index as usize * (b.header.max_entries as usize) << 6; + let mut begin = (index as usize * (b.header.max_entries as usize)) << 6; for i in 0..b.header.nr_entries as usize { if begin > self.nr_entries as usize { - return Err(array::value_err("bitset size exceeds expectation".to_string())); + return Err(array::value_err( + "bitset size exceeds expectation".to_string(), + )); } let end: usize = std::cmp::min(begin + 64, self.nr_entries as usize); @@ -77,7 +79,7 @@ pub fn read_bitset( root: u64, nr_entries: usize, ignore_none_fatal: bool, -)-> (CheckedBitSet, Option) { +) -> (CheckedBitSet, Option) { let w = ArrayWalker::new(engine, ignore_none_fatal); let mut v = BitsetVisitor::new(nr_entries); let err = w.walk(&mut v, root); @@ -85,7 +87,7 @@ pub fn read_bitset( Ok(()) => None, Err(e) => Some(e), }; - return (v.get_bitset(), e); + (v.get_bitset(), e) } // TODO: multi-threaded is possible @@ -95,7 +97,7 @@ pub fn read_bitset_with_sm( nr_entries: usize, sm: Arc>, ignore_none_fatal: bool, -)-> array::Result<(CheckedBitSet, Option)> { +) -> array::Result<(CheckedBitSet, Option)> { let w = ArrayWalker::new_with_sm(engine, sm, ignore_none_fatal)?; let mut v = BitsetVisitor::new(nr_entries); let err = w.walk(&mut v, root); @@ -103,5 +105,5 @@ pub fn read_bitset_with_sm( Ok(()) => None, Err(e) => Some(e), }; - return Ok((v.get_bitset(), e)); + Ok((v.get_bitset(), e)) } diff --git a/src/pdata/btree.rs b/src/pdata/btree.rs index 8f72856..14105a9 100644 --- a/src/pdata/btree.rs +++ b/src/pdata/btree.rs @@ -560,7 +560,10 @@ pub fn unpack_node( for k in &keys { if let Some(l) = last { if k <= l { - return Err(node_err(&path, &format!("keys out of order: {} <= {}", k, l))); + return Err(node_err( + &path, + &format!("keys out of order: {} <= {}", k, l), + )); } } diff --git a/src/pdata/btree_builder.rs b/src/pdata/btree_builder.rs index 93242bc..3fad55e 100644 --- a/src/pdata/btree_builder.rs +++ b/src/pdata/btree_builder.rs @@ -138,7 +138,7 @@ pub struct WriteResult { /// Write a node to a free metadata block. fn write_node_(w: &mut WriteBatcher, mut node: Node) -> Result { let keys = node.get_keys(); - let first_key = keys.first().unwrap_or(&0u64).clone(); + let first_key = *keys.first().unwrap_or(&0u64); let b = w.alloc()?; node.set_block(b.loc); @@ -285,8 +285,8 @@ impl<'a, V: Pack + Unpack + Clone> NodeBuilder { /// Any shared nodes that are used have their block incremented in /// the space map. Will only increment the ref count for values /// contained in the nodes if it unpacks them. - pub fn push_nodes(&mut self, w: &mut WriteBatcher, nodes: &Vec) -> Result<()> { - assert!(nodes.len() > 0); + pub fn push_nodes(&mut self, w: &mut WriteBatcher, nodes: &[NodeSummary]) -> Result<()> { + assert!(!nodes.is_empty()); // As a sanity check we make sure that all the shared nodes contain the // minimum nr of entries. @@ -298,7 +298,7 @@ impl<'a, V: Pack + Unpack + Clone> NodeBuilder { } // Decide if we're going to use the pre-built nodes. - if (self.values.len() > 0) && (self.values.len() < half_full) { + if !self.values.is_empty() && (self.values.len() < half_full) { // To avoid writing an under populated node we have to grab some // values from the first of the shared nodes. let (keys, values) = self.read_node(w, nodes.get(0).unwrap().block)?; @@ -336,7 +336,7 @@ impl<'a, V: Pack + Unpack + Clone> NodeBuilder { pub fn complete(mut self, w: &mut WriteBatcher) -> Result> { let half_full = self.max_entries_per_node / 2; - if (self.values.len() > 0) && (self.values.len() < half_full) && (self.nodes.len() > 0) { + if !self.values.is_empty() && (self.values.len() < half_full) && !self.nodes.is_empty() { // We don't have enough values to emit a node. So we're going to // have to rebalance with the previous node. self.unshift_node(w)?; @@ -344,7 +344,7 @@ impl<'a, V: Pack + Unpack + Clone> NodeBuilder { self.emit_all(w)?; - if self.nodes.len() == 0 { + if self.nodes.is_empty() { self.emit_empty_leaf(w)? } @@ -461,7 +461,7 @@ impl Builder { self.leaf_builder.push_value(w, k, v) } - pub fn push_leaves(&mut self, w: &mut WriteBatcher, leaves: &Vec) -> Result<()> { + pub fn push_leaves(&mut self, w: &mut WriteBatcher, leaves: &[NodeSummary]) -> Result<()> { self.leaf_builder.push_nodes(w, leaves) } diff --git a/src/pdata/btree_walker.rs b/src/pdata/btree_walker.rs index 3de7c85..801f1cd 100644 --- a/src/pdata/btree_walker.rs +++ b/src/pdata/btree_walker.rs @@ -69,10 +69,7 @@ impl BTreeWalker { fn failed(&self, b: u64) -> Option { let fails = self.fails.lock().unwrap(); - match fails.get(&b) { - None => None, - Some(e) => Some(e.clone()), - } + fails.get(&b).cloned() } fn set_fail(&self, b: u64, err: BTreeError) { diff --git a/src/pdata/space_map_checker.rs b/src/pdata/space_map_checker.rs index dc41bc4..9a944a1 100644 --- a/src/pdata/space_map_checker.rs +++ b/src/pdata/space_map_checker.rs @@ -45,8 +45,10 @@ impl<'a> NodeVisitor for OverflowChecker<'a> { let v = values[n]; let expected = self.sm.get(k).unwrap(); if expected != v { - return Err(value_err(format!("Bad reference count for {} block {}. Expected {}, but space map contains {}.", - self.kind, k, expected, v))); + return Err(value_err(format!( + "Bad reference count for {} block {}. Expected {}, but space map contains {}.", + self.kind, k, expected, v + ))); } } @@ -76,10 +78,10 @@ fn inc_entries(sm: &ASpaceMap, entries: &[IndexEntry]) -> Result<()> { // Compare the refernece counts in bitmaps against the expected values // // `sm` - The in-core space map of expected reference counts -fn check_low_ref_counts<'a>( +fn check_low_ref_counts( engine: Arc, report: Arc, - kind: &'a str, + kind: &str, entries: Vec, sm: ASpaceMap, ) -> Result> { @@ -215,7 +217,12 @@ pub fn check_disk_space_map( metadata_sm: ASpaceMap, ignore_non_fatal: bool, ) -> Result> { - let entries = gather_disk_index_entries(engine.clone(), root.bitmap_root, metadata_sm.clone(), ignore_non_fatal)?; + let entries = gather_disk_index_entries( + engine.clone(), + root.bitmap_root, + metadata_sm.clone(), + ignore_non_fatal, + )?; // check overflow ref-counts { @@ -239,8 +246,15 @@ pub fn check_metadata_space_map( metadata_sm: ASpaceMap, ignore_non_fatal: bool, ) -> Result> { - count_btree_blocks::(engine.clone(), &mut vec![0], root.ref_count_root, metadata_sm.clone(), false)?; - let entries = gather_metadata_index_entries(engine.clone(), root.bitmap_root, metadata_sm.clone())?; + count_btree_blocks::( + engine.clone(), + &mut vec![0], + root.ref_count_root, + metadata_sm.clone(), + false, + )?; + let entries = + gather_metadata_index_entries(engine.clone(), root.bitmap_root, metadata_sm.clone())?; // check overflow ref-counts { @@ -259,7 +273,7 @@ pub fn check_metadata_space_map( pub fn repair_space_map( engine: Arc, entries: Vec, - sm: ASpaceMap + sm: ASpaceMap, ) -> Result<()> { let sm = sm.lock().unwrap(); diff --git a/src/pdata/space_map_disk.rs b/src/pdata/space_map_disk.rs index da2301e..2dee628 100644 --- a/src/pdata/space_map_disk.rs +++ b/src/pdata/space_map_disk.rs @@ -371,7 +371,7 @@ pub fn write_metadata_sm(w: &mut WriteBatcher, sm: &dyn SpaceMap) -> Result Result<()> { } let metadata_root = unpack::(&sb.metadata_sm_root[0..])?; - let mut path = Vec::new(); - path.push(0); + let mut path = vec![0]; // Device details. We read this once to get the number of thin devices, and hence the // maximum metadata ref count. Then create metadata space map, and reread to increment @@ -349,7 +347,10 @@ pub struct CheckMaps { pub data_sm: Arc>, } -pub fn check_with_maps(engine: Arc, report: Arc) -> Result { +pub fn check_with_maps( + engine: Arc, + report: Arc, +) -> Result { let ctx = mk_context(engine.clone(), report.clone())?; report.set_title("Checking thin metadata"); @@ -359,18 +360,12 @@ pub fn check_with_maps(engine: Arc, report: Arc(&sb.metadata_sm_root[0..])?; - let mut path = Vec::new(); - path.push(0); + let mut path = vec![0]; // Device details. We read this once to get the number of thin devices, and hence the // maximum metadata ref count. Then create metadata space map, and reread to increment // the ref counts for that metadata. - let devs = btree_to_map::( - &mut path, - engine.clone(), - false, - sb.details_root, - )?; + let devs = btree_to_map::(&mut path, engine.clone(), false, sb.details_root)?; let nr_devs = devs.len(); let metadata_sm = core_sm(engine.get_nr_blocks(), nr_devs as u32); inc_superblock(&metadata_sm)?; diff --git a/src/thin/dump.rs b/src/thin/dump.rs index db89b6e..47e4b32 100644 --- a/src/thin/dump.rs +++ b/src/thin/dump.rs @@ -255,8 +255,7 @@ fn collect_leaves( let mut w = LeafWalker::new(ctx.engine.clone(), sm.deref_mut(), false); let mut v = CollectLeaves::new(); - let mut path = Vec::new(); - path.push(0); + let mut path = vec![0]; // ctx.report.set_title(&format!("collecting {}", *r)); w.walk::(&mut path, &mut v, *r)?; @@ -323,8 +322,7 @@ fn build_metadata(ctx: &Context, sb: &Superblock) -> Result { report.set_title("Reading superblock"); //let metadata_root = unpack::(&sb.metadata_sm_root[0..])?; //let data_root = unpack::(&sb.data_sm_root[0..])?; - let mut path = Vec::new(); - path.push(0); + let mut path = vec![0]; report.set_title("Reading device details"); let details = btree_to_map::(&mut path, engine.clone(), true, sb.details_root)?; @@ -385,6 +383,7 @@ fn build_metadata(ctx: &Context, sb: &Superblock) -> Result { //------------------------------------------ +#[allow(dead_code)] fn gather_entries(g: &mut Gatherer, es: &[Entry]) { g.new_seq(); for e in es { @@ -399,6 +398,7 @@ fn gather_entries(g: &mut Gatherer, es: &[Entry]) { } } +#[allow(dead_code)] fn entries_to_runs(runs: &BTreeMap>, es: &[Entry]) -> Vec { use Entry::*; @@ -427,6 +427,7 @@ fn entries_to_runs(runs: &BTreeMap>, es: &[Entry]) -> Vec { // FIXME: do we really need to track kr? // FIXME: I think this may be better done as part of restore. +#[allow(dead_code)] fn optimise_metadata(md: Metadata) -> Result { use Entry::*; diff --git a/src/thin/restore.rs b/src/thin/restore.rs index e6a0a15..612dd93 100644 --- a/src/thin/restore.rs +++ b/src/thin/restore.rs @@ -89,7 +89,7 @@ impl<'a> Pass1<'a> { if let Some((name, nodes)) = current { Ok((name, nodes.complete(self.w)?)) } else { - let msg = format!("Unbalanced tag"); + let msg = "Unbalanced tag".to_string(); Err(anyhow!(msg)) } } @@ -154,7 +154,7 @@ impl<'a> MetadataVisitor for Pass1<'a> { } Ok(Visit::Continue) } else { - let msg = format!("Mapping tags must appear within a or tag."); + let msg = "Mapping tags must appear within a or tag.".to_string(); Err(anyhow!(msg)) } } diff --git a/src/thin/superblock.rs b/src/thin/superblock.rs index db7074c..f066032 100644 --- a/src/thin/superblock.rs +++ b/src/thin/superblock.rs @@ -110,14 +110,14 @@ fn pack_superblock(sb: &Superblock, w: &mut W) -> Result<()> { } w.write_u64::(sb.block)?; - w.write_all(&vec![0; UUID_SIZE])?; + w.write_all(&[0; UUID_SIZE])?; w.write_u64::(MAGIC)?; w.write_u32::(sb.version)?; w.write_u32::(sb.time)?; w.write_u64::(sb.transaction_id)?; w.write_u64::(sb.metadata_snap)?; - w.write_all(&vec![0; SPACE_MAP_ROOT_SIZE])?; // data sm root - w.write_all(&vec![0; SPACE_MAP_ROOT_SIZE])?; // metadata sm root + w.write_all(&[0; SPACE_MAP_ROOT_SIZE])?; // data sm root + w.write_all(&[0; SPACE_MAP_ROOT_SIZE])?; // metadata sm root w.write_u64::(sb.mapping_root)?; w.write_u64::(sb.details_root)?; w.write_u32::(sb.data_block_size)?; From b7bf82b8f2e70986af6a4de19280ce41560e3602 Mon Sep 17 00:00:00 2001 From: Ming-Hung Tsai Date: Wed, 12 May 2021 02:12:11 +0800 Subject: [PATCH 23/24] [all] Fix newline in version string --- src/bin/cache_check.rs | 2 +- src/bin/cache_dump.rs | 2 +- src/bin/thin_check.rs | 2 +- src/bin/thin_dump.rs | 2 +- src/bin/thin_explore.rs | 2 +- src/bin/thin_metadata_pack.rs | 2 +- src/bin/thin_metadata_unpack.rs | 2 +- src/bin/thin_restore.rs | 2 +- src/bin/thin_shrink.rs | 2 +- src/version.rs | 6 +++++- tests/cache_check.rs | 6 +++--- tests/thin_check.rs | 6 +++--- tests/thin_delta.rs | 6 +++--- tests/thin_metadata_pack.rs | 6 +++--- tests/thin_metadata_unpack.rs | 6 +++--- tests/thin_repair.rs | 6 +++--- tests/thin_restore.rs | 6 +++--- tests/thin_rmap.rs | 6 +++--- 18 files changed, 38 insertions(+), 34 deletions(-) diff --git a/src/bin/cache_check.rs b/src/bin/cache_check.rs index 94355dc..4db7e9b 100644 --- a/src/bin/cache_check.rs +++ b/src/bin/cache_check.rs @@ -13,7 +13,7 @@ use thinp::report::*; fn main() { let parser = App::new("cache_check") - .version(thinp::version::TOOLS_VERSION) + .version(thinp::version::tools_version()) .arg( Arg::with_name("INPUT") .help("Specify the input device to check") diff --git a/src/bin/cache_dump.rs b/src/bin/cache_dump.rs index edf8b4c..3f585bf 100644 --- a/src/bin/cache_dump.rs +++ b/src/bin/cache_dump.rs @@ -9,7 +9,7 @@ use thinp::cache::dump::{dump, CacheDumpOptions}; fn main() { let parser = App::new("cache_dump") - .version(thinp::version::TOOLS_VERSION) + .version(thinp::version::tools_version()) .arg( Arg::with_name("INPUT") .help("Specify the input device to check") diff --git a/src/bin/thin_check.rs b/src/bin/thin_check.rs index 307b439..1c1ea27 100644 --- a/src/bin/thin_check.rs +++ b/src/bin/thin_check.rs @@ -14,7 +14,7 @@ use thinp::thin::check::{check, ThinCheckOptions, MAX_CONCURRENT_IO}; fn main() { let parser = App::new("thin_check") - .version(thinp::version::TOOLS_VERSION) + .version(thinp::version::tools_version()) .about("Validates thin provisioning metadata on a device or file.") .arg( Arg::with_name("QUIET") diff --git a/src/bin/thin_dump.rs b/src/bin/thin_dump.rs index 3954249..3008e35 100644 --- a/src/bin/thin_dump.rs +++ b/src/bin/thin_dump.rs @@ -13,7 +13,7 @@ use thinp::thin::dump::{dump, ThinDumpOptions}; fn main() { let parser = App::new("thin_check") - .version(thinp::version::TOOLS_VERSION) + .version(thinp::version::tools_version()) .about("Validates thin provisioning metadata on a device or file.") .arg( Arg::with_name("QUIET") diff --git a/src/bin/thin_explore.rs b/src/bin/thin_explore.rs index c4918fd..aa2b54d 100644 --- a/src/bin/thin_explore.rs +++ b/src/bin/thin_explore.rs @@ -847,7 +847,7 @@ fn explore(path: &Path, node_path: Option>) -> Result<()> { fn main() -> Result<()> { let parser = App::new("thin_explore") - .version(thinp::version::TOOLS_VERSION) + .version(thinp::version::tools_version()) .about("A text user interface for examining thin metadata.") .arg( Arg::with_name("NODE_PATH") diff --git a/src/bin/thin_metadata_pack.rs b/src/bin/thin_metadata_pack.rs index d4b731f..e29678a 100644 --- a/src/bin/thin_metadata_pack.rs +++ b/src/bin/thin_metadata_pack.rs @@ -8,7 +8,7 @@ use thinp::file_utils; fn main() { let parser = App::new("thin_metadata_pack") - .version(thinp::version::TOOLS_VERSION) + .version(thinp::version::tools_version()) .about("Produces a compressed file of thin metadata. Only packs metadata blocks that are actually used.") .arg(Arg::with_name("INPUT") .help("Specify thinp metadata binary device/file") diff --git a/src/bin/thin_metadata_unpack.rs b/src/bin/thin_metadata_unpack.rs index ca6403f..0664557 100644 --- a/src/bin/thin_metadata_unpack.rs +++ b/src/bin/thin_metadata_unpack.rs @@ -10,7 +10,7 @@ use std::process::exit; fn main() { let parser = App::new("thin_metadata_unpack") - .version(thinp::version::TOOLS_VERSION) + .version(thinp::version::tools_version()) .about("Unpack a compressed file of thin metadata.") .arg( Arg::with_name("INPUT") diff --git a/src/bin/thin_restore.rs b/src/bin/thin_restore.rs index 93f62e9..243f3a4 100644 --- a/src/bin/thin_restore.rs +++ b/src/bin/thin_restore.rs @@ -13,7 +13,7 @@ use thinp::thin::restore::{restore, ThinRestoreOptions}; fn main() { let parser = App::new("thin_restore") - .version(thinp::version::TOOLS_VERSION) + .version(thinp::version::tools_version()) .about("Convert XML format metadata to binary.") .arg( Arg::with_name("OVERRIDE_MAPPING_ROOT") diff --git a/src/bin/thin_shrink.rs b/src/bin/thin_shrink.rs index b748b30..0e27f79 100644 --- a/src/bin/thin_shrink.rs +++ b/src/bin/thin_shrink.rs @@ -12,7 +12,7 @@ use thinp::file_utils; fn main() { let parser = App::new("thin_shrink") - .version(thinp::version::TOOLS_VERSION) + .version(thinp::version::tools_version()) .about("Rewrite xml metadata and move data in an inactive pool.") .arg( Arg::with_name("INPUT") diff --git a/src/version.rs b/src/version.rs index fe73777..19ea2d5 100644 --- a/src/version.rs +++ b/src/version.rs @@ -1 +1,5 @@ -pub const TOOLS_VERSION: &str = include_str!("../VERSION"); +const TOOLS_VERSION: &str = include_str!("../VERSION"); + +pub fn tools_version() -> &'static str { + TOOLS_VERSION.trim_end() +} diff --git a/tests/cache_check.rs b/tests/cache_check.rs index 8d92e5d..41c81bb 100644 --- a/tests/cache_check.rs +++ b/tests/cache_check.rs @@ -1,6 +1,6 @@ use anyhow::Result; use duct::cmd; -use thinp::version::TOOLS_VERSION; +use thinp::version::tools_version; mod common; @@ -12,14 +12,14 @@ use common::*; #[test] fn accepts_v() -> Result<()> { let stdout = cache_check!("-V").read()?; - assert_eq!(stdout, TOOLS_VERSION); + assert_eq!(stdout, tools_version()); Ok(()) } #[test] fn accepts_version() -> Result<()> { let stdout = cache_check!("--version").read()?; - assert_eq!(stdout, TOOLS_VERSION); + assert_eq!(stdout, tools_version()); Ok(()) } diff --git a/tests/thin_check.rs b/tests/thin_check.rs index c31b31b..498612f 100644 --- a/tests/thin_check.rs +++ b/tests/thin_check.rs @@ -1,6 +1,6 @@ use anyhow::Result; use thinp::file_utils; -use thinp::version::TOOLS_VERSION; +use thinp::version::tools_version; mod common; @@ -13,14 +13,14 @@ use common::*; #[test] fn accepts_v() -> Result<()> { let stdout = thin_check!("-V").read()?; - assert_eq!(stdout, TOOLS_VERSION); + assert_eq!(stdout, tools_version()); Ok(()) } #[test] fn accepts_version() -> Result<()> { let stdout = thin_check!("--version").read()?; - assert_eq!(stdout, TOOLS_VERSION); + assert_eq!(stdout, tools_version()); Ok(()) } diff --git a/tests/thin_delta.rs b/tests/thin_delta.rs index 458a307..337ac57 100644 --- a/tests/thin_delta.rs +++ b/tests/thin_delta.rs @@ -1,5 +1,5 @@ use anyhow::Result; -use thinp::version::TOOLS_VERSION; +use thinp::version::tools_version; mod common; use common::test_dir::*; @@ -10,14 +10,14 @@ use common::*; #[test] fn accepts_v() -> Result<()> { let stdout = thin_delta!("-V").read()?; - assert_eq!(stdout, TOOLS_VERSION); + assert_eq!(stdout, tools_version()); Ok(()) } #[test] fn accepts_version() -> Result<()> { let stdout = thin_delta!("--version").read()?; - assert_eq!(stdout, TOOLS_VERSION); + assert_eq!(stdout, tools_version()); Ok(()) } diff --git a/tests/thin_metadata_pack.rs b/tests/thin_metadata_pack.rs index 131cc95..84e3187 100644 --- a/tests/thin_metadata_pack.rs +++ b/tests/thin_metadata_pack.rs @@ -1,5 +1,5 @@ use anyhow::Result; -use thinp::version::TOOLS_VERSION; +use thinp::version::tools_version; mod common; use common::test_dir::*; @@ -10,14 +10,14 @@ use common::*; #[test] fn accepts_v() -> Result<()> { let stdout = thin_metadata_pack!("-V").read()?; - assert!(stdout.contains(TOOLS_VERSION)); + assert!(stdout.contains(tools_version())); Ok(()) } #[test] fn accepts_version() -> Result<()> { let stdout = thin_metadata_pack!("--version").read()?; - assert!(stdout.contains(TOOLS_VERSION)); + assert!(stdout.contains(tools_version())); Ok(()) } diff --git a/tests/thin_metadata_unpack.rs b/tests/thin_metadata_unpack.rs index 33d3f0c..7a5d5ee 100644 --- a/tests/thin_metadata_unpack.rs +++ b/tests/thin_metadata_unpack.rs @@ -1,5 +1,5 @@ use anyhow::Result; -use thinp::version::TOOLS_VERSION; +use thinp::version::tools_version; mod common; use common::test_dir::*; @@ -10,14 +10,14 @@ use common::*; #[test] fn accepts_v() -> Result<()> { let stdout = thin_metadata_unpack!("-V").read()?; - assert!(stdout.contains(TOOLS_VERSION)); + assert!(stdout.contains(tools_version())); Ok(()) } #[test] fn accepts_version() -> Result<()> { let stdout = thin_metadata_unpack!("--version").read()?; - assert!(stdout.contains(TOOLS_VERSION)); + assert!(stdout.contains(tools_version())); Ok(()) } diff --git a/tests/thin_repair.rs b/tests/thin_repair.rs index 19f7969..bc836d6 100644 --- a/tests/thin_repair.rs +++ b/tests/thin_repair.rs @@ -1,6 +1,6 @@ use anyhow::Result; use std::str::from_utf8; -use thinp::version::TOOLS_VERSION; +use thinp::version::tools_version; mod common; use common::test_dir::*; @@ -11,14 +11,14 @@ use common::*; #[test] fn accepts_v() -> Result<()> { let stdout = thin_repair!("-V").read()?; - assert_eq!(stdout, TOOLS_VERSION); + assert_eq!(stdout, tools_version()); Ok(()) } #[test] fn accepts_version() -> Result<()> { let stdout = thin_repair!("--version").read()?; - assert_eq!(stdout, TOOLS_VERSION); + assert_eq!(stdout, tools_version()); Ok(()) } diff --git a/tests/thin_restore.rs b/tests/thin_restore.rs index c8efa45..d2b85d0 100644 --- a/tests/thin_restore.rs +++ b/tests/thin_restore.rs @@ -1,7 +1,7 @@ use anyhow::Result; use std::str::from_utf8; use thinp::file_utils; -use thinp::version::TOOLS_VERSION; +use thinp::version::tools_version; mod common; use common::test_dir::*; @@ -12,14 +12,14 @@ use common::*; #[test] fn accepts_v() -> Result<()> { let stdout = thin_restore!("-V").read()?; - assert_eq!(stdout, TOOLS_VERSION); + assert_eq!(stdout, tools_version()); Ok(()) } #[test] fn accepts_version() -> Result<()> { let stdout = thin_restore!("--version").read()?; - assert_eq!(stdout, TOOLS_VERSION); + assert_eq!(stdout, tools_version()); Ok(()) } diff --git a/tests/thin_rmap.rs b/tests/thin_rmap.rs index 3797edf..7b2eaba 100644 --- a/tests/thin_rmap.rs +++ b/tests/thin_rmap.rs @@ -1,5 +1,5 @@ use anyhow::Result; -use thinp::version::TOOLS_VERSION; +use thinp::version::tools_version; mod common; use common::test_dir::*; @@ -10,14 +10,14 @@ use common::*; #[test] fn accepts_v() -> Result<()> { let stdout = thin_rmap!("-V").read()?; - assert_eq!(stdout, TOOLS_VERSION); + assert_eq!(stdout, tools_version()); Ok(()) } #[test] fn accepts_version() -> Result<()> { let stdout = thin_rmap!("--version").read()?; - assert_eq!(stdout, TOOLS_VERSION); + assert_eq!(stdout, tools_version()); Ok(()) } From 1bbb63f06ba2a0d64cb0b81445ef0a7959b5dbe9 Mon Sep 17 00:00:00 2001 From: Ming-Hung Tsai Date: Wed, 12 May 2021 15:50:14 +0800 Subject: [PATCH 24/24] [cache_check (rust)] Fix discard bitset size checking --- src/cache/check.rs | 2 +- src/pdata/bitset.rs | 30 +++++++++++++++--------------- 2 files changed, 16 insertions(+), 16 deletions(-) diff --git a/src/cache/check.rs b/src/cache/check.rs index 15b129a..26cda61 100644 --- a/src/cache/check.rs +++ b/src/cache/check.rs @@ -337,7 +337,7 @@ pub fn check(opts: CacheCheckOptions) -> anyhow::Result<()> { let (_discard_bits, err) = read_bitset_with_sm( engine.clone(), sb.discard_root, - sb.cache_blocks as usize, + sb.discard_nr_blocks as usize, metadata_sm.clone(), opts.ignore_non_fatal, )?; diff --git a/src/pdata/bitset.rs b/src/pdata/bitset.rs index ef23ea8..2586e74 100644 --- a/src/pdata/bitset.rs +++ b/src/pdata/bitset.rs @@ -31,15 +31,15 @@ impl CheckedBitSet { } struct BitsetVisitor { - nr_entries: usize, + nr_bits: usize, bits: Mutex, } impl BitsetVisitor { - pub fn new(nr_entries: usize) -> Self { + pub fn new(nr_bits: usize) -> Self { BitsetVisitor { - nr_entries, - bits: Mutex::new(CheckedBitSet::with_capacity(nr_entries)), + nr_bits, + bits: Mutex::new(CheckedBitSet::with_capacity(nr_bits)), } } @@ -51,15 +51,15 @@ impl BitsetVisitor { impl ArrayVisitor for BitsetVisitor { fn visit(&self, index: u64, b: ArrayBlock) -> array::Result<()> { let mut begin = (index as usize * (b.header.max_entries as usize)) << 6; + if begin >= self.nr_bits as usize { + return Err(array::value_err(format!( + "bitset size exceeds limit: {} bits", + self.nr_bits + ))); + } for i in 0..b.header.nr_entries as usize { - if begin > self.nr_entries as usize { - return Err(array::value_err( - "bitset size exceeds expectation".to_string(), - )); - } - - let end: usize = std::cmp::min(begin + 64, self.nr_entries as usize); + let end: usize = std::cmp::min(begin + 64, self.nr_bits as usize); let mut mask = 1; let bits = b.values[i]; @@ -77,11 +77,11 @@ impl ArrayVisitor for BitsetVisitor { pub fn read_bitset( engine: Arc, root: u64, - nr_entries: usize, + nr_bits: usize, ignore_none_fatal: bool, ) -> (CheckedBitSet, Option) { let w = ArrayWalker::new(engine, ignore_none_fatal); - let mut v = BitsetVisitor::new(nr_entries); + let mut v = BitsetVisitor::new(nr_bits); let err = w.walk(&mut v, root); let e = match err { Ok(()) => None, @@ -94,12 +94,12 @@ pub fn read_bitset( pub fn read_bitset_with_sm( engine: Arc, root: u64, - nr_entries: usize, + nr_bits: usize, sm: Arc>, ignore_none_fatal: bool, ) -> array::Result<(CheckedBitSet, Option)> { let w = ArrayWalker::new_with_sm(engine, sm, ignore_none_fatal)?; - let mut v = BitsetVisitor::new(nr_entries); + let mut v = BitsetVisitor::new(nr_bits); let err = w.walk(&mut v, root); let e = match err { Ok(()) => None,