From 5d0b23beeafb8a671487ba11ee3137ae50749048 Mon Sep 17 00:00:00 2001 From: Joe Thornber Date: Wed, 1 May 2013 16:16:23 +0100 Subject: [PATCH] Some btree visitor tidying. Introduce node_location to replace the long list of parameters. Also add a depth field to keep track of the depth from root. --- persistent-data/data-structures/btree.h | 22 +++++--- persistent-data/data-structures/btree.tcc | 50 +++++++++++++------ .../data-structures/btree_checker.h | 48 ++++++++---------- persistent-data/space-maps/disk.cc | 11 ++-- thin-provisioning/device_checker.cc | 25 ++++------ thin-provisioning/metadata_dumper.cc | 32 ++++++------ unit-tests/btree_t.cc | 16 +++--- 7 files changed, 109 insertions(+), 95 deletions(-) diff --git a/persistent-data/data-structures/btree.h b/persistent-data/data-structures/btree.h index 6803c74..ac1b0d1 100644 --- a/persistent-data/data-structures/btree.h +++ b/persistent-data/data-structures/btree.h @@ -263,6 +263,14 @@ namespace persistent_data { std::list::write_ref> spine_; block_address root_; }; + + // Used when visiting the nodes that make up a btree. + struct node_location { + unsigned level; + unsigned depth; + bool sub_root; + boost::optional key; + }; } template @@ -308,23 +316,25 @@ namespace persistent_data { // inspect the individual nodes that make up a btree. class visitor { public: - virtual ~visitor() {} typedef boost::shared_ptr ptr; + typedef btree_detail::node_location node_location; + + virtual ~visitor() {} // 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 sub_root, boost::optional key, + virtual bool visit_internal(node_location const &l, internal_node const &n) = 0; - virtual bool visit_internal_leaf(unsigned level, bool sub_root, boost::optional key, + virtual bool visit_internal_leaf(node_location const &l, internal_node const &n) = 0; - virtual bool visit_leaf(unsigned level, bool sub_root, boost::optional key, + virtual bool visit_leaf(node_location const &l, leaf_node const &n) = 0; virtual void visit_complete() {} }; // Walks the tree in depth first order - void visit(typename visitor::ptr visitor) const; + void visit_depth_first(typename visitor::ptr visitor) const; private: template @@ -353,7 +363,7 @@ namespace persistent_data { int *index); void walk_tree(typename visitor::ptr visitor, - unsigned level, bool root, boost::optional key, + btree_detail::node_location const &loc, block_address b) const; typename persistent_data::transaction_manager::ptr tm_; diff --git a/persistent-data/data-structures/btree.tcc b/persistent-data/data-structures/btree.tcc index 733696a..106775f 100644 --- a/persistent-data/data-structures/btree.tcc +++ b/persistent-data/data-structures/btree.tcc @@ -742,37 +742,57 @@ insert_location(btree_detail::shadow_spine &spine, template void -btree::visit(typename visitor::ptr visitor) const +btree::visit_depth_first(typename visitor::ptr visitor) const { - walk_tree(visitor, 0, true, boost::optional(), root_); + node_location loc; + + loc.level = 0; + loc.depth = 0; + loc.sub_root = true; + loc.key = boost::optional(); + + walk_tree(visitor, loc, root_); visitor->visit_complete(); } template void -btree:: -walk_tree(typename visitor::ptr visitor, - unsigned level, bool sub_root, - boost::optional key, - block_address b) const +btree::walk_tree(typename visitor::ptr visitor, + node_location const &loc, + block_address b) const { using namespace btree_detail; read_ref blk = tm_->read_lock(b); internal_node o = to_node(blk); if (o.get_type() == INTERNAL) { - if (visitor->visit_internal(level, sub_root, key, o)) - for (unsigned i = 0; i < o.get_nr_entries(); i++) - walk_tree(visitor, level, false, o.key_at(i), o.value_at(i)); + if (visitor->visit_internal(loc, o)) + for (unsigned i = 0; i < o.get_nr_entries(); i++) { + node_location loc2(loc); - } else if (level < Levels - 1) { - if (visitor->visit_internal_leaf(level, sub_root, key, o)) - for (unsigned i = 0; i < o.get_nr_entries(); i++) - walk_tree(visitor, level + 1, true, boost::optional(o.key_at(i)), o.value_at(i)); + loc2.depth = loc.depth + 1; + loc2.sub_root = false; + loc2.key = boost::optional(o.key_at(i)); + + walk_tree(visitor, loc2, o.value_at(i)); + } + + } else if (loc.level < Levels - 1) { + if (visitor->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.depth = loc.depth + 1; + loc2.sub_root = true; + loc2.key = boost::optional(o.key_at(i)); + + walk_tree(visitor, loc, o.value_at(i)); + } } else { leaf_node ov = to_node(blk); - visitor->visit_leaf(level, sub_root, key, ov); + visitor->visit_leaf(loc, ov); } } diff --git a/persistent-data/data-structures/btree_checker.h b/persistent-data/data-structures/btree_checker.h index 9ebe2e9..80ea189 100644 --- a/persistent-data/data-structures/btree_checker.h +++ b/persistent-data/data-structures/btree_checker.h @@ -57,31 +57,27 @@ namespace persistent_data { template class btree_checker : public btree::visitor { public: + typedef btree_detail::node_location node_location; + btree_checker(block_counter &counter, bool avoid_repeated_visits = true) : counter_(counter), errs_(new error_set("btree errors")), avoid_repeated_visits_(avoid_repeated_visits) { } - bool visit_internal(unsigned level, - bool sub_root, - optional key, + bool visit_internal(node_location const &loc, btree_detail::node_ref const &n) { - return check_internal(level, sub_root, key, n); + return check_internal(loc, n); } - bool visit_internal_leaf(unsigned level, - bool sub_root, - optional key, + bool visit_internal_leaf(node_location const &loc, btree_detail::node_ref const &n) { - return check_leaf(level, sub_root, key, n); + return check_leaf(loc, n); } - bool visit_leaf(unsigned level, - bool sub_root, - optional key, + bool visit_leaf(node_location const &loc, btree_detail::node_ref const &n) { - return check_leaf(level, sub_root, key, n); + return check_leaf(loc, n); } error_set::ptr get_errors() const { @@ -94,19 +90,17 @@ namespace persistent_data { } private: - bool check_internal(unsigned level, - bool sub_root, - optional key, + bool check_internal(node_location const &loc, btree_detail::node_ref const &n) { if (!already_visited(n) && check_sum(n) && check_block_nr(n) && check_max_entries(n) && - check_nr_entries(n, sub_root) && + check_nr_entries(n, loc.sub_root) && check_ordered_keys(n) && - check_parent_key(sub_root ? optional() : key, n)) { - if (sub_root) - new_root(level); + check_parent_key(loc.sub_root ? optional() : loc.key, n)) { + if (loc.sub_root) + new_root(loc.level); return true; } @@ -115,21 +109,19 @@ namespace persistent_data { } template - bool check_leaf(unsigned level, - bool sub_root, - optional key, - btree_detail::node_ref const &n) { + bool check_leaf(node_location const &loc, + btree_detail::node_ref const &n) { if (!already_visited(n) && check_sum(n) && check_block_nr(n) && check_max_entries(n) && - check_nr_entries(n, sub_root) && + check_nr_entries(n, loc.sub_root) && check_ordered_keys(n) && - check_parent_key(sub_root ? optional() : key, n)) { - if (sub_root) - new_root(level); + check_parent_key(loc.sub_root ? optional() : loc.key, n)) { + if (loc.sub_root) + new_root(loc.level); - return check_leaf_key(level, n); + return check_leaf_key(loc.level, n); } return false; diff --git a/persistent-data/space-maps/disk.cc b/persistent-data/space-maps/disk.cc index 4f13423..a9368ad 100644 --- a/persistent-data/space-maps/disk.cc +++ b/persistent-data/space-maps/disk.cc @@ -361,7 +361,7 @@ namespace { virtual void check(block_counter &counter) const { ref_count_checker::ptr v(new ref_count_checker(counter)); - ref_counts_.visit(v); + ref_counts_.visit_depth_first(v); block_address nr_entries = div_up(get_nr_blocks(), ENTRIES_PER_BLOCK); indexes_->check(counter, nr_entries); @@ -488,17 +488,16 @@ namespace { class bitmap_tree_validator : public btree_checker<1, index_entry_traits> { public: + typedef typename btree_checker<1, index_entry_traits>::visitor::node_location node_location; typedef boost::shared_ptr ptr; bitmap_tree_validator(block_counter &counter) : btree_checker<1, index_entry_traits>(counter) { } - bool visit_leaf(unsigned level, - bool sub_root, - optional key, + bool visit_leaf(node_location const &loc, btree_detail::node_ref const &n) { - bool r = btree_checker<1, index_entry_traits>::visit_leaf(level, sub_root, key, n); + bool r = btree_checker<1, index_entry_traits>::visit_leaf(loc, n); if (!r) return r; @@ -586,7 +585,7 @@ namespace { virtual void check(block_counter &counter, block_address nr_index_entries) const { bitmap_tree_validator::ptr v(new bitmap_tree_validator(counter)); - bitmaps_.visit(v); + bitmaps_.visit_depth_first(v); v->check_all_index_entries_present(nr_index_entries); } diff --git a/thin-provisioning/device_checker.cc b/thin-provisioning/device_checker.cc index 071a2ca..a0dffbc 100644 --- a/thin-provisioning/device_checker.cc +++ b/thin-provisioning/device_checker.cc @@ -14,14 +14,13 @@ using namespace thin_provisioning; namespace { // FIXME: duplication with metadata.cc transaction_manager::ptr - open_tm(block_manager<>::ptr bm) { + open_core_tm(block_manager<>::ptr bm) { space_map::ptr sm(new core_map(bm->get_nr_blocks())); sm->inc(SUPERBLOCK_LOCATION); transaction_manager::ptr tm(new transaction_manager(bm, sm)); return tm; } - class device_visitor : public btree<1, device_details_traits>::visitor { public: typedef boost::shared_ptr ptr; @@ -31,26 +30,20 @@ namespace { : checker_(counter) { } - bool visit_internal(unsigned level, - bool sub_root, - optional key, + bool visit_internal(node_location const &loc, btree_detail::node_ref const &n) { - return checker_.visit_internal(level, sub_root, key, n); + return checker_.visit_internal(loc, n); } - bool visit_internal_leaf(unsigned level, - bool sub_root, - optional key, + bool visit_internal_leaf(node_location const &loc, btree_detail::node_ref const &n) { - return checker_.visit_internal_leaf(level, sub_root, key, n); + return checker_.visit_internal_leaf(loc, n); } - bool visit_leaf(unsigned level, - bool sub_root, - optional key, + bool visit_leaf(node_location const &loc, btree_detail::node_ref const &n) { - if (!checker_.visit_leaf(level, sub_root, key, n)) + if (!checker_.visit_leaf(loc, n)) return false; for (unsigned i = 0; i < n.get_nr_entries(); i++) @@ -83,13 +76,13 @@ device_checker::check() { block_counter counter; device_visitor::ptr v(new device_visitor(counter)); - transaction_manager::ptr tm(open_tm(bm_)); + transaction_manager::ptr tm(open_core_tm(bm_)); detail_tree::ptr details(new detail_tree(tm, root_, device_details_traits::ref_counter())); damage_list_ptr damage(new damage_list); try { - details->visit(v); + details->visit_depth_first(v); } catch (std::exception const &e) { diff --git a/thin-provisioning/metadata_dumper.cc b/thin-provisioning/metadata_dumper.cc index c442b6d..86043f4 100644 --- a/thin-provisioning/metadata_dumper.cc +++ b/thin-provisioning/metadata_dumper.cc @@ -27,6 +27,7 @@ namespace { class mappings_extractor : public btree<2, block_traits>::visitor { public: typedef boost::shared_ptr ptr; + typedef btree_detail::node_location node_location; typedef btree_checker<2, block_traits> checker; mappings_extractor(uint64_t dev_id, emitter::ptr e, @@ -41,20 +42,20 @@ namespace { found_errors_(false) { } - bool visit_internal(unsigned level, bool sub_root, boost::optional key, + bool visit_internal(node_location const &loc, btree_detail::node_ref const &n) { - if (!checker_.visit_internal(level, sub_root, key, n)) { + if (!checker_.visit_internal(loc, n)) { found_errors_ = true; return false; } - return (sub_root && key) ? (*key == dev_id_) : true; + return (loc.sub_root && loc.key) ? (*loc.key == dev_id_) : true; } - bool visit_internal_leaf(unsigned level, bool sub_root, boost::optional key, + bool visit_internal_leaf(node_location const &loc, btree_detail::node_ref const &n) { - if (!checker_.visit_internal_leaf(level, sub_root, key, n)) { + if (!checker_.visit_internal_leaf(loc, n)) { found_errors_ = true; return false; } @@ -62,9 +63,9 @@ namespace { return true; } - bool visit_leaf(unsigned level, bool sub_root, boost::optional maybe_key, + bool visit_leaf(node_location const &loc, btree_detail::node_ref const &n) { - if (!checker_.visit_leaf(level, sub_root, maybe_key, n)) { + if (!checker_.visit_leaf(loc, n)) { found_errors_ = true; return false; } @@ -136,6 +137,7 @@ namespace { class details_extractor : public btree<1, device_details_traits>::visitor { public: + typedef typename btree<1, device_details_traits>::visitor::node_location node_location; typedef boost::shared_ptr ptr; typedef btree_checker<1, device_details_traits> checker; @@ -144,19 +146,19 @@ namespace { checker_(counter_, false) { } - bool visit_internal(unsigned level, bool sub_root, boost::optional key, + bool visit_internal(node_location const &loc, btree_detail::node_ref const &n) { - return checker_.visit_internal(level, sub_root, key, n); + return checker_.visit_internal(loc, n); } - bool visit_internal_leaf(unsigned level, bool sub_root, boost::optional key, + bool visit_internal_leaf(node_location const &loc, btree_detail::node_ref const &n) { - return checker_.visit_internal_leaf(level, sub_root, key, n); + return checker_.visit_internal_leaf(loc, n); } - bool visit_leaf(unsigned level, bool sub_root, boost::optional maybe_key, + bool visit_leaf(node_location const &loc, btree_detail::node_ref const &n) { - if (!checker_.visit_leaf(level, sub_root, maybe_key, n)) + if (!checker_.visit_leaf(loc, n)) return false; for (unsigned i = 0; i < n.get_nr_entries(); i++) @@ -198,7 +200,7 @@ thin_provisioning::metadata_dump(metadata::ptr md, emitter::ptr e, bool repair) details_extractor::ptr de(new details_extractor); - md->details_->visit(de); + md->details_->visit_depth_first(de); if (de->corruption() && !repair) throw runtime_error("corruption in device details tree"); @@ -216,7 +218,7 @@ thin_provisioning::metadata_dump(metadata::ptr md, emitter::ptr e, bool repair) dd.snapshotted_time_); mappings_extractor::ptr me(new mappings_extractor(dev_id, e, md->metadata_sm_, md->data_sm_)); - md->mappings_->visit(me); + md->mappings_->visit_depth_first(me); if (me->corruption() && !repair) { ostringstream out; diff --git a/unit-tests/btree_t.cc b/unit-tests/btree_t.cc index 1b4663a..41a8e53 100644 --- a/unit-tests/btree_t.cc +++ b/unit-tests/btree_t.cc @@ -52,26 +52,24 @@ namespace { // class constraint_visitor : public btree<1, uint64_traits>::visitor { public: + typedef btree_detail::node_location node_location; typedef btree_detail::node_ref internal_node; typedef btree_detail::node_ref leaf_node; - bool visit_internal(unsigned level, bool sub_root, - boost::optional key, + bool visit_internal(node_location const &loc, internal_node const &n) { check_duplicate_block(n.get_location()); return true; } - bool visit_internal_leaf(unsigned level, bool sub_root, - boost::optional key, - internal_node const &n) { + bool visit_internal_leaf(node_location const &loc, + internal_node const &n) { check_duplicate_block(n.get_location()); return true; } - bool visit_leaf(unsigned level, bool sub_root, - boost::optional key, - leaf_node const &n) { + bool visit_leaf(node_location const &loc, + leaf_node const &n) { check_duplicate_block(n.get_location()); return true; } @@ -94,7 +92,7 @@ namespace { typedef btree<1, uint64_traits> tree_type; tree_type::visitor::ptr v(new constraint_visitor); - tree->visit(v); + tree->visit_depth_first(v); } }