From b42408ef41e5df44bdd627bdfc2ae0b487e4f4b2 Mon Sep 17 00:00:00 2001 From: Ming-Hung Tsai Date: Mon, 19 Oct 2020 16:25:10 +0800 Subject: [PATCH 1/9] [thin] Introduce thin_patch_superblock to override superblock fields --- Makefile.in | 1 + thin-provisioning/commands.cc | 1 + thin-provisioning/commands.h | 7 + thin-provisioning/thin_patch_superblock.cc | 193 +++++++++++++++++++++ 4 files changed, 202 insertions(+) create mode 100644 thin-provisioning/thin_patch_superblock.cc diff --git a/Makefile.in b/Makefile.in index b1fd4aa..2c3c09f 100644 --- a/Makefile.in +++ b/Makefile.in @@ -134,6 +134,7 @@ DEVTOOLS_SOURCE=\ thin-provisioning/thin_generate_metadata.cc \ thin-provisioning/thin_generate_mappings.cc \ thin-provisioning/variable_chunk_stream.cc \ + thin-provisioning/thin_patch_superblock.cc \ thin-provisioning/thin_show_metadata.cc \ thin-provisioning/thin_scan.cc \ ui/ui.cc diff --git a/thin-provisioning/commands.cc b/thin-provisioning/commands.cc index 23399af..262b02d 100644 --- a/thin-provisioning/commands.cc +++ b/thin-provisioning/commands.cc @@ -25,6 +25,7 @@ thin_provisioning::register_thin_commands(base::application &app) app.add_cmd(command::ptr(new thin_generate_damage_cmd())); app.add_cmd(command::ptr(new thin_generate_metadata_cmd())); app.add_cmd(command::ptr(new thin_generate_mappings_cmd())); + app.add_cmd(command::ptr(new thin_patch_superblock_cmd())); app.add_cmd(command::ptr(new thin_show_duplicates_cmd())); app.add_cmd(command::ptr(new thin_show_metadata_cmd())); app.add_cmd(command::ptr(new thin_journal_cmd())); diff --git a/thin-provisioning/commands.h b/thin-provisioning/commands.h index 906c340..88e1346 100644 --- a/thin-provisioning/commands.h +++ b/thin-provisioning/commands.h @@ -124,6 +124,13 @@ namespace thin_provisioning { virtual int run(int argc, char **argv); }; + class thin_patch_superblock_cmd : public base::command { + public: + thin_patch_superblock_cmd(); + virtual void usage(std::ostream &out) const; + virtual int run(int argc, char **argv); + }; + class thin_show_metadata_cmd : public base::command { public: thin_show_metadata_cmd(); diff --git a/thin-provisioning/thin_patch_superblock.cc b/thin-provisioning/thin_patch_superblock.cc new file mode 100644 index 0000000..7150805 --- /dev/null +++ b/thin-provisioning/thin_patch_superblock.cc @@ -0,0 +1,193 @@ +#include + +#include "persistent-data/file_utils.h" +#include "persistent-data/space-maps/disk_structures.h" +#include "thin-provisioning/commands.h" +#include "thin-provisioning/metadata.h" +#include "thin-provisioning/superblock.h" +#include "version.h" + +using namespace persistent_data; +using namespace sm_disk_detail; +using namespace thin_provisioning; + +//---------------------------------------------------------------- + +namespace { + struct flags { + flags(); + bool check_conformance(); + + string output; + + boost::optional transaction_id; + block_address data_mapping_root; + block_address device_details_root; + + block_address metadata_bitmap_root; + block_address metadata_ref_count_root; + + block_address data_bitmap_root; + block_address data_ref_count_root; + }; + + flags::flags() + : data_mapping_root(0), device_details_root(0), + metadata_bitmap_root(0), metadata_ref_count_root(0), + data_bitmap_root(0), data_ref_count_root(0) { + } + + bool flags::check_conformance() { + if (!output.size()) + return false; + + return true; + } + + void patch_space_map_root(void *root, + block_address bitmap_root, + block_address ref_count_root) { + sm_disk_detail::sm_root v; + sm_disk_detail::sm_root_disk d; + + memcpy(&d, root, sizeof(d)); + sm_root_traits::unpack(d, v); + + if (bitmap_root) + v.bitmap_root_ = bitmap_root; + if (ref_count_root) + v.ref_count_root_ = ref_count_root; + + sm_root_traits::pack(v, d); + memcpy(root, &d, sizeof(d)); + } + + int patch_superblock(flags const &fs) { + block_manager::ptr bm = open_bm(fs.output, block_manager::READ_WRITE); + superblock_detail::superblock sb = read_superblock(bm, superblock_detail::SUPERBLOCK_LOCATION); + + if (fs.transaction_id) + sb.trans_id_ = *fs.transaction_id; + + if (fs.data_mapping_root) + sb.data_mapping_root_ = fs.data_mapping_root; + + if (fs.device_details_root) + sb.device_details_root_ = fs.device_details_root; + + patch_space_map_root(sb.metadata_space_map_root_, + fs.metadata_bitmap_root, + fs.metadata_ref_count_root); + + patch_space_map_root(sb.data_space_map_root_, + fs.data_bitmap_root, + fs.data_ref_count_root); + + write_superblock(bm, sb); + + return 0; + } +} + +//---------------------------------------------------------------- + +thin_patch_superblock_cmd::thin_patch_superblock_cmd() + : command("thin_patch_superblock") +{ +} + +void +thin_patch_superblock_cmd::usage(std::ostream &out) const +{ + out << "Usage: " << get_name() << " [options]\n" + << "Options:\n" + << " {-h|--help}\n" + << " {-o|--output} \n" + << " {--transaction-id} \n" + << " {--data-mapping-root} \n" + << " {--device-details-root} \n" + << " {--metadata-bitmap-root} \n" + << " {--metadata-ref-count-root} \n" + << " {--data-bitmap-root} \n" + << " {--data-ref-count-root} \n" + << " {-V|--version}" << endl; +} + +int +thin_patch_superblock_cmd::run(int argc, char **argv) +{ + int c; + flags fs; + const char shortopts[] = "ho:V"; + + const struct option longopts[] = { + { "help", no_argument, NULL, 'h'}, + { "output", required_argument, NULL, 'o'}, + { "transaction-id", required_argument, NULL, 1 }, + { "data-mapping-root", required_argument, NULL, 2 }, + { "device-details-root", required_argument, NULL, 3 }, + { "metadata-bitmap-root", required_argument, NULL, 4 }, + { "metadata-ref-count-root", required_argument, NULL, 5 }, + { "data-bitmap-root", required_argument, NULL, 6 }, + { "data-ref-count-root", required_argument, NULL, 7 }, + { "version", no_argument, NULL, 'V'}, + { NULL, no_argument, NULL, 0 } + }; + + while ((c = getopt_long(argc, argv, shortopts, longopts, NULL)) != -1) { + switch(c) { + case 'h': + usage(cout); + return 0; + + case 'o': + fs.output = optarg; + break; + + case 1: + fs.transaction_id = parse_uint64(optarg, "transaction_id"); + break; + + case 2: + fs.data_mapping_root = parse_uint64(optarg, "data_mapping_root"); + break; + + case 3: + fs.device_details_root = parse_uint64(optarg, "device_details_root"); + break; + + case 4: + fs.metadata_bitmap_root = parse_uint64(optarg, "metadata_bitmap_root"); + break; + + case 5: + fs.metadata_ref_count_root = parse_uint64(optarg, "metadata_ref_count_root"); + break; + + case 6: + fs.data_bitmap_root = parse_uint64(optarg, "data_bitmap_root"); + break; + + case 7: + fs.data_ref_count_root = parse_uint64(optarg, "data_ref_count_root"); + break; + + case 'V': + cout << THIN_PROVISIONING_TOOLS_VERSION << endl; + return 0; + + default: + usage(cerr); + return 1; + } + } + + if (!fs.check_conformance()) { + usage(cerr); + return 1; + } + + return patch_superblock(fs); +} + +//---------------------------------------------------------------- From 1d5b52b0dda512f85dc8bc5ea40321eccaf4b393 Mon Sep 17 00:00:00 2001 From: Ming-Hung Tsai Date: Tue, 20 Oct 2020 15:18:06 +0800 Subject: [PATCH 2/9] [thin_delta] Clean up duplicated code --- thin-provisioning/thin_delta.cc | 105 ++++++++++---------------------- 1 file changed, 31 insertions(+), 74 deletions(-) diff --git a/thin-provisioning/thin_delta.cc b/thin-provisioning/thin_delta.cc index 5b31608..e5b368f 100644 --- a/thin-provisioning/thin_delta.cc +++ b/thin-provisioning/thin_delta.cc @@ -22,47 +22,7 @@ using namespace thin_provisioning; //---------------------------------------------------------------- -namespace local { - class application { - public: - application(string const &cmd) - : cmd_(cmd) { - } - - void usage(ostream &out) { - out << "Usage: " << cmd_ << " [options] \n" - << "Options:\n" - << " {--thin1, --snap1}\n" - << " {--thin2, --snap2}\n" - << " {-m, --metadata-snap} [block#]\n" - << " {--verbose}\n" - << " {-h|--help}\n" - << " {-V|--version}" << endl; - } - - void die(string const &msg) { - cerr << msg << endl; - usage(cerr); - exit(1); - } - - uint64_t parse_int(string const &str, string const &desc) { - try { - return boost::lexical_cast(str); - - } catch (...) { - ostringstream out; - out << "Couldn't parse " << desc << ": '" << str << "'"; - die(out.str()); - } - - return 0; // never get here - } - - private: - string cmd_; - }; - +namespace { struct flags { flags() : verbose(false), @@ -96,13 +56,6 @@ namespace local { uint64_t vbegin_, dbegin_, len_; }; - ostream &operator <<(ostream &out, mapping const &m) { - out << "mapping[vbegin = " << m.vbegin_ - << ", dbegin = " << m.dbegin_ - << ", len = " << m.len_ << "]"; - return out; - } - //-------------------------------- template @@ -540,7 +493,7 @@ namespace local { out << "\n"; } - void delta_(application &app, flags const &fs) { + void delta_(flags const &fs) { mapping_recorder mr1; mapping_recorder mr2; damage_visitor damage_v; @@ -558,7 +511,7 @@ namespace local { if (!snap1_root) { ostringstream out; out << "Unable to find mapping tree for snap1 (" << *fs.snap1 << ")"; - app.die(out.str()); + throw std::runtime_error(out.str()); } single_mapping_tree snap1(*md->tm_, *snap1_root, @@ -570,7 +523,7 @@ namespace local { if (!snap2_root) { ostringstream out; out << "Unable to find mapping tree for snap2 (" << *fs.snap2 << ")"; - app.die(out.str()); + throw std::runtime_error(out.str()); } single_mapping_tree snap2(*md->tm_, *snap2_root, @@ -607,12 +560,12 @@ namespace local { end_superblock(is); } - int delta(application &app, flags const &fs) { + int delta(flags const &fs) { try { - delta_(app, fs); + delta_(fs); } catch (exception const &e) { - app.die(e.what()); - return 1; // never get here + cerr << e.what() << endl; + return 1; } return 0; @@ -631,27 +584,31 @@ thin_delta_cmd::thin_delta_cmd() void thin_delta_cmd::usage(std::ostream &out) const { - // FIXME: finish + out << "Usage: " << get_name() << " [options] \n" + << "Options:\n" + << " {--thin1, --snap1}\n" + << " {--thin2, --snap2}\n" + << " {-m, --metadata-snap} [block#]\n" + << " {--verbose}\n" + << " {-h|--help}\n" + << " {-V|--version}" << endl; } int thin_delta_cmd::run(int argc, char **argv) { - using namespace local; - int c; flags fs; - local::application app(basename(argv[0])); char const shortopts[] = "hVm::"; option const longopts[] = { { "help", no_argument, NULL, 'h' }, + { "metadata-snap", optional_argument, NULL, 'm' }, { "version", no_argument, NULL, 'V' }, { "thin1", required_argument, NULL, 1 }, { "snap1", required_argument, NULL, 1 }, { "thin2", required_argument, NULL, 2 }, { "snap2", required_argument, NULL, 2 }, - { "metadata-snap", optional_argument, NULL, 'm' }, { "verbose", no_argument, NULL, 4 }, { NULL, no_argument, NULL, 0 } }; @@ -659,25 +616,25 @@ thin_delta_cmd::run(int argc, char **argv) while ((c = getopt_long(argc, argv, shortopts, longopts, NULL)) != -1) { switch (c) { case 'h': - app.usage(cout); + usage(cout); return 0; + case 'm': + fs.use_metadata_snap = true; + if (optarg) + fs.metadata_snap = parse_uint64(optarg, "metadata snapshot block"); + break; + case 'V': cout << THIN_PROVISIONING_TOOLS_VERSION << endl; return 0; case 1: - fs.snap1 = app.parse_int(optarg, "thin id 1"); + fs.snap1 = parse_uint64(optarg, "thin id 1"); break; case 2: - fs.snap2 = app.parse_int(optarg, "thin id 2"); - break; - - case 'm': - fs.use_metadata_snap = true; - if (optarg) - fs.metadata_snap = app.parse_int(optarg, "metadata snapshot block"); + fs.snap2 = parse_uint64(optarg, "thin id 2"); break; case 4: @@ -685,23 +642,23 @@ thin_delta_cmd::run(int argc, char **argv) break; default: - app.usage(cerr); + usage(cerr); return 1; } } if (argc == optind) - app.die("No input device provided."); + die("No input device provided."); else fs.dev = argv[optind]; if (!fs.snap1) - app.die("--snap1 not specified."); + die("--snap1 not specified."); if (!fs.snap2) - app.die("--snap2 not specified."); + die("--snap2 not specified."); - return delta(app, fs); + return delta(fs); } //---------------------------------------------------------------- From 7ceb500fc8f648c5cbf776da36a5b9c92bfa5fcc Mon Sep 17 00:00:00 2001 From: Ming-Hung Tsai Date: Tue, 20 Oct 2020 14:50:21 +0800 Subject: [PATCH 3/9] [thin_delta] Support comparing two specific subtrees --- thin-provisioning/thin_delta.cc | 67 ++++++++++++++++++++++++++------- 1 file changed, 54 insertions(+), 13 deletions(-) diff --git a/thin-provisioning/thin_delta.cc b/thin-provisioning/thin_delta.cc index e5b368f..0eb119e 100644 --- a/thin-provisioning/thin_delta.cc +++ b/thin-provisioning/thin_delta.cc @@ -36,6 +36,8 @@ namespace { boost::optional metadata_snap; boost::optional snap1; boost::optional snap2; + boost::optional root1; + boost::optional root2; }; //-------------------------------- @@ -481,9 +483,26 @@ namespace { out << "\n"; } - void begin_diff(indented_stream &out, uint64_t snap1, uint64_t snap2) { + // FIXME: always show the blocknr? + void begin_diff(indented_stream &out, + boost::optional snap1, + boost::optional root1, + boost::optional snap2, + boost::optional root2) { out.indent(); - out << "\n"; + out << "\n"; out.inc(); } @@ -505,8 +524,12 @@ namespace { metadata::ptr md(fs.use_metadata_snap ? new metadata(bm, fs.metadata_snap) : new metadata(bm)); sb = md->sb_; - dev_tree::key k = {*fs.snap1}; - boost::optional snap1_root = md->mappings_top_level_->lookup(k); + boost::optional snap1_root; + if (fs.snap1) { + dev_tree::key k = {*fs.snap1}; + snap1_root = md->mappings_top_level_->lookup(k); + } else if (fs.root1) + snap1_root = *fs.root1; if (!snap1_root) { ostringstream out; @@ -517,8 +540,12 @@ namespace { single_mapping_tree snap1(*md->tm_, *snap1_root, mapping_tree_detail::block_traits::ref_counter(md->tm_->get_sm())); - k[0] = *fs.snap2; - boost::optional snap2_root = md->mappings_top_level_->lookup(k); + boost::optional snap2_root; + if (fs.snap2) { + dev_tree::key k = {*fs.snap2}; + snap2_root = md->mappings_top_level_->lookup(k); + } else if (fs.root2) + snap2_root = *fs.root2; if (!snap2_root) { ostringstream out; @@ -546,7 +573,7 @@ namespace { sb.metadata_snap_ ? boost::optional(sb.metadata_snap_) : boost::optional()); - begin_diff(is, *fs.snap1, *fs.snap2); + begin_diff(is, fs.snap1, fs.root1, fs.snap2, fs.root2); if (fs.verbose) { verbose_emitter e(is); @@ -586,8 +613,8 @@ thin_delta_cmd::usage(std::ostream &out) const { out << "Usage: " << get_name() << " [options] \n" << "Options:\n" - << " {--thin1, --snap1}\n" - << " {--thin2, --snap2}\n" + << " {--thin1, --snap1, --root1}\n" + << " {--thin2, --snap2, --root2}\n" << " {-m, --metadata-snap} [block#]\n" << " {--verbose}\n" << " {-h|--help}\n" @@ -610,6 +637,8 @@ thin_delta_cmd::run(int argc, char **argv) { "thin2", required_argument, NULL, 2 }, { "snap2", required_argument, NULL, 2 }, { "verbose", no_argument, NULL, 4 }, + { "root1", required_argument, NULL, 5 }, + { "root2", required_argument, NULL, 6 }, { NULL, no_argument, NULL, 0 } }; @@ -641,6 +670,14 @@ thin_delta_cmd::run(int argc, char **argv) fs.verbose = true; break; + case 5: + fs.root1 = parse_uint64(optarg, "thin root 1"); + break; + + case 6: + fs.root2 = parse_uint64(optarg, "thin root 2"); + break; + default: usage(cerr); return 1; @@ -652,11 +689,15 @@ thin_delta_cmd::run(int argc, char **argv) else fs.dev = argv[optind]; - if (!fs.snap1) - die("--snap1 not specified."); + if (!fs.snap1 && !fs.root1) + die("--snap1 or --root1 not specified."); + if (!!fs.snap1 && !!fs.root1) + die("--snap1 and --root1 are not compatible."); - if (!fs.snap2) - die("--snap2 not specified."); + if (!fs.snap2 && !fs.root2) + die("--snap2 or --root2 not specified."); + if (!!fs.snap2 && !!fs.root2) + die("--snap2 and --root2 are not compatible."); return delta(fs); } From becdbbdb49d28d3f23bf664ac24804da62cf5242 Mon Sep 17 00:00:00 2001 From: Ming-Hung Tsai Date: Tue, 24 Nov 2020 14:53:55 +0800 Subject: [PATCH 4/9] [build] Update Rust package version --- Cargo.lock | 41 ++++++++++++++++++++++++++++++++++++++++- Cargo.toml | 2 +- 2 files changed, 41 insertions(+), 2 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 4349d0f..761163b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -65,6 +65,18 @@ version = "1.2.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "cf1de2fe8c75bc145a2f577add951f8134889b4795d47466a54a5c846d691693" +[[package]] +name = "bitvec" +version = "0.19.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a7ba35e9565969edb811639dbebfe34edc0368e472c5018474c8eb2543397f81" +dependencies = [ + "funty", + "radium", + "tap", + "wyz", +] + [[package]] name = "byteorder" version = "1.3.4" @@ -188,6 +200,12 @@ dependencies = [ "miniz_oxide", ] +[[package]] +name = "funty" +version = "1.0.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "0ba62103ce691c2fd80fbae2213dfdda9ce60804973ac6b6e97de818ea7f52c8" + [[package]] name = "futures" version = "0.3.5" @@ -396,8 +414,11 @@ dependencies = [ [[package]] name = "nom" -version = "6.0.0-alpha1" +version = "6.0.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "4489ccc7d668957ddf64af7cd027c081728903afa6479d35da7e99bf5728f75f" dependencies = [ + "bitvec", "lexical-core", "memchr", "version_check", @@ -555,6 +576,12 @@ dependencies = [ "proc-macro2", ] +[[package]] +name = "radium" +version = "0.5.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "941ba9d78d8e2f7ce474c015eea4d9c6d25b6a3327f9832ee29a4de27f91bbb8" + [[package]] name = "rand" version = "0.7.3" @@ -683,6 +710,12 @@ dependencies = [ "unicode-xid", ] +[[package]] +name = "tap" +version = "1.0.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "36474e732d1affd3a6ed582781b3683df3d0563714c59c39591e8ff707cf078e" + [[package]] name = "tempfile" version = "3.1.0" @@ -895,3 +928,9 @@ name = "winapi-x86_64-pc-windows-gnu" version = "0.4.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "712e227841d057c1ee1cd2fb22fa7e5a5461ae8e48fa2ca79ec42cfc1931183f" + +[[package]] +name = "wyz" +version = "0.2.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "85e60b0d1b5f99db2556934e21937020776a5d31520bf169e851ac44e6420214" diff --git a/Cargo.toml b/Cargo.toml index 74836d5..a98b956 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -21,7 +21,7 @@ io-uring = "0.3" indicatif = "0.15" libc = "0.2.71" nix = "0.17" -nom = { path = "/home/ejt/builds/nom/" } +nom = "6.0.0" num_cpus = "1.13" num-derive = "0.3" num-traits = "0.2" From f364de35bca8fef3cd6160b07efb79624b38a8f0 Mon Sep 17 00:00:00 2001 From: Ming-Hung Tsai Date: Tue, 18 Aug 2020 16:08:30 +0800 Subject: [PATCH 5/9] [unit-tests] Fix unflushed trashed blocks and variable referencing --- unit-tests/space_map_t.cc | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/unit-tests/space_map_t.cc b/unit-tests/space_map_t.cc index 7b38d38..c0b88c8 100644 --- a/unit-tests/space_map_t.cc +++ b/unit-tests/space_map_t.cc @@ -368,6 +368,7 @@ namespace { sm_disk_detail::sm_root root; get_root(root); test::zero_block(bm_, root.bitmap_root_); + bm_->flush(); } // TODO: trash the bitmap corresponding to a given key @@ -376,12 +377,14 @@ namespace { load_ies(entries); ASSERT_LT(index, entries.size()); test::zero_block(bm_, entries[index].blocknr_); + bm_->flush(); } void trash_ref_count_root() { sm_disk_detail::sm_root root; get_root(root); test::zero_block(bm_, root.ref_count_root_); + bm_->flush(); } // TODO: trash the node corresponding to a given key @@ -394,6 +397,7 @@ namespace { ASSERT_GT(ref_count_blocks_.size(), 0u); test::zero_block(bm_, *ref_count_blocks_.begin()); + bm_->flush(); } std::set ref_count_blocks_; @@ -450,17 +454,20 @@ namespace { sm_disk_detail::sm_root root; get_data_sm_root(root); test::zero_block(bm_, root.bitmap_root_); + bm_->flush(); } // TODO: trash the bitmap corresponding to a given key void trash_bitmap_block() { test::zero_block(bm_, *bitmap_blocks_.begin()); + bm_->flush(); } void trash_ref_count_root() { sm_disk_detail::sm_root root; get_data_sm_root(root); test::zero_block(bm_, root.ref_count_root_); + bm_->flush(); } // TODO: trash the node corresponding to a given key @@ -471,6 +478,7 @@ namespace { ASSERT_GT(ref_count_blocks_.size(), 0u); test::zero_block(bm_, *ref_count_blocks_.begin()); + bm_->flush(); } std::set index_store_blocks_; @@ -749,7 +757,7 @@ TEST_F(MetaSMCountingTests, test_trashed_bitmap_root) // explicitly test open_metadata_sm() block_manager::ptr bm(new block_manager("./test.data", NR_BLOCKS, MAX_LOCKS, block_manager::READ_WRITE)); space_map::ptr core_sm{create_core_map(NR_BLOCKS)}; - transaction_manager::ptr tm(new transaction_manager(bm_, core_sm)); + transaction_manager::ptr tm(new transaction_manager(bm, core_sm)); ASSERT_THROW(persistent_data::open_metadata_sm(*tm, &d), runtime_error); } From 1fe8a0dbde9f5e004b11430a9097a61b327967fe Mon Sep 17 00:00:00 2001 From: Ming-Hung Tsai Date: Fri, 21 Aug 2020 18:26:48 +0800 Subject: [PATCH 6/9] [thin_check] Allow using --clear-needs-check and --skip-mappings together Although it is not recommended to clear the flag without a full examination, however, the usage has been documented as an approach to reduce lvchange run time [1]. For the purpose of backward compatibility and avoiding boot failure after upgrading thin_check [2], the limitation is now removed. [1] https://wiki.archlinux.org/index.php/LVM#Thinly-provisioned_root_volume_device_times_out [2] Community feedback on previous commit: https://github.com/jthornber/thin-provisioning-tools/commit/b278f4f --- tests/thin_check.rs | 18 +++++-- thin-provisioning/metadata_checker.cc | 71 ++++++++++++++------------- thin-provisioning/metadata_checker.h | 3 ++ 3 files changed, 53 insertions(+), 39 deletions(-) diff --git a/tests/thin_check.rs b/tests/thin_check.rs index 8ff53f7..c31b31b 100644 --- a/tests/thin_check.rs +++ b/tests/thin_check.rs @@ -166,11 +166,6 @@ fn clear_needs_check_incompatible_opts() -> Result<()> { "--super-block-only", &md ))?; - run_fail(thin_check!( - "--clear-needs-check-flag", - "--skip-mappings", - &md - ))?; run_fail(thin_check!( "--clear-needs-check-flag", "--ignore-non-fatal-errors", @@ -204,6 +199,19 @@ fn no_clear_needs_check_if_error() -> Result<()> { Ok(()) } +#[test] +fn clear_needs_check_if_skip_mappings() -> Result<()> { + let mut td = TestDir::new()?; + let md = prep_metadata(&mut td)?; + set_needs_check(&md)?; + generate_metadata_leaks(&md, 1, 0, 1)?; + + assert!(get_needs_check(&md)?); + thin_check!("--clear-needs-check-flag", "--skip-mappings", &md).run()?; + assert!(!get_needs_check(&md)?); + Ok(()) +} + #[test] fn metadata_leaks_are_non_fatal() -> Result<()> { let mut td = TestDir::new()?; diff --git a/thin-provisioning/metadata_checker.cc b/thin-provisioning/metadata_checker.cc index 0b26eca..a398ce8 100644 --- a/thin-provisioning/metadata_checker.cc +++ b/thin-provisioning/metadata_checker.cc @@ -371,7 +371,8 @@ namespace { out_(cerr, 2), info_out_(cout, 0), expected_rc_(true), // set stop on the first error - err_(NO_ERROR) { + err_(NO_ERROR), + metadata_checked_(false) { if (output_opts == OUTPUT_QUIET) { out_.disable(); @@ -381,6 +382,22 @@ namespace { sb_location_ = get_superblock_location(); } + void check_and_repair() { + check(); + if (options_.fix_metadata_leaks_) + fix_metadata_leaks(options_.open_transaction_); + if (options_.clear_needs_check_) + clear_needs_check_flag(); + } + + bool get_status() const { + if (options_.ignore_non_fatal_) + return (err_ == FATAL) ? false : true; + + return (err_ == NO_ERROR) ? true : false; + } + + private: void check() { block_manager::ptr bm = open_bm(path_, block_manager::READ_ONLY, !options_.use_metadata_snap_); @@ -419,10 +436,12 @@ namespace { } else err_ << examine_data_mappings(tm, sb, options_.data_mapping_opts_, out_, optional()); + + metadata_checked_ = true; } bool fix_metadata_leaks(bool open_transaction) { - if (!verify_preconditions_before_fixing()) { + if (!metadata_checked_) { out_ << "metadata has not been fully examined" << end_message(); return false; } @@ -458,7 +477,7 @@ namespace { } bool clear_needs_check_flag() { - if (!verify_preconditions_before_fixing()) { + if (!metadata_checked_) { out_ << "metadata has not been fully examined" << end_message(); return false; } @@ -480,14 +499,6 @@ namespace { return true; } - bool get_status() const { - if (options_.ignore_non_fatal_) - return (err_ == FATAL) ? false : true; - - return (err_ == NO_ERROR) ? true : false; - } - - private: block_address get_superblock_location() { block_address sb_location = superblock_detail::SUPERBLOCK_LOCATION; @@ -545,19 +556,6 @@ namespace { return err; } - bool verify_preconditions_before_fixing() const { - if (options_.use_metadata_snap_ || - !!options_.override_mapping_root_ || - options_.sm_opts_ != check_options::SPACE_MAP_FULL || - options_.data_mapping_opts_ != check_options::DATA_MAPPING_LEVEL2) - return false; - - if (!expected_rc_.get_counts().size()) - return false; - - return true; - } - std::string const &path_; check_options options_; nested_output out_; @@ -565,6 +563,7 @@ namespace { block_address sb_location_; block_counter expected_rc_; base::error_state err_; // metadata state + bool metadata_checked_; }; } @@ -628,12 +627,22 @@ bool check_options::check_conformance() { cerr << "cannot perform fix with an overridden mapping root" << endl; return false; } + } - if (data_mapping_opts_ != DATA_MAPPING_LEVEL2 || - sm_opts_ != SPACE_MAP_FULL) { - cerr << "cannot perform fix without a full examination" << endl; + if (fix_metadata_leaks_ && + (data_mapping_opts_ != DATA_MAPPING_LEVEL2 || sm_opts_ != SPACE_MAP_FULL)) { + cerr << "cannot perform fix without a full examination" << endl; + return false; + } + + if (clear_needs_check_) { + if (data_mapping_opts_ == DATA_MAPPING_NONE) { + cerr << "cannot perform fix without partially examination" << endl; return false; } + + if (data_mapping_opts_ != DATA_MAPPING_LEVEL2 || sm_opts_ != SPACE_MAP_FULL) + cerr << "clearing needs_check without a full examination is not suggested" << endl; } return true; @@ -647,13 +656,7 @@ thin_provisioning::check_metadata(std::string const &path, output_options output_opts) { metadata_checker checker(path, check_opts, output_opts); - - checker.check(); - if (check_opts.fix_metadata_leaks_) - checker.fix_metadata_leaks(check_opts.open_transaction_); - if (check_opts.clear_needs_check_) - checker.clear_needs_check_flag(); - + checker.check_and_repair(); return checker.get_status(); } diff --git a/thin-provisioning/metadata_checker.h b/thin-provisioning/metadata_checker.h index b4afbdc..ea66dc3 100644 --- a/thin-provisioning/metadata_checker.h +++ b/thin-provisioning/metadata_checker.h @@ -48,11 +48,14 @@ namespace thin_provisioning { void set_auto_repair(); void set_clear_needs_check(); + // flags for checking bool use_metadata_snap_; data_mapping_options data_mapping_opts_; space_map_options sm_opts_; boost::optional override_mapping_root_; bool ignore_non_fatal_; + + // flags for repairing bool fix_metadata_leaks_; bool clear_needs_check_; bool open_transaction_; From 61f07573e1f0c5af0ef0978fd7ff97cd5ef64a7b Mon Sep 17 00:00:00 2001 From: Ming-Hung Tsai Date: Sat, 22 Aug 2020 04:17:50 +0800 Subject: [PATCH 7/9] [metadata_counter] Count under populated nodes if the option is provided --- .../data-structures/btree_counter.h | 22 ++++++++++++++----- thin-provisioning/metadata_checker.cc | 10 +++++---- thin-provisioning/metadata_counter.cc | 14 +++++++----- thin-provisioning/metadata_counter.h | 3 ++- 4 files changed, 32 insertions(+), 17 deletions(-) diff --git a/persistent-data/data-structures/btree_counter.h b/persistent-data/data-structures/btree_counter.h index 5e07a81..0c79d34 100644 --- a/persistent-data/data-structures/btree_counter.h +++ b/persistent-data/data-structures/btree_counter.h @@ -14,12 +14,14 @@ namespace persistent_data { public: typedef btree tree; - counting_visitor(block_counter &bc, ValueCounter &vc) + counting_visitor(block_counter &bc, ValueCounter &vc, + bool ignore_non_fatal = false) : bc_(bc), vc_(vc), error_outcome_(bc.stop_on_error() ? tree::visitor::RETHROW_EXCEPTION : - tree::visitor::EXCEPTION_HANDLED) { + tree::visitor::EXCEPTION_HANDLED), + ignore_non_fatal_(ignore_non_fatal) { } virtual bool visit_internal(node_location const &l, @@ -66,7 +68,7 @@ namespace persistent_data { if (!checker_.check_block_nr(n) || !checker_.check_value_size(n) || !checker_.check_max_entries(n) || - !checker_.check_nr_entries(n, l.is_sub_root()) || + !check_nr_entries(l, n) || !checker_.check_ordered_keys(n) || !checker_.check_parent_key(n, l.is_sub_root() ? boost::optional() : l.key)) return false; @@ -83,7 +85,7 @@ namespace persistent_data { if (!checker_.check_block_nr(n) || !checker_.check_value_size(n) || !checker_.check_max_entries(n) || - !checker_.check_nr_entries(n, l.is_sub_root()) || + !check_nr_entries(l, n) || !checker_.check_ordered_keys(n) || !checker_.check_parent_key(n, l.is_sub_root() ? boost::optional() : l.key) || !checker_.check_leaf_key(n, last_leaf_key_[l.level()])) @@ -109,11 +111,18 @@ namespace persistent_data { return !seen; } + template + bool check_nr_entries(node_location const &loc, + btree_detail::node_ref const &n) { + return ignore_non_fatal_ || checker_.check_nr_entries(n, loc.is_sub_root()); + } + block_counter &bc_; ValueCounter &vc_; btree_node_checker checker_; boost::optional last_leaf_key_[Levels]; error_outcome error_outcome_; + bool ignore_non_fatal_; }; } @@ -141,8 +150,9 @@ namespace persistent_data { // walked. This walk should only be done once you're sure the tree // is not corrupt. template - void count_btree_blocks(btree const &tree, block_counter &bc, ValueCounter &vc) { - btree_count_detail::counting_visitor v(bc, vc); + void count_btree_blocks(btree const &tree, block_counter &bc, ValueCounter &vc, + bool ignore_non_fatal = false) { + btree_count_detail::counting_visitor v(bc, vc, ignore_non_fatal); tree.visit_depth_first(v); } } diff --git a/thin-provisioning/metadata_checker.cc b/thin-provisioning/metadata_checker.cc index a398ce8..30e5831 100644 --- a/thin-provisioning/metadata_checker.cc +++ b/thin-provisioning/metadata_checker.cc @@ -303,11 +303,12 @@ namespace { error_state check_metadata_space_map_counts(transaction_manager::ptr tm, superblock_detail::superblock const &sb, block_counter &bc, - nested_output &out) { + nested_output &out, + bool ignore_non_fatal) { out << "checking space map counts" << end_message(); nested_output::nest _ = out.push(); - if (!count_metadata(tm, sb, bc)) + if (!count_metadata(tm, sb, bc, false, ignore_non_fatal)) return FATAL; // Finally we need to check the metadata space map agrees @@ -428,7 +429,7 @@ namespace { // if we're checking everything, and there were no errors, // then we should check the space maps too. - err_ << examine_metadata_space_map(tm, sb, options_.sm_opts_, out_, expected_rc_); + err_ << examine_metadata_space_map(tm, sb, options_.sm_opts_, options_.ignore_non_fatal_, out_, expected_rc_); // verify ref-counts of data blocks if (err_ != FATAL && core_sm) @@ -541,13 +542,14 @@ namespace { examine_metadata_space_map(transaction_manager::ptr tm, superblock_detail::superblock const &sb, check_options::space_map_options option, + bool ignore_non_fatal, nested_output &out, block_counter &bc) { error_state err = NO_ERROR; switch (option) { case check_options::SPACE_MAP_FULL: - err << check_metadata_space_map_counts(tm, sb, bc, out); + err << check_metadata_space_map_counts(tm, sb, bc, out, ignore_non_fatal); break; default: break; // do nothing diff --git a/thin-provisioning/metadata_counter.cc b/thin-provisioning/metadata_counter.cc index 9014402..7a40ed1 100644 --- a/thin-provisioning/metadata_counter.cc +++ b/thin-provisioning/metadata_counter.cc @@ -10,14 +10,15 @@ using namespace thin_provisioning; namespace { bool count_trees(transaction_manager::ptr tm, superblock_detail::superblock const &sb, - block_counter &bc) { + block_counter &bc, + bool ignore_non_fatal) { // Count the device tree { noop_value_counter vc; device_tree dtree(*tm, sb.device_details_root_, device_tree_detail::device_details_traits::ref_counter()); - count_btree_blocks(dtree, bc, vc); + count_btree_blocks(dtree, bc, vc, ignore_non_fatal); } // Count the mapping tree @@ -25,7 +26,7 @@ namespace { noop_value_counter vc; mapping_tree mtree(*tm, sb.data_mapping_root_, mapping_tree_detail::block_traits::ref_counter(space_map::ptr())); - count_btree_blocks(mtree, bc, vc); + count_btree_blocks(mtree, bc, vc, ignore_non_fatal); } return true; @@ -65,19 +66,20 @@ namespace { bool thin_provisioning::count_metadata(transaction_manager::ptr tm, superblock_detail::superblock const &sb, block_counter &bc, - bool skip_metadata_snap) { + bool skip_metadata_snap, + bool ignore_non_fatal) { bool ret = true; // Count the superblock bc.inc(superblock_detail::SUPERBLOCK_LOCATION); - ret &= count_trees(tm, sb, bc); + ret &= count_trees(tm, sb, bc, ignore_non_fatal); // Count the metadata snap, if present if (!skip_metadata_snap && sb.metadata_snap_ != superblock_detail::SUPERBLOCK_LOCATION) { bc.inc(sb.metadata_snap_); superblock_detail::superblock snap = read_superblock(tm->get_bm(), sb.metadata_snap_); - ret &= count_trees(tm, snap, bc); + ret &= count_trees(tm, snap, bc, ignore_non_fatal); } ret &= count_space_maps(tm, sb, bc); diff --git a/thin-provisioning/metadata_counter.h b/thin-provisioning/metadata_counter.h index c5916a6..3a38f82 100644 --- a/thin-provisioning/metadata_counter.h +++ b/thin-provisioning/metadata_counter.h @@ -10,7 +10,8 @@ namespace thin_provisioning { bool count_metadata(transaction_manager::ptr tm, superblock_detail::superblock const &sb, block_counter &bc, - bool skip_metadata_snap = false); + bool skip_metadata_snap = false, + bool ignore_non_fatal = false); } //---------------------------------------------------------------- From c932a76f0855d7c4b55fb789ce28f11fe961a997 Mon Sep 17 00:00:00 2001 From: Ming-Hung Tsai Date: Sat, 22 Aug 2020 16:40:33 +0800 Subject: [PATCH 8/9] [unit-tests] Add underfull nodes counting tests --- unit-tests/btree_counter_t.cc | 72 ++++++++++++++++++++++++++++++++--- 1 file changed, 66 insertions(+), 6 deletions(-) diff --git a/unit-tests/btree_counter_t.cc b/unit-tests/btree_counter_t.cc index 39412c7..0aa01c0 100644 --- a/unit-tests/btree_counter_t.cc +++ b/unit-tests/btree_counter_t.cc @@ -28,11 +28,21 @@ namespace { tm_(bm_, sm_) { } - void check_nr_metadata_blocks_is_ge(unsigned n) { - block_counter bc; + size_t get_nr_metadata_blocks(bool ignore_non_fatal = false, + bool stop_on_error = false) { + block_counter bc(stop_on_error); noop_value_counter vc; - count_btree_blocks(*tree_, bc, vc); - ASSERT_THAT(bc.get_counts().size(), Ge(n)); + count_btree_blocks(*tree_, bc, vc, ignore_non_fatal); + return bc.get_counts().size(); + } + + void damage_first_leaf_underfull() { + bcache::validator::ptr v = create_btree_node_validator(); + + block_address b = get_first_leaf(); + block_manager::write_ref blk = bm_->write_lock(b, v); + btree<1, uint64_traits>::leaf_node n = to_node(blk); + n.set_nr_entries(1); } with_temp_directory dir_; @@ -53,6 +63,19 @@ namespace { void commit() { block_manager::write_ref superblock(bm_->superblock(SUPERBLOCK)); } + + block_address get_first_leaf() { + bcache::validator::ptr v = create_btree_node_validator(); + + block_manager::read_ref root = bm_->read_lock(tree_->get_root(), v); + btree<1, uint64_traits>::internal_node n = to_node(root); + while (n.get_type() == INTERNAL) { + block_manager::read_ref internal = bm_->read_lock(n.value_at(0), v); + n = to_node(internal); + } + + return n.get_block_nr(); + } }; } @@ -62,7 +85,7 @@ TEST_F(BTreeCounterTests, count_empty_tree) { tree_.reset(new btree<1, uint64_traits>(tm_, rc_)); tm_.get_bm()->flush(); - check_nr_metadata_blocks_is_ge(1); + ASSERT_GE(get_nr_metadata_blocks(), 1u); } TEST_F(BTreeCounterTests, count_populated_tree) @@ -75,7 +98,44 @@ TEST_F(BTreeCounterTests, count_populated_tree) } tm_.get_bm()->flush(); - check_nr_metadata_blocks_is_ge(40); + ASSERT_GE(get_nr_metadata_blocks(), 40u); +} + +TEST_F(BTreeCounterTests, count_underfull_nodes) +{ + tree_.reset(new btree<1, uint64_traits>(tm_, rc_)); + + for (unsigned i = 0; i < 10000; i++) { + uint64_t key[1] = {i}; + tree_->insert(key, 0ull); + } + + tm_.get_bm()->flush(); + size_t nr_blocks = get_nr_metadata_blocks(); + + damage_first_leaf_underfull(); + tm_.get_bm()->flush(); + + // underfull nodes are not counted + bool ignore_non_fatal = false; + bool stop_on_error = false; + ASSERT_EQ(get_nr_metadata_blocks(ignore_non_fatal, stop_on_error), nr_blocks - 1); + + // underfull nodes are counted + ignore_non_fatal = true; + stop_on_error = false; + ASSERT_EQ(get_nr_metadata_blocks(ignore_non_fatal, stop_on_error), nr_blocks); + + // logical errors like underfull nodes don't result in exceptions, + // therefore the stop_on_error flag has no effect. + // FIXME: is it necessary to stop the counting on logical errors? + ignore_non_fatal = false; + stop_on_error = true; + ASSERT_EQ(get_nr_metadata_blocks(ignore_non_fatal, stop_on_error), nr_blocks - 1); + + ignore_non_fatal = true; + stop_on_error = true; + ASSERT_EQ(get_nr_metadata_blocks(ignore_non_fatal, stop_on_error), nr_blocks); } //---------------------------------------------------------------- From 565c656ed232f584f35042bfa31f6f8d58127aaf Mon Sep 17 00:00:00 2001 From: Ming-Hung Tsai Date: Mon, 24 Aug 2020 23:45:38 +0800 Subject: [PATCH 9/9] [thin_generate_damage] Do not open a new transaction to prevent ref-count underflow There's a chance that thin_generate_damage tries to change ref-counts of space map blocks due to its random nature, which could lead to problems. If the ref-counts of metadata space map blocks (shadow source) is changed to zero, then the ref-counts will become underflow after a shadow operation. In-place space map modification is a way to prevent that value underflow. An alternative approach is to avoid changing ref-counts of space map blocks. --- thin-provisioning/damage_generator.cc | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/thin-provisioning/damage_generator.cc b/thin-provisioning/damage_generator.cc index f45dad8..007cc43 100644 --- a/thin-provisioning/damage_generator.cc +++ b/thin-provisioning/damage_generator.cc @@ -63,6 +63,14 @@ void damage_generator::create_metadata_leaks(block_address nr_leaks, std::set leaks; find_blocks(md_->metadata_sm_, nr_leaks, expected, leaks); + block_counter bc(true); + md_->metadata_sm_->count_metadata(bc); + block_address nr_blocks = md_->metadata_sm_->get_nr_blocks(); + for (block_address b = 0; b < nr_blocks; b++) { + if (bc.get_count(b)) + md_->tm_->mark_shadowed(b); + } + for (auto const &b : leaks) md_->metadata_sm_->set_count(b, actual); }