From 6ce838b83c28d9b64f34cf0c68cfebaf9affac11 Mon Sep 17 00:00:00 2001

From: Ming-Hung Tsai <mtsai@redhat.com>

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<space_map::ptr>());

+

+			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<bcache::block_address> 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