Sanitise btree_detail::node_location

persistent-data/data-structures/btree.h

@@ -26,6 +26,7 @@
 #include <boost/noncopyable.hpp>
 #include <boost/optional.hpp>
 #include <list>
+#include <deque>
 
 //----------------------------------------------------------------
 
@@ -266,9 +267,36 @@ namespace persistent_data {
 
 		// Used when visiting the nodes that make up a btree.
 		struct node_location {
-			unsigned level;
+			node_location()
+				: depth(0) {
+			}
+
+			void inc_depth() {
+				depth++;
+			}
+
+			void push_key(uint64_t k) {
+				path.push_back(k);
+				depth = 0;
+			}
+
+			bool is_sub_root() const {
+				return depth == 0; // && path.size();
+			}
+
+			unsigned level() const {
+				return path.size();
+			}
+
+			// Keys used to access this sub tree
+			std::deque<uint64_t> path;
+
+			// in this sub tree
 			unsigned depth;
-			bool sub_root;
+
+			// This is the key from the parent node to this
+			// node.  If this node is a root then there will be
+			// no parent, and hence no key.
 			boost::optional<uint64_t> key;
 		};
 	}

persistent-data/data-structures/btree.tcc

@@ -746,11 +746,6 @@ btree<Levels, ValueTraits>::visit_depth_first(visitor &v) const
 {
 	node_location loc;
 
-	loc.level = 0;
-	loc.depth = 0;
-	loc.sub_root = true;
-	loc.key = boost::optional<uint64_t>();
-
 	walk_tree(v, loc, root_);
 	v.visit_complete();
 }
@@ -785,29 +780,28 @@ btree<Levels, ValueTraits>::walk_tree_internal(visitor &v,
 
 	read_ref blk = tm_->read_lock(b, validator_);
 	internal_node o = to_node<uint64_traits>(blk);
+
+	// FIXME: use a switch statement
 	if (o.get_type() == INTERNAL) {
 		if (v.visit_internal(loc, o))
 			for (unsigned i = 0; i < o.get_nr_entries(); i++) {
 				node_location loc2(loc);
 
-				loc2.depth = loc.depth + 1;
+				loc2.inc_depth();
-				loc2.sub_root = false;
+				loc2.key = o.key_at(i);
-				loc2.key = boost::optional<uint64_t>(o.key_at(i));
 
 				walk_tree(v, loc2, o.value_at(i));
 			}
 
-	} else if (loc.level < Levels - 1) {
+	} else if (loc.path.size() < Levels - 1) {
 		if (v.visit_internal_leaf(loc, o))
 			for (unsigned i = 0; i < o.get_nr_entries(); i++) {
 				node_location loc2(loc);
 
-				loc2.level = loc.level + 1;
+				loc2.push_key(o.key_at(i));
-				loc2.depth = loc.depth + 1;
+				loc2.key = optional<uint64_t>();
-				loc2.sub_root = true;
-				loc2.key = boost::optional<uint64_t>(o.key_at(i));
 
-				walk_tree(v, loc, o.value_at(i));
+				walk_tree(v, loc2, o.value_at(i));
 			}
 
 	} else {

persistent-data/data-structures/btree_checker.h

@@ -95,11 +95,11 @@ namespace persistent_data {
 			if (!already_visited(n) &&
 			    check_block_nr(n) &&
 			    check_max_entries(n) &&
-			    check_nr_entries(n, loc.sub_root) &&
+			    check_nr_entries(n, loc.is_sub_root()) &&
 			    check_ordered_keys(n) &&
-			    check_parent_key(loc.sub_root ? optional<uint64_t>() : loc.key, n)) {
+			    check_parent_key(loc.is_sub_root() ? optional<uint64_t>() : loc.key, n)) {
-				if (loc.sub_root)
+				if (loc.is_sub_root())
-					new_root(loc.level);
+					new_root(loc.level());
 
 				return true;
 			}
@@ -113,13 +113,13 @@ namespace persistent_data {
 			if (!already_visited(n) &&
 			    check_block_nr(n) &&
 			    check_max_entries(n) &&
-			    check_nr_entries(n, loc.sub_root) &&
+			    check_nr_entries(n, loc.is_sub_root()) &&
 			    check_ordered_keys(n) &&
-			    check_parent_key(loc.sub_root ? optional<uint64_t>() : loc.key, n)) {
+			    check_parent_key(loc.is_sub_root() ? optional<uint64_t>() : loc.key, n)) {
-				if (loc.sub_root)
+				if (loc.is_sub_root())
-					new_root(loc.level);
+					new_root(loc.level());
 
-				return check_leaf_key(loc.level, n);
+				return check_leaf_key(loc.level(), n);
 			}
 
 			return false;

persistent-data/data-structures/btree_damage_visitor.h

@@ -38,6 +38,8 @@ namespace persistent_data {
 			return out;
 		}
 
+		// Tracks damage in a single level btree.  Use multiple
+		// trackers if you have a multilayer tree.
 		class damage_tracker {
 		public:
 			damage_tracker()
@@ -190,11 +192,11 @@ namespace persistent_data {
 			if (!already_visited(n) &&
 			    check_block_nr(n) &&
 			    check_max_entries(n) &&
-			    check_nr_entries(n, loc.sub_root) &&
+			    check_nr_entries(n, loc.is_sub_root()) &&
 			    check_ordered_keys(n) &&
-			    check_parent_key(loc.sub_root ? optional<uint64_t>() : loc.key, n)) {
+			    check_parent_key(loc.is_sub_root() ? optional<uint64_t>() : loc.key, n)) {
-				if (loc.sub_root)
+				if (loc.is_sub_root())
-					new_root(loc.level);
+					new_root(loc.level());
 
 				good_internal(n.key_at(0));
 				return true;
@@ -209,13 +211,13 @@ namespace persistent_data {
 			if (!already_visited(n) &&
 			    check_block_nr(n) &&
 			    check_max_entries(n) &&
-			    check_nr_entries(n, loc.sub_root) &&
+			    check_nr_entries(n, loc.is_sub_root()) &&
 			    check_ordered_keys(n) &&
-			    check_parent_key(loc.sub_root ? optional<uint64_t>() : loc.key, n)) {
+			    check_parent_key(loc.is_sub_root() ? optional<uint64_t>() : loc.key, n)) {
-				if (loc.sub_root)
+				if (loc.is_sub_root())
-					new_root(loc.level);
+					new_root(loc.level());
 
-				bool r = check_leaf_key(loc.level, n);
+				bool r = check_leaf_key(loc.level(), n);
 				if (r && n.get_nr_entries() > 0)
 					good_leaf(n.key_at(0), n.key_at(n.get_nr_entries() - 1) + 1);
 
@@ -291,7 +293,7 @@ namespace persistent_data {
 			block_address min = n.get_max_entries() / 3;
 			if (!is_root && (n.get_nr_entries() < min)) {
 				ostringstream out;
-				out << "too few entries in btree: "
+				out << "too few entries in btree_node: "
 				    << n.get_nr_entries()
 				    << ", expected at least "
 				    << min

thin-provisioning/metadata_dumper.cc

@@ -50,7 +50,7 @@ namespace {
 				return false;
 			}
 
-			return (loc.sub_root && loc.key) ? (*loc.key == dev_id_) : true;
+			return (loc.is_sub_root() && loc.key) ? (*loc.key == dev_id_) : true;
 		}
 
 		bool visit_internal_leaf(node_location const &loc,

unit-tests/btree_damage_visitor_t.cc

@@ -196,7 +196,7 @@ namespace {
 
 			ni->leaf = leaf;
 			ni->depth = loc.depth;
-			ni->level = loc.level;
+			ni->level = loc.level();
 			ni->b = n.get_location();
 
 			if (n.get_nr_entries())
@@ -235,30 +235,80 @@ namespace {
 		MOCK_METHOD1(visit, void(btree_detail::damage const &));
 	};
 
-	class BTreeDamageVisitorTests : public Test {
+	class DamageTests : public Test {
 	public:
-		BTreeDamageVisitorTests()
+		DamageTests()
 			: bm_(create_bm<BLOCK_SIZE>(NR_BLOCKS)),
 			  sm_(setup_core_map()),
-			  tm_(new transaction_manager(bm_, sm_)),
+			  tm_(new transaction_manager(bm_, sm_)) {
-			  tree_(new btree<1, thing_traits>(tm_, rc_)) {
 		}
 
-		space_map::ptr setup_core_map() {
+		virtual ~DamageTests() {}
-			space_map::ptr sm(new core_map(NR_BLOCKS));
-			sm->inc(SUPERBLOCK);
-			return sm;
-		}
 
 		void tree_complete() {
 			commit();
 			discover_layout();
 		}
 
+		void run() {
+			commit();
+			run_();
+		}
+
 		void trash_block(block_address b) {
 			::test::zero_block(bm_, b);
 		}
 
+		//--------------------------------
+
+		void expect_no_values() {
+			EXPECT_CALL(value_visitor_, visit(_)).Times(0);
+		}
+
+		void expect_no_damage() {
+			EXPECT_CALL(damage_visitor_, visit(_)).Times(0);
+		}
+
+		void expect_damage(unsigned level, range<uint64_t> keys) {
+			EXPECT_CALL(damage_visitor_, visit(Eq(damage(level, keys, "foo")))).Times(1);
+		}
+
+		//--------------------------------
+
+		with_temp_directory dir_;
+		block_manager<>::ptr bm_;
+		space_map::ptr sm_;
+		transaction_manager::ptr tm_;
+		thing_traits::ref_counter rc_;
+
+		optional<btree_layout> layout_;
+
+		value_visitor_mock value_visitor_;
+		damage_visitor_mock damage_visitor_;
+
+	private:
+		space_map::ptr setup_core_map() {
+			space_map::ptr sm(new core_map(NR_BLOCKS));
+			sm->inc(SUPERBLOCK);
+			return sm;
+		}
+
+		void commit() {
+			block_manager<>::write_ref superblock(bm_->superblock(SUPERBLOCK));
+		}
+
+		virtual void discover_layout() = 0;
+		virtual void run_() = 0;
+	};
+
+	//--------------------------------
+
+	class BTreeDamageVisitorTests : public DamageTests {
+	public:
+		BTreeDamageVisitorTests()
+			: tree_(new btree<1, thing_traits>(tm_, rc_)) {
+		}
+
 		void insert_values(unsigned nr) {
 			for (unsigned i = 0; i < nr; i++) {
 				uint64_t key[1] = {i};
@@ -268,10 +318,6 @@ namespace {
 			}
 		}
 
-		void expect_no_values() {
-			EXPECT_CALL(value_visitor_, visit(_)).Times(0);
-		}
-
 		void expect_value_range(uint64_t begin, uint64_t end) {
 			while (begin < end) {
 				EXPECT_CALL(value_visitor_, visit(Eq(thing(begin, begin + 1234)))).Times(1);
@@ -287,49 +333,76 @@ namespace {
 			EXPECT_CALL(value_visitor_, visit(Eq(thing(n, n + 1234)))).Times(1);
 		}
 
-		void expect_no_damage() {
+		btree<1, thing_traits>::ptr tree_;
-			EXPECT_CALL(damage_visitor_, visit(_)).Times(0);
+
+	private:
+		virtual void discover_layout() {
+			btree_layout_visitor<1, thing_traits> visitor;
+			tree_->visit_depth_first(visitor);
+			layout_ = visitor.get_layout();
 		}
 
-		void expect_damage(unsigned level, range<uint64_t> keys) {
+		virtual void run_() {
-			EXPECT_CALL(damage_visitor_, visit(Eq(damage(level, keys, "foo")))).Times(1);
-		}
-
-		void run() {
-			// We must commit before we do the test to ensure
-			// all the block numbers and checksums are written
-			// to the btree nodes.
-			commit();
-
 			block_counter counter;
 			btree_damage_visitor<value_visitor_mock, damage_visitor_mock, 1, thing_traits>
 				visitor(counter, value_visitor_, damage_visitor_);
 			tree_->visit_depth_first(visitor);
 		}
+	};
 
-		with_temp_directory dir_;
+	//--------------------------------
-		block_manager<>::ptr bm_;
-		space_map::ptr sm_;
-		transaction_manager::ptr tm_;
 
-		thing_traits::ref_counter rc_;
+	// 2 level btree
-		btree<1, thing_traits>::ptr tree_;
+	class BTreeDamageVisitor2Tests : public DamageTests {
-
+	public:
-		optional<btree_layout> layout_;
+		BTreeDamageVisitor2Tests()
-
+			: tree_(new btree<2, thing_traits>(tm_, rc_)) {
-		value_visitor_mock value_visitor_;
-		damage_visitor_mock damage_visitor_;
-
-	private:
-		void commit() {
-			block_manager<>::write_ref superblock(bm_->superblock(SUPERBLOCK));
 		}
 
-		void discover_layout() {
+		void insert_values(unsigned nr_sub_trees, unsigned nr_values) {
-			btree_layout_visitor<1, thing_traits> visitor;
+			for (unsigned i = 0; i < nr_sub_trees; i++)
+				insert_sub_tree_values(i, nr_values);
+		}
+
+		void insert_sub_tree_values(unsigned sub_tree, unsigned nr_values) {
+			for (unsigned i = 0; i < nr_values; i++) {
+				uint64_t key[2] = {sub_tree, i};
+				thing value(key_to_value(key));
+				tree_->insert(key, value);
+			}
+		}
+
+		void expect_values(unsigned nr_sub_trees, unsigned nr_values) {
+			for (unsigned i = 0; i < nr_sub_trees; i++)
+				expect_sub_tree_values(i, nr_values);
+		}
+
+		void expect_sub_tree_values(unsigned sub_tree, unsigned nr_values) {
+			for (unsigned i = 0; i < nr_values; i++) {
+				uint64_t key[2] = {sub_tree, i};
+				EXPECT_CALL(value_visitor_, visit(Eq(key_to_value(key))));
+			}
+		}
+
+		btree<2, thing_traits>::ptr tree_;
+
+	private:
+		thing key_to_value(uint64_t key[2]) {
+			return thing(key[0] * 1000000 + key[1], key[1] + 1234);
+		}
+
+		virtual void discover_layout() {
+			btree_layout_visitor<2, thing_traits> visitor;
 			tree_->visit_depth_first(visitor);
 			layout_ = visitor.get_layout();
 		}
+
+		virtual void run_() {
+			block_counter counter;
+			btree_damage_visitor<value_visitor_mock, damage_visitor_mock, 2, thing_traits>
+				visitor(counter, value_visitor_, damage_visitor_);
+			tree_->visit_depth_first(visitor);
+		}
 	};
 }
 
@@ -454,3 +527,33 @@ TEST_F(BTreeDamageVisitorTests, damaged_internal)
 }
 
 //----------------------------------------------------------------
+
+TEST_F(BTreeDamageVisitor2Tests, empty_tree)
+{
+	expect_no_damage();
+	expect_no_values();
+
+	run();
+}
+
+TEST_F(BTreeDamageVisitor2Tests, tree_with_a_trashed_root)
+{
+	trash_block(tree_->get_root());
+
+	expect_no_values();
+	expect_damage(0, range<uint64_t>(0ull));
+
+	run();
+}
+
+TEST_F(BTreeDamageVisitor2Tests, populated_tree_with_no_damage)
+{
+	insert_values(10, 10);
+
+	expect_values(10, 10);
+	expect_no_damage();
+
+	run();
+}
+
+//----------------------------------------------------------------

unit-tests/metadata_checker_t.cc

@@ -115,7 +115,7 @@ namespace {
 
 			ni->leaf = leaf;
 			ni->depth = loc.depth;
-			ni->level = loc.level;
+			ni->level = loc.level();
 			ni->b = n.get_location();
 
 			if (n.get_nr_entries())