Merge pull request #56 from mingnus/v0.7-devel-fix

Fix unnecessary value visiting in counting_visitor

Makefile.in

@@ -67,6 +67,7 @@ SOURCE=\
 	persistent-data/data-structures/bitset.cc \
 	persistent-data/data-structures/bloom_filter.cc \
 	persistent-data/data-structures/btree.cc \
+	persistent-data/data-structures/btree_node_checker.cc \
 	persistent-data/error_set.cc \
 	persistent-data/file_utils.cc \
 	persistent-data/hex_dump.cc \

persistent-data/data-structures/btree_counter.h

@@ -2,44 +2,36 @@
 #define PERSISTENT_DATA_DATA_STRUCTURES_BTREE_COUNTER_H
 
 #include "persistent-data/data-structures/btree.h"
-#include "persistent-data/data-structures/btree_base_visitor.h"
+#include "persistent-data/data-structures/btree_node_checker.h"
-#include "persistent-data/data-structures/btree_damage_visitor.h"
 #include "persistent-data/block_counter.h"
 
 //----------------------------------------------------------------
 
 namespace persistent_data {
 	namespace btree_count_detail {
-		template <typename ValueVisitor, typename DamageVisitor, unsigned Levels, typename ValueTraits, typename ValueCounter>
+		template <unsigned Levels, typename ValueTraits, typename ValueCounter>
-		class counting_visitor : public btree_damage_visitor<ValueVisitor, DamageVisitor, Levels, ValueTraits> {
+		class counting_visitor : public btree<Levels, ValueTraits>::visitor {
-			typedef btree_damage_visitor<ValueVisitor, DamageVisitor, Levels, ValueTraits> BtreeDamageVisitor;
 		public:
 			typedef btree<Levels, ValueTraits> tree;
 
-			counting_visitor(ValueVisitor &value_visitor,
+			counting_visitor(block_counter &bc, ValueCounter &vc)
-					 DamageVisitor &damage_visitor,
+				: bc_(bc),
-					 block_counter &bc,
-					 ValueCounter &vc)
-				: BtreeDamageVisitor(value_visitor, damage_visitor, false),
-				  bc_(bc),
 				  vc_(vc) {
 			}
 
 			virtual bool visit_internal(node_location const &l,
 						    typename tree::internal_node const &n) {
-				return BtreeDamageVisitor::visit_internal(l, n) ?
+				return check_internal(l, n) ? visit_node(n) : false;
-					visit_node(n) : false;
 			}
 
 			virtual bool visit_internal_leaf(node_location const &l,
 							 typename tree::internal_node const &n) {
-				return BtreeDamageVisitor::visit_internal_leaf(l, n) ?
+				return check_leaf(l, n) ? visit_node(n) : false;
-					visit_node(n) : false;
 			}
 
 			virtual bool visit_leaf(node_location const &l,
 						typename tree::leaf_node const &n) {
-				if (BtreeDamageVisitor::visit_leaf(l, n) && visit_node(n)) {
+				if (check_leaf(l, n) && visit_node(n)) {
 					unsigned nr = n.get_nr_entries();
 
 					for (unsigned i = 0; i < nr; i++) {
@@ -55,7 +47,57 @@ namespace persistent_data {
 				return false;
 			}
 
+			typedef typename btree<Levels, ValueTraits>::visitor::error_outcome error_outcome;
+
+			error_outcome error_accessing_node(node_location const &l, block_address b,
+							   std::string const &what) {
+				return btree<Levels, ValueTraits>::visitor::EXCEPTION_HANDLED;
+			}
+
 		private:
+			bool check_internal(node_location const &l,
+					    btree_detail::node_ref<block_traits> const &n) {
+				if (l.is_sub_root())
+					new_root(l.level());
+
+				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()) ||
+				    !checker_.check_ordered_keys(n) ||
+				    !checker_.check_parent_key(n, l.is_sub_root() ? boost::optional<uint64_t>() : l.key))
+					return false;
+
+				return true;
+			}
+
+			template <typename ValueTraits2>
+			bool check_leaf(node_location const &l,
+				        btree_detail::node_ref<ValueTraits2> const &n) {
+				if (l.is_sub_root())
+					new_root(l.level());
+
+				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()) ||
+				    !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()]))
+					return false;
+
+				if (n.get_nr_entries() > 0)
+					last_leaf_key_[l.level()] = n.key_at(n.get_nr_entries() - 1);
+
+				return true;
+			}
+
+			void new_root(unsigned level) {
+				// we're starting a new subtree, so should
+				// reset the last_leaf value.
+				last_leaf_key_[level] = boost::optional<uint64_t>();
+			}
+
 			template <typename Node>
 			bool visit_node(Node const &n) {
 				block_address b = n.get_location();
@@ -66,6 +108,8 @@ namespace persistent_data {
 
 			block_counter &bc_;
 			ValueCounter &vc_;
+			btree_node_checker checker_;
+			boost::optional<uint64_t> last_leaf_key_[Levels];
 		};
 	}
 
@@ -94,21 +138,7 @@ namespace persistent_data {
 	// is not corrupt.
 	template <unsigned Levels, typename ValueTraits, typename ValueCounter>
 	void count_btree_blocks(btree<Levels, ValueTraits> const &tree, block_counter &bc, ValueCounter &vc) {
-		typedef noop_value_visitor<typename ValueTraits::value_type> NoopValueVisitor;
+		btree_count_detail::counting_visitor<Levels, ValueTraits, ValueCounter> v(bc, vc);
-		NoopValueVisitor noop_vv;
-		noop_damage_visitor noop_dv;
-		btree_count_detail::counting_visitor<NoopValueVisitor, noop_damage_visitor, Levels, ValueTraits, ValueCounter> v(noop_vv, noop_dv, bc, vc);
-		tree.visit_depth_first(v);
-	}
-
-	template <unsigned Levels, typename ValueTraits>
-	void count_btree_blocks(btree<Levels, ValueTraits> const &tree, block_counter &bc) {
-		typedef noop_value_visitor<typename ValueTraits::value_type> NoopValueVisitor;
-		NoopValueVisitor noop_vv;
-		noop_damage_visitor noop_dv;
-		typedef noop_value_counter<typename ValueTraits::value_type> NoopValueCounter;
-		NoopValueCounter vc;
-		btree_count_detail::counting_visitor<NoopValueVisitor, noop_damage_visitor, Levels, ValueTraits, NoopValueCounter> v(noop_vv, noop_dv, bc, vc);
 		tree.visit_depth_first(v);
 	}
 }

persistent-data/data-structures/btree_damage_visitor.h

@@ -2,6 +2,7 @@
 #define PERSISTENT_DATA_DATA_STRUCTURES_DAMAGE_VISITOR_H
 
 #include "persistent-data/data-structures/btree.h"
+#include "persistent-data/data-structures/btree_node_checker.h"
 #include "persistent-data/run.h"
 
 //----------------------------------------------------------------
@@ -158,9 +159,8 @@ namespace persistent_data {
 			typedef boost::optional<run64> maybe_run64;
 
 			btree_damage_visitor(ValueVisitor &value_visitor,
-					     DamageVisitor &damage_visitor,
+					     DamageVisitor &damage_visitor)
-					     bool avoid_repeated_visits = true)
+				: avoid_repeated_visits_(true),
-				: avoid_repeated_visits_(avoid_repeated_visits),
 				  value_visitor_(value_visitor),
 				  damage_visitor_(damage_visitor) {
 			}
@@ -223,44 +223,53 @@ namespace persistent_data {
 
 			bool check_internal(node_location const &loc,
 					    btree_detail::node_ref<block_traits> const &n) {
-				if (!already_visited(n) &&
+				if (loc.is_sub_root())
-				    check_block_nr(n) &&
+					new_root(loc.level());
-				    check_value_size(n) &&
-				    check_max_entries(n) &&
-				    check_nr_entries(n, loc.is_sub_root()) &&
-				    check_ordered_keys(n) &&
-				    check_parent_key(loc.is_sub_root() ? boost::optional<uint64_t>() : loc.key, n)) {
-					if (loc.is_sub_root())
-						new_root(loc.level());
 
-					good_internal(n.key_at(0));
+				if (already_visited(n))
-					return true;
+					return false;
+				else if (!checker_.check_block_nr(n) ||
+					 !checker_.check_value_size(n) ||
+					 !checker_.check_max_entries(n) ||
+					 !checker_.check_nr_entries(n, loc.is_sub_root()) ||
+					 !checker_.check_ordered_keys(n) ||
+					 !checker_.check_parent_key(n, loc.is_sub_root() ? boost::optional<uint64_t>() : loc.key)) {
+					report_damage(checker_.get_last_error_string());
+
+					return false;
 				}
 
-				return false;
+				good_internal(n.key_at(0));
+
+				return true;
 			}
 
 			template <typename ValueTraits2>
 			bool check_leaf(node_location const &loc,
 					btree_detail::node_ref<ValueTraits2> const &n) {
-				if (!already_visited(n) &&
+				if (loc.is_sub_root())
-				    check_block_nr(n) &&
+					new_root(loc.level());
-				    check_value_size(n) &&
-				    check_max_entries(n) &&
-				    check_nr_entries(n, loc.is_sub_root()) &&
-				    check_ordered_keys(n) &&
-				    check_parent_key(loc.is_sub_root() ? boost::optional<uint64_t>() : loc.key, n)) {
-					if (loc.is_sub_root())
-						new_root(loc.level());
 
-					bool r = check_leaf_key(loc.level(), n);
+				if (already_visited(n))
-					if (r && n.get_nr_entries() > 0)
+					return false;
-						good_leaf(n.key_at(0), n.key_at(n.get_nr_entries() - 1) + 1);
+				else if (!checker_.check_block_nr(n) ||
+					 !checker_.check_value_size(n) ||
+					 !checker_.check_max_entries(n) ||
+					 !checker_.check_nr_entries(n, loc.is_sub_root()) ||
+					 !checker_.check_ordered_keys(n) ||
+					 !checker_.check_parent_key(n, loc.is_sub_root() ? boost::optional<uint64_t>() : loc.key) ||
+					 !checker_.check_leaf_key(n, last_leaf_key_[loc.level()])) {
+					report_damage(checker_.get_last_error_string());
 
-					return r;
+					return false;
 				}
 
-				return false;
+				if (n.get_nr_entries() > 0) {
+					last_leaf_key_[loc.level()] = n.key_at(n.get_nr_entries() - 1);
+					good_leaf(n.key_at(0), n.key_at(n.get_nr_entries() - 1) + 1);
+				}
+
+				return true;
 			}
 
 			template <typename node>
@@ -277,140 +286,6 @@ namespace persistent_data {
 				return false;
 			}
 
-			template <typename node>
-			bool check_block_nr(node const &n) {
-				if (n.get_location() != n.get_block_nr()) {
-					std::ostringstream out;
-					out << "block number mismatch: actually "
-					    << n.get_location()
-					    << ", claims " << n.get_block_nr();
-
-					report_damage(out.str());
-					return false;
-				}
-
-				return true;
-			}
-
-			template <typename node>
-			bool check_value_size(node const &n) {
-				if (!n.value_sizes_match()) {
-					report_damage(n.value_mismatch_string());
-					return false;
-				}
-
-				return true;
-			}
-
-			template <typename node>
-			bool check_max_entries(node const &n) {
-				size_t elt_size = sizeof(uint64_t) + n.get_value_size();
-				if (elt_size * n.get_max_entries() + sizeof(node_header) > MD_BLOCK_SIZE) {
-					std::ostringstream out;
-					out << "max entries too large: " << n.get_max_entries()
-					    << " (block " << n.get_location() << ")";
-					report_damage(out.str());
-					return false;
-				}
-
-				if (n.get_max_entries() % 3) {
-					std::ostringstream out;
-					out << "max entries is not divisible by 3: " << n.get_max_entries()
-					    << " (block " << n.get_location() << ")";
-					report_damage(out.str());
-					return false;
-				}
-
-				return true;
-			}
-
-			template <typename node>
-			bool check_nr_entries(node const &n, bool is_root) {
-				if (n.get_nr_entries() > n.get_max_entries()) {
-					std::ostringstream out;
-					out << "bad nr_entries: "
-					    << n.get_nr_entries() << " < "
-					    << n.get_max_entries()
-					    << " (block " << n.get_location() << ")";
-					report_damage(out.str());
-					return false;
-				}
-
-				block_address min = n.get_max_entries() / 3;
-				if (!is_root && (n.get_nr_entries() < min)) {
-					ostringstream out;
-					out << "too few entries in btree_node: "
-					    << n.get_nr_entries()
-					    << ", expected at least "
-					    << min
-					    << " (block " << n.get_location()
-					    << ", max_entries = " << n.get_max_entries() << ")";
-					report_damage(out.str());
-					return false;
-				}
-
-				return true;
-			}
-
-			template <typename node>
-			bool check_ordered_keys(node const &n) {
-				unsigned nr_entries = n.get_nr_entries();
-
-				if (nr_entries == 0)
-					return true; // can only happen if a root node
-
-				uint64_t last_key = n.key_at(0);
-
-				for (unsigned i = 1; i < nr_entries; i++) {
-					uint64_t k = n.key_at(i);
-					if (k <= last_key) {
-						ostringstream out;
-						out << "keys are out of order, " << k << " <= " << last_key
-						    << " (block " << n.get_location() << ")";
-						report_damage(out.str());
-						return false;
-					}
-					last_key = k;
-				}
-
-				return true;
-			}
-
-			template <typename node>
-			bool check_parent_key(boost::optional<uint64_t> key, node const &n) {
-				if (!key)
-					return true;
-
-				if (*key > n.key_at(0)) {
-					ostringstream out;
-					out << "parent key mismatch: parent was " << *key
-					    << ", but lowest in node was " << n.key_at(0)
-					    << " (block " << n.get_location() << ")";
-					report_damage(out.str());
-					return false;
-				}
-
-				return true;
-			}
-
-			template <typename node>
-			bool check_leaf_key(unsigned level, node const &n) {
-				if (n.get_nr_entries() == 0)
-					return true; // can only happen if a root node
-
-				if (last_leaf_key_[level] && *last_leaf_key_[level] >= n.key_at(0)) {
-					ostringstream out;
-					out << "the last key of the previous leaf was " << *last_leaf_key_[level]
-					    << " and the first key of this leaf is " << n.key_at(0)
-					    << " (block " << n.get_location() << ")";
-					report_damage(out.str());
-					return false;
-				}
-
-				last_leaf_key_[level] = n.key_at(n.get_nr_entries() - 1);
-				return true;
-			}
-
 			void new_root(unsigned level) {
 				// we're starting a new subtree, so should
 				// reset the last_leaf value.
@@ -488,6 +363,7 @@ namespace persistent_data {
 			std::set<block_address> seen_;
 			boost::optional<uint64_t> last_leaf_key_[Levels];
 
+			btree_node_checker checker_;
 			path_tracker path_tracker_;
 			damage_tracker dt_;
 			std::list<std::string> damage_reasons_;

persistent-data/data-structures/btree_node_checker.cc

@@ -0,0 +1,127 @@
+#include "btree_node_checker.h"
+
+#include <sstream>
+
+using persistent_data::btree_detail::btree_node_checker;
+
+//----------------------------------------------------------------
+
+btree_node_checker::error_type btree_node_checker::get_last_error() {
+	return last_error_;
+}
+
+std::string btree_node_checker::get_last_error_string() {
+	switch (last_error_) {
+	case BLOCK_NR_MISMATCH:
+		return block_nr_mismatch_string();
+	case VALUE_SIZES_MISMATCH:
+		return value_sizes_mismatch_string();
+	case MAX_ENTRIES_TOO_LARGE:
+		return max_entries_too_large_string();
+	case MAX_ENTRIES_NOT_DIVISIBLE:
+		return max_entries_not_divisible_string();
+	case NR_ENTRIES_TOO_LARGE:
+		return nr_entries_too_large_string();
+	case NR_ENTRIES_TOO_SMALL:
+		return nr_entries_too_small_string();
+	case KEYS_OUT_OF_ORDER:
+		return keys_out_of_order_string();
+	case PARENT_KEY_MISMATCH:
+		return parent_key_mismatch_string();
+	case LEAF_KEY_OVERLAPPED:
+		return leaf_key_overlapped_string();
+	default:
+		return std::string();
+	}
+}
+
+void btree_node_checker::reset() {
+	last_error_ = NO_ERROR;
+}
+
+std::string btree_node_checker::block_nr_mismatch_string() {
+	std::ostringstream out;
+	out << "block number mismatch: actually "
+	    << error_location_
+	    << ", claims " << error_block_nr_;
+
+	return out.str();
+}
+
+std::string btree_node_checker::value_sizes_mismatch_string() {
+	std::ostringstream out;
+	out << "value size mismatch: expected " << error_value_sizes_[1]
+	    << ", but got " << error_value_sizes_[0]
+	    << ". This is not the btree you are looking for."
+	    << " (block " << error_location_ << ")";
+
+	return out.str();
+}
+
+std::string btree_node_checker::max_entries_too_large_string() {
+	std::ostringstream out;
+	out << "max entries too large: " << error_max_entries_
+	    << " (block " << error_location_ << ")";
+
+	return out.str();
+}
+
+std::string btree_node_checker::max_entries_not_divisible_string() {
+	std::ostringstream out;
+	out << "max entries is not divisible by 3: " << error_max_entries_
+	    << " (block " << error_location_ << ")";
+
+	return out.str();
+}
+
+std::string btree_node_checker::nr_entries_too_large_string() {
+	std::ostringstream out;
+	out << "bad nr_entries: "
+	    << error_nr_entries_ << " < "
+	    << error_max_entries_
+	    << " (block " << error_location_ << ")";
+
+	return out.str();
+}
+
+std::string btree_node_checker::nr_entries_too_small_string() {
+	std::ostringstream out;
+	out << "too few entries in btree_node: "
+	    << error_nr_entries_
+	    << ", expected at least "
+	    << (error_max_entries_ / 3)
+	    << " (block " << error_location_
+	    << ", max_entries = " << error_max_entries_ << ")";
+
+	return out.str();
+}
+
+std::string btree_node_checker::keys_out_of_order_string() {
+	std::ostringstream out;
+	out << "keys are out of order, "
+	    << error_keys_[0] << " <= " << error_keys_[1]
+	    << " (block " << error_location_ << ")";
+
+	return out.str();
+}
+
+std::string btree_node_checker::parent_key_mismatch_string() {
+	std::ostringstream out;
+	out << "parent key mismatch: parent was " << error_keys_[1]
+	    << ", but lowest in node was " << error_keys_[0]
+	    << " (block " << error_location_ << ")";
+
+	return out.str();
+}
+
+std::string btree_node_checker::leaf_key_overlapped_string() {
+	std::ostringstream out;
+	out << "the last key of the previous leaf was " << error_keys_[1]
+	    << " and the first key of this leaf is " << error_keys_[0]
+	    << " (block " << error_location_ << ")";
+
+	return out.str();
+}
+
+//----------------------------------------------------------------
+

persistent-data/data-structures/btree_node_checker.h

@@ -0,0 +1,206 @@
+#ifndef BTREE_NODE_CHECKER_H
+#define BTREE_NODE_CHECKER_H
+
+#include "block-cache/block_cache.h"
+#include "persistent-data/block.h"
+#include "persistent-data/data-structures/btree.h"
+#include "persistent-data/data-structures/btree_disk_structures.h"
+
+#include <boost/optional.hpp>
+#include <string>
+
+using bcache::block_address;
+
+//----------------------------------------------------------------
+
+namespace persistent_data {
+	namespace btree_detail {
+		class btree_node_checker {
+		public:
+			enum error_type {
+				NO_ERROR,
+				BLOCK_NR_MISMATCH,
+				VALUE_SIZES_MISMATCH,
+				MAX_ENTRIES_TOO_LARGE,
+				MAX_ENTRIES_NOT_DIVISIBLE,
+				NR_ENTRIES_TOO_LARGE,
+				NR_ENTRIES_TOO_SMALL,
+				KEYS_OUT_OF_ORDER,
+				VALUE_SIZE_MISMATCH,
+				PARENT_KEY_MISMATCH,
+				LEAF_KEY_OVERLAPPED,
+			};
+
+			btree_node_checker():
+				last_error_(NO_ERROR),
+				error_location_(0),
+				error_block_nr_(0),
+				error_nr_entries_(0),
+				error_max_entries_(0),
+				error_value_sizes_{0, 0},
+				error_keys_{0, 0} {
+			}
+
+			template <typename ValueTraits>
+			bool check_block_nr(btree_detail::node_ref<ValueTraits> const &n) {
+				if (n.get_location() != n.get_block_nr()) {
+					last_error_ = BLOCK_NR_MISMATCH;
+					error_block_nr_ = n.get_block_nr();
+					error_location_ = n.get_location();
+
+					return false;
+				}
+
+				return true;
+			}
+
+			template <typename ValueTraits>
+			bool check_value_size(btree_detail::node_ref<ValueTraits> const &n) {
+				if (!n.value_sizes_match()) {
+					last_error_ = VALUE_SIZES_MISMATCH;
+					error_location_ = n.get_location();
+					error_value_sizes_[0] = n.get_value_size();
+					error_value_sizes_[1] = sizeof(typename ValueTraits::disk_type);
+					return false;
+				}
+
+				return true;
+			}
+
+			template <typename ValueTraits>
+			bool check_max_entries(btree_detail::node_ref<ValueTraits> const &n) {
+				size_t elt_size = sizeof(uint64_t) + n.get_value_size();
+				if (elt_size * n.get_max_entries() + sizeof(node_header) > MD_BLOCK_SIZE) {
+					last_error_ = MAX_ENTRIES_TOO_LARGE;
+					error_location_ = n.get_location();
+					error_max_entries_ = n.get_max_entries();
+
+					return false;
+				}
+
+				if (n.get_max_entries() % 3) {
+					last_error_ = MAX_ENTRIES_NOT_DIVISIBLE;
+					error_location_ = n.get_location();
+					error_max_entries_ = n.get_max_entries();
+
+					return false;
+				}
+
+				return true;
+			}
+
+			template <typename ValueTraits>
+			bool check_nr_entries(btree_detail::node_ref<ValueTraits> const &n,
+					      bool is_root) {
+				if (n.get_nr_entries() > n.get_max_entries()) {
+					last_error_ = NR_ENTRIES_TOO_LARGE;
+					error_location_ = n.get_location();
+					error_nr_entries_ = n.get_nr_entries();
+					error_max_entries_ = n.get_max_entries();
+
+					return false;
+				}
+
+				block_address min = n.get_max_entries() / 3;
+				if (!is_root && (n.get_nr_entries() < min)) {
+					last_error_ = NR_ENTRIES_TOO_SMALL;
+					error_location_ = n.get_location();
+					error_nr_entries_ = n.get_nr_entries();
+					error_max_entries_ = n.get_max_entries();
+
+					return false;
+				}
+
+				return true;
+			}
+
+			template <typename ValueTraits>
+			bool check_ordered_keys(btree_detail::node_ref<ValueTraits> const &n) {
+				unsigned nr_entries = n.get_nr_entries();
+
+				if (nr_entries == 0)
+					return true; // can only happen if a root node
+
+				uint64_t last_key = n.key_at(0);
+
+				for (unsigned i = 1; i < nr_entries; i++) {
+					uint64_t k = n.key_at(i);
+					if (k <= last_key) {
+						last_error_ = KEYS_OUT_OF_ORDER;
+						error_location_ = n.get_location();
+						error_keys_[0] = k;
+						error_keys_[1] = last_key;
+
+						return false;
+					}
+					last_key = k;
+				}
+
+				return true;
+			}
+
+			template <typename ValueTraits>
+			bool check_parent_key(btree_detail::node_ref<ValueTraits> const &n,
+					      boost::optional<uint64_t> key) {
+				if (!key)
+					return true;
+
+				if (*key > n.key_at(0)) {
+					last_error_ = PARENT_KEY_MISMATCH;
+					error_location_ = n.get_location();
+					error_keys_[0] = n.key_at(0);
+					error_keys_[1] = *key;
+
+					return false;
+				}
+
+				return true;
+			}
+
+			template <typename ValueTraits>
+			bool check_leaf_key(btree_detail::node_ref<ValueTraits> const &n,
+					    boost::optional<uint64_t> key) {
+				if (n.get_nr_entries() == 0)
+					return true; // can only happen if a root node
+
+				if (key && *key >= n.key_at(0)) {
+					last_error_ = LEAF_KEY_OVERLAPPED;
+					error_location_ = n.get_location();
+					error_keys_[0] = n.key_at(0);
+					error_keys_[1] = *key;
+
+					return false;
+				}
+
+				return true;
+			}
+
+			error_type get_last_error();
+			std::string get_last_error_string();
+			void reset();
+
+		private:
+			std::string block_nr_mismatch_string();
+			std::string value_sizes_mismatch_string();
+			std::string max_entries_too_large_string();
+			std::string max_entries_not_divisible_string();
+			std::string nr_entries_too_large_string();
+			std::string nr_entries_too_small_string();
+			std::string keys_out_of_order_string();
+			std::string parent_key_mismatch_string();
+			std::string leaf_key_overlapped_string();
+
+			error_type last_error_;
+			block_address error_location_;
+			block_address error_block_nr_;
+			uint32_t error_nr_entries_;
+			uint32_t error_max_entries_;
+			uint32_t error_value_sizes_[2];
+			uint64_t error_keys_[2];
+		};
+	}
+}
+
+//----------------------------------------------------------------
+
+#endif

thin-provisioning/metadata_counter.cc

@@ -66,7 +66,6 @@ void thin_provisioning::count_metadata(transaction_manager::ptr tm,
 		count_trees(tm, snap, bc);
 	}
 
-	count_trees(tm, sb, bc);
 	count_space_maps(tm, sb, bc);
 }
 

thin-provisioning/thin_check.cc

@@ -31,6 +31,7 @@
 #include "persistent-data/file_utils.h"
 #include "thin-provisioning/device_tree.h"
 #include "thin-provisioning/mapping_tree.h"
+#include "thin-provisioning/metadata_counter.h"
 #include "thin-provisioning/superblock.h"
 #include "thin-provisioning/commands.h"
 
@@ -169,58 +170,13 @@ namespace {
 		bool clear_needs_check_flag_on_success;
 	};
 
-	void count_trees(transaction_manager::ptr tm,
-			 superblock_detail::superblock &sb,
-			 block_counter &bc) {
-
-		// 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 the mapping tree
-		{
-			noop_value_counter<mapping_tree_detail::block_time> vc;
-			mapping_tree mtree(*tm, sb.data_mapping_root_,
-					   mapping_tree_detail::block_traits::ref_counter(tm->get_sm()));
-			count_btree_blocks(mtree, bc, vc);
-		}
-	}
-
 	error_state check_space_map_counts(flags const &fs, nested_output &out,
 					   superblock_detail::superblock &sb,
 					   block_manager<>::ptr bm,
 					   transaction_manager::ptr tm) {
 		block_counter bc;
 
-		// Count the superblock
+		count_metadata(tm, sb, bc);
-		bc.inc(superblock_detail::SUPERBLOCK_LOCATION);
-		count_trees(tm, sb, bc);
-
-		// Count the metadata snap, if present
-		if (sb.metadata_snap_ != superblock_detail::SUPERBLOCK_LOCATION) {
-			bc.inc(sb.metadata_snap_);
-
-			superblock_detail::superblock snap = read_superblock(bm, sb.metadata_snap_);
-			count_trees(tm, snap, bc);
-		}
-
-		// Count the metadata space map
-		{
-			persistent_space_map::ptr metadata_sm =
-				open_metadata_sm(*tm, static_cast<void *>(&sb.metadata_space_map_root_));
-			metadata_sm->count_metadata(bc);
-		}
-
-		// Count the data space map
-		{
-			persistent_space_map::ptr data_sm =
-				open_disk_sm(*tm, static_cast<void *>(&sb.data_space_map_root_));
-			data_sm->count_metadata(bc);
-		}
 
 		// Finally we need to check the metadata space map agrees
 		// with the counts we've just calculated.

thin-provisioning/thin_ll_dump.cc

@@ -24,6 +24,8 @@
 #include "persistent-data/file_utils.h"
 #include "persistent-data/data-structures/btree.h"
 #include "persistent-data/data-structures/btree_counter.h"
+#include "persistent-data/data-structures/btree_damage_visitor.h"
+#include "persistent-data/data-structures/btree_node_checker.h"
 #include "persistent-data/data-structures/simple_traits.h"
 #include "persistent-data/space-maps/core.h"
 #include "persistent-data/space-maps/disk_structures.h"
@@ -33,70 +35,11 @@
 #include "version.h"
 
 using namespace thin_provisioning;
+using namespace persistent_data;
 
 //----------------------------------------------------------------
 
 namespace {
-	// extracted from btree_damage_visitor.h
-	template <typename node>
-	bool check_block_nr(node const &n) {
-		if (n.get_location() != n.get_block_nr()) {
-			return false;
-		}
-		return true;
-	}
-
-	// extracted from btree_damage_visitor.h
-	template <typename node>
-	bool check_max_entries(node const &n) {
-		size_t elt_size = sizeof(uint64_t) + n.get_value_size();
-		if (elt_size * n.get_max_entries() + sizeof(node_header) > MD_BLOCK_SIZE) {
-			return false;
-		}
-
-		if (n.get_max_entries() % 3) {
-			return false;
-		}
-
-		return true;
-	}
-
-	// extracted from btree_damage_visitor.h
-	template <typename node>
-	bool check_nr_entries(node const &n, bool is_root) {
-		if (n.get_nr_entries() > n.get_max_entries()) {
-			return false;
-		}
-
-		block_address min = n.get_max_entries() / 3;
-		if (!is_root && (n.get_nr_entries() < min)) {
-			return false;
-		}
-
-		return true;
-	}
-
-	// extracted from btree_damage_visitor.h
-	template <typename node>
-	bool check_ordered_keys(node const &n) {
-		unsigned nr_entries = n.get_nr_entries();
-
-		if (nr_entries == 0)
-			return true; // can only happen if a root node
-
-		uint64_t last_key = n.key_at(0);
-
-		for (unsigned i = 1; i < nr_entries; i++) {
-			uint64_t k = n.key_at(i);
-			if (k <= last_key) {
-				return false;
-			}
-			last_key = k;
-		}
-
-		return true;
-	}
-
 	transaction_manager::ptr
 	open_tm(block_manager<>::ptr bm) {
 		space_map::ptr sm(new core_map(bm->get_nr_blocks()));
@@ -136,19 +79,20 @@ namespace {
 			if ((n.get_value_size() == sizeof(mapping_tree_detail::block_traits::disk_type) ||
 			     n.get_value_size() == sizeof(device_tree_detail::device_details_traits::disk_type)) &&
 			    !bc_.get_count(n.get_location()) &&
-			    check_block_nr(n) &&
+			    checker_.check_block_nr(n) &&
 			    (((flags & INTERNAL_NODE) && !(flags & LEAF_NODE)) ||
 			     (flags & LEAF_NODE)) &&
 			    nv_->check_raw(n.raw()) &&
-			    check_max_entries(n) &&
+			    checker_.check_max_entries(n) &&
-			    check_nr_entries(n, true) &&
+			    checker_.check_nr_entries(n, true) &&
-			    check_ordered_keys(n))
+			    checker_.check_ordered_keys(n))
 				return true;
 			return false;
 		}
 
 		bcache::validator::ptr nv_;
 		block_counter const &bc_;
+		btree_detail::btree_node_checker checker_;
 	};
 
 	//-------------------------------------------------------------------