Merge pull request #183 from mingnus/2021-07-08-thin-repair

First pass at thin_repair and cache_repair

src/bin/cache_repair.rs

@@ -0,0 +1,79 @@
+extern crate clap;
+extern crate thinp;
+
+use atty::Stream;
+use clap::{App, Arg};
+use std::path::Path;
+use std::process;
+use std::process::exit;
+use std::sync::Arc;
+use thinp::cache::repair::{repair, CacheRepairOptions};
+use thinp::file_utils;
+use thinp::report::*;
+
+fn main() {
+    let parser = App::new("cache_repair")
+        .version(thinp::version::tools_version())
+        .about("Repair binary cache metadata, and write it to a different device or file")
+        // flags
+        .arg(
+            Arg::with_name("ASYNC_IO")
+                .help("Force use of io_uring for synchronous io")
+                .long("async-io")
+                .hidden(true),
+        )
+        .arg(
+            Arg::with_name("QUIET")
+                .help("Suppress output messages, return only exit code.")
+                .short("q")
+                .long("quiet"),
+        )
+        // options
+        .arg(
+            Arg::with_name("INPUT")
+                .help("Specify the input device")
+                .short("i")
+                .long("input")
+                .value_name("INPUT")
+                .required(true),
+        )
+        .arg(
+            Arg::with_name("OUTPUT")
+                .help("Specify the output device")
+                .short("o")
+                .long("output")
+                .value_name("OUTPUT")
+                .required(true),
+        );
+
+    let matches = parser.get_matches();
+    let input_file = Path::new(matches.value_of("INPUT").unwrap());
+    let output_file = Path::new(matches.value_of("OUTPUT").unwrap());
+
+    if !file_utils::file_exists(input_file) {
+        eprintln!("Couldn't find input file '{:?}'.", &input_file);
+        exit(1);
+    }
+
+    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 = CacheRepairOptions {
+        input: &input_file,
+        output: &output_file,
+        async_io: matches.is_present("ASYNC_IO"),
+        report,
+    };
+
+    if let Err(reason) = repair(opts) {
+        eprintln!("{}", reason);
+        process::exit(1);
+    }
+}

src/bin/cache_restore.rs

@@ -29,6 +29,12 @@ fn main() {
                 .value_name("OVERRIDE_MAPPING_ROOT")
                 .takes_value(true),
         )
+        .arg(
+            Arg::with_name("QUIET")
+                .help("Suppress output messages, return only exit code.")
+                .short("q")
+                .long("quiet"),
+        )
         // options
         .arg(
             Arg::with_name("INPUT")

src/bin/thin_repair.rs

@@ -0,0 +1,86 @@
+extern crate clap;
+extern crate thinp;
+
+use atty::Stream;
+use clap::{App, Arg};
+use std::path::Path;
+use std::process;
+use std::process::exit;
+use std::sync::Arc;
+use thinp::file_utils;
+use thinp::report::*;
+use thinp::thin::repair::{repair, ThinRepairOptions};
+
+fn main() {
+    let parser = App::new("thin_repair")
+        .version(thinp::version::tools_version())
+        .about("Repair thin-provisioning metadata, and write it to different device or file")
+        // flags
+        .arg(
+            Arg::with_name("ASYNC_IO")
+                .help("Force use of io_uring for synchronous io")
+                .long("async-io")
+                .hidden(true),
+        )
+        .arg(
+            Arg::with_name("QUIET")
+                .help("Suppress output messages, return only exit code.")
+                .short("q")
+                .long("quiet"),
+        )
+        // options
+        .arg(
+            Arg::with_name("INPUT")
+                .help("Specify the input device")
+                .short("i")
+                .long("input")
+                .value_name("INPUT")
+                .required(true),
+        )
+        .arg(
+            Arg::with_name("OUTPUT")
+                .help("Specify the output device")
+                .short("o")
+                .long("output")
+                .value_name("OUTPUT")
+                .required(true),
+        )
+        .arg(
+            Arg::with_name("OVERRIDE_MAPPING_ROOT")
+                .help("Specify a mapping root to use")
+                .long("override-mapping-root")
+                .value_name("OVERRIDE_MAPPING_ROOT")
+                .takes_value(true),
+        );
+
+    let matches = parser.get_matches();
+    let input_file = Path::new(matches.value_of("INPUT").unwrap());
+    let output_file = Path::new(matches.value_of("OUTPUT").unwrap());
+
+    if !file_utils::file_exists(input_file) {
+        eprintln!("Couldn't find input file '{:?}'.", &input_file);
+        exit(1);
+    }
+
+    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 = ThinRepairOptions {
+        input: &input_file,
+        output: &output_file,
+        async_io: matches.is_present("ASYNC_IO"),
+        report,
+    };
+
+    if let Err(reason) = repair(opts) {
+        eprintln!("{}", reason);
+        process::exit(1);
+    }
+}

src/bin/thin_restore.rs

@@ -22,6 +22,12 @@ fn main() {
                 .long("async-io")
                 .hidden(true),
         )
+        .arg(
+            Arg::with_name("QUIET")
+                .help("Suppress output messages, return only exit code.")
+                .short("q")
+                .long("quiet"),
+        )
         // options
         .arg(
             Arg::with_name("INPUT")
@@ -33,7 +39,7 @@ fn main() {
         )
         .arg(
             Arg::with_name("OUTPUT")
-                .help("Specify the output device to check")
+                .help("Specify the output device")
                 .short("o")
                 .long("output")
                 .value_name("OUTPUT")

src/cache/dump.rs

@@ -7,9 +7,10 @@ use std::path::Path;
 use std::sync::{Arc, Mutex};
 
 use crate::cache::hint::Hint;
+use crate::cache::ir::{self, MetadataVisitor};
 use crate::cache::mapping::Mapping;
 use crate::cache::superblock::*;
-use crate::cache::xml::{self, MetadataVisitor};
+use crate::cache::xml;
 use crate::io_engine::{AsyncIoEngine, IoEngine, SyncIoEngine};
 use crate::pdata::array::{self, ArrayBlock};
 use crate::pdata::array_walker::*;
@@ -58,7 +59,7 @@ mod format1 {
                     continue;
                 }
 
-                let m = xml::Map {
+                let m = ir::Map {
                     cblock,
                     oblock: map.oblock,
                     dirty: map.is_dirty(),
@@ -133,7 +134,7 @@ mod format2 {
                     // default to dirty if the bitset is damaged
                     dirty = true;
                 }
-                let m = xml::Map {
+                let m = ir::Map {
                     cblock,
                     oblock: map.oblock,
                     dirty,
@@ -175,7 +176,7 @@ impl<'a> ArrayVisitor<Hint> for HintEmitter<'a> {
                 continue;
             }
 
-            let h = xml::Hint {
+            let h = ir::Hint {
                 cblock,
                 data: hint.hint.to_vec(),
             };
@@ -217,16 +218,13 @@ fn mk_context(opts: &CacheDumpOptions) -> anyhow::Result<Context> {
     Ok(Context { engine })
 }
 
-fn dump_metadata(
+pub fn dump_metadata(
-    ctx: &Context,
+    engine: Arc<dyn IoEngine + Send + Sync>,
-    w: &mut dyn Write,
+    out: &mut dyn MetadataVisitor,
     sb: &Superblock,
     _repair: bool,
 ) -> anyhow::Result<()> {
-    let engine = &ctx.engine;
+    let xml_sb = ir::Superblock {
-
-    let mut out = xml::XmlWriter::new(w);
-    let xml_sb = xml::Superblock {
         uuid: "".to_string(),
         block_size: sb.data_block_size,
         nr_cache_blocks: sb.cache_blocks,
@@ -239,7 +237,7 @@ fn dump_metadata(
     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);
+            let mut emitter = format1::MappingEmitter::new(sb.cache_blocks as usize, out);
             w.walk(&mut emitter, sb.mapping_root)?;
             emitter.get_valid()
         }
@@ -262,7 +260,7 @@ fn dump_metadata(
 
             let w = ArrayWalker::new(engine.clone(), false);
             let mut emitter =
-                format2::MappingEmitter::new(sb.cache_blocks as usize, dirty_bits, &mut out);
+                format2::MappingEmitter::new(sb.cache_blocks as usize, dirty_bits, out);
             w.walk(&mut emitter, sb.mapping_root)?;
             emitter.get_valid()
         }
@@ -275,7 +273,7 @@ fn dump_metadata(
     out.hints_b()?;
     {
         let w = ArrayWalker::new(engine.clone(), false);
-        let mut emitter = HintEmitter::new(&mut out, valid_mappings);
+        let mut emitter = HintEmitter::new(out, valid_mappings);
         w.walk(&mut emitter, sb.hint_root)?;
     }
     out.hints_e()?;
@@ -288,17 +286,17 @@ fn dump_metadata(
 
 pub fn dump(opts: CacheDumpOptions) -> anyhow::Result<()> {
     let ctx = mk_context(&opts)?;
-    let engine = &ctx.engine;
+    let sb = read_superblock(ctx.engine.as_ref(), SUPERBLOCK_LOCATION)?;
-    let sb = read_superblock(engine.as_ref(), SUPERBLOCK_LOCATION)?;
 
-    let mut writer: Box<dyn Write>;
+    let writer: Box<dyn Write>;
     if opts.output.is_some() {
         writer = Box::new(BufWriter::new(File::create(opts.output.unwrap())?));
     } else {
         writer = Box::new(BufWriter::new(std::io::stdout()));
     }
+    let mut out = xml::XmlWriter::new(writer);
 
-    dump_metadata(&ctx, &mut writer, &sb, opts.repair)
+    dump_metadata(ctx.engine.clone(), &mut out, &sb, opts.repair)
 }
 
 //------------------------------------------

src/cache/ir.rs

@@ -0,0 +1,60 @@
+use anyhow::Result;
+
+//------------------------------------------
+
+#[derive(Clone)]
+pub struct Superblock {
+    pub uuid: String,
+    pub block_size: u32,
+    pub nr_cache_blocks: u32,
+    pub policy: String,
+    pub hint_width: u32,
+}
+
+#[derive(Clone)]
+pub struct Map {
+    pub cblock: u32,
+    pub oblock: u64,
+    pub dirty: bool,
+}
+
+#[derive(Clone)]
+pub struct Hint {
+    pub cblock: u32,
+    pub data: Vec<u8>,
+}
+
+#[derive(Clone)]
+pub struct Discard {
+    pub begin: u64,
+    pub end: u64,
+}
+
+//------------------------------------------
+
+#[derive(Clone)]
+pub enum Visit {
+    Continue,
+    Stop,
+}
+
+pub trait MetadataVisitor {
+    fn superblock_b(&mut self, sb: &Superblock) -> Result<Visit>;
+    fn superblock_e(&mut self) -> Result<Visit>;
+
+    fn mappings_b(&mut self) -> Result<Visit>;
+    fn mappings_e(&mut self) -> Result<Visit>;
+    fn mapping(&mut self, m: &Map) -> Result<Visit>;
+
+    fn hints_b(&mut self) -> Result<Visit>;
+    fn hints_e(&mut self) -> Result<Visit>;
+    fn hint(&mut self, h: &Hint) -> Result<Visit>;
+
+    fn discards_b(&mut self) -> Result<Visit>;
+    fn discards_e(&mut self) -> Result<Visit>;
+    fn discard(&mut self, d: &Discard) -> Result<Visit>;
+
+    fn eof(&mut self) -> Result<Visit>;
+}
+
+//------------------------------------------

src/cache/mod.rs

@@ -1,7 +1,9 @@
 pub mod check;
 pub mod dump;
 pub mod hint;
+pub mod ir;
 pub mod mapping;
+pub mod repair;
 pub mod restore;
 pub mod superblock;
 pub mod xml;

src/cache/repair.rs

@@ -0,0 +1,68 @@
+use anyhow::Result;
+use std::path::Path;
+use std::sync::Arc;
+
+use crate::cache::dump::*;
+use crate::cache::restore::*;
+use crate::cache::superblock::*;
+use crate::io_engine::*;
+use crate::pdata::space_map::*;
+use crate::report::*;
+use crate::write_batcher::*;
+
+//------------------------------------------
+
+pub struct CacheRepairOptions<'a> {
+    pub input: &'a Path,
+    pub output: &'a Path,
+    pub async_io: bool,
+    pub report: Arc<Report>,
+}
+
+struct Context {
+    _report: Arc<Report>,
+    engine_in: Arc<dyn IoEngine + Send + Sync>,
+    engine_out: Arc<dyn IoEngine + Send + Sync>,
+}
+
+const MAX_CONCURRENT_IO: u32 = 1024;
+
+fn new_context(opts: &CacheRepairOptions) -> Result<Context> {
+    let engine_in: Arc<dyn IoEngine + Send + Sync>;
+    let engine_out: Arc<dyn IoEngine + Send + Sync>;
+
+    if opts.async_io {
+        engine_in = Arc::new(AsyncIoEngine::new(opts.input, MAX_CONCURRENT_IO, true)?);
+        engine_out = Arc::new(AsyncIoEngine::new(opts.output, MAX_CONCURRENT_IO, true)?);
+    } else {
+        let nr_threads = std::cmp::max(8, num_cpus::get() * 2);
+        engine_in = Arc::new(SyncIoEngine::new(opts.input, nr_threads, true)?);
+        engine_out = Arc::new(SyncIoEngine::new(opts.output, nr_threads, true)?);
+    }
+
+    Ok(Context {
+        _report: opts.report.clone(),
+        engine_in,
+        engine_out,
+    })
+}
+
+//------------------------------------------
+
+pub fn repair(opts: CacheRepairOptions) -> Result<()> {
+    let ctx = new_context(&opts)?;
+
+    let sb = read_superblock(ctx.engine_in.as_ref(), SUPERBLOCK_LOCATION)?;
+
+    let sm = core_sm(ctx.engine_out.get_nr_blocks(), u32::MAX);
+    let mut w = WriteBatcher::new(
+        ctx.engine_out.clone(),
+        sm.clone(),
+        ctx.engine_out.get_batch_size(),
+    );
+    let mut restorer = Restorer::new(&mut w);
+
+    dump_metadata(ctx.engine_in, &mut restorer, &sb, true)
+}
+
+//------------------------------------------

src/cache/restore.rs

@@ -1,4 +1,4 @@
-use anyhow::{anyhow, Result};
+use anyhow::Result;
 
 use std::convert::TryInto;
 use std::fs::OpenOptions;
@@ -7,9 +7,10 @@ use std::path::Path;
 use std::sync::Arc;
 
 use crate::cache::hint::Hint;
+use crate::cache::ir::{self, MetadataVisitor, Visit};
 use crate::cache::mapping::{Mapping, MappingFlags};
 use crate::cache::superblock::*;
-use crate::cache::xml::{self, MetadataVisitor, Visit};
+use crate::cache::xml;
 use crate::io_engine::*;
 use crate::math::*;
 use crate::pdata::array_builder::*;
@@ -55,17 +56,9 @@ fn mk_context(opts: &CacheRestoreOptions) -> anyhow::Result<Context> {
 
 //------------------------------------------
 
-struct RestoreResult {
+pub struct Restorer<'a> {
-    sb: xml::Superblock,
-    mapping_root: u64,
-    dirty_root: Option<u64>,
-    hint_root: u64,
-    discard_root: u64,
-}
-
-struct Restorer<'a> {
     write_batcher: &'a mut WriteBatcher,
-    sb: Option<xml::Superblock>,
+    sb: Option<ir::Superblock>,
     mapping_builder: Option<ArrayBuilder<Mapping>>,
     dirty_builder: Option<ArrayBuilder<u64>>,
     hint_builder: Option<ArrayBuilder<Hint>>,
@@ -77,7 +70,7 @@ struct Restorer<'a> {
 }
 
 impl<'a> Restorer<'a> {
-    fn new(w: &'a mut WriteBatcher) -> Restorer<'a> {
+    pub fn new(w: &'a mut WriteBatcher) -> Restorer<'a> {
         Restorer {
             write_batcher: w,
             sb: None,
@@ -92,27 +85,47 @@ impl<'a> Restorer<'a> {
         }
     }
 
-    fn get_result(self) -> Result<RestoreResult> {
+    fn finalize(&mut self) -> Result<()> {
-        self.write_batcher.flush()?;
+        // build metadata space map
+        let metadata_sm_root = build_metadata_sm(self.write_batcher)?;
 
-        if self.sb.is_none() || self.discard_root.is_none() {
+        let sb = self.sb.as_ref().unwrap();
-            return Err(anyhow!("No superblock found in xml file"));
+        let mapping_root = self.mapping_root.as_ref().unwrap();
-        }
+        let hint_root = self.hint_root.as_ref().unwrap();
-        if self.mapping_root.is_none() || self.hint_root.is_none() {
+        let discard_root = self.discard_root.as_ref().unwrap();
-            return Err(anyhow!("No mappings or hints sections in xml file"));
+        let sb = Superblock {
-        }
+            flags: SuperblockFlags {
-        Ok(RestoreResult {
+                clean_shutdown: true,
-            sb: self.sb.unwrap(),
+                needs_check: false,
-            mapping_root: self.mapping_root.unwrap(),
+            },
-            dirty_root: self.dirty_root,
+            block: SUPERBLOCK_LOCATION,
-            hint_root: self.hint_root.unwrap(),
+            version: 2,
-            discard_root: self.discard_root.unwrap(),
+            policy_name: sb.policy.as_bytes().to_vec(),
-        })
+            policy_version: vec![2, 0, 0],
+            policy_hint_size: sb.hint_width,
+            metadata_sm_root,
+            mapping_root: *mapping_root,
+            dirty_root: self.dirty_root, // dirty_root is optional
+            hint_root: *hint_root,
+            discard_root: *discard_root,
+            discard_block_size: 0,
+            discard_nr_blocks: 0,
+            data_block_size: sb.block_size,
+            cache_blocks: sb.nr_cache_blocks,
+            compat_flags: 0,
+            compat_ro_flags: 0,
+            incompat_flags: 0,
+            read_hits: 0,
+            read_misses: 9,
+            write_hits: 0,
+            write_misses: 0,
+        };
+        write_superblock(self.write_batcher.engine.as_ref(), SUPERBLOCK_LOCATION, &sb)
     }
 }
 
 impl<'a> MetadataVisitor for Restorer<'a> {
-    fn superblock_b(&mut self, sb: &xml::Superblock) -> Result<Visit> {
+    fn superblock_b(&mut self, sb: &ir::Superblock) -> Result<Visit> {
         self.sb = Some(sb.clone());
         self.write_batcher.alloc()?;
         self.mapping_builder = Some(ArrayBuilder::new(sb.nr_cache_blocks as u64));
@@ -126,6 +139,7 @@ impl<'a> MetadataVisitor for Restorer<'a> {
     }
 
     fn superblock_e(&mut self) -> Result<Visit> {
+        self.finalize()?;
         Ok(Visit::Continue)
     }
 
@@ -157,7 +171,7 @@ impl<'a> MetadataVisitor for Restorer<'a> {
         Ok(Visit::Continue)
     }
 
-    fn mapping(&mut self, m: &xml::Map) -> Result<Visit> {
+    fn mapping(&mut self, m: &ir::Map) -> Result<Visit> {
         let map = Mapping {
             oblock: m.oblock,
             flags: MappingFlags::Valid as u32,
@@ -198,7 +212,7 @@ impl<'a> MetadataVisitor for Restorer<'a> {
         Ok(Visit::Continue)
     }
 
-    fn hint(&mut self, h: &xml::Hint) -> Result<Visit> {
+    fn hint(&mut self, h: &ir::Hint) -> Result<Visit> {
         let hint = Hint {
             hint: h.data[..].try_into().unwrap(),
         };
@@ -215,7 +229,7 @@ impl<'a> MetadataVisitor for Restorer<'a> {
         Ok(Visit::Continue)
     }
 
-    fn discard(&mut self, _d: &xml::Discard) -> Result<Visit> {
+    fn discard(&mut self, _d: &ir::Discard) -> Result<Visit> {
         Ok(Visit::Continue)
     }
 
@@ -251,39 +265,6 @@ pub fn restore(opts: CacheRestoreOptions) -> Result<()> {
     // build cache mappings
     let mut restorer = Restorer::new(&mut w);
     xml::read(input, &mut restorer)?;
-    let result = restorer.get_result()?;
-
-    // build metadata space map
-    let metadata_sm_root = build_metadata_sm(&mut w)?;
-
-    let sb = Superblock {
-        flags: SuperblockFlags {
-            clean_shutdown: true,
-            needs_check: false,
-        },
-        block: SUPERBLOCK_LOCATION,
-        version: 2,
-        policy_name: result.sb.policy.as_bytes().to_vec(),
-        policy_version: vec![2, 0, 0],
-        policy_hint_size: result.sb.hint_width,
-        metadata_sm_root,
-        mapping_root: result.mapping_root,
-        dirty_root: result.dirty_root,
-        hint_root: result.hint_root,
-        discard_root: result.discard_root,
-        discard_block_size: 0,
-        discard_nr_blocks: 0,
-        data_block_size: result.sb.block_size,
-        cache_blocks: result.sb.nr_cache_blocks,
-        compat_flags: 0,
-        compat_ro_flags: 0,
-        incompat_flags: 0,
-        read_hits: 0,
-        read_misses: 9,
-        write_hits: 0,
-        write_misses: 0,
-    };
-    write_superblock(ctx.engine.as_ref(), SUPERBLOCK_LOCATION, &sb)?;
 
     Ok(())
 }

src/cache/xml.rs

@@ -6,63 +6,11 @@ use std::io::{Read, Write};
 use quick_xml::events::{BytesEnd, BytesStart, Event};
 use quick_xml::{Reader, Writer};
 
+use crate::cache::ir::*;
 use crate::xml::*;
 
 //---------------------------------------
 
-#[derive(Clone)]
-pub struct Superblock {
-    pub uuid: String,
-    pub block_size: u32,
-    pub nr_cache_blocks: u32,
-    pub policy: String,
-    pub hint_width: u32,
-}
-
-#[derive(Clone)]
-pub struct Map {
-    pub cblock: u32,
-    pub oblock: u64,
-    pub dirty: bool,
-}
-
-#[derive(Clone)]
-pub struct Hint {
-    pub cblock: u32,
-    pub data: Vec<u8>,
-}
-
-#[derive(Clone)]
-pub struct Discard {
-    pub begin: u64,
-    pub end: u64,
-}
-
-#[derive(Clone)]
-pub enum Visit {
-    Continue,
-    Stop,
-}
-
-pub trait MetadataVisitor {
-    fn superblock_b(&mut self, sb: &Superblock) -> Result<Visit>;
-    fn superblock_e(&mut self) -> Result<Visit>;
-
-    fn mappings_b(&mut self) -> Result<Visit>;
-    fn mappings_e(&mut self) -> Result<Visit>;
-    fn mapping(&mut self, m: &Map) -> Result<Visit>;
-
-    fn hints_b(&mut self) -> Result<Visit>;
-    fn hints_e(&mut self) -> Result<Visit>;
-    fn hint(&mut self, h: &Hint) -> Result<Visit>;
-
-    fn discards_b(&mut self) -> Result<Visit>;
-    fn discards_e(&mut self) -> Result<Visit>;
-    fn discard(&mut self, d: &Discard) -> Result<Visit>;
-
-    fn eof(&mut self) -> Result<Visit>;
-}
-
 pub struct XmlWriter<W: Write> {
     w: Writer<W>,
 }

src/shrink/toplevel.rs

@@ -6,7 +6,8 @@ use std::os::unix::fs::OpenOptionsExt;
 use std::path::Path;
 
 use crate::shrink::copier::{self, Region};
-use crate::thin::xml::{self, Visit};
+use crate::thin::ir::{self, MetadataVisitor, Visit};
+use crate::thin::xml;
 
 //---------------------------------------
 
@@ -34,8 +35,8 @@ impl Pass1 {
     }
 }
 
-impl xml::MetadataVisitor for Pass1 {
+impl MetadataVisitor for Pass1 {
-    fn superblock_b(&mut self, sb: &xml::Superblock) -> Result<Visit> {
+    fn superblock_b(&mut self, sb: &ir::Superblock) -> Result<Visit> {
         self.allocated_blocks.grow(sb.nr_data_blocks as usize);
         self.block_size = Some(sb.data_block_size as u64);
         Ok(Visit::Continue)
@@ -53,7 +54,7 @@ impl xml::MetadataVisitor for Pass1 {
         todo!();
     }
 
-    fn device_b(&mut self, _d: &xml::Device) -> Result<Visit> {
+    fn device_b(&mut self, _d: &ir::Device) -> Result<Visit> {
         Ok(Visit::Continue)
     }
 
@@ -61,7 +62,7 @@ impl xml::MetadataVisitor for Pass1 {
         Ok(Visit::Continue)
     }
 
-    fn map(&mut self, m: &xml::Map) -> Result<Visit> {
+    fn map(&mut self, m: &ir::Map) -> Result<Visit> {
         for i in m.data_begin..(m.data_begin + m.len) {
             if i > self.nr_blocks {
                 self.nr_high_blocks += 1;
@@ -99,8 +100,8 @@ impl<W: Write> Pass2<W> {
     }
 }
 
-impl<W: Write> xml::MetadataVisitor for Pass2<W> {
+impl<W: Write> MetadataVisitor for Pass2<W> {
-    fn superblock_b(&mut self, sb: &xml::Superblock) -> Result<Visit> {
+    fn superblock_b(&mut self, sb: &ir::Superblock) -> Result<Visit> {
         self.writer.superblock_b(sb)
     }
 
@@ -116,7 +117,7 @@ impl<W: Write> xml::MetadataVisitor for Pass2<W> {
         todo!();
     }
 
-    fn device_b(&mut self, d: &xml::Device) -> Result<Visit> {
+    fn device_b(&mut self, d: &ir::Device) -> Result<Visit> {
         self.writer.device_b(d)
     }
 
@@ -124,7 +125,7 @@ impl<W: Write> xml::MetadataVisitor for Pass2<W> {
         self.writer.device_e()
     }
 
-    fn map(&mut self, m: &xml::Map) -> Result<Visit> {
+    fn map(&mut self, m: &ir::Map) -> Result<Visit> {
         if m.data_begin + m.len < self.nr_blocks {
             // no remapping needed.
             self.writer.map(m)?;
@@ -134,7 +135,7 @@ impl<W: Write> xml::MetadataVisitor for Pass2<W> {
             let mut written = 0;
 
             for r in remaps {
-                self.writer.map(&xml::Map {
+                self.writer.map(&ir::Map {
                     thin_begin: m.thin_begin + written,
                     data_begin: r.start,
                     time: m.time,
@@ -493,7 +494,7 @@ fn build_copy_regions(remaps: &[(BlockRange, BlockRange)], block_size: u64) -> V
     rs
 }
 
-fn process_xml<MV: xml::MetadataVisitor>(input_path: &Path, pass: &mut MV) -> Result<()> {
+fn process_xml<MV: MetadataVisitor>(input_path: &Path, pass: &mut MV) -> Result<()> {
     let input = OpenOptions::new()
         .read(true)
         .write(false)

src/thin/dump.rs

@@ -1,31 +1,27 @@
 use anyhow::{anyhow, Result};
-use std::collections::{BTreeMap, BTreeSet};
 use std::fs::File;
 use std::io::BufWriter;
 use std::io::Write;
-use std::ops::DerefMut;
 use std::path::Path;
 use std::sync::{Arc, Mutex};
 
 use crate::checksum;
 use crate::io_engine::{AsyncIoEngine, Block, IoEngine, SyncIoEngine};
 use crate::pdata::btree::{self, *};
-use crate::pdata::btree_leaf_walker::*;
 use crate::pdata::btree_walker::*;
-use crate::pdata::space_map::*;
 use crate::pdata::space_map_common::*;
 use crate::pdata::unpack::*;
 use crate::report::*;
 use crate::thin::block_time::*;
-use crate::thin::device_detail::*;
+use crate::thin::ir::{self, MetadataVisitor};
-use crate::thin::runs::*;
+use crate::thin::metadata::*;
 use crate::thin::superblock::*;
-use crate::thin::xml::{self, MetadataVisitor};
+use crate::thin::xml;
 
 //------------------------------------------
 
 struct RunBuilder {
-    run: Option<xml::Map>,
+    run: Option<ir::Map>,
 }
 
 impl RunBuilder {
@@ -33,12 +29,12 @@ impl RunBuilder {
         RunBuilder { run: None }
     }
 
-    fn next(&mut self, thin_block: u64, data_block: u64, time: u32) -> Option<xml::Map> {
+    fn next(&mut self, thin_block: u64, data_block: u64, time: u32) -> Option<ir::Map> {
-        use xml::Map;
+        use ir::Map;
 
         match self.run {
             None => {
-                self.run = Some(xml::Map {
+                self.run = Some(ir::Map {
                     thin_begin: thin_block,
                     data_begin: data_block,
                     time,
@@ -46,7 +42,7 @@ impl RunBuilder {
                 });
                 None
             }
-            Some(xml::Map {
+            Some(ir::Map {
                 thin_begin,
                 data_begin,
                 time: mtime,
@@ -70,7 +66,7 @@ impl RunBuilder {
         }
     }
 
-    fn complete(&mut self) -> Option<xml::Map> {
+    fn complete(&mut self) -> Option<ir::Map> {
         self.run.take()
     }
 }
@@ -78,7 +74,7 @@ impl RunBuilder {
 //------------------------------------------
 
 struct MVInner<'a> {
-    md_out: &'a mut dyn xml::MetadataVisitor,
+    md_out: &'a mut dyn MetadataVisitor,
     builder: RunBuilder,
 }
 
@@ -89,7 +85,7 @@ struct MappingVisitor<'a> {
 //------------------------------------------
 
 impl<'a> MappingVisitor<'a> {
-    fn new(md_out: &'a mut dyn xml::MetadataVisitor) -> MappingVisitor<'a> {
+    fn new(md_out: &'a mut dyn MetadataVisitor) -> MappingVisitor<'a> {
         MappingVisitor {
             inner: Mutex::new(MVInner {
                 md_out,
@@ -176,297 +172,6 @@ fn mk_context(opts: &ThinDumpOptions) -> Result<Context> {
 
 //------------------------------------------
 
-type DefId = u64;
-type ThinId = u32;
-
-#[derive(Clone)]
-enum Entry {
-    Leaf(u64),
-    Ref(DefId),
-}
-
-#[derive(Clone)]
-struct Mapping {
-    kr: KeyRange,
-    entries: Vec<Entry>,
-}
-
-#[derive(Clone)]
-struct Device {
-    thin_id: ThinId,
-    detail: DeviceDetail,
-    map: Mapping,
-}
-
-#[derive(Clone)]
-struct Def {
-    def_id: DefId,
-    map: Mapping,
-}
-
-#[derive(Clone)]
-struct Metadata {
-    defs: Vec<Def>,
-    devs: Vec<Device>,
-}
-
-//------------------------------------------
-
-struct CollectLeaves {
-    leaves: Vec<Entry>,
-}
-
-impl CollectLeaves {
-    fn new() -> CollectLeaves {
-        CollectLeaves { leaves: Vec::new() }
-    }
-}
-
-impl LeafVisitor<BlockTime> for CollectLeaves {
-    fn visit(&mut self, _kr: &KeyRange, b: u64) -> btree::Result<()> {
-        self.leaves.push(Entry::Leaf(b));
-        Ok(())
-    }
-
-    fn visit_again(&mut self, b: u64) -> btree::Result<()> {
-        self.leaves.push(Entry::Ref(b));
-        Ok(())
-    }
-
-    fn end_walk(&mut self) -> btree::Result<()> {
-        Ok(())
-    }
-}
-
-fn collect_leaves(
-    ctx: &Context,
-    roots: &BTreeSet<u64>,
-    mut sm: Box<dyn SpaceMap>,
-) -> Result<BTreeMap<u64, Vec<Entry>>> {
-    let mut map: BTreeMap<u64, Vec<Entry>> = BTreeMap::new();
-
-    ctx.report
-        .set_title(&format!("Collecting leaves for {} roots", roots.len()));
-
-    // FIXME: we don't want any leaves in shared.
-    for r in roots.iter() {
-        let old_count = sm.get(*r).expect("couldn't get count from space map.");
-        sm.set(*r, 0).expect("couldn't set count in space map.");
-
-        let mut w = LeafWalker::new(ctx.engine.clone(), sm.deref_mut(), false);
-        let mut v = CollectLeaves::new();
-
-        let mut path = vec![0];
-
-        // ctx.report.set_title(&format!("collecting {}", *r));
-        w.walk::<CollectLeaves, BlockTime>(&mut path, &mut v, *r)?;
-        sm.set(*r, old_count)
-            .expect("couldn't set count in space map.");
-
-        map.insert(*r, v.leaves);
-    }
-
-    Ok(map)
-}
-
-//------------------------------------------
-
-#[allow(dead_code)]
-fn find_shared_nodes(
-    ctx: &Context,
-    roots: &BTreeMap<u64, (Vec<u64>, u64)>,
-) -> Result<(BTreeSet<u64>, Box<dyn SpaceMap>)> {
-    let nr_metadata_blocks = ctx.engine.get_nr_blocks();
-    let mut sm = core_sm_without_mutex(nr_metadata_blocks, roots.len() as u32);
-    let mut v = NoopLeafVisitor {};
-    let mut w = LeafWalker::new(ctx.engine.clone(), sm.deref_mut(), false);
-
-    for (thin_id, (path, root)) in roots {
-        let mut path = path.clone();
-        ctx.report.info(&format!("scanning {}", thin_id));
-        w.walk::<NoopLeafVisitor, BlockTime>(&mut path, &mut v, *root)?;
-    }
-
-    // We have to get the leaves so w is consumed and the &mut on sm
-    // is dropped.
-    let _leaves = w.get_leaves();
-    let mut shared = BTreeSet::new();
-    {
-        for i in 0..sm.get_nr_blocks().unwrap() {
-            if sm.get(i).expect("couldn't get count from space map.") > 1 {
-                shared.insert(i);
-            }
-        }
-    }
-
-    /*
-    // FIXME: why?!!
-    // we're not interested in leaves (roots will get re-added later).
-    {
-        for i in 0..leaves.len() {
-            if leaves.contains(i) {
-                shared.remove(&(i as u64));
-            }
-        }
-    }
-    */
-
-    Ok((shared, sm))
-}
-
-//------------------------------------------
-
-fn build_metadata(ctx: &Context, sb: &Superblock) -> Result<Metadata> {
-    let report = &ctx.report;
-    let engine = &ctx.engine;
-
-    // superblock
-    report.set_title("Reading superblock");
-    //let metadata_root = unpack::<SMRoot>(&sb.metadata_sm_root[0..])?;
-    //let data_root = unpack::<SMRoot>(&sb.data_sm_root[0..])?;
-    let mut path = vec![0];
-
-    report.set_title("Reading device details");
-    let details = btree_to_map::<DeviceDetail>(&mut path, engine.clone(), true, sb.details_root)?;
-
-    report.set_title("Reading mappings roots");
-    let roots;
-    {
-        let sm = Arc::new(Mutex::new(RestrictedSpaceMap::new(engine.get_nr_blocks())));
-        roots =
-            btree_to_map_with_path::<u64>(&mut path, engine.clone(), sm, true, sb.mapping_root)?;
-    }
-
-    let sm = Box::new(RestrictedSpaceMap::new(engine.get_nr_blocks()));
-    let mapping_roots = roots.values().map(|(_, root)| *root).collect();
-    let entry_map = collect_leaves(&ctx, &mapping_roots, sm)?;
-    let defs = Vec::new();
-    let mut devs = Vec::new();
-
-    let mut seen = BTreeSet::new();
-    for (thin_id, (_path, root)) in roots {
-        let id = thin_id as u64;
-        let detail = details.get(&id).expect("couldn't find device details");
-        seen.insert(root);
-        let es = entry_map.get(&root).unwrap();
-        let kr = KeyRange::new(); // FIXME: finish
-        devs.push(Device {
-            thin_id: thin_id as u32,
-            detail: *detail,
-            map: Mapping {
-                kr,
-                entries: es.to_vec(),
-            },
-        });
-    }
-
-    Ok(Metadata { defs, devs })
-}
-
-//------------------------------------------
-
-#[allow(dead_code)]
-fn gather_entries(g: &mut Gatherer, es: &[Entry]) {
-    g.new_seq();
-    for e in es {
-        match e {
-            Entry::Leaf(b) => {
-                g.next(*b);
-            }
-            Entry::Ref(_id) => {
-                g.new_seq();
-            }
-        }
-    }
-}
-
-#[allow(dead_code)]
-fn entries_to_runs(runs: &BTreeMap<u64, Vec<u64>>, es: &[Entry]) -> Vec<Entry> {
-    use Entry::*;
-
-    let mut result = Vec::new();
-    let mut entry_index = 0;
-    while entry_index < es.len() {
-        match es[entry_index] {
-            Ref(id) => {
-                result.push(Ref(id));
-                entry_index += 1;
-            }
-            Leaf(b) => {
-                if let Some(run) = runs.get(&b) {
-                    result.push(Ref(b));
-                    entry_index += run.len();
-                } else {
-                    result.push(Leaf(b));
-                    entry_index += 1;
-                }
-            }
-        }
-    }
-
-    result
-}
-
-// 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<Metadata> {
-    use Entry::*;
-
-    let mut g = Gatherer::new();
-    for d in &md.defs {
-        gather_entries(&mut g, &d.map.entries);
-    }
-
-    for d in &md.devs {
-        gather_entries(&mut g, &d.map.entries);
-    }
-
-    let mut defs = Vec::new();
-    let mut devs = Vec::new();
-    let mut runs = BTreeMap::new();
-    for run in g.gather() {
-        runs.insert(run[0], run);
-    }
-    eprintln!("{} runs", runs.len());
-
-    // The runs become additional defs that just contain leaves.
-    for (head, run) in runs.iter() {
-        let kr = KeyRange::new();
-        let entries: Vec<Entry> = run.iter().map(|b| Leaf(*b)).collect();
-        defs.push(Def {
-            def_id: *head,
-            map: Mapping { kr, entries },
-        });
-    }
-
-    // Expand old defs to use the new atomic runs
-    for d in &md.defs {
-        let kr = KeyRange::new();
-        let entries = entries_to_runs(&runs, &d.map.entries);
-
-        defs.push(Def {
-            def_id: d.def_id,
-            map: Mapping { kr, entries },
-        });
-    }
-
-    // Expand old devs to use the new atomic runs
-    for d in &md.devs {
-        let kr = KeyRange::new();
-        let entries = entries_to_runs(&runs, &d.map.entries);
-        devs.push(Device {
-            thin_id: d.thin_id,
-            detail: d.detail,
-            map: Mapping { kr, entries },
-        });
-    }
-
-    Ok(Metadata { defs, devs })
-}
-
-//------------------------------------------
-
 fn emit_leaf(v: &mut MappingVisitor, b: &Block) -> Result<()> {
     use Node::*;
     let path = Vec::new();
@@ -516,20 +221,20 @@ where
     Ok(())
 }
 
-fn emit_leaves(ctx: &Context, out: &mut dyn xml::MetadataVisitor, ls: &[u64]) -> Result<()> {
+fn emit_leaves(engine: Arc<dyn IoEngine>, out: &mut dyn MetadataVisitor, ls: &[u64]) -> Result<()> {
     let mut v = MappingVisitor::new(out);
     let proc = |b| {
         emit_leaf(&mut v, &b)?;
         Ok(())
     };
 
-    read_for(ctx.engine.clone(), ls, proc)?;
+    read_for(engine, ls, proc)?;
     v.end_walk().map_err(|_| anyhow!("failed to emit leaves"))
 }
 
-fn emit_entries<W: Write>(
+fn emit_entries(
-    ctx: &Context,
+    engine: Arc<dyn IoEngine>,
-    out: &mut xml::XmlWriter<W>,
+    out: &mut dyn MetadataVisitor,
     entries: &[Entry],
 ) -> Result<()> {
     let mut leaves = Vec::new();
@@ -541,7 +246,7 @@ fn emit_entries<W: Write>(
             }
             Entry::Ref(id) => {
                 if !leaves.is_empty() {
-                    emit_leaves(&ctx, out, &leaves[0..])?;
+                    emit_leaves(engine.clone(), out, &leaves[0..])?;
                     leaves.clear();
                 }
                 let str = format!("{}", id);
@@ -551,18 +256,22 @@ fn emit_entries<W: Write>(
     }
 
     if !leaves.is_empty() {
-        emit_leaves(&ctx, out, &leaves[0..])?;
+        emit_leaves(engine, out, &leaves[0..])?;
     }
 
     Ok(())
 }
 
-fn dump_metadata(ctx: &Context, w: &mut dyn Write, sb: &Superblock, md: &Metadata) -> Result<()> {
+pub fn dump_metadata(
+    engine: Arc<dyn IoEngine>,
+    out: &mut dyn MetadataVisitor,
+    sb: &Superblock,
+    md: &Metadata,
+) -> Result<()> {
     let data_root = unpack::<SMRoot>(&sb.data_sm_root[0..])?;
-    let mut out = xml::XmlWriter::new(w);
+    let out_sb = ir::Superblock {
-    let xml_sb = xml::Superblock {
         uuid: "".to_string(),
-        time: sb.time as u64,
+        time: sb.time,
         transaction: sb.transaction_id,
         flags: None,
         version: Some(2),
@@ -570,26 +279,26 @@ fn dump_metadata(ctx: &Context, w: &mut dyn Write, sb: &Superblock, md: &Metadat
         nr_data_blocks: data_root.nr_blocks,
         metadata_snap: None,
     };
-    out.superblock_b(&xml_sb)?;
+    out.superblock_b(&out_sb)?;
 
-    ctx.report.set_title("Dumping shared regions");
+    // ctx.report.set_title("Dumping shared regions");
     for d in &md.defs {
         out.def_shared_b(&format!("{}", d.def_id))?;
-        emit_entries(ctx, &mut out, &d.map.entries)?;
+        emit_entries(engine.clone(), out, &d.map.entries)?;
         out.def_shared_e()?;
     }
 
-    ctx.report.set_title("Dumping devices");
+    // ctx.report.set_title("Dumping devices");
     for dev in &md.devs {
-        let device = xml::Device {
+        let device = ir::Device {
             dev_id: dev.thin_id,
             mapped_blocks: dev.detail.mapped_blocks,
             transaction: dev.detail.transaction_id,
-            creation_time: dev.detail.creation_time as u64,
+            creation_time: dev.detail.creation_time,
-            snap_time: dev.detail.snapshotted_time as u64,
+            snap_time: dev.detail.snapshotted_time,
         };
         out.device_b(&device)?;
-        emit_entries(ctx, &mut out, &dev.map.entries)?;
+        emit_entries(engine.clone(), out, &dev.map.entries)?;
         out.device_e()?;
     }
     out.superblock_e()?;
@@ -603,20 +312,21 @@ fn dump_metadata(ctx: &Context, w: &mut dyn Write, sb: &Superblock, md: &Metadat
 pub fn dump(opts: ThinDumpOptions) -> Result<()> {
     let ctx = mk_context(&opts)?;
     let sb = read_superblock(ctx.engine.as_ref(), SUPERBLOCK_LOCATION)?;
-    let md = build_metadata(&ctx, &sb)?;
+    let md = build_metadata(ctx.engine.clone(), &sb)?;
-
-    let mut writer: Box<dyn Write>;
-    if opts.output.is_some() {
-        writer = Box::new(BufWriter::new(File::create(opts.output.unwrap())?));
-    } else {
-        writer = Box::new(BufWriter::new(std::io::stdout()));
-    }
 
     ctx.report
         .set_title("Optimising metadata to improve leaf packing");
     let md = optimise_metadata(md)?;
 
-    dump_metadata(&ctx, &mut writer, &sb, &md)
+    let writer: Box<dyn Write>;
+    if opts.output.is_some() {
+        writer = Box::new(BufWriter::new(File::create(opts.output.unwrap())?));
+    } else {
+        writer = Box::new(BufWriter::new(std::io::stdout()));
+    }
+    let mut out = xml::XmlWriter::new(writer);
+
+    dump_metadata(ctx.engine, &mut out, &sb, &md)
 }
 
 //------------------------------------------

src/thin/ir.rs

@@ -0,0 +1,60 @@
+use anyhow::Result;
+
+//------------------------------------------
+
+#[derive(Clone)]
+pub struct Superblock {
+    pub uuid: String,
+    pub time: u32,
+    pub transaction: u64,
+    pub flags: Option<u32>,
+    pub version: Option<u32>,
+    pub data_block_size: u32,
+    pub nr_data_blocks: u64,
+    pub metadata_snap: Option<u64>,
+}
+
+#[derive(Clone)]
+pub struct Device {
+    pub dev_id: u32,
+    pub mapped_blocks: u64,
+    pub transaction: u64,
+    pub creation_time: u32,
+    pub snap_time: u32,
+}
+
+#[derive(Clone)]
+pub struct Map {
+    pub thin_begin: u64,
+    pub data_begin: u64,
+    pub time: u32,
+    pub len: u64,
+}
+
+//------------------------------------------
+
+#[derive(Clone)]
+pub enum Visit {
+    Continue,
+    Stop,
+}
+
+pub trait MetadataVisitor {
+    fn superblock_b(&mut self, sb: &Superblock) -> Result<Visit>;
+    fn superblock_e(&mut self) -> Result<Visit>;
+
+    // Defines a shared sub tree.  May only contain a 'map' (no 'ref' allowed).
+    fn def_shared_b(&mut self, name: &str) -> Result<Visit>;
+    fn def_shared_e(&mut self) -> Result<Visit>;
+
+    // A device contains a number of 'map' or 'ref' items.
+    fn device_b(&mut self, d: &Device) -> Result<Visit>;
+    fn device_e(&mut self) -> Result<Visit>;
+
+    fn map(&mut self, m: &Map) -> Result<Visit>;
+    fn ref_shared(&mut self, name: &str) -> Result<Visit>;
+
+    fn eof(&mut self) -> Result<Visit>;
+}
+
+//------------------------------------------

src/thin/metadata.rs

@@ -0,0 +1,236 @@
+use anyhow::Result;
+use std::collections::{BTreeMap, BTreeSet};
+use std::sync::{Arc, Mutex};
+
+use crate::io_engine::IoEngine;
+use crate::pdata::btree::{self, *};
+use crate::pdata::btree_leaf_walker::*;
+use crate::pdata::btree_walker::*;
+use crate::pdata::space_map::*;
+use crate::thin::block_time::*;
+use crate::thin::device_detail::*;
+use crate::thin::runs::*;
+use crate::thin::superblock::*;
+
+//------------------------------------------
+
+type DefId = u64;
+type ThinId = u32;
+
+#[derive(Clone)]
+pub enum Entry {
+    Leaf(u64),
+    Ref(DefId),
+}
+
+#[derive(Clone)]
+pub struct Mapping {
+    pub kr: KeyRange,
+    pub entries: Vec<Entry>,
+}
+
+#[derive(Clone)]
+pub struct Device {
+    pub thin_id: ThinId,
+    pub detail: DeviceDetail,
+    pub map: Mapping,
+}
+
+#[derive(Clone)]
+pub struct Def {
+    pub def_id: DefId,
+    pub map: Mapping,
+}
+
+#[derive(Clone)]
+pub struct Metadata {
+    pub defs: Vec<Def>,
+    pub devs: Vec<Device>,
+}
+
+//------------------------------------------
+
+struct CollectLeaves {
+    leaves: Vec<Entry>,
+}
+
+impl CollectLeaves {
+    fn new() -> CollectLeaves {
+        CollectLeaves { leaves: Vec::new() }
+    }
+}
+
+impl LeafVisitor<BlockTime> for CollectLeaves {
+    fn visit(&mut self, _kr: &KeyRange, b: u64) -> btree::Result<()> {
+        self.leaves.push(Entry::Leaf(b));
+        Ok(())
+    }
+
+    fn visit_again(&mut self, b: u64) -> btree::Result<()> {
+        self.leaves.push(Entry::Ref(b));
+        Ok(())
+    }
+
+    fn end_walk(&mut self) -> btree::Result<()> {
+        Ok(())
+    }
+}
+
+fn collect_leaves(
+    engine: Arc<dyn IoEngine + Send + Sync>,
+    roots: &BTreeSet<u64>,
+) -> Result<BTreeMap<u64, Vec<Entry>>> {
+    let mut map: BTreeMap<u64, Vec<Entry>> = BTreeMap::new();
+    let mut sm = RestrictedSpaceMap::new(engine.get_nr_blocks());
+
+    for r in roots {
+        let mut w = LeafWalker::new(engine.clone(), &mut sm, false);
+        let mut v = CollectLeaves::new();
+        let mut path = vec![0];
+        w.walk::<CollectLeaves, BlockTime>(&mut path, &mut v, *r)?;
+
+        map.insert(*r, v.leaves);
+    }
+
+    Ok(map)
+}
+
+//------------------------------------------
+
+pub fn build_metadata(
+    engine: Arc<dyn IoEngine + Send + Sync>,
+    sb: &Superblock,
+) -> Result<Metadata> {
+    let mut path = vec![0];
+
+    // report.set_title("Reading device details");
+    let details = btree_to_map::<DeviceDetail>(&mut path, engine.clone(), true, sb.details_root)?;
+
+    // report.set_title("Reading mappings roots");
+    let roots;
+    {
+        let sm = Arc::new(Mutex::new(RestrictedSpaceMap::new(engine.get_nr_blocks())));
+        roots =
+            btree_to_map_with_path::<u64>(&mut path, engine.clone(), sm, true, sb.mapping_root)?;
+    }
+
+    // report.set_title(&format!("Collecting leaves for {} roots", roots.len()));
+    let mapping_roots = roots.values().map(|(_, root)| *root).collect();
+    let entry_map = collect_leaves(engine.clone(), &mapping_roots)?;
+
+    let defs = Vec::new();
+    let mut devs = Vec::new();
+    for (thin_id, (_path, root)) in roots {
+        let id = thin_id as u64;
+        let detail = details.get(&id).expect("couldn't find device details");
+        let es = entry_map.get(&root).unwrap();
+        let kr = KeyRange::new(); // FIXME: finish
+        devs.push(Device {
+            thin_id: thin_id as u32,
+            detail: *detail,
+            map: Mapping {
+                kr,
+                entries: es.to_vec(),
+            },
+        });
+    }
+
+    Ok(Metadata { defs, devs })
+}
+
+//------------------------------------------
+
+fn gather_entries(g: &mut Gatherer, es: &[Entry]) {
+    g.new_seq();
+    for e in es {
+        match e {
+            Entry::Leaf(b) => {
+                g.next(*b);
+            }
+            Entry::Ref(_id) => {
+                g.new_seq();
+            }
+        }
+    }
+}
+
+fn build_runs(devs: &[Device]) -> BTreeMap<u64, Vec<u64>> {
+    let mut g = Gatherer::new();
+
+    for d in devs {
+        gather_entries(&mut g, &d.map.entries);
+    }
+
+    // The runs become defs that just contain leaves.
+    let mut runs = BTreeMap::new();
+    for run in g.gather() {
+        runs.insert(run[0], run);
+    }
+
+    runs
+}
+
+fn entries_to_runs(runs: &BTreeMap<u64, Vec<u64>>, es: &[Entry]) -> Vec<Entry> {
+    use Entry::*;
+
+    let mut result = Vec::new();
+    let mut entry_index = 0;
+    while entry_index < es.len() {
+        match es[entry_index] {
+            Ref(id) => {
+                result.push(Ref(id));
+                entry_index += 1;
+            }
+            Leaf(b) => {
+                if let Some(run) = runs.get(&b) {
+                    result.push(Ref(b));
+                    entry_index += run.len();
+                } else {
+                    result.push(Leaf(b));
+                    entry_index += 1;
+                }
+            }
+        }
+    }
+
+    result
+}
+
+fn build_defs(runs: BTreeMap<u64, Vec<u64>>) -> Vec<Def> {
+    let mut defs = Vec::new();
+    for (head, run) in runs.iter() {
+        let kr = KeyRange::new();
+        let entries: Vec<Entry> = run.iter().map(|b| Entry::Leaf(*b)).collect();
+        defs.push(Def {
+            def_id: *head,
+            map: Mapping { kr, entries },
+        });
+    }
+
+    defs
+}
+
+// FIXME: do we really need to track kr?
+// FIXME: I think this may be better done as part of restore.
+pub fn optimise_metadata(md: Metadata) -> Result<Metadata> {
+    let runs = build_runs(&md.devs);
+    eprintln!("{} runs", runs.len());
+
+    // Expand old devs to use the new atomic runs
+    let mut devs = Vec::new();
+    for d in &md.devs {
+        let kr = KeyRange::new();
+        let entries = entries_to_runs(&runs, &d.map.entries);
+        devs.push(Device {
+            thin_id: d.thin_id,
+            detail: d.detail,
+            map: Mapping { kr, entries },
+        });
+    }
+
+    let defs = build_defs(runs);
+
+    Ok(Metadata { defs, devs })
+}
+
+//------------------------------------------

src/thin/mod.rs

@@ -2,6 +2,9 @@ pub mod block_time;
 pub mod check;
 pub mod device_detail;
 pub mod dump;
+pub mod ir;
+pub mod metadata;
+pub mod repair;
 pub mod restore;
 pub mod runs;
 pub mod superblock;

src/thin/repair.rs

@@ -0,0 +1,71 @@
+use anyhow::Result;
+use std::path::Path;
+use std::sync::Arc;
+
+use crate::io_engine::*;
+use crate::pdata::space_map::*;
+use crate::report::*;
+use crate::thin::dump::*;
+use crate::thin::metadata::*;
+use crate::thin::restore::*;
+use crate::thin::superblock::*;
+use crate::write_batcher::*;
+
+//------------------------------------------
+
+pub struct ThinRepairOptions<'a> {
+    pub input: &'a Path,
+    pub output: &'a Path,
+    pub async_io: bool,
+    pub report: Arc<Report>,
+}
+
+struct Context {
+    report: Arc<Report>,
+    engine_in: Arc<dyn IoEngine + Send + Sync>,
+    engine_out: Arc<dyn IoEngine + Send + Sync>,
+}
+
+const MAX_CONCURRENT_IO: u32 = 1024;
+
+fn new_context(opts: &ThinRepairOptions) -> Result<Context> {
+    let engine_in: Arc<dyn IoEngine + Send + Sync>;
+    let engine_out: Arc<dyn IoEngine + Send + Sync>;
+
+    if opts.async_io {
+        engine_in = Arc::new(AsyncIoEngine::new(opts.input, MAX_CONCURRENT_IO, true)?);
+        engine_out = Arc::new(AsyncIoEngine::new(opts.output, MAX_CONCURRENT_IO, true)?);
+    } else {
+        let nr_threads = std::cmp::max(8, num_cpus::get() * 2);
+        engine_in = Arc::new(SyncIoEngine::new(opts.input, nr_threads, true)?);
+        engine_out = Arc::new(SyncIoEngine::new(opts.output, nr_threads, true)?);
+    }
+
+    Ok(Context {
+        report: opts.report.clone(),
+        engine_in,
+        engine_out,
+    })
+}
+
+//------------------------------------------
+
+pub fn repair(opts: ThinRepairOptions) -> Result<()> {
+    let ctx = new_context(&opts)?;
+
+    let sb = read_superblock(ctx.engine_in.as_ref(), SUPERBLOCK_LOCATION)?;
+    let md = build_metadata(ctx.engine_in.clone(), &sb)?;
+    let md = optimise_metadata(md)?;
+
+    let sm = core_sm(ctx.engine_out.get_nr_blocks(), u32::MAX);
+    let mut w = WriteBatcher::new(
+        ctx.engine_out.clone(),
+        sm.clone(),
+        ctx.engine_out.get_batch_size(),
+    );
+    let mut restorer = Restorer::new(&mut w, ctx.report);
+
+    dump_metadata(ctx.engine_in, &mut restorer, &sb, &md)
+}
+
+//------------------------------------------

src/thin/restore.rs

@@ -10,14 +10,16 @@ use std::sync::{Arc, Mutex};
 use crate::io_engine::*;
 use crate::pdata::btree_builder::*;
 use crate::pdata::space_map::*;
+use crate::pdata::space_map_common::SMRoot;
 use crate::pdata::space_map_disk::*;
 use crate::pdata::space_map_metadata::*;
 use crate::pdata::unpack::Pack;
 use crate::report::*;
 use crate::thin::block_time::*;
 use crate::thin::device_detail::*;
+use crate::thin::ir::{self, MetadataVisitor, Visit};
 use crate::thin::superblock::{self, *};
-use crate::thin::xml::{self, *};
+use crate::thin::xml;
 use crate::write_batcher::*;
 
 //------------------------------------------
@@ -57,13 +59,7 @@ impl std::fmt::Display for MappedSection {
 
 //------------------------------------------
 
-struct RestoreResult {
+pub struct Restorer<'a> {
-    sb: xml::Superblock,
-    devices: BTreeMap<u32, (DeviceDetail, u64)>,
-    data_sm: Arc<Mutex<dyn SpaceMap>>,
-}
-
-struct Restorer<'a> {
     w: &'a mut WriteBatcher,
     report: Arc<Report>,
 
@@ -74,13 +70,13 @@ struct Restorer<'a> {
     current_map: Option<(MappedSection, NodeBuilder<BlockTime>)>,
     current_dev: Option<DeviceDetail>,
 
-    sb: Option<xml::Superblock>,
+    sb: Option<ir::Superblock>,
     devices: BTreeMap<u32, (DeviceDetail, u64)>,
     data_sm: Option<Arc<Mutex<dyn SpaceMap>>>,
 }
 
 impl<'a> Restorer<'a> {
-    fn new(w: &'a mut WriteBatcher, report: Arc<Report>) -> Self {
+    pub fn new(w: &'a mut WriteBatcher, report: Arc<Report>) -> Self {
         Restorer {
             w,
             report,
@@ -93,17 +89,6 @@ impl<'a> Restorer<'a> {
         }
     }
 
-    fn get_result(self) -> Result<RestoreResult> {
-        if self.sb.is_none() {
-            return Err(anyhow!("No superblock found in xml file"));
-        }
-        Ok(RestoreResult {
-            sb: self.sb.unwrap(),
-            devices: self.devices,
-            data_sm: self.data_sm.unwrap(),
-        })
-    }
-
     fn begin_section(&mut self, section: MappedSection) -> Result<Visit> {
         if let Some((outer, _)) = self.current_map.as_ref() {
             let msg = format!(
@@ -133,17 +118,65 @@ impl<'a> Restorer<'a> {
             Err(anyhow!(msg))
         }
     }
+
+    // Build the device details and the top level mapping trees
+    fn build_device_details(&mut self) -> Result<(u64, u64)> {
+        let mut details_builder: BTreeBuilder<DeviceDetail> =
+            BTreeBuilder::new(Box::new(NoopRC {}));
+        let mut dev_builder: BTreeBuilder<u64> = BTreeBuilder::new(Box::new(NoopRC {}));
+        for (thin_id, (detail, root)) in self.devices.iter() {
+            details_builder.push_value(self.w, *thin_id as u64, *detail)?;
+            dev_builder.push_value(self.w, *thin_id as u64, *root)?;
+        }
+        let details_root = details_builder.complete(self.w)?;
+        let mapping_root = dev_builder.complete(self.w)?;
+
+        Ok((details_root, mapping_root))
+    }
+
+    fn finalize(&mut self) -> Result<()> {
+        let (details_root, mapping_root) = self.build_device_details()?;
+
+        // Build data space map
+        let data_sm = self.data_sm.as_ref().unwrap();
+        let data_sm_root = build_data_sm(self.w, data_sm.lock().unwrap().deref())?;
+
+        // Build metadata space map
+        let (metadata_sm, metadata_sm_root) = build_metadata_sm(self.w)?;
+
+        // Write the superblock
+        let sb = self.sb.as_ref().unwrap();
+        let sb = superblock::Superblock {
+            flags: SuperblockFlags { needs_check: false },
+            block: SUPERBLOCK_LOCATION,
+            version: 2,
+            time: sb.time as u32,
+            transaction_id: sb.transaction,
+            metadata_snap: 0,
+            data_sm_root,
+            metadata_sm_root,
+            mapping_root,
+            details_root,
+            data_block_size: sb.data_block_size,
+            nr_metadata_blocks: metadata_sm.nr_blocks,
+        };
+        write_superblock(self.w.engine.as_ref(), SUPERBLOCK_LOCATION, &sb)
+    }
 }
 
 impl<'a> MetadataVisitor for Restorer<'a> {
-    fn superblock_b(&mut self, sb: &xml::Superblock) -> Result<Visit> {
+    fn superblock_b(&mut self, sb: &ir::Superblock) -> Result<Visit> {
         self.sb = Some(sb.clone());
         self.data_sm = Some(core_sm(sb.nr_data_blocks, u32::MAX));
-        self.w.alloc()?;
+        let b = self.w.alloc()?;
+        if b.loc != SUPERBLOCK_LOCATION {
+            return Err(anyhow!("superblock was occupied"));
+        }
         Ok(Visit::Continue)
     }
 
     fn superblock_e(&mut self) -> Result<Visit> {
+        self.finalize()?;
         Ok(Visit::Continue)
     }
 
@@ -160,7 +193,7 @@ impl<'a> MetadataVisitor for Restorer<'a> {
         }
     }
 
-    fn device_b(&mut self, d: &Device) -> Result<Visit> {
+    fn device_b(&mut self, d: &ir::Device) -> Result<Visit> {
         self.report
             .info(&format!("building btree for device {}", d.dev_id));
         self.current_dev = Some(DeviceDetail {
@@ -186,7 +219,7 @@ impl<'a> MetadataVisitor for Restorer<'a> {
         }
     }
 
-    fn map(&mut self, m: &Map) -> Result<Visit> {
+    fn map(&mut self, m: &ir::Map) -> Result<Visit> {
         if let Some((_, builder)) = self.current_map.as_mut() {
             for i in 0..m.len {
                 let bt = BlockTime {
@@ -248,13 +281,13 @@ fn build_data_sm(w: &mut WriteBatcher, sm: &dyn SpaceMap) -> Result<Vec<u8>> {
 
 /// Writes the metadata space map to disk.  Returns the space map root that needs
 /// to be written to the superblock.
-fn build_metadata_sm(w: &mut WriteBatcher) -> Result<Vec<u8>> {
+fn build_metadata_sm(w: &mut WriteBatcher) -> Result<(SMRoot, Vec<u8>)> {
     let mut sm_root = vec![0u8; SPACE_MAP_ROOT_SIZE];
     let mut cur = Cursor::new(&mut sm_root);
     let r = write_metadata_sm(w)?;
     r.pack(&mut cur)?;
 
-    Ok(sm_root)
+    Ok((r, sm_root))
 }
 
 //------------------------------------------
@@ -302,42 +335,8 @@ pub fn restore(opts: ThinRestoreOptions) -> Result<()> {
 
     let sm = core_sm(ctx.engine.get_nr_blocks(), max_count);
     let mut w = WriteBatcher::new(ctx.engine.clone(), sm.clone(), ctx.engine.get_batch_size());
-    let mut restorer = Restorer::new(&mut w, ctx.report.clone());
+    let mut restorer = Restorer::new(&mut w, ctx.report);
     xml::read(input, &mut restorer)?;
-    let result = restorer.get_result()?;
-
-    // Build the device details and top level mapping tree
-    let mut details_builder: BTreeBuilder<DeviceDetail> = BTreeBuilder::new(Box::new(NoopRC {}));
-    let mut dev_builder: BTreeBuilder<u64> = BTreeBuilder::new(Box::new(NoopRC {}));
-    for (thin_id, (detail, root)) in &result.devices {
-        details_builder.push_value(&mut w, *thin_id as u64, *detail)?;
-        dev_builder.push_value(&mut w, *thin_id as u64, *root)?;
-    }
-    let details_root = details_builder.complete(&mut w)?;
-    let mapping_root = dev_builder.complete(&mut w)?;
-
-    // Build data space map
-    let data_sm_root = build_data_sm(&mut w, result.data_sm.lock().unwrap().deref())?;
-
-    // Build metadata space map
-    let metadata_sm_root = build_metadata_sm(&mut w)?;
-
-    // Write the superblock
-    let sb = superblock::Superblock {
-        flags: SuperblockFlags { needs_check: false },
-        block: SUPERBLOCK_LOCATION,
-        version: 2,
-        time: result.sb.time as u32,
-        transaction_id: result.sb.transaction,
-        metadata_snap: 0,
-        data_sm_root,
-        metadata_sm_root,
-        mapping_root,
-        details_root,
-        data_block_size: result.sb.data_block_size,
-        nr_metadata_blocks: ctx.engine.get_nr_blocks(),
-    };
-    write_superblock(ctx.engine.as_ref(), SUPERBLOCK_LOCATION, &sb)?;
 
     Ok(())
 }

src/thin/xml.rs

@@ -4,63 +4,11 @@ use std::{io::prelude::*, io::BufReader, io::Write};
 use quick_xml::events::{BytesEnd, BytesStart, Event};
 use quick_xml::{Reader, Writer};
 
+use crate::thin::ir::*;
 use crate::xml::*;
 
 //---------------------------------------
 
-#[derive(Clone)]
-pub struct Superblock {
-    pub uuid: String,
-    pub time: u64,
-    pub transaction: u64,
-    pub flags: Option<u32>,
-    pub version: Option<u32>,
-    pub data_block_size: u32,
-    pub nr_data_blocks: u64,
-    pub metadata_snap: Option<u64>,
-}
-
-#[derive(Clone)]
-pub struct Device {
-    pub dev_id: u32,
-    pub mapped_blocks: u64,
-    pub transaction: u64,
-    pub creation_time: u64,
-    pub snap_time: u64,
-}
-
-#[derive(Clone)]
-pub struct Map {
-    pub thin_begin: u64,
-    pub data_begin: u64,
-    pub time: u32,
-    pub len: u64,
-}
-
-#[derive(Clone)]
-pub enum Visit {
-    Continue,
-    Stop,
-}
-
-pub trait MetadataVisitor {
-    fn superblock_b(&mut self, sb: &Superblock) -> Result<Visit>;
-    fn superblock_e(&mut self) -> Result<Visit>;
-
-    // Defines a shared sub tree.  May only contain a 'map' (no 'ref' allowed).
-    fn def_shared_b(&mut self, name: &str) -> Result<Visit>;
-    fn def_shared_e(&mut self) -> Result<Visit>;
-
-    // A device contains a number of 'map' or 'ref' items.
-    fn device_b(&mut self, d: &Device) -> Result<Visit>;
-    fn device_e(&mut self) -> Result<Visit>;
-
-    fn map(&mut self, m: &Map) -> Result<Visit>;
-    fn ref_shared(&mut self, name: &str) -> Result<Visit>;
-
-    fn eof(&mut self) -> Result<Visit>;
-}
-
 pub struct XmlWriter<W: Write> {
     w: Writer<W>,
 }
@@ -178,7 +126,7 @@ impl<W: Write> MetadataVisitor for XmlWriter<W> {
 
 fn parse_superblock(e: &BytesStart) -> Result<Superblock> {
     let mut uuid: Option<String> = None;
-    let mut time: Option<u64> = None;
+    let mut time: Option<u32> = None;
     let mut transaction: Option<u64> = None;
     let mut flags: Option<u32> = None;
     let mut version: Option<u32> = None;
@@ -190,7 +138,7 @@ fn parse_superblock(e: &BytesStart) -> Result<Superblock> {
         let kv = a.unwrap();
         match kv.key {
             b"uuid" => uuid = Some(string_val(&kv)),
-            b"time" => time = Some(u64_val(&kv)?),
+            b"time" => time = Some(u32_val(&kv)?),
             b"transaction" => transaction = Some(u64_val(&kv)?),
             b"flags" => flags = Some(u32_val(&kv)?),
             b"version" => version = Some(u32_val(&kv)?),
@@ -235,8 +183,8 @@ fn parse_device(e: &BytesStart) -> Result<Device> {
     let mut dev_id: Option<u32> = None;
     let mut mapped_blocks: Option<u64> = None;
     let mut transaction: Option<u64> = None;
-    let mut creation_time: Option<u64> = None;
+    let mut creation_time: Option<u32> = None;
-    let mut snap_time: Option<u64> = None;
+    let mut snap_time: Option<u32> = None;
 
     for a in e.attributes() {
         let kv = a.unwrap();
@@ -244,8 +192,8 @@ fn parse_device(e: &BytesStart) -> Result<Device> {
             b"dev_id" => dev_id = Some(u32_val(&kv)?),
             b"mapped_blocks" => mapped_blocks = Some(u64_val(&kv)?),
             b"transaction" => transaction = Some(u64_val(&kv)?),
-            b"creation_time" => creation_time = Some(u64_val(&kv)?),
+            b"creation_time" => creation_time = Some(u32_val(&kv)?),
-            b"snap_time" => snap_time = Some(u64_val(&kv)?),
+            b"snap_time" => snap_time = Some(u32_val(&kv)?),
             _ => return bad_attr("device", kv.key),
         }
     }

tests/common/cache_xml_generator.rs

@@ -3,12 +3,13 @@ use rand::prelude::*;
 use std::collections::HashSet;
 use std::fs::OpenOptions;
 use std::path::Path;
+use thinp::cache::ir::{self, MetadataVisitor};
 use thinp::cache::xml;
 
 //------------------------------------------
 
 pub trait XmlGen {
-    fn generate_xml(&mut self, v: &mut dyn xml::MetadataVisitor) -> Result<()>;
+    fn generate_xml(&mut self, v: &mut dyn MetadataVisitor) -> Result<()>;
 }
 
 pub fn write_xml(path: &Path, g: &mut dyn XmlGen) -> Result<()> {
@@ -50,8 +51,8 @@ impl CacheGen {
 }
 
 impl XmlGen for CacheGen {
-    fn generate_xml(&mut self, v: &mut dyn xml::MetadataVisitor) -> Result<()> {
+    fn generate_xml(&mut self, v: &mut dyn MetadataVisitor) -> Result<()> {
-        v.superblock_b(&xml::Superblock {
+        v.superblock_b(&ir::Superblock {
             uuid: "".to_string(),
             block_size: self.block_size,
             nr_cache_blocks: self.nr_cache_blocks,
@@ -77,7 +78,7 @@ impl XmlGen for CacheGen {
 
                 used.insert(oblock);
                 // FIXME: dirty should vary
-                v.mapping(&xml::Map {
+                v.mapping(&ir::Map {
                     cblock: cblocks[n as usize],
                     oblock,
                     dirty: false,

tests/common/input_arg.rs

@@ -63,7 +63,7 @@ where
     P: InputProgram<'a>,
 {
     let args: [&str; 0] = [];
-    let stderr = run_fail(P::path(), &args)?;
+    let stderr = run_fail(P::path(), args)?;
     assert!(stderr.contains(P::missing_input_arg()));
     Ok(())
 }
@@ -85,8 +85,7 @@ where
     let mut td = TestDir::new()?;
     let output = mk_zeroed_md(&mut td)?;
     ensure_untouched(&output, || {
-        let args = args!["-o", &output];
+        let stderr = run_fail(P::path(), args!["-o", &output])?;
-        let stderr = run_fail(P::path(), &args)?;
         assert!(stderr.contains(P::missing_input_arg()));
         Ok(())
     })

tests/common/thin.rs

@@ -28,8 +28,7 @@ pub fn mk_valid_md(td: &mut TestDir) -> Result<PathBuf> {
     write_xml(&xml, &mut gen)?;
 
     let _file = file_utils::create_sized_file(&md, 4096 * 4096);
-    let args = args!["-i", &xml, "-o", &md];
+    run_ok(THIN_RESTORE, args!["-i", &xml, "-o", &md])?;
-    run_ok(THIN_RESTORE, &args)?;
 
     Ok(md)
 }
@@ -40,11 +39,11 @@ pub fn mk_valid_md(td: &mut TestDir) -> Result<PathBuf> {
 pub fn prep_metadata(td: &mut TestDir) -> Result<PathBuf> {
     let md = mk_zeroed_md(td)?;
     let args = args!["-o", &md, "--format", "--nr-data-blocks", "102400"];
-    run_ok(THIN_GENERATE_METADATA, &args)?;
+    run_ok(THIN_GENERATE_METADATA, args)?;
 
     // Create a 2GB device
     let args = args!["-o", &md, "--create-thin", "1"];
-    run_ok(THIN_GENERATE_METADATA, &args)?;
+    run_ok(THIN_GENERATE_METADATA, args)?;
     let args = args![
         "-o",
         &md,
@@ -55,7 +54,7 @@ pub fn prep_metadata(td: &mut TestDir) -> Result<PathBuf> {
         "--rw=randwrite",
         "--seq-nr=16"
     ];
-    run_ok(THIN_GENERATE_MAPPINGS, &args)?;
+    run_ok(THIN_GENERATE_MAPPINGS, args)?;
 
     // Take a few snapshots.
     let mut snap_id = 2;
@@ -63,7 +62,7 @@ pub fn prep_metadata(td: &mut TestDir) -> Result<PathBuf> {
         // take a snapshot
         let snap_id_str = snap_id.to_string();
         let args = args!["-o", &md, "--create-snap", &snap_id_str, "--origin", "1"];
-        run_ok(THIN_GENERATE_METADATA, &args)?;
+        run_ok(THIN_GENERATE_METADATA, args)?;
 
         // partially overwrite the origin (64MB)
         let args = args![
@@ -78,7 +77,7 @@ pub fn prep_metadata(td: &mut TestDir) -> Result<PathBuf> {
             "--rw=randwrite",
             "--seq-nr=16"
         ];
-        run_ok(THIN_GENERATE_MAPPINGS, &args)?;
+        run_ok(THIN_GENERATE_MAPPINGS, args)?;
         snap_id += 1;
     }
 
@@ -87,7 +86,7 @@ pub fn prep_metadata(td: &mut TestDir) -> Result<PathBuf> {
 
 pub fn set_needs_check(md: &PathBuf) -> Result<()> {
     let args = args!["-o", &md, "--set-needs-check"];
-    run_ok(THIN_GENERATE_METADATA, &args)?;
+    run_ok(THIN_GENERATE_METADATA, args)?;
     Ok(())
 }
 
@@ -111,7 +110,7 @@ pub fn generate_metadata_leaks(
         "--actual",
         &actual_str
     ];
-    run_ok(THIN_GENERATE_DAMAGE, &args)?;
+    run_ok(THIN_GENERATE_DAMAGE, args)?;
 
     Ok(())
 }

tests/common/thin_xml_generator.rs

@@ -4,12 +4,13 @@ use std::collections::VecDeque;
 use std::fs::OpenOptions;
 use std::ops::Range;
 use std::path::Path;
+use thinp::thin::ir::{self, MetadataVisitor};
 use thinp::thin::xml;
 
 //------------------------------------------
 
 pub trait XmlGen {
-    fn generate_xml(&mut self, v: &mut dyn xml::MetadataVisitor) -> Result<()>;
+    fn generate_xml(&mut self, v: &mut dyn MetadataVisitor) -> Result<()>;
 }
 
 pub fn write_xml(path: &Path, g: &mut dyn XmlGen) -> Result<()> {
@@ -24,8 +25,8 @@ pub fn write_xml(path: &Path, g: &mut dyn XmlGen) -> Result<()> {
     g.generate_xml(&mut w)
 }
 
-fn common_sb(nr_blocks: u64) -> xml::Superblock {
+fn common_sb(nr_blocks: u64) -> ir::Superblock {
-    xml::Superblock {
+    ir::Superblock {
         uuid: "".to_string(),
         time: 0,
         transaction: 0,
@@ -42,7 +43,7 @@ fn common_sb(nr_blocks: u64) -> xml::Superblock {
 pub struct EmptyPoolS {}
 
 impl XmlGen for EmptyPoolS {
-    fn generate_xml(&mut self, v: &mut dyn xml::MetadataVisitor) -> Result<()> {
+    fn generate_xml(&mut self, v: &mut dyn MetadataVisitor) -> Result<()> {
         v.superblock_b(&common_sb(1024))?;
         v.superblock_e()?;
         Ok(())
@@ -70,16 +71,16 @@ impl SingleThinS {
 }
 
 impl XmlGen for SingleThinS {
-    fn generate_xml(&mut self, v: &mut dyn xml::MetadataVisitor) -> Result<()> {
+    fn generate_xml(&mut self, v: &mut dyn MetadataVisitor) -> Result<()> {
         v.superblock_b(&common_sb(self.old_nr_data_blocks))?;
-        v.device_b(&xml::Device {
+        v.device_b(&ir::Device {
             dev_id: 0,
             mapped_blocks: self.len,
             transaction: 0,
             creation_time: 0,
             snap_time: 0,
         })?;
-        v.map(&xml::Map {
+        v.map(&ir::Map {
             thin_begin: 0,
             data_begin: self.offset,
             time: 0,
@@ -146,7 +147,7 @@ fn mk_runs(thin_id: u32, total_len: u64, run_len: std::ops::Range<u64>) -> Vec<T
 }
 
 impl XmlGen for FragmentedS {
-    fn generate_xml(&mut self, v: &mut dyn xml::MetadataVisitor) -> Result<()> {
+    fn generate_xml(&mut self, v: &mut dyn MetadataVisitor) -> Result<()> {
         // Allocate each thin fully, in runs between 1 and 16.
         let mut runs = Vec::new();
         for thin in 0..self.nr_thins {
@@ -188,7 +189,7 @@ impl XmlGen for FragmentedS {
         // write the xml
         v.superblock_b(&common_sb(self.old_nr_data_blocks))?;
         for thin in 0..self.nr_thins {
-            v.device_b(&xml::Device {
+            v.device_b(&ir::Device {
                 dev_id: thin,
                 mapped_blocks: self.thin_size,
                 transaction: 0,
@@ -201,7 +202,7 @@ impl XmlGen for FragmentedS {
                     continue;
                 }
 
-                v.map(&xml::Map {
+                v.map(&ir::Map {
                     thin_begin: m.thin_begin,
                     data_begin: m.data_begin,
                     time: 0,
@@ -329,8 +330,8 @@ struct ThinDev {
 }
 
 impl ThinDev {
-    fn emit(&self, v: &mut dyn xml::MetadataVisitor) -> Result<()> {
+    fn emit(&self, v: &mut dyn MetadataVisitor) -> Result<()> {
-        v.device_b(&xml::Device {
+        v.device_b(&ir::Device {
             dev_id: self.thin_id,
             mapped_blocks: self.dev_size,
             transaction: 0,
@@ -342,7 +343,7 @@ impl ThinDev {
         for r in &self.runs {
             match r {
                 Run::Mapped { data_begin, len } => {
-                    v.map(&xml::Map {
+                    v.map(&ir::Map {
                         thin_begin: b,
                         data_begin: *data_begin,
                         time: 0,
@@ -522,7 +523,7 @@ impl SnapS {
 }
 
 impl XmlGen for SnapS {
-    fn generate_xml(&mut self, v: &mut dyn xml::MetadataVisitor) -> Result<()> {
+    fn generate_xml(&mut self, v: &mut dyn MetadataVisitor) -> Result<()> {
         let mut allocator = Allocator::new_shuffled(self.old_nr_data_blocks, 64..512);
         let origin = mk_origin(0, self.len, &mut allocator)?;
 

tests/thin_shrink.rs

@@ -6,7 +6,8 @@ use std::io::{Cursor, Read, Seek, SeekFrom, Write};
 use std::path::Path;
 
 use thinp::file_utils;
-use thinp::thin::xml::{self, Visit};
+use thinp::thin::ir::{self, MetadataVisitor, Visit};
+use thinp::thin::xml;
 
 mod common;
 use common::test_dir::*;
@@ -91,8 +92,8 @@ struct ThinXmlVisitor<'a, V: ThinVisitor> {
     thin_id: Option<u32>,
 }
 
-impl<'a, V: ThinVisitor> xml::MetadataVisitor for ThinXmlVisitor<'a, V> {
+impl<'a, V: ThinVisitor> MetadataVisitor for ThinXmlVisitor<'a, V> {
-    fn superblock_b(&mut self, sb: &xml::Superblock) -> Result<Visit> {
+    fn superblock_b(&mut self, sb: &ir::Superblock) -> Result<Visit> {
         self.block_size = Some(sb.data_block_size);
         Ok(Visit::Continue)
     }
@@ -109,7 +110,7 @@ impl<'a, V: ThinVisitor> xml::MetadataVisitor for ThinXmlVisitor<'a, V> {
         todo!();
     }
 
-    fn device_b(&mut self, d: &xml::Device) -> Result<Visit> {
+    fn device_b(&mut self, d: &ir::Device) -> Result<Visit> {
         self.thin_id = Some(d.dev_id);
         Ok(Visit::Continue)
     }
@@ -118,7 +119,7 @@ impl<'a, V: ThinVisitor> xml::MetadataVisitor for ThinXmlVisitor<'a, V> {
         Ok(Visit::Continue)
     }
 
-    fn map(&mut self, m: &xml::Map) -> Result<Visit> {
+    fn map(&mut self, m: &ir::Map) -> Result<Visit> {
         for i in 0..m.len {
             let block = ThinBlock {
                 thin_id: self.thin_id.unwrap(),