From 6ce838b83c28d9b64f34cf0c68cfebaf9affac11 Mon Sep 17 00:00:00 2001 From: Ming-Hung Tsai Date: Fri, 21 Aug 2020 18:26:48 +0800 Subject: [PATCH] [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 (cherry picked from commit f4675e3f32aad3d1518869e4f3824e05230c6a5d) --- functional-tests/thin-functional-tests.scm | 65 +++++++++++++--- thin-provisioning/metadata_checker.cc | 88 +++++++++++----------- thin-provisioning/metadata_checker.h | 5 +- thin-provisioning/thin_check.cc | 2 +- 4 files changed, 106 insertions(+), 54 deletions(-) diff --git a/functional-tests/thin-functional-tests.scm b/functional-tests/thin-functional-tests.scm index 37d3df9..5b1423c 100644 --- a/functional-tests/thin-functional-tests.scm +++ b/functional-tests/thin-functional-tests.scm @@ -189,15 +189,6 @@ (run-fail (thin-check "--auto-repair" "--skip-mappings" md)) (run-fail (thin-check "--auto-repair" "--ignore-non-fatal-errors" md)))) - (define-scenario (thin-check incompatible-options clear-needs-check-flag) - "Incompatible options should cause failure" - (with-valid-metadata (md) - (run-fail (thin-check "--clear-needs-check-flag" "-m" md)) - (run-fail (thin-check "--clear-needs-check-flag" "--override-mapping-root 123" md)) - (run-fail (thin-check "--clear-needs-check-flag" "--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" md)))) - (define-scenario (thin-check superblock-only-valid) "--super-block-only check passes on valid metadata" (with-valid-metadata (md) @@ -230,6 +221,62 @@ (with-valid-metadata (md) (run-ok (thin-check "--clear-needs-check-flag" md)))) + (define-scenario (thin-check mixing-clear-needs-check-flag super-block-only-should-pass) + "Accepts --clear-needs-check and --super-block-only" + (with-valid-metadata (md) + (run-ok (thin-check "--clear-needs-check-flag" "--super-block-only" md)))) + + (define-scenario (thin-check mixing-clear-needs-check-flag skip-mappings-should-pass) + "Accepts --clear-needs-check and --skip-mappings" + (with-valid-metadata (md) + (run-ok (thin-check "--clear-needs-check-flag" "--skip-mappings" md)))) + + (define-scenario (thin-check mixing-clear-needs-check-flag ignore-non-fatal-errors-should-pass) + "Accepts --clear-needs-check and --ignore-non-fatal-errors" + (with-valid-metadata (md) + (run-ok (thin-check "--clear-needs-check-flag" "--ignore-non-fatal-errors" md)))) + + (define-scenario (thin-check try-clear-needs-check-flag super-block-only-should-clear) + "--clear-needs-check is a noop while using --super-block-only" + (with-valid-metadata (md) + (set-needs-check-flag md) + (assert-metadata-needs-check md) + (run-ok (thin-check "--clear-needs-check-flag" "--super-block-only" md)) + (assert-metadata-clean md))) + + (define-scenario (thin-check try-clear-needs-check-flag skip-mappings-should-clear) + "--clear-needs-check is a noop while using --skip-mappings" + (with-valid-metadata (md) + (set-needs-check-flag md) + (assert-metadata-needs-check md) + (run-ok (thin-check "--clear-needs-check-flag" "--skip-mappings" md)) + (assert-metadata-clean md))) + + (define-scenario (thin-check try-clear-needs-check-flag ignore-non-fatal-errors-should-clear) + "--clear-needs-check works while using --ignore-non-fatal-errors" + (with-valid-metadata (md) + (set-needs-check-flag md) + (assert-metadata-needs-check md) + (run-ok (thin-check "--clear-needs-check-flag" "--ignore-non-fatal-errors" md)) + (assert-metadata-clean md))) + + (define-scenario (thin-check try-clear-needs-check-flag no-errors-should-clear) + "--clear-needs-check works if there's no errors" + (with-valid-metadata (md) + (set-needs-check-flag md) + (assert-metadata-needs-check md) + (run-ok (thin-check "--clear-needs-check-flag" md)) + (assert-metadata-clean md))) + + (define-scenario (thin-check try-clear-needs-check-flag fatal-errors-should-keep) + "--clear-needs-check is a noop if there's fatal errors" + (with-valid-metadata (md) + (set-needs-check-flag md) + (tamper-mapping-root md 10) + (assert-metadata-needs-check md) + (run-fail (thin-check "--clear-needs-check-flag" md)) + (assert-metadata-needs-check md))) + (define-scenario (thin-check auto-repair) "Accepts --auto-repair" (with-valid-metadata (md) diff --git a/thin-provisioning/metadata_checker.cc b/thin-provisioning/metadata_checker.cc index e81e22c..1c9734e 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,7 +382,29 @@ namespace { sb_location_ = get_superblock_location(); } - void check() { + void check_and_repair() { + if (!check()) + return; + + if (!options_.use_metadata_snap_ && + !options_.override_mapping_root_) { + if (options_.sm_opts_ == check_options::SPACE_MAP_FULL && + 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: + bool check() { block_manager::ptr bm = open_bm(path_, block_manager::READ_ONLY, !options_.use_metadata_snap_); @@ -389,7 +412,7 @@ namespace { if (err_ == FATAL) { if (check_for_xml(bm)) out_ << "This looks like XML. thin_check only checks the binary metadata format." << end_message(); - return; + return false; } transaction_manager::ptr tm = open_tm(bm, sb_location_); @@ -407,7 +430,7 @@ namespace { err_ << examine_data_mappings(tm, sb, options_.data_mapping_opts_, out_, core_sm); if (err_ == FATAL) - return; + return false; // if we're checking everything, and there were no errors, // then we should check the space maps too. @@ -419,10 +442,14 @@ namespace { } else err_ << examine_data_mappings(tm, sb, options_.data_mapping_opts_, out_, optional()); + + metadata_checked_ = true; + + return 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,12 +485,13 @@ 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; } - if (err_ != NO_ERROR) + if (err_ == FATAL || + (err_ == NON_FATAL && !options_.ignore_non_fatal_)) return false; block_manager::ptr bm = open_bm(path_, block_manager::READ_WRITE); @@ -480,14 +508,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 +565,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 +572,7 @@ namespace { block_address sb_location_; block_counter expected_rc_; base::error_state err_; // metadata state + bool metadata_checked_; }; } @@ -603,8 +611,9 @@ void check_options::set_ignore_non_fatal() { ignore_non_fatal_ = true; } -void check_options::set_fix_metadata_leaks() { +void check_options::set_auto_repair() { fix_metadata_leaks_ = true; + clear_needs_check_ = true; } void check_options::set_clear_needs_check() { @@ -612,7 +621,7 @@ void check_options::set_clear_needs_check() { } bool check_options::check_conformance() { - if (fix_metadata_leaks_ || clear_needs_check_) { + if (fix_metadata_leaks_) { if (ignore_non_fatal_) { cerr << "cannot perform fix by ignoring non-fatal errors" << endl; return false; @@ -627,12 +636,12 @@ 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; - return false; - } + 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; } return true; @@ -646,14 +655,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.fix_metadata_leaks_ || - 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 5569d27..ea66dc3 100644 --- a/thin-provisioning/metadata_checker.h +++ b/thin-provisioning/metadata_checker.h @@ -45,14 +45,17 @@ namespace thin_provisioning { void set_override_mapping_root(bcache::block_address b); void set_metadata_snap(); void set_ignore_non_fatal(); - void set_fix_metadata_leaks(); + 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_; diff --git a/thin-provisioning/thin_check.cc b/thin-provisioning/thin_check.cc index 60f7838..e3c9db3 100644 --- a/thin-provisioning/thin_check.cc +++ b/thin-provisioning/thin_check.cc @@ -166,7 +166,7 @@ thin_check_cmd::run(int argc, char **argv) case 6: // auto-repair - fs.check_opts.set_fix_metadata_leaks(); + fs.check_opts.set_auto_repair(); break; default: -- 2.31.1