From f364de35bca8fef3cd6160b07efb79624b38a8f0 Mon Sep 17 00:00:00 2001 From: Ming-Hung Tsai Date: Tue, 18 Aug 2020 16:08:30 +0800 Subject: [PATCH 1/5] [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 2/5] [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 3/5] [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 4/5] [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 5/5] [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); }