Code review of cache_dump/check

Added support for metadata format2 to cache_dump.  Nothing tested.

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"

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<Mapping> for CheckMappingVisitor {
+impl ArrayVisitor<Mapping> for CheckMappingVisitor {
     fn visit(&self, _index: u64, m: Mapping) -> anyhow::Result<()> {
         if !m.is_valid() {
             return Ok(());
@@ -72,20 +71,16 @@ impl ArrayBlockVisitor<Mapping> for CheckMappingVisitor {
 
 //------------------------------------------
 
-struct CheckHintVisitor<Width> {
-    _not_used: PhantomData<Width>,
-}
+struct CheckHintVisitor {}
 
-impl<Width> CheckHintVisitor<Width> {
-    fn new() -> CheckHintVisitor<Width> {
-        CheckHintVisitor {
-            _not_used: PhantomData,
-        }
+impl CheckHintVisitor {
+    fn new() -> CheckHintVisitor {
+        CheckHintVisitor {}
     }
 }
 
-impl<Width: typenum::Unsigned> ArrayBlockVisitor<Hint<Width>> for CheckHintVisitor<Width> {
-    fn visit(&self, _index: u64, _hint: Hint<Width>) -> anyhow::Result<()> {
+impl ArrayVisitor<Hint> 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<Context> {
     let engine: Arc<dyn IoEngine + Send + Sync>;
 
     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::<Width>::new());
-        w.walk(c, sb.hint_root)?;
+        let mut c = CheckHintVisitor::new();
+        w.walk(&mut c, sb.hint_root)?;
     }
 
     if !opts.skip_discards {

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<Mutex<dyn MetadataVisitor>>,
-}
+mod format1 {
+    use super::*;
 
-impl MappingEmitter {
-    pub fn new(emitter: Arc<Mutex<dyn MetadataVisitor>>) -> MappingEmitter {
-        MappingEmitter {
-            emitter,
+    struct Inner<'a> {
+        visitor: &'a mut dyn MetadataVisitor,
+        valid_mappings: FixedBitSet,
+    }
+
+    pub struct MappingEmitter<'a> {
+        inner: Mutex<Inner<'a>>,
+    }
+
+    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<Mapping> 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<Mapping> 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<FixedBitSet>,
+    }
+
+    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<u64> 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<Inner<'a>>,
+    }
+
+    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<Mapping> 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<Width> {
-    emitter: Arc<Mutex<dyn MetadataVisitor>>,
-    _not_used: PhantomData<Width>,
+struct HintEmitter<'a> {
+    emitter: Mutex<&'a mut dyn MetadataVisitor>,
+    valid_mappings: FixedBitSet,
 }
 
-impl<Width> HintEmitter<Width> {
-    pub fn new(emitter: Arc<Mutex<dyn MetadataVisitor>>) -> HintEmitter<Width> {
+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<Width: typenum::Unsigned> ArrayBlockVisitor<Hint<Width>> for HintEmitter<Width> {
-    fn visit(&self, index: u64, hint: Hint<Width>) -> anyhow::Result<()> {
-        // TODO: skip invalid blocks
+impl<'a> ArrayVisitor<Hint> 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<dyn IoEngine + Send + Sync>,
 }
@@ -97,25 +198,19 @@ fn mk_context(opts: &CacheDumpOptions) -> anyhow::Result<Context> {
     let engine: Arc<dyn IoEngine + Send + Sync>;
 
     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::<Width>::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(())
 }

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<Width> {
-    pub hint: [u8; 4], // FIXME: support various hint sizes
-    _not_used: PhantomData<Width>,
+pub struct Hint {
+    pub hint: [u8; 4],
 }
 
-impl<Width: typenum::Unsigned> Unpack for Hint<Width> {
+impl Unpack for Hint {
     fn disk_size() -> u32 {
-        Width::to_u32()
+        4
     }
 
-    // FIXME: support different width
-    fn unpack(i: &[u8]) -> IResult<&[u8], Hint<Width>> {
-        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,
             },
         ))
     }

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

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};
 
 //------------------------------------------
 

src/pdata/array_walker.rs

@@ -14,20 +14,20 @@ pub struct ArrayWalker {
 }
 
 // FIXME: define another Result type for array visiting?
-pub trait ArrayBlockVisitor<V: Unpack> {
+pub trait ArrayVisitor<V: Unpack> {
     fn visit(&self, index: u64, v: V) -> anyhow::Result<()>;
 }
 
-struct BlockValueVisitor<V> {
+struct BlockValueVisitor<'a, V> {
     engine: Arc<dyn IoEngine + Send + Sync>,
-    array_block_visitor: Box<dyn ArrayBlockVisitor<V>>,
+    array_block_visitor: &'a mut dyn ArrayVisitor<V>,
 }
 
-impl<V: Unpack + Copy> BlockValueVisitor<V> {
+impl<'a, V: Unpack + Copy> BlockValueVisitor<'a, V> {
     pub fn new(
         e: Arc<dyn IoEngine + Send + Sync>,
-        v: Box<dyn ArrayBlockVisitor<V>>,
-    ) -> BlockValueVisitor<V> {
+        v: &'a mut dyn ArrayVisitor<V>,
+    ) -> BlockValueVisitor<'a, V> {
         BlockValueVisitor {
             engine: e,
             array_block_visitor: v,
@@ -44,7 +44,7 @@ impl<V: Unpack + Copy> BlockValueVisitor<V> {
     }
 }
 
-impl<V: Unpack + Copy> NodeVisitor<u64> for BlockValueVisitor<V> {
+impl<'a, V: Unpack + Copy> NodeVisitor<u64> 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<V>(&self, visitor: Box<dyn ArrayBlockVisitor<V>>, root: u64) -> Result<()>
+    pub fn walk<V>(&self, visitor: &mut dyn ArrayVisitor<V>, root: u64) -> Result<()>
     where
         V: Unpack + Copy,
     {