From c94f560be8fe67f64d511cbbb84d84ef8b10a436 Mon Sep 17 00:00:00 2001 From: Joe Thornber Date: Thu, 28 May 2020 14:35:51 +0100 Subject: [PATCH 1/6] [thin_check] Check the ref counts in the data space map. Hadn't realised this was being done. --- thin-provisioning/metadata_checker.cc | 94 ++++++++++++++++++++++----- thin-provisioning/metadata_checker.h | 10 +-- 2 files changed, 84 insertions(+), 20 deletions(-) diff --git a/thin-provisioning/metadata_checker.cc b/thin-provisioning/metadata_checker.cc index 02c12c6..3e48f87 100644 --- a/thin-provisioning/metadata_checker.cc +++ b/thin-provisioning/metadata_checker.cc @@ -18,11 +18,13 @@ #include "base/nested_output.h" #include "persistent-data/file_utils.h" +#include "persistent-data/space-maps/core.h" #include "thin-provisioning/metadata.h" #include "thin-provisioning/metadata_checker.h" #include "thin-provisioning/metadata_counter.h" #include "thin-provisioning/superblock.h" +using namespace boost; using namespace persistent_data; using namespace thin_provisioning; @@ -84,6 +86,20 @@ namespace { //-------------------------------- + class data_ref_counter : public mapping_tree_detail::mapping_visitor { + public: + data_ref_counter(space_map::ptr sm) + : sm_(sm) { + } + + virtual void visit(btree_path const &path, mapping_tree_detail::block_time const &bt) { + sm_->inc(bt.block_); + } + + private: + space_map::ptr sm_; + }; + class mapping_reporter : public mapping_tree_detail::damage_visitor { public: mapping_reporter(nested_output &out) @@ -161,14 +177,20 @@ namespace { error_state examine_mapping_tree_(transaction_manager::ptr tm, superblock_detail::superblock const &sb, - nested_output &out) { + nested_output &out, + optional data_sm) { out << "examining mapping tree" << end_message(); nested_output::nest _ = out.push(); mapping_reporter mapping_rep(out); mapping_tree mtree(*tm, sb.data_mapping_root_, mapping_tree_detail::block_traits::ref_counter(tm->get_sm())); - check_mapping_tree(mtree, mapping_rep); + + if (data_sm) { + data_ref_counter dcounter(*data_sm); + walk_mapping_tree(mtree, dcounter, mapping_rep); + } else + check_mapping_tree(mtree, mapping_rep); return mapping_rep.get_error(); } @@ -184,9 +206,10 @@ namespace { error_state examine_mapping_tree(transaction_manager::ptr tm, superblock_detail::superblock const &sb, - nested_output &out) { + nested_output &out, + optional data_sm) { error_state err = examine_devices_tree_(tm, sb, out); - err << examine_mapping_tree_(tm, sb, out); + err << examine_mapping_tree_(tm, sb, out, data_sm); return err; } @@ -222,6 +245,34 @@ namespace { return err; } + error_state compare_space_maps(space_map::ptr actual, space_map::ptr expected, + nested_output &out) + { + error_state err = NO_ERROR; + auto nr_blocks = actual->get_nr_blocks(); + + if (expected->get_nr_blocks() != nr_blocks) { + out << "internal error: nr blocks in space maps differ" + << end_message(); + err << FATAL; + } else { + for (block_address b = 0; b < nr_blocks; b++) { + auto a_count = actual->get_count(b); + auto e_count = actual->get_count(b); + + if (a_count != e_count) { + out << "data reference counts differ for block " << b + << ", expected " << e_count + << ", but got " << a_count + << end_message(); + err << (a_count > e_count ? NON_FATAL : FATAL); + } + } + } + + return err; + } + void print_info(transaction_manager::ptr tm, superblock_detail::superblock const &sb, nested_output &out) @@ -272,12 +323,22 @@ namespace { print_info(tm, sb, info_out_); - err << examine_data_mappings(tm, sb, options_.check_data_mappings_, out_); + if (options_.sm_opts_ == check_options::SPACE_MAP_FULL) { + space_map::ptr data_sm{open_disk_sm(*tm, &sb.data_space_map_root_)}; + optional core_sm{create_core_map(data_sm->get_nr_blocks())}; + err << examine_data_mappings(tm, sb, options_.check_data_mappings_, out_, core_sm); - // if we're checking everything, and there were no errors, - // then we should check the space maps too. - if (err != FATAL) - err << examine_metadata_space_map(tm, sb, options_.check_metadata_space_map_, out_); + // if we're checking everything, and there were no errors, + // then we should check the space maps too. + if (err != FATAL) { + err << examine_metadata_space_map(tm, sb, options_.sm_opts_, out_); + + if (core_sm) + err << compare_space_maps(data_sm, *core_sm, out_); + } + } else + err << examine_data_mappings(tm, sb, options_.check_data_mappings_, out_, + optional()); return err; } @@ -287,7 +348,8 @@ namespace { examine_data_mappings(transaction_manager::ptr tm, superblock_detail::superblock const &sb, check_options::data_mapping_options option, - nested_output &out) { + nested_output &out, + optional data_sm) { error_state err = NO_ERROR; switch (option) { @@ -295,7 +357,7 @@ namespace { err << examine_top_level_mapping_tree(tm, sb, out); break; case check_options::DATA_MAPPING_LEVEL2: - err << examine_mapping_tree(tm, sb, out); + err << examine_mapping_tree(tm, sb, out, data_sm); break; default: break; // do nothing @@ -307,12 +369,12 @@ namespace { static error_state examine_metadata_space_map(transaction_manager::ptr tm, superblock_detail::superblock const &sb, - check_options::metadata_space_map_options option, + check_options::space_map_options option, nested_output &out) { error_state err = NO_ERROR; switch (option) { - case check_options::METADATA_SPACE_MAP_FULL: + case check_options::SPACE_MAP_FULL: err << check_space_map_counts(tm, sb, out); break; default: @@ -333,17 +395,17 @@ namespace { check_options::check_options() : check_data_mappings_(DATA_MAPPING_LEVEL2), - check_metadata_space_map_(METADATA_SPACE_MAP_FULL) { + sm_opts_(SPACE_MAP_FULL) { } void check_options::set_superblock_only() { check_data_mappings_ = DATA_MAPPING_NONE; - check_metadata_space_map_ = METADATA_SPACE_MAP_NONE; + sm_opts_ = SPACE_MAP_NONE; } void check_options::set_skip_mappings() { check_data_mappings_ = DATA_MAPPING_LEVEL1; - check_metadata_space_map_ = METADATA_SPACE_MAP_NONE; + sm_opts_ = SPACE_MAP_NONE; } void check_options::set_override_mapping_root(block_address b) { diff --git a/thin-provisioning/metadata_checker.h b/thin-provisioning/metadata_checker.h index 7cf6683..8f058fc 100644 --- a/thin-provisioning/metadata_checker.h +++ b/thin-provisioning/metadata_checker.h @@ -33,9 +33,9 @@ namespace thin_provisioning { DATA_MAPPING_LEVEL2, }; - enum metadata_space_map_options { - METADATA_SPACE_MAP_NONE, - METADATA_SPACE_MAP_FULL, + enum space_map_options { + SPACE_MAP_NONE, + SPACE_MAP_FULL, }; check_options(); @@ -45,7 +45,9 @@ namespace thin_provisioning { void set_override_mapping_root(bcache::block_address b); data_mapping_options check_data_mappings_; - metadata_space_map_options check_metadata_space_map_; + + space_map_options sm_opts_; + boost::optional override_mapping_root_; }; From 16a10d2554d97f69aec25cee524f4babd734d5e5 Mon Sep 17 00:00:00 2001 From: Joe Thornber Date: Thu, 28 May 2020 14:43:03 +0100 Subject: [PATCH 2/6] [thin_check] Remove the metadata_checker base class. It's really only a single method. --- thin-provisioning/metadata_checker.cc | 21 ++++++++++----------- thin-provisioning/metadata_checker.h | 17 ++++------------- thin-provisioning/thin_check.cc | 3 +-- 3 files changed, 15 insertions(+), 26 deletions(-) diff --git a/thin-provisioning/metadata_checker.cc b/thin-provisioning/metadata_checker.cc index 3e48f87..21383bb 100644 --- a/thin-provisioning/metadata_checker.cc +++ b/thin-provisioning/metadata_checker.cc @@ -289,11 +289,11 @@ namespace { //-------------------------------- - class base_metadata_checker : public metadata_checker { + class metadata_checker { public: - base_metadata_checker(block_manager::ptr bm, - check_options check_opts, - output_options output_opts) + metadata_checker(block_manager::ptr bm, + check_options check_opts, + output_options output_opts) : bm_(bm), options_(check_opts), out_(cerr, 2), @@ -412,14 +412,13 @@ void check_options::set_override_mapping_root(block_address b) { override_mapping_root_ = b; } -metadata_checker::ptr -thin_provisioning::create_base_checker(block_manager::ptr bm, - check_options const &check_opts, - output_options output_opts) +base::error_state +thin_provisioning::check_metadata(block_manager::ptr bm, + check_options const &check_opts, + output_options output_opts) { - metadata_checker::ptr checker; - checker = metadata_checker::ptr(new base_metadata_checker(bm, check_opts, output_opts)); - return checker; + metadata_checker checker(bm, check_opts, output_opts); + return checker.check(); } //---------------------------------------------------------------- diff --git a/thin-provisioning/metadata_checker.h b/thin-provisioning/metadata_checker.h index 8f058fc..6f25587 100644 --- a/thin-provisioning/metadata_checker.h +++ b/thin-provisioning/metadata_checker.h @@ -56,19 +56,10 @@ namespace thin_provisioning { OUTPUT_QUIET, }; - class metadata_checker { - public: - typedef std::shared_ptr ptr; - - virtual ~metadata_checker() {} - - virtual base::error_state check() = 0; - }; - - metadata_checker::ptr - create_base_checker(persistent_data::block_manager::ptr bm, - check_options const &check_opts, - output_options output_opts); + base::error_state + check_metadata(persistent_data::block_manager::ptr bm, + check_options const &check_opts, + output_options output_opts); } //---------------------------------------------------------------- diff --git a/thin-provisioning/thin_check.cc b/thin-provisioning/thin_check.cc index 79ea171..6b65395 100644 --- a/thin-provisioning/thin_check.cc +++ b/thin-provisioning/thin_check.cc @@ -78,8 +78,7 @@ namespace { block_manager::ptr bm = open_bm(path); output_options output_opts = !fs.quiet ? OUTPUT_NORMAL : OUTPUT_QUIET; - metadata_checker::ptr checker = create_base_checker(bm, fs.check_opts, output_opts); - error_state err = checker->check(); + error_state err = check_metadata(bm, fs.check_opts, output_opts); if (fs.ignore_non_fatal_errors) success = (err == FATAL) ? false : true; From 86704deacb8b21ffdc64cd9ef1ee54d5e22ac1a1 Mon Sep 17 00:00:00 2001 From: Joe Thornber Date: Tue, 9 Jun 2020 14:19:59 +0100 Subject: [PATCH 3/6] [thin_check] Add support for --metadata-snap. Not tested yet. --- .../scenario-string-constants.scm | 1 + thin-provisioning/metadata_checker.cc | 23 ++++++++++---- thin-provisioning/metadata_checker.h | 4 +-- thin-provisioning/superblock.cc | 5 ++-- thin-provisioning/superblock.h | 3 +- thin-provisioning/thin_check.cc | 30 +++++++++++++------ 6 files changed, 46 insertions(+), 20 deletions(-) diff --git a/functional-tests/scenario-string-constants.scm b/functional-tests/scenario-string-constants.scm index 88c9ab9..70ae39c 100644 --- a/functional-tests/scenario-string-constants.scm +++ b/functional-tests/scenario-string-constants.scm @@ -29,6 +29,7 @@ Options: {-q|--quiet} {-h|--help} {-V|--version} + {-m|--metadata-snap} {--override-mapping-root} {--clear-needs-check-flag} {--ignore-non-fatal-errors} diff --git a/thin-provisioning/metadata_checker.cc b/thin-provisioning/metadata_checker.cc index 21383bb..2c17caf 100644 --- a/thin-provisioning/metadata_checker.cc +++ b/thin-provisioning/metadata_checker.cc @@ -137,12 +137,13 @@ namespace { //-------------------------------- error_state examine_superblock(block_manager::ptr bm, + block_address sb_location, nested_output &out) { out << "examining superblock" << end_message(); nested_output::nest _ = out.push(); superblock_reporter sb_rep(out); - check_superblock(bm, sb_rep); + check_superblock(bm, sb_rep, sb_location); return sb_rep.get_error(); } @@ -307,18 +308,24 @@ namespace { error_state check() { error_state err = NO_ERROR; + auto sb_location = superblock_detail::SUPERBLOCK_LOCATION; - err << examine_superblock(bm_, out_); - + if (options_.use_metadata_snap_) { + superblock_detail::superblock sb = read_superblock(bm_, sb_location); + sb_location = sb.metadata_snap_; + if (sb_location == superblock_detail::SUPERBLOCK_LOCATION) + throw runtime_error("No metadata snapshot found."); + } + + err << examine_superblock(bm_, sb_location, out_); if (err == FATAL) { if (check_for_xml(bm_)) out_ << "This looks like XML. thin_check only checks the binary metadata format." << end_message(); return err; } - superblock_detail::superblock sb = read_superblock(bm_); - transaction_manager::ptr tm = - open_tm(bm_, superblock_detail::SUPERBLOCK_LOCATION); + superblock_detail::superblock sb = read_superblock(bm_, sb_location); + transaction_manager::ptr tm = open_tm(bm_, sb_location); sb.data_mapping_root_ = mapping_root(sb, options_); print_info(tm, sb, info_out_); @@ -412,6 +419,10 @@ void check_options::set_override_mapping_root(block_address b) { override_mapping_root_ = b; } +void check_options::set_metadata_snap() { + sm_opts_ = SPACE_MAP_NONE; +} + base::error_state thin_provisioning::check_metadata(block_manager::ptr bm, check_options const &check_opts, diff --git a/thin-provisioning/metadata_checker.h b/thin-provisioning/metadata_checker.h index 6f25587..1b94d7e 100644 --- a/thin-provisioning/metadata_checker.h +++ b/thin-provisioning/metadata_checker.h @@ -43,11 +43,11 @@ namespace thin_provisioning { void set_superblock_only(); void set_skip_mappings(); void set_override_mapping_root(bcache::block_address b); + void set_metadata_snap(); + bool use_metadata_snap_; data_mapping_options check_data_mappings_; - space_map_options sm_opts_; - boost::optional override_mapping_root_; }; diff --git a/thin-provisioning/superblock.cc b/thin-provisioning/superblock.cc index 7b1c493..3521f1d 100644 --- a/thin-provisioning/superblock.cc +++ b/thin-provisioning/superblock.cc @@ -194,11 +194,12 @@ namespace thin_provisioning { void check_superblock(block_manager::ptr bm, - superblock_detail::damage_visitor &visitor) { + superblock_detail::damage_visitor &visitor, + block_address sb_location) { using namespace superblock_detail; try { - bm->read_lock(SUPERBLOCK_LOCATION, superblock_validator()); + bm->read_lock(sb_location, superblock_validator()); } catch (std::exception const &e) { visitor.visit(superblock_corruption(e.what())); diff --git a/thin-provisioning/superblock.h b/thin-provisioning/superblock.h index 62bceb7..9704062 100644 --- a/thin-provisioning/superblock.h +++ b/thin-provisioning/superblock.h @@ -139,7 +139,8 @@ namespace thin_provisioning { superblock_detail::superblock const &sb); void check_superblock(persistent_data::block_manager::ptr bm, - superblock_detail::damage_visitor &visitor); + superblock_detail::damage_visitor &visitor, + persistent_data::block_address sb_location = superblock_detail::SUPERBLOCK_LOCATION); } //---------------------------------------------------------------- diff --git a/thin-provisioning/thin_check.cc b/thin-provisioning/thin_check.cc index 6b65395..c8397ed 100644 --- a/thin-provisioning/thin_check.cc +++ b/thin-provisioning/thin_check.cc @@ -109,15 +109,16 @@ thin_check_cmd::thin_check_cmd() void thin_check_cmd::usage(std::ostream &out) const { - out << "Usage: " << get_name() << " [options] {device|file}" << endl - << "Options:" << endl - << " {-q|--quiet}" << endl - << " {-h|--help}" << endl - << " {-V|--version}" << endl - << " {--override-mapping-root}" << endl - << " {--clear-needs-check-flag}" << endl - << " {--ignore-non-fatal-errors}" << endl - << " {--skip-mappings}" << endl + out << "Usage: " << get_name() << " [options] {device|file}\n" + << "Options:\n" + << " {-q|--quiet}\n" + << " {-h|--help}\n" + << " {-V|--version}\n" + << " {-m|--metadata-snap}\n" + << " {--override-mapping-root}\n" + << " {--clear-needs-check-flag}\n" + << " {--ignore-non-fatal-errors}\n" + << " {--skip-mappings}\n" << " {--super-block-only}" << endl; } @@ -132,6 +133,7 @@ thin_check_cmd::run(int argc, char **argv) { "quiet", no_argument, NULL, 'q'}, { "help", no_argument, NULL, 'h'}, { "version", no_argument, NULL, 'V'}, + { "metadata-snap", no_argument, NULL, 'm'}, { "super-block-only", no_argument, NULL, 1}, { "skip-mappings", no_argument, NULL, 2}, { "ignore-non-fatal-errors", no_argument, NULL, 3}, @@ -154,6 +156,10 @@ thin_check_cmd::run(int argc, char **argv) cout << THIN_PROVISIONING_TOOLS_VERSION << endl; return 0; + case 'm': + fs.check_opts.set_metadata_snap(); + break; + case 1: // super-block-only fs.check_opts.set_superblock_only(); @@ -185,6 +191,12 @@ thin_check_cmd::run(int argc, char **argv) } } + if (fs.clear_needs_check_flag_on_success && fs.check_opts.use_metadata_snap_) { + cerr << "--metadata-snap cannot be combined with --clear-needs-check-flag."; + usage(cerr); + exit(1); + } + if (argc == optind) { if (!fs.quiet) { cerr << "No input file provided." << endl; From 324c0050bf9351288459636efb7de9c938117787 Mon Sep 17 00:00:00 2001 From: Joe Thornber Date: Wed, 10 Jun 2020 10:32:36 +0100 Subject: [PATCH 4/6] [install-rust-tools] add dep to generate man pages --- Makefile.in | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/Makefile.in b/Makefile.in index 8ba7622..fbdf181 100644 --- a/Makefile.in +++ b/Makefile.in @@ -318,7 +318,6 @@ install: bin/pdata_tools $(MANPAGES) $(INSTALL_DATA) man8/thin_repair.8 $(MANPATH)/man8 $(INSTALL_DATA) man8/thin_restore.8 $(MANPATH)/man8 $(INSTALL_DATA) man8/thin_rmap.8 $(MANPATH)/man8 - $(INSTALL_DATA) man8/thin_metadata_size.8 $(MANPATH)/man8 $(INSTALL_DATA) man8/era_check.8 $(MANPATH)/man8 $(INSTALL_DATA) man8/era_dump.8 $(MANPATH)/man8 @@ -333,10 +332,16 @@ ifeq ("@DEVTOOLS@", "yes") ln -s -f pdata_tools $(BINDIR)/thin_scan endif -.PHONY: install install-rust-tools +.PHONY: install install-rust-tools rust-tools -install-rust-tools: - cargo install --path . --root $(BINDIR) +rust-tools: + cargo build --release + +install-rust-tools: man8/thin_metadata_pack.8 man8/thin_metadata_unpack.8 rust-tools + $(INSTALL_PROGRAM) target/release/thin_metadata_pack $(BINDIR) + $(INSTALL_PROGRAM) target/release/thin_metadata_unpack $(BINDIR) + $(STRIP) $(BINDIR)/thin_metadata_pack + $(STRIP) $(BINDIR)/thin_metadata_unpack $(INSTALL_DATA) man8/thin_metadata_pack.8 $(MANPATH)/man8 $(INSTALL_DATA) man8/thin_metadata_unpack.8 $(MANPATH)/man8 From 0b5afc6cb02df4fd3908c75307cf6c0303d9d81a Mon Sep 17 00:00:00 2001 From: Joe Thornber Date: Fri, 12 Jun 2020 13:41:47 +0100 Subject: [PATCH 5/6] [thin_check] fix bugs in thin_check -m --- thin-provisioning/metadata_checker.cc | 4 +++- thin-provisioning/thin_check.cc | 5 +++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/thin-provisioning/metadata_checker.cc b/thin-provisioning/metadata_checker.cc index 2c17caf..ed3d20d 100644 --- a/thin-provisioning/metadata_checker.cc +++ b/thin-provisioning/metadata_checker.cc @@ -401,7 +401,8 @@ namespace { //---------------------------------------------------------------- check_options::check_options() - : check_data_mappings_(DATA_MAPPING_LEVEL2), + : use_metadata_snap_(false), + check_data_mappings_(DATA_MAPPING_LEVEL2), sm_opts_(SPACE_MAP_FULL) { } @@ -420,6 +421,7 @@ void check_options::set_override_mapping_root(block_address b) { } void check_options::set_metadata_snap() { + use_metadata_snap_ = true; sm_opts_ = SPACE_MAP_NONE; } diff --git a/thin-provisioning/thin_check.cc b/thin-provisioning/thin_check.cc index c8397ed..05f5582 100644 --- a/thin-provisioning/thin_check.cc +++ b/thin-provisioning/thin_check.cc @@ -76,7 +76,8 @@ namespace { return 1; } - block_manager::ptr bm = open_bm(path); + block_manager::ptr bm = open_bm(path, block_manager::READ_ONLY, + !fs.check_opts.use_metadata_snap_); output_options output_opts = !fs.quiet ? OUTPUT_NORMAL : OUTPUT_QUIET; error_state err = check_metadata(bm, fs.check_opts, output_opts); @@ -128,7 +129,7 @@ thin_check_cmd::run(int argc, char **argv) int c; flags fs; - char const shortopts[] = "qhV"; + char const shortopts[] = "qhVm"; option const longopts[] = { { "quiet", no_argument, NULL, 'q'}, { "help", no_argument, NULL, 'h'}, From dc5bb3559beb648582bda352cbb4940816565e0d Mon Sep 17 00:00:00 2001 From: Joe Thornber Date: Fri, 12 Jun 2020 19:09:19 +0100 Subject: [PATCH 6/6] [build] remove need for zlib. Disappeared with the C++ version of thin_metadata_pack --- Makefile.in | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile.in b/Makefile.in index fbdf181..59f8f14 100644 --- a/Makefile.in +++ b/Makefile.in @@ -167,7 +167,7 @@ CXXFLAGS+=@CXXDEBUG_FLAG@ CXXFLAGS+=@CXX_STRERROR_FLAG@ CXXFLAGS+=@LFS_FLAGS@ INCLUDES+=-I$(TOP_BUILDDIR) -I$(TOP_DIR) -I$(TOP_DIR)/thin-provisioning -LIBS:=-laio -lexpat -lz -lboost_iostreams -ldl +LIBS:=-laio -lexpat -lboost_iostreams -ldl ifeq ("@DEVTOOLS@", "yes") LIBS+=-lncurses