Merge pull request #155 from mingnus/thin-check-fix-metadata-leaks

thin_check fixes

persistent-data/data-structures/btree_counter.h

@@ -14,12 +14,14 @@ namespace persistent_data {
 		public:
 			typedef btree<Levels, ValueTraits> 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<uint64_t>() : 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<uint64_t>() : l.key) ||
 				    !checker_.check_leaf_key(n, last_leaf_key_[l.level()]))
@@ -109,11 +111,18 @@ namespace persistent_data {
 				return !seen;
 			}
 
+			template <typename ValueTraits2>
+			bool check_nr_entries(node_location const &loc,
+					      btree_detail::node_ref<ValueTraits2> 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<uint64_t> 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 <unsigned Levels, typename ValueTraits, typename ValueCounter>
-	void count_btree_blocks(btree<Levels, ValueTraits> const &tree, block_counter &bc, ValueCounter &vc) {
+	void count_btree_blocks(btree<Levels, ValueTraits> const &tree, block_counter &bc, ValueCounter &vc,
-		btree_count_detail::counting_visitor<Levels, ValueTraits, ValueCounter> v(bc, vc);
+				bool ignore_non_fatal = false) {
+		btree_count_detail::counting_visitor<Levels, ValueTraits, ValueCounter> v(bc, vc, ignore_non_fatal);
 		tree.visit_depth_first(v);
 	}
 }

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()?;

thin-provisioning/damage_generator.cc

@@ -63,6 +63,14 @@ void damage_generator::create_metadata_leaks(block_address nr_leaks,
 	std::set<block_address> 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);
 }

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
@@ -371,7 +372,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 +383,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_);
@@ -411,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)
@@ -419,10 +437,12 @@ namespace {
 			} else
 				err_ << examine_data_mappings(tm, sb, options_.data_mapping_opts_, out_,
 							      optional<space_map::ptr>());
+
+			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 +478,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 +500,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;
@@ -530,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
@@ -545,19 +558,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 +565,7 @@ namespace {
 		block_address sb_location_;
 		block_counter expected_rc_;
 		base::error_state err_; // metadata state
+		bool metadata_checked_;
 	};
 }
 
@@ -628,12 +629,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 ||
+	if (fix_metadata_leaks_ &&
-		    sm_opts_ != SPACE_MAP_FULL) {
+	    (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 +658,7 @@ thin_provisioning::check_metadata(std::string const &path,
 				  output_options output_opts)
 {
 	metadata_checker checker(path, check_opts, output_opts);
-
+	checker.check_and_repair();
-	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();
-
 	return checker.get_status();
 }
 

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<bcache::block_address> override_mapping_root_;
 		bool ignore_non_fatal_;
+
+		// flags for repairing
 		bool fix_metadata_leaks_;
 		bool clear_needs_check_;
 		bool open_transaction_;

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<device_tree_detail::device_details> 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<mapping_tree_detail::block_time> 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);

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);
 }
 
 //----------------------------------------------------------------

unit-tests/btree_counter_t.cc

@@ -28,11 +28,21 @@ namespace {
 			  tm_(bm_, sm_) {
 		}
 
-		void check_nr_metadata_blocks_is_ge(unsigned n) {
+		size_t get_nr_metadata_blocks(bool ignore_non_fatal = false,
-			block_counter bc;
+					      bool stop_on_error = false) {
+			block_counter bc(stop_on_error);
 			noop_value_counter<uint64_t> vc;
-			count_btree_blocks(*tree_, bc, vc);
+			count_btree_blocks(*tree_, bc, vc, ignore_non_fatal);
-			ASSERT_THAT(bc.get_counts().size(), Ge(n));
+			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<uint64_traits>(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<block_traits>(root);
+			while (n.get_type() == INTERNAL) {
+				block_manager::read_ref internal = bm_->read_lock(n.value_at(0), v);
+				n = to_node<block_traits>(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);
 }
 
 //----------------------------------------------------------------

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<block_address> 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<block_address> 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);
 }