From e727bc943a4591a6deae02e679b6507fcf15e476 Mon Sep 17 00:00:00 2001 From: Joe Thornber Date: Fri, 2 Sep 2011 11:26:42 +0100 Subject: [PATCH] check keys are strictly ordered, and parent keys are correct --- btree.h | 11 +++++--- btree.tcc | 14 ++++----- btree_checker.h | 72 +++++++++++++++++++++++++++++++++++++++++++---- metadata.cc | 15 ++++++---- space_map_disk.cc | 13 +++++---- 5 files changed, 96 insertions(+), 29 deletions(-) diff --git a/btree.h b/btree.h index 238f65f..5be91e2 100644 --- a/btree.h +++ b/btree.h @@ -325,9 +325,12 @@ namespace persistent_data { // The bool return values indicate whether the walk // should be continued into sub trees of the node (true == continue). - virtual bool visit_internal(unsigned level, bool is_root, internal_node const &n) = 0; - virtual bool visit_internal_leaf(unsigned level, bool is_root, internal_node const &n) = 0; - virtual bool visit_leaf(unsigned level, bool is_root, leaf_node const &n) = 0; + virtual bool visit_internal(unsigned level, boost::optional key, + internal_node const &n) = 0; + virtual bool visit_internal_leaf(unsigned level, boost::optional key, + internal_node const &n) = 0; + virtual bool visit_leaf(unsigned level, boost::optional key, + leaf_node const &n) = 0; }; // Walks the tree in depth first order @@ -356,7 +359,7 @@ namespace persistent_data { int *index); void walk_tree(typename visitor::ptr visitor, - unsigned level, bool is_root, + unsigned level, boost::optional key, block_address b) const; typename persistent_data::transaction_manager::ptr tm_; diff --git a/btree.tcc b/btree.tcc index 09e5210..da33eeb 100644 --- a/btree.tcc +++ b/btree.tcc @@ -592,14 +592,14 @@ template void btree::visit(typename visitor::ptr visitor) const { - walk_tree(visitor, 0, true, root_); + walk_tree(visitor, 0, boost::optional(), root_); } template void btree:: walk_tree(typename visitor::ptr visitor, - unsigned level, bool is_root, + unsigned level, boost::optional key, block_address b) const { using namespace btree_detail; @@ -607,18 +607,18 @@ walk_tree(typename visitor::ptr visitor, read_ref blk = tm_->read_lock(b); internal_node o = to_node(blk); if (o.get_type() == INTERNAL) { - if (visitor->visit_internal(level, is_root, o)) + if (visitor->visit_internal(level, key, o)) for (unsigned i = 0; i < o.get_nr_entries(); i++) - walk_tree(visitor, level, false, o.value_at(i)); + walk_tree(visitor, level, o.key_at(i), o.value_at(i)); } else if (level < Levels - 1) { - if (visitor->visit_internal_leaf(level, is_root, o)) + if (visitor->visit_internal_leaf(level, key, o)) for (unsigned i = 0; i < o.get_nr_entries(); i++) - walk_tree(visitor, level + 1, true, o.value_at(i)); + walk_tree(visitor, level + 1, boost::optional(), o.value_at(i)); } else { leaf_node ov = to_node(blk); - visitor->visit_leaf(level, is_root, ov); + visitor->visit_leaf(level, key, ov); } } diff --git a/btree_checker.h b/btree_checker.h index f8f4dee..9bfbadc 100644 --- a/btree_checker.h +++ b/btree_checker.h @@ -72,36 +72,47 @@ namespace persistent_data { errs_(new error_set("btree errors")) { } - bool visit_internal(unsigned level, bool is_root, + bool visit_internal(unsigned level, + optional key, btree_detail::node_ref const &n) { if (already_visited(n)) return false; check_block_nr(n); check_max_entries(n); - check_nr_entries(n, is_root); + check_nr_entries(n, !key); + check_ordered_keys(n); + check_parent_key(key, n); return true; } - bool visit_internal_leaf(unsigned level, bool is_root, + bool visit_internal_leaf(unsigned level, + optional key, btree_detail::node_ref const &n) { if (already_visited(n)) return false; check_block_nr(n); check_max_entries(n); - check_nr_entries(n, is_root); + check_nr_entries(n, !key); + check_ordered_keys(n); + check_parent_key(key, n); + check_leaf_key(level, n); return true; } - bool visit_leaf(unsigned level, bool is_root, + bool visit_leaf(unsigned level, + optional key, btree_detail::node_ref const &n) { if (already_visited(n)) return false; check_block_nr(n); check_max_entries(n); - check_nr_entries(n, is_root); + check_nr_entries(n, !key); + check_ordered_keys(n); + check_parent_key(key, n); + check_leaf_key(level, n); return true; } @@ -180,9 +191,58 @@ namespace persistent_data { } } + template + void check_ordered_keys(node const &n) const { + unsigned nr_entries = n.get_nr_entries(); + + if (nr_entries == 0) + return; // 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; + throw runtime_error(out.str()); + } + last_key = k; + } + } + + template + void check_parent_key(boost::optional key, node const &n) const { + if (!key) + return; + + if (*key > n.key_at(0)) { + ostringstream out; + out << "parent key mismatch: parent was " << *key + << ", but lowest in node was " << n.key_at(0); + throw runtime_error(out.str()); + } + } + + template + void check_leaf_key(unsigned level, node const &n) { + if (n.get_nr_entries() == 0) + return; // 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); + throw runtime_error(out.str()); + } + + last_leaf_key_[level] = n.key_at(n.get_nr_entries() - 1); + } + block_counter &counter_; std::set seen_; error_set::ptr errs_; + boost::optional last_leaf_key_[Levels]; }; } diff --git a/metadata.cc b/metadata.cc index b8b18b3..59f157b 100644 --- a/metadata.cc +++ b/metadata.cc @@ -56,10 +56,11 @@ namespace { // Sharing can only occur in level 1 nodes. // FIXME: not true once we start having held roots. - bool visit_internal_leaf(unsigned level, bool is_root, + bool visit_internal_leaf(unsigned level, + optional key, btree_detail::node_ref const &n) { - bool r = btree_checker<2, block_traits>::visit_internal_leaf(level, is_root, n); + bool r = btree_checker<2, block_traits>::visit_internal_leaf(level, key, n); if (!r && level == 0) { throw runtime_error("unexpected sharing in level 0 of mapping tree."); } @@ -70,9 +71,10 @@ namespace { return r; } - bool visit_leaf(unsigned level, bool is_root, + bool visit_leaf(unsigned level, + optional key, btree_detail::node_ref const &n) { - bool r = btree_checker<2, block_traits>::visit_leaf(level, is_root, n); + bool r = btree_checker<2, block_traits>::visit_leaf(level, key, n); if (r) for (unsigned i = 0; i < n.get_nr_entries(); i++) @@ -98,9 +100,10 @@ namespace { : btree_checker<1, device_details_traits>(counter) { } - bool visit_leaf(unsigned level, bool is_root, + bool visit_leaf(unsigned level, + optional key, btree_detail::node_ref const &n) { - bool r = btree_checker<1, device_details_traits>::visit_leaf(level, is_root, n); + bool r = btree_checker<1, device_details_traits>::visit_leaf(level, key, n); if (r) for (unsigned i = 0; i < n.get_nr_entries(); i++) diff --git a/space_map_disk.cc b/space_map_disk.cc index 20e0add..49e72d0 100644 --- a/space_map_disk.cc +++ b/space_map_disk.cc @@ -109,11 +109,11 @@ namespace { } }; - class ref_count_validator : public btree_checker<1, ref_count_traits> { + class ref_count_checker : public btree_checker<1, ref_count_traits> { public: - typedef boost::shared_ptr ptr; + typedef boost::shared_ptr ptr; - ref_count_validator(block_counter &counter) + ref_count_checker(block_counter &counter) : btree_checker<1, ref_count_traits>(counter) { } }; @@ -239,7 +239,7 @@ namespace { } virtual void check(block_counter &counter) const { - ref_count_validator::ptr v(new ref_count_validator(counter)); + ref_count_checker::ptr v(new ref_count_checker(counter)); ref_counts_.visit(v); } @@ -315,9 +315,10 @@ namespace { : btree_checker<1, index_entry_traits>(counter) { } - bool visit_leaf(unsigned level, bool is_root, + bool visit_leaf(unsigned level, + optional key, btree_detail::node_ref const &n) { - bool r = btree_checker<1, index_entry_traits>::visit_leaf(level, is_root, n); + bool r = btree_checker<1, index_entry_traits>::visit_leaf(level, key, n); if (!r) return r;