From e6a4ba53f06b1d84ca34976372d1f254562105d7 Mon Sep 17 00:00:00 2001 From: Ming-Hung Tsai Date: Thu, 3 Jan 2019 17:59:42 +0800 Subject: [PATCH 1/8] [build] add options for gprof --- configure.ac | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/configure.ac b/configure.ac index d2ac9ad..54bae31 100644 --- a/configure.ac +++ b/configure.ac @@ -112,7 +112,18 @@ AC_ARG_ENABLE(debug, AC_HELP_STRING([--enable-debug], [enable debugging]), AC_MSG_RESULT($DEBUG) if test x$DEBUG = xyes; then - CXXDEBUG_FLAG=-g + CXXDEBUG_FLAG+=" -g" +fi + +################################################################################ +dnl -- Enable gprof +AC_MSG_CHECKING(whether to enable gprof) +AC_ARG_ENABLE(gprof, AC_HELP_STRING([--enable-gprof], [enable gprof]), + GPROF=$enableval, GPROF=no) +AC_MSG_RESULT($GPROF) + +if test x$GPROF = xyes; then + CXXDEBUG_FLAG+=" -pg" fi ################################################################################ From d6a8c03aa2ea60e08289486d25f6fc5bbd890e05 Mon Sep 17 00:00:00 2001 From: Ming-Hung Tsai Date: Sat, 15 Feb 2020 17:58:36 +0800 Subject: [PATCH 2/8] [btree_damage_visitor] factor out non-template functions --- Makefile.in | 1 + .../data-structures/btree_damage_visitor.cc | 56 ++++++++++++--- .../data-structures/btree_damage_visitor.h | 69 +++---------------- 3 files changed, 55 insertions(+), 71 deletions(-) diff --git a/Makefile.in b/Makefile.in index a623b06..180d474 100644 --- a/Makefile.in +++ b/Makefile.in @@ -80,6 +80,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_damage_visitor.cc \ persistent-data/data-structures/btree_node_checker.cc \ persistent-data/error_set.cc \ persistent-data/file_utils.cc \ diff --git a/persistent-data/data-structures/btree_damage_visitor.cc b/persistent-data/data-structures/btree_damage_visitor.cc index d6a6dee..915335a 100644 --- a/persistent-data/data-structures/btree_damage_visitor.cc +++ b/persistent-data/data-structures/btree_damage_visitor.cc @@ -16,13 +16,13 @@ damage_tracker::bad_node() damaged_ = true; } -maybe_range64 +damage_tracker::maybe_run64 damage_tracker::good_internal(block_address begin) { - maybe_range64 r; + maybe_run64 r; if (damaged_) { - r = maybe_range64(range64(damage_begin_, begin)); + r = maybe_run64(run64(damage_begin_, begin)); damaged_ = false; } @@ -30,13 +30,13 @@ damage_tracker::good_internal(block_address begin) return r; } -maybe_range64 -damage_tracker::good_leaf(uint64_t begin, uint64_t end) +damage_tracker::maybe_run64 +damage_tracker::good_leaf(block_address begin, block_address end) { - maybe_range64 r; + maybe_run64 r; if (damaged_) { - r = maybe_range64(range64(damage_begin_, begin)); + r = maybe_run64(run64(damage_begin_, begin)); damaged_ = false; } @@ -44,13 +44,49 @@ damage_tracker::good_leaf(uint64_t begin, uint64_t end) return r; } -maybe_range64 +damage_tracker::maybe_run64 damage_tracker::end() { + maybe_run64 r; + if (damaged_) - return maybe_range64(damage_begin_); + r = maybe_run64(damage_begin_); else - return maybe_range64(); + r = maybe_run64(); + + damaged_ = false; + damage_begin_ = 0; + + return r; +} + +//---------------------------------------------------------------- + +path_tracker::path_tracker() +{ + // We push an empty path, to ensure there + // is always a current_path. + paths_.push_back(btree_path()); +} + +btree_path const * +path_tracker::next_path(btree_path const &p) +{ + if (p != current_path()) { + if (paths_.size() == 2) + paths_.pop_front(); + paths_.push_back(p); + + return &paths_.front(); + } + + return NULL; +} + +btree_path const & +path_tracker::current_path() const +{ + return paths_.back(); } //---------------------------------------------------------------- diff --git a/persistent-data/data-structures/btree_damage_visitor.h b/persistent-data/data-structures/btree_damage_visitor.h index 1ff16a8..0d2031a 100644 --- a/persistent-data/data-structures/btree_damage_visitor.h +++ b/persistent-data/data-structures/btree_damage_visitor.h @@ -38,57 +38,20 @@ namespace persistent_data { // trackers if you have a multilayer tree. class damage_tracker { public: - damage_tracker() - : damaged_(false), - damage_begin_(0) { - } + damage_tracker(); typedef run run64; typedef boost::optional maybe_run64; - void bad_node() { - damaged_ = true; - } + void bad_node(); - maybe_run64 good_internal(block_address begin) { - maybe_run64 r; - - if (damaged_) { - r = maybe_run64(run64(damage_begin_, begin)); - damaged_ = false; - } - - damage_begin_ = begin; - return r; - } + maybe_run64 good_internal(block_address begin); // remember 'end' is the one-past-the-end value, so // take the last key in the leaf and add one. - maybe_run64 good_leaf(block_address begin, block_address end) { - maybe_run64 r; + maybe_run64 good_leaf(block_address begin, block_address end); - if (damaged_) { - r = maybe_run64(run64(damage_begin_, begin)); - damaged_ = false; - } - - damage_begin_ = end; - return r; - } - - maybe_run64 end() { - maybe_run64 r; - - if (damaged_) - r = maybe_run64(damage_begin_); - else - r = maybe_run64(); - - damaged_ = false; - damage_begin_ = 0; - - return r; - } + maybe_run64 end(); private: bool damaged_; @@ -99,28 +62,12 @@ namespace persistent_data { // different sub tree (by looking at the btree_path). class path_tracker { public: - path_tracker() { - // We push an empty path, to ensure there - // is always a current_path. - paths_.push_back(btree_path()); - } + path_tracker(); // returns the old path if the tree has changed. - btree_path const *next_path(btree_path const &p) { - if (p != current_path()) { - if (paths_.size() == 2) - paths_.pop_front(); - paths_.push_back(p); + btree_path const *next_path(btree_path const &p); - return &paths_.front(); - } - - return NULL; - } - - btree_path const ¤t_path() const { - return paths_.back(); - } + btree_path const ¤t_path() const; private: std::list paths_; From c85ea5ef769e8ae27920f03326d2e292e3c64f87 Mon Sep 17 00:00:00 2001 From: Ming-Hung Tsai Date: Sun, 16 Feb 2020 16:43:31 +0800 Subject: [PATCH 3/8] [thin_check] factor out metadata_checker --- thin-provisioning/metadata_checker.cc | 639 ++++++++++++-------------- thin-provisioning/metadata_checker.h | 152 ++---- thin-provisioning/thin_check.cc | 254 +--------- 3 files changed, 332 insertions(+), 713 deletions(-) diff --git a/thin-provisioning/metadata_checker.cc b/thin-provisioning/metadata_checker.cc index 7f3124a..2765d55 100644 --- a/thin-provisioning/metadata_checker.cc +++ b/thin-provisioning/metadata_checker.cc @@ -16,423 +16,348 @@ // with thin-provisioning-tools. If not, see // . +#include "base/nested_output.h" #include "persistent-data/file_utils.h" #include "thin-provisioning/metadata.h" #include "thin-provisioning/metadata_checker.h" +#include "thin-provisioning/metadata_counter.h" +#include "thin-provisioning/superblock.h" using namespace persistent_data; using namespace thin_provisioning; //---------------------------------------------------------------- -#if 0 -void -metadata_damage::set_message(std::string const &message) -{ - message_ = message; -} - -std::string const & -metadata_damage::get_message() const -{ - return message_; -} - -//-------------------------------- - -void -super_block_corruption::visit(metadata_damage_visitor &visitor) const -{ - visitor.visit(*this); -} - -bool -super_block_corruption::operator ==(super_block_corruption const &rhs) const -{ - return true; -} - -//-------------------------------- - -missing_device_details::missing_device_details(range64 missing) - : missing_(missing) -{ -} - -void -missing_device_details::visit(metadata_damage_visitor &visitor) const -{ - visitor.visit(*this); -} - -bool -missing_device_details::operator ==(missing_device_details const &rhs) const -{ - return missing_ == rhs.missing_; -} - -//-------------------------------- - -missing_devices::missing_devices(range64 missing) - : missing_(missing) -{ -} - -void -missing_devices::visit(metadata_damage_visitor &visitor) const -{ - visitor.visit(*this); -} - -bool -missing_devices::operator ==(missing_devices const &rhs) const -{ - return missing_ == rhs.missing_; -} - -//-------------------------------- - -missing_mappings::missing_mappings(uint64_t dev, range64 missing) - : dev_(dev), - missing_(missing) -{ -} - -void -missing_mappings::visit(metadata_damage_visitor &visitor) const -{ - visitor.visit(*this); -} - -bool -missing_mappings::operator ==(missing_mappings const &rhs) const -{ - return dev_ == rhs.dev_ && missing_ == rhs.missing_; -} - -//-------------------------------- - -bad_metadata_ref_count::bad_metadata_ref_count(block_address b, - ref_t actual, - ref_t expected) - : b_(b), - actual_(actual), - expected_(expected) -{ -} - -void -bad_metadata_ref_count::visit(metadata_damage_visitor &visitor) const -{ - visitor.visit(*this); -} - -bool -bad_metadata_ref_count::operator ==(bad_metadata_ref_count const &rhs) const -{ - return b_ == rhs.b_ && actual_ == rhs.actual_ && expected_ == rhs.expected_; -} - -//-------------------------------- - -bad_data_ref_count::bad_data_ref_count(block_address b, - ref_t actual, - ref_t expected) - : b_(b), - actual_(actual), - expected_(expected) -{ -} - -void -bad_data_ref_count::visit(metadata_damage_visitor &visitor) const -{ - visitor.visit(*this); -} - -bool -bad_data_ref_count::operator ==(bad_data_ref_count const &rhs) const -{ - return b_ == rhs.b_ && actual_ == rhs.actual_ && expected_ == rhs.expected_; -} - -//-------------------------------- - -missing_metadata_ref_counts::missing_metadata_ref_counts(range64 missing) - : missing_(missing) -{ -} - -void -missing_metadata_ref_counts::visit(metadata_damage_visitor &visitor) const -{ - visitor.visit(*this); -} - -bool -missing_metadata_ref_counts::operator ==(missing_metadata_ref_counts const &rhs) const -{ - return missing_ == rhs.missing_; -} - -//-------------------------------- - -missing_data_ref_counts::missing_data_ref_counts(range64 missing) - : missing_(missing) -{ -} - -void -missing_data_ref_counts::visit(metadata_damage_visitor &visitor) const -{ - visitor.visit(*this); -} - -bool -missing_data_ref_counts::operator ==(missing_data_ref_counts const &rhs) const -{ - return missing_ == rhs.missing_; -} - -//-------------------------------- - -void -metadata_damage_visitor::visit(metadata_damage const &damage) -{ - damage.visit(*this); -} - -//-------------------------------- - -checker::checker(block_manager::ptr bm) - : bm_(bm) -{ -} - -//---------------------------------------------------------------- - -#if 0 - namespace { - // As well as the standard btree checks, we build up a set of what - // devices having mappings defined, which can later be cross - // referenced with the details tree. A separate block_counter is - // used to later verify the data space map. - class mapping_validator : public btree<2, block_traits>::visitor { + class superblock_reporter : public superblock_detail::damage_visitor { public: - typedef boost::shared_ptr ptr; - typedef btree_checker<2, block_traits> checker; - - mapping_validator(block_counter &metadata_counter, block_counter &data_counter) - : checker_(metadata_counter), - data_counter_(data_counter) - { + superblock_reporter(nested_output &out) + : out_(out), + err_(NO_ERROR) { } - bool visit_internal(unsigned level, - bool sub_root, - optional key, - btree_detail::node_ref const &n) { - return checker_.visit_internal(level, sub_root, key, n); + virtual void visit(superblock_detail::superblock_corruption const &d) { + out_ << "superblock is corrupt" << end_message(); + { + nested_output::nest _ = out_.push(); + out_ << d.desc_ << end_message(); + } + err_ << FATAL; } - bool visit_internal_leaf(unsigned level, - bool sub_root, - optional key, - btree_detail::node_ref const &n) { - - bool r = checker_.visit_internal_leaf(level, sub_root, key, n); - - for (unsigned i = 0; i < n.get_nr_entries(); i++) - devices_.insert(n.key_at(i)); - - return r; - } - - bool visit_leaf(unsigned level, - bool sub_root, - optional key, - btree_detail::node_ref const &n) { - bool r = checker_.visit_leaf(level, sub_root, key, n); - - if (r) - for (unsigned i = 0; i < n.get_nr_entries(); i++) - data_counter_.inc(n.value_at(i).block_); - - return r; - } - - set const &get_devices() const { - return devices_; + base::error_state get_error() const { + return err_; } private: - checker checker_; - block_counter &data_counter_; - set devices_; + nested_output &out_; + error_state err_; }; - struct check_count : public space_map::iterator { - check_count(string const &desc, block_counter const &expected) - : bad_(false), - expected_(expected), - errors_(new error_set(desc)) { + //-------------------------------- + + class devices_reporter : public device_tree_detail::damage_visitor { + public: + devices_reporter(nested_output &out) + : out_(out), + err_(NO_ERROR) { } - virtual void operator() (block_address b, ref_t actual) { - ref_t expected = expected_.get_count(b); - - if (actual != expected) { - ostringstream out; - out << b << ": was " << actual - << ", expected " << expected; - errors_->add_child(out.str()); - bad_ = true; + virtual void visit(device_tree_detail::missing_devices const &d) { + out_ << "missing devices: " << d.keys_ << end_message(); + { + nested_output::nest _ = out_.push(); + out_ << d.desc_ << end_message(); } + + err_ << FATAL; } - bool bad_; - block_counter const &expected_; - error_set::ptr errors_; + error_state get_error() const { + return err_; + } + + private: + nested_output &out_; + error_state err_; }; - optional - check_ref_counts(string const &desc, block_counter const &counts, - space_map::ptr sm) { + //-------------------------------- - check_count checker(desc, counts); - sm->iterate(checker); - return checker.bad_ ? optional(checker.errors_) : optional(); + class mapping_reporter : public mapping_tree_detail::damage_visitor { + public: + mapping_reporter(nested_output &out) + : out_(out), + err_(NO_ERROR) { + } + + virtual void visit(mapping_tree_detail::missing_devices const &d) { + out_ << "missing all mappings for devices: " << d.keys_ << end_message(); + { + nested_output::nest _ = out_.push(); + out_ << d.desc_ << end_message(); + } + err_ << FATAL; + } + + virtual void visit(mapping_tree_detail::missing_mappings const &d) { + out_ << "thin device " << d.thin_dev_ << " is missing mappings " << d.keys_ << end_message(); + { + nested_output::nest _ = out_.push(); + out_ << d.desc_ << end_message(); + } + err_ << FATAL; + } + + error_state get_error() const { + return err_; + } + + private: + nested_output &out_; + error_state err_; + }; + + //-------------------------------- + + error_state examine_superblock(block_manager<>::ptr bm, + nested_output &out) { + out << "examining superblock" << end_message(); + nested_output::nest _ = out.push(); + + superblock_reporter sb_rep(out); + check_superblock(bm, sb_rep); + + return sb_rep.get_error(); } - class metadata_checker { - public: - metadata_checker(string const &dev_path) - : bm_(open_bm(dev_path)), - errors_(new error_set("Errors in metadata")) { + error_state examine_devices_tree_(transaction_manager::ptr tm, + superblock_detail::superblock const &sb, + nested_output &out) { + out << "examining devices tree" << end_message(); + nested_output::nest _ = out.push(); + + devices_reporter dev_rep(out); + device_tree dtree(*tm, sb.device_details_root_, + device_tree_detail::device_details_traits::ref_counter()); + check_device_tree(dtree, dev_rep); + + return dev_rep.get_error(); + } + + error_state examine_top_level_mapping_tree_(transaction_manager::ptr tm, + superblock_detail::superblock const &sb, + nested_output &out) { + out << "examining top level of mapping tree" << end_message(); + nested_output::nest _ = out.push(); + + mapping_reporter mapping_rep(out); + dev_tree dtree(*tm, sb.data_mapping_root_, + mapping_tree_detail::mtree_traits::ref_counter(*tm)); + check_mapping_tree(dtree, mapping_rep); + + return mapping_rep.get_error(); + } + + error_state examine_mapping_tree_(transaction_manager::ptr tm, + superblock_detail::superblock const &sb, + nested_output &out) { + out << "examining mapping tree" << end_message(); + nested_output::nest _ = out.push(); + + mapping_reporter mapping_rep(out); + mapping_tree mtree(*tm, sb.data_mapping_root_, + mapping_tree_detail::block_traits::ref_counter(tm->get_sm())); + check_mapping_tree(mtree, mapping_rep); + + return mapping_rep.get_error(); + } + + error_state examine_top_level_mapping_tree(transaction_manager::ptr tm, + superblock_detail::superblock const &sb, + nested_output &out) { + error_state err = examine_devices_tree_(tm, sb, out); + err << examine_top_level_mapping_tree_(tm, sb, out); + + return err; + } + + error_state examine_mapping_tree(transaction_manager::ptr tm, + superblock_detail::superblock const &sb, + nested_output &out) { + error_state err = examine_devices_tree_(tm, sb, out); + err << examine_mapping_tree_(tm, sb, out); + + return err; + } + + error_state check_space_map_counts(transaction_manager::ptr tm, + superblock_detail::superblock const &sb, + nested_output &out) { + out << "checking space map counts" << end_message(); + nested_output::nest _ = out.push(); + + block_counter bc; + count_metadata(tm, sb, bc); + + // Finally we need to check the metadata space map agrees + // with the counts we've just calculated. + error_state err = NO_ERROR; + persistent_space_map::ptr metadata_sm = + open_metadata_sm(*tm, static_cast(&sb.metadata_space_map_root_)); + for (unsigned b = 0; b < metadata_sm->get_nr_blocks(); b++) { + ref_t c_actual = metadata_sm->get_count(b); + ref_t c_expected = bc.get_count(b); + + if (c_actual != c_expected) { + out << "metadata reference counts differ for block " << b + << ", expected " << c_expected + << ", but got " << c_actual + << end_message(); + + err << (c_actual > c_expected ? NON_FATAL : FATAL); + } } - boost::optional check() { -#if 1 - superblock sb = read_superblock(); + return err; + } - // FIXME: check version? + void print_info(transaction_manager::ptr tm, + superblock_detail::superblock const &sb, + nested_output &out) + { + out << "TRANSACTION_ID=" << sb.trans_id_ << "\n" + << "METADATA_FREE_BLOCKS=" << tm->get_sm()->get_nr_free() + << end_message(); + } - check_mappings(); + block_address mapping_root(superblock_detail::superblock const &sb, check_options const &opts) + { + return opts.override_mapping_root_ ? *opts.override_mapping_root_ : sb.data_mapping_root_; + } - return (errors_->get_children().size() > 0) ? - optional(errors_) : - optional(); -#else - error_set::ptr errors(new error_set("Errors in metadata")); + //-------------------------------- + class base_metadata_checker : public metadata_checker { + public: + base_metadata_checker(block_manager<>::ptr bm, + check_options check_opts, + output_options output_opts) + : bm_(bm), + options_(check_opts), + out_(cerr, 2), + info_out_(cout, 0) { - if (md->sb_.metadata_snap_) { - block_manager<>::ptr bm = md->tm_->get_bm(); + if (output_opts == OUTPUT_QUIET) { + out_.disable(); + info_out_.disable(); + } + } + error_state check() { + error_state err = NO_ERROR; - block_address root = md->sb_.metadata_snap_; + err << examine_superblock(bm_, out_); - metadata_counter.inc(root); - - superblock sb; - block_manager<>::read_ref r = bm->read_lock(root); - superblock_disk const *sbd = reinterpret_cast(&r.data()); - superblock_traits::unpack(*sbd, sb); - - metadata_counter.inc(sb.data_mapping_root_); - metadata_counter.inc(sb.device_details_root_); + if (err == FATAL) { + if (check_for_xml(bm_)) + out_ << "This looks like XML. thin_check only checks the binary metadata format." << end_message(); + return err; } + superblock_detail::superblock sb = read_superblock(bm_); + transaction_manager::ptr tm = + open_tm(bm_, superblock_detail::SUPERBLOCK_LOCATION); + sb.data_mapping_root_ = mapping_root(sb, options_); - set const &mapped_devs = mv->get_devices(); - details_validator::ptr dv(new details_validator(metadata_counter)); - md->details_->visit(dv); + print_info(tm, sb, info_out_); - set const &details_devs = dv->get_devices(); + err << examine_data_mappings(tm, sb, options_.check_data_mappings_, out_); - for (set::const_iterator it = mapped_devs.begin(); it != mapped_devs.end(); ++it) - if (details_devs.count(*it) == 0) { - ostringstream out; - out << "mapping exists for device " << *it - << ", yet there is no entry in the details tree."; - throw runtime_error(out.str()); - } + // if we're checking everything, and there were no errors, + // then we should check the space maps too. + if (err != FATAL) + err << examine_metadata_space_map(tm, sb, options_.check_metadata_space_map_, out_); - metadata_counter.inc(SUPERBLOCK_LOCATION); - md->metadata_sm_->check(metadata_counter); - - md->data_sm_->check(metadata_counter); - errors->add_child(check_ref_counts("Errors in metadata block reference counts", - metadata_counter, md->metadata_sm_)); - errors->add_child(check_ref_counts("Errors in data block reference counts", - data_counter, md->data_sm_)); - - return (errors->get_children().size() > 0) ? - optional(errors) : - optional(); -#endif + return err; } private: - static block_manager<>::ptr - open_bm(string const &dev_path) { - block_address nr_blocks = thin_provisioning::get_nr_blocks(dev_path); - return block_manager<>::ptr(new block_manager<>(dev_path, nr_blocks, 1, block_manager<>::READ_ONLY)); + static error_state + examine_data_mappings(transaction_manager::ptr tm, + superblock_detail::superblock const &sb, + check_options::data_mapping_options option, + nested_output &out) { + error_state err = NO_ERROR; + + switch (option) { + case check_options::DATA_MAPPING_LEVEL1: + err << examine_top_level_mapping_tree(tm, sb, out); + break; + case check_options::DATA_MAPPING_LEVEL2: + err << examine_mapping_tree(tm, sb, out); + break; + default: + break; // do nothing + } + + return err; } - // FIXME: common code with metadata.cc - superblock read_superblock() { - superblock sb; - block_manager<>::read_ref r = bm_->read_lock(SUPERBLOCK_LOCATION, superblock_validator()); - superblock_disk const *sbd = reinterpret_cast(&r.data()); - superblock_traits::unpack(*sbd, sb); - return sb; + static error_state + examine_metadata_space_map(transaction_manager::ptr tm, + superblock_detail::superblock const &sb, + check_options::metadata_space_map_options option, + nested_output &out) { + error_state err = NO_ERROR; + + switch (option) { + case check_options::METADATA_SPACE_MAP_FULL: + err << check_space_map_counts(tm, sb, out); + break; + default: + break; // do nothing + } + + return err; } - void check_mappings() { - mapping_validator::ptr mv( - new mapping_validator(metadata_counter_, - data_counter_)); - - - - md->mappings_->visit(mv); - } - - typedef block_manager<>::read_ref read_ref; - typedef block_manager<>::write_ref write_ref; - typedef boost::shared_ptr ptr; - block_manager<>::ptr bm_; - error_set::ptr errors_; - - block_counter metadata_counter_, data_counter_; - - -#if 0 - tm_ptr tm_; - superblock sb_; - - checked_space_map::ptr metadata_sm_; - checked_space_map::ptr data_sm_; - detail_tree::ptr details_; - dev_tree::ptr mappings_top_level_; - mapping_tree::ptr mappings_; -#endif + check_options options_; + nested_output out_; + nested_output info_out_; }; } - //---------------------------------------------------------------- -boost::optional -thin_provisioning::metadata_check(std::string const &dev_path) +check_options::check_options() + : check_data_mappings_(DATA_MAPPING_LEVEL2), + check_metadata_space_map_(METADATA_SPACE_MAP_FULL) { +} + +void check_options::set_superblock_only() { + check_data_mappings_ = DATA_MAPPING_NONE; + check_metadata_space_map_ = METADATA_SPACE_MAP_NONE; +} + +void check_options::set_skip_mappings() { + check_data_mappings_ = DATA_MAPPING_LEVEL1; + check_metadata_space_map_ = METADATA_SPACE_MAP_NONE; +} + +void check_options::set_override_mapping_root(block_address b) { + override_mapping_root_ = b; +} + +metadata_checker::ptr +thin_provisioning::create_base_checker(block_manager<>::ptr bm, + check_options const &check_opts, + output_options output_opts) { - metadata_checker checker(dev_path); - return checker.check(); + metadata_checker::ptr checker; + checker = metadata_checker::ptr(new base_metadata_checker(bm, check_opts, output_opts)); + return checker; } //---------------------------------------------------------------- -#endif -#endif diff --git a/thin-provisioning/metadata_checker.h b/thin-provisioning/metadata_checker.h index b2121d6..4b84c4b 100644 --- a/thin-provisioning/metadata_checker.h +++ b/thin-provisioning/metadata_checker.h @@ -19,142 +19,54 @@ #ifndef METADATA_CHECKER_H #define METADATA_CHECKER_H +#include "base/error_state.h" +#include "block-cache/block_cache.h" #include "persistent-data/block.h" -#include "persistent-data/error_set.h" -#include "persistent-data/run.h" -#include "persistent-data/space_map.h" - -#include //---------------------------------------------------------------- namespace thin_provisioning { - // FIXME: should take a block manager or transaction manager - void check_metadata(std::string const &path); + struct check_options { + enum data_mapping_options { + DATA_MAPPING_NONE, + DATA_MAPPING_LEVEL1, + DATA_MAPPING_LEVEL2, + }; + enum metadata_space_map_options { + METADATA_SPACE_MAP_NONE, + METADATA_SPACE_MAP_FULL, + }; + check_options(); + void set_superblock_only(); + void set_skip_mappings(); + void set_override_mapping_root(bcache::block_address b); + data_mapping_options check_data_mappings_; + metadata_space_map_options check_metadata_space_map_; + boost::optional override_mapping_root_; + }; + enum output_options { + OUTPUT_NORMAL, + OUTPUT_QUIET, + }; - - - -#if 0 - - // Base class for all types of metadata damage. Used in reporting. - class metadata_damage { + class metadata_checker { public: - typedef boost::shared_ptr ptr; - virtual ~metadata_damage() {} - virtual void visit(metadata_damage_visitor &visitor) const = 0; + typedef boost::shared_ptr ptr; - void set_message(std::string const &message); - std::string const &get_message() const; + virtual ~metadata_checker() {} - private: - std::string message_; + virtual base::error_state check() = 0; }; - // FIXME: there's a mix of abstraction here, some classes represent - // the actual damage on disk (bad_ref_count), others represent the - // repercussions (missing_mapping). Need to revist, probably once - // we've got the reporting layer in. - - class super_block_corruption : public metadata_damage { - void visit(metadata_damage_visitor &visitor) const; - bool operator ==(super_block_corruption const &rhs) const; - }; - - typedef base::run run64; - - struct missing_device_details : public metadata_damage { - missing_device_details(run64 missing); - virtual void visit(metadata_damage_visitor &visitor) const; - bool operator ==(missing_device_details const &rhs) const; - - run64 missing_; - }; - - struct missing_devices : public metadata_damage { - missing_devices(run64 missing); - virtual void visit(metadata_damage_visitor &visitor) const; - bool operator ==(missing_devices const &rhs) const; - - run64 missing_; - }; - - struct missing_mappings : public metadata_damage { - missing_mappings(uint64_t dev, run64 missing); - virtual void visit(metadata_damage_visitor &visitor) const; - bool operator ==(missing_mappings const &rhs) const; - - uint64_t dev_; - run64 missing_; - }; - - struct bad_metadata_ref_count : public metadata_damage { - bad_metadata_ref_count(block_address b, - ref_t actual, - ref_t expected); - - virtual void visit(metadata_damage_visitor &visitor) const; - bool operator ==(bad_metadata_ref_count const &rhs) const; - - block_address b_; - ref_t actual_; - ref_t expected_; - }; - - struct bad_data_ref_count : public metadata_damage { - bad_data_ref_count(block_address b, - ref_t actual, - ref_t expected); - - virtual void visit(metadata_damage_visitor &visitor) const; - bool operator ==(bad_data_ref_count const &rhs) const; - - block_address b_; - ref_t actual_; - ref_t expected_; - }; - - struct missing_metadata_ref_counts : public metadata_damage { - missing_metadata_ref_counts(run64 missing); - virtual void visit(metadata_damage_visitor &visitor) const; - bool operator ==(missing_metadata_ref_counts const &rhs) const; - - run64 missing_; - }; - - struct missing_data_ref_counts : public metadata_damage { - missing_data_ref_counts(run64 missing); - virtual void visit(metadata_damage_visitor &visitor) const; - bool operator ==(missing_data_ref_counts const &rhs) const; - - run64 missing_; - }; - - class metadata_damage_visitor { - public: - typedef boost::shared_ptr ptr; - - virtual ~metadata_damage_visitor() {} - - void visit(metadata_damage const &damage); - virtual void visit(super_block_corruption const &damage) = 0; - virtual void visit(missing_device_details const &damage) = 0; - virtual void visit(missing_devices const &damage) = 0; - virtual void visit(missing_mappings const &damage) = 0; - virtual void visit(bad_metadata_ref_count const &damage) = 0; - virtual void visit(bad_data_ref_count const &damage) = 0; - virtual void visit(missing_metadata_ref_counts const &damage) = 0; - virtual void visit(missing_data_ref_counts const &damage) = 0; - }; - - typedef std::deque damage_list; - typedef boost::shared_ptr damage_list_ptr; -#endif + metadata_checker::ptr + create_base_checker(persistent_data::block_manager<>::ptr bm, + check_options const &check_opts, + output_options output_opts); } //---------------------------------------------------------------- diff --git a/thin-provisioning/thin_check.cc b/thin-provisioning/thin_check.cc index 6373603..36ba41b 100644 --- a/thin-provisioning/thin_check.cc +++ b/thin-provisioning/thin_check.cc @@ -28,258 +28,34 @@ #include "base/application.h" #include "base/error_state.h" #include "base/file_utils.h" -#include "base/nested_output.h" -#include "persistent-data/data-structures/btree_counter.h" -#include "persistent-data/space-maps/core.h" -#include "persistent-data/space-maps/disk.h" #include "persistent-data/file_utils.h" -#include "thin-provisioning/metadata.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" +#include "thin-provisioning/metadata_checker.h" +#include "thin-provisioning/superblock.h" using namespace base; using namespace std; +using namespace persistent_data; using namespace thin_provisioning; //---------------------------------------------------------------- namespace { - class superblock_reporter : public superblock_detail::damage_visitor { - public: - superblock_reporter(nested_output &out) - : out_(out), - err_(NO_ERROR) { - } - - virtual void visit(superblock_detail::superblock_corruption const &d) { - out_ << "superblock is corrupt" << end_message(); - { - nested_output::nest _ = out_.push(); - out_ << d.desc_ << end_message(); - } - err_ << FATAL; - } - - base::error_state get_error() const { - return err_; - } - - private: - nested_output &out_; - error_state err_; - }; - - //-------------------------------- - - class devices_reporter : public device_tree_detail::damage_visitor { - public: - devices_reporter(nested_output &out) - : out_(out), - err_(NO_ERROR) { - } - - virtual void visit(device_tree_detail::missing_devices const &d) { - out_ << "missing devices: " << d.keys_ << end_message(); - { - nested_output::nest _ = out_.push(); - out_ << d.desc_ << end_message(); - } - - err_ << FATAL; - } - - error_state get_error() const { - return err_; - } - - private: - nested_output &out_; - error_state err_; - }; - - //-------------------------------- - - class mapping_reporter : public mapping_tree_detail::damage_visitor { - public: - mapping_reporter(nested_output &out) - : out_(out), - err_(NO_ERROR) { - } - - virtual void visit(mapping_tree_detail::missing_devices const &d) { - out_ << "missing all mappings for devices: " << d.keys_ << end_message(); - { - nested_output::nest _ = out_.push(); - out_ << d.desc_ << end_message(); - } - err_ << FATAL; - } - - virtual void visit(mapping_tree_detail::missing_mappings const &d) { - out_ << "thin device " << d.thin_dev_ << " is missing mappings " << d.keys_ << end_message(); - { - nested_output::nest _ = out_.push(); - out_ << d.desc_ << end_message(); - } - err_ << FATAL; - } - - error_state get_error() const { - return err_; - } - - private: - nested_output &out_; - error_state err_; - }; - - //-------------------------------- - struct flags { flags() - : check_device_tree(true), - check_mapping_tree_level1(true), - check_mapping_tree_level2(true), - ignore_non_fatal_errors(false), + : ignore_non_fatal_errors(false), quiet(false), clear_needs_check_flag_on_success(false) { } - bool check_device_tree; - bool check_mapping_tree_level1; - bool check_mapping_tree_level2; + check_options check_opts; bool ignore_non_fatal_errors; bool quiet; - boost::optional override_mapping_root; bool clear_needs_check_flag_on_success; }; - 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_metadata(tm, sb, bc); - - // Finally we need to check the metadata space map agrees - // with the counts we've just calculated. - error_state err = NO_ERROR; - nested_output::nest _ = out.push(); - persistent_space_map::ptr metadata_sm = - open_metadata_sm(*tm, static_cast(&sb.metadata_space_map_root_)); - for (unsigned b = 0; b < metadata_sm->get_nr_blocks(); b++) { - ref_t c_actual = metadata_sm->get_count(b); - ref_t c_expected = bc.get_count(b); - - if (c_actual != c_expected) { - out << "metadata reference counts differ for block " << b - << ", expected " << c_expected - << ", but got " << c_actual - << end_message(); - - err << (c_actual > c_expected ? NON_FATAL : FATAL); - } - } - - return err; - } - - block_address mapping_root(superblock_detail::superblock const &sb, flags const &fs) - { - return fs.override_mapping_root ? *fs.override_mapping_root : sb.data_mapping_root_; - } - - void print_info(superblock_detail::superblock const &sb, - transaction_manager::ptr tm) - { - cout << "TRANSACTION_ID=" << sb.trans_id_ << "\n"; - cout << "METADATA_FREE_BLOCKS=" << tm->get_sm()->get_nr_free() << "\n"; - } - - error_state metadata_check(string const &path, flags fs) { - nested_output out(cerr, 2); - if (fs.quiet) - out.disable(); - - if (file_utils::get_file_length(path) < persistent_data::MD_BLOCK_SIZE) { - out << "Metadata device/file too small. Is this binary metadata?" - << end_message(); - return FATAL; - } - - block_manager<>::ptr bm = open_bm(path); - - superblock_reporter sb_rep(out); - devices_reporter dev_rep(out); - mapping_reporter mapping_rep(out); - - out << "examining superblock" << end_message(); - { - nested_output::nest _ = out.push(); - check_superblock(bm, sb_rep); - } - - if (sb_rep.get_error() == FATAL) { - if (check_for_xml(bm)) - out << "This looks like XML. thin_check only checks the binary metadata format." << end_message(); - return FATAL; - } - - superblock_detail::superblock sb = read_superblock(bm); - transaction_manager::ptr tm = - open_tm(bm, superblock_detail::SUPERBLOCK_LOCATION); - - if (!fs.quiet) - print_info(sb, tm); - - if (fs.check_device_tree) { - out << "examining devices tree" << end_message(); - { - nested_output::nest _ = out.push(); - device_tree dtree(*tm, sb.device_details_root_, - device_tree_detail::device_details_traits::ref_counter()); - check_device_tree(dtree, dev_rep); - } - } - - if (fs.check_mapping_tree_level1 && !fs.check_mapping_tree_level2) { - out << "examining top level of mapping tree" << end_message(); - { - nested_output::nest _ = out.push(); - dev_tree dtree(*tm, mapping_root(sb, fs), - mapping_tree_detail::mtree_traits::ref_counter(*tm)); - check_mapping_tree(dtree, mapping_rep); - } - - } else if (fs.check_mapping_tree_level2) { - out << "examining mapping tree" << end_message(); - { - nested_output::nest _ = out.push(); - mapping_tree mtree(*tm, mapping_root(sb, fs), - mapping_tree_detail::block_traits::ref_counter(tm->get_sm())); - check_mapping_tree(mtree, mapping_rep); - } - } - - error_state err = NO_ERROR; - err << sb_rep.get_error() << mapping_rep.get_error() << dev_rep.get_error(); - - // if we're checking everything, and there were no errors, - // then we should check the space maps too. - if (fs.check_device_tree && fs.check_mapping_tree_level2 && err != FATAL) { - out << "checking space map counts" << end_message(); - err << check_space_map_counts(fs, out, sb, bm, tm); - } - - return err; - } - void clear_needs_check(string const &path) { block_manager<>::ptr bm = open_bm(path, block_manager<>::READ_WRITE); @@ -291,11 +67,19 @@ namespace { // Returns 0 on success, 1 on failure (this gets returned directly // by main). int check(string const &path, flags fs) { - error_state err; bool success = false; try { - err = metadata_check(path, fs); + if (file_utils::get_file_length(path) < persistent_data::MD_BLOCK_SIZE) { + cerr << "Metadata device/file too small. Is this binary metadata?" + << endl; + return 1; + } + + block_manager<>::ptr bm = open_bm(path); + output_options output_opts = !fs.quiet ? OUTPUT_NORMAL : OUTPUT_QUIET; + metadata_checker::ptr checker = create_base_checker(bm, fs.check_opts, output_opts); + error_state err = checker->check(); if (fs.ignore_non_fatal_errors) success = (err == FATAL) ? false : true; @@ -373,14 +157,12 @@ thin_check_cmd::run(int argc, char **argv) case 1: // super-block-only - fs.check_device_tree = false; - fs.check_mapping_tree_level1 = false; - fs.check_mapping_tree_level2 = false; + fs.check_opts.set_superblock_only(); break; case 2: // skip-mappings - fs.check_mapping_tree_level2 = false; + fs.check_opts.set_skip_mappings(); break; case 3: @@ -395,7 +177,7 @@ thin_check_cmd::run(int argc, char **argv) case 5: // override-mapping-root - fs.override_mapping_root = boost::lexical_cast(optarg); + fs.check_opts.set_override_mapping_root(boost::lexical_cast(optarg)); break; default: From 955e11bc28ad146be77ea118e5bd58a1282aece4 Mon Sep 17 00:00:00 2001 From: Ming-Hung Tsai Date: Sat, 22 Feb 2020 17:37:22 +0800 Subject: [PATCH 4/8] [block-cache] fix potential file descriptor leak Encapsulate file descriptor into an object, to ensure that an fd will be closed properly while exception raised, e.g., the block_cache throws exception during the block_manager's construction. --- base/file_utils.cc | 28 ++++++++++++++-------------- base/file_utils.h | 15 ++++++++++++--- block-cache/block_cache.cc | 11 +++++------ block-cache/block_cache.h | 5 +++-- persistent-data/block.h | 7 ++++--- persistent-data/block.tcc | 2 +- thin-provisioning/cache_stream.cc | 2 +- thin-provisioning/cache_stream.h | 2 +- unit-tests/bcache_t.cc | 2 +- 9 files changed, 42 insertions(+), 32 deletions(-) diff --git a/base/file_utils.cc b/base/file_utils.cc index 7883cfe..e4f3722 100644 --- a/base/file_utils.cc +++ b/base/file_utils.cc @@ -39,14 +39,16 @@ namespace { //---------------------------------------------------------------- -int -file_utils::open_file(string const &path, int flags) { - int fd = ::open(path.c_str(), OPEN_FLAGS | flags, DEFAULT_MODE); - if (fd < 0) +file_utils::file_descriptor::file_descriptor(string const &path, int flags) { + fd_ = ::open(path.c_str(), OPEN_FLAGS | flags, DEFAULT_MODE); + if (fd_ < 0) syscall_failed("open", "Note: you cannot run this tool with these options on live metadata."); +} - return fd; +file_utils::file_descriptor::~file_descriptor() { + close(fd_); + fd_ = -1; } bool @@ -76,7 +78,7 @@ file_utils::check_file_exists(string const &file, bool must_be_regular_file) { throw runtime_error("Not a regular file"); } -int +file_utils::file_descriptor file_utils::create_block_file(string const &path, off_t file_size) { if (file_exists(path)) { ostringstream out; @@ -84,16 +86,16 @@ file_utils::create_block_file(string const &path, off_t file_size) { throw runtime_error(out.str()); } - int fd = open_file(path, O_CREAT | O_EXCL | O_RDWR); + file_descriptor fd(path, O_CREAT | O_EXCL | O_RDWR); - int r = ::ftruncate(fd, file_size); + int r = ::ftruncate(fd.fd_, file_size); if (r < 0) syscall_failed("ftruncate"); return fd; } -int +file_utils::file_descriptor file_utils::open_block_file(string const &path, off_t min_size, bool writeable, bool excl) { if (!file_exists(path)) { ostringstream out; @@ -105,7 +107,7 @@ file_utils::open_block_file(string const &path, off_t min_size, bool writeable, if (excl) flags |= O_EXCL; - return open_file(path, flags); + return file_descriptor(path, flags); } uint64_t @@ -146,17 +148,15 @@ file_utils::zero_superblock(std::string const &path) { char *buffer; unsigned const SUPERBLOCK_SIZE = 4096; - int fd = open_block_file(path, SUPERBLOCK_SIZE, true, true); + file_descriptor fd = open_block_file(path, SUPERBLOCK_SIZE, true, true); buffer = reinterpret_cast(aligned_alloc(SUPERBLOCK_SIZE, SUPERBLOCK_SIZE)); if (!buffer) throw runtime_error("out of memory"); memset(buffer, 0, SUPERBLOCK_SIZE); - if (::write(fd, buffer, SUPERBLOCK_SIZE) != SUPERBLOCK_SIZE) + if (::write(fd.fd_, buffer, SUPERBLOCK_SIZE) != SUPERBLOCK_SIZE) throw runtime_error("couldn't zero superblock"); - - ::close(fd); } //---------------------------------------------------------------- diff --git a/base/file_utils.h b/base/file_utils.h index 3edcc9e..686e923 100644 --- a/base/file_utils.h +++ b/base/file_utils.h @@ -8,11 +8,20 @@ //---------------------------------------------------------------- namespace file_utils { - int open_file(std::string const &path, int flags); + struct file_descriptor { + // file_descriptor is movable but not copyable + file_descriptor(file_descriptor &&) = default; + file_descriptor& operator=(file_descriptor &&) = default; + file_descriptor(std::string const &path, int flags); + virtual ~file_descriptor(); + + int fd_; + }; + bool file_exists(std::string const &path); void check_file_exists(std::string const &file, bool must_be_regular_file = true); - int create_block_file(std::string const &path, off_t file_size); - int open_block_file(std::string const &path, off_t min_size, bool writeable, bool excl = true); + file_descriptor create_block_file(std::string const &path, off_t file_size); + file_descriptor open_block_file(std::string const &path, off_t min_size, bool writeable, bool excl = true); uint64_t get_file_length(std::string const &file); void zero_superblock(std::string const &path); } diff --git a/block-cache/block_cache.cc b/block-cache/block_cache.cc index 5a367f4..18a1606 100644 --- a/block-cache/block_cache.cc +++ b/block-cache/block_cache.cc @@ -15,6 +15,7 @@ #include using namespace bcache; +using namespace file_utils; //---------------------------------------------------------------- @@ -290,7 +291,7 @@ block_cache::setup_control_block(block &b) size_t block_size_bytes = block_size_ << SECTOR_SHIFT; memset(cb, 0, sizeof(*cb)); - cb->aio_fildes = fd_; + cb->aio_fildes = fd_.fd_; cb->u.c.buf = b.data_; cb->u.c.offset = block_size_bytes * b.index_; @@ -371,8 +372,9 @@ block_cache::calc_nr_buckets(unsigned nr_blocks) return r; } -block_cache::block_cache(int fd, sector_t block_size, uint64_t on_disk_blocks, size_t mem) - : nr_locked_(0), +block_cache::block_cache(file_descriptor &fd, sector_t block_size, uint64_t on_disk_blocks, size_t mem) + : fd_(fd), + nr_locked_(0), nr_dirty_(0), nr_io_pending_(0), read_hits_(0), @@ -386,7 +388,6 @@ block_cache::block_cache(int fd, sector_t block_size, uint64_t on_disk_blocks, s int r; unsigned nr_cache_blocks = calc_nr_cache_blocks(mem, block_size); - fd_ = fd; block_size_ = block_size; nr_data_blocks_ = on_disk_blocks; nr_cache_blocks_ = nr_cache_blocks; @@ -418,8 +419,6 @@ block_cache::~block_cache() if (aio_context_) io_destroy(aio_context_); - ::close(fd_); - #if 0 std::cerr << "\nblock cache stats\n" << "=================\n" diff --git a/block-cache/block_cache.h b/block-cache/block_cache.h index 650c9bd..3658e89 100644 --- a/block-cache/block_cache.h +++ b/block-cache/block_cache.h @@ -2,6 +2,7 @@ #define BLOCK_CACHE_H #include "base/container_of.h" +#include "base/file_utils.h" #include #include @@ -185,7 +186,7 @@ namespace bcache { //-------------------------------- - block_cache(int fd, sector_t block_size, + block_cache(file_utils::file_descriptor &fd, sector_t block_size, uint64_t max_nr_blocks, size_t mem); ~block_cache(); @@ -247,7 +248,7 @@ namespace bcache { //-------------------------------- - int fd_; + file_utils::file_descriptor &fd_; sector_t block_size_; uint64_t nr_data_blocks_; uint64_t nr_cache_blocks_; diff --git a/persistent-data/block.h b/persistent-data/block.h index 8a0419e..d5127c2 100644 --- a/persistent-data/block.h +++ b/persistent-data/block.h @@ -136,11 +136,12 @@ namespace persistent_data { private: uint64_t choose_cache_size(block_address nr_blocks) const; - int open_or_create_block_file(std::string const &path, off_t file_size, - mode m, bool excl); + file_utils::file_descriptor open_or_create_block_file(std::string const &path, + off_t file_size, + mode m, bool excl); void check(block_address b) const; - int fd_; + file_utils::file_descriptor fd_; mutable block_cache bc_; unsigned superblock_ref_count_; }; diff --git a/persistent-data/block.tcc b/persistent-data/block.tcc index 4d70151..e743360 100644 --- a/persistent-data/block.tcc +++ b/persistent-data/block.tcc @@ -154,7 +154,7 @@ namespace persistent_data { } template - int + file_utils::file_descriptor block_manager::open_or_create_block_file(std::string const &path, off_t file_size, mode m, bool excl) { switch (m) { diff --git a/thin-provisioning/cache_stream.cc b/thin-provisioning/cache_stream.cc index 7ebcd1c..002b6ba 100644 --- a/thin-provisioning/cache_stream.cc +++ b/thin-provisioning/cache_stream.cc @@ -18,7 +18,7 @@ cache_stream::cache_stream(string const &path, // hack because cache uses LRU rather than MRU cache_blocks_((cache_mem / block_size) / 2u), - fd_(file_utils::open_file(path, O_RDONLY | O_EXCL)), + fd_(path, O_RDONLY | O_EXCL), v_(new bcache::noop_validator()), cache_(new block_cache(fd_, block_size / 512, nr_blocks_, cache_mem)), current_index_(0) { diff --git a/thin-provisioning/cache_stream.h b/thin-provisioning/cache_stream.h index ddcbe15..7779281 100644 --- a/thin-provisioning/cache_stream.h +++ b/thin-provisioning/cache_stream.h @@ -38,7 +38,7 @@ namespace thin_provisioning { block_address nr_blocks_; block_address cache_blocks_; - int fd_; + file_utils::file_descriptor fd_; validator::ptr v_; std::unique_ptr cache_; diff --git a/unit-tests/bcache_t.cc b/unit-tests/bcache_t.cc index 67c45e9..cf21539 100644 --- a/unit-tests/bcache_t.cc +++ b/unit-tests/bcache_t.cc @@ -38,7 +38,7 @@ TEST(BCacheTests, cleaned_on_demand) unsigned const NR_BLOCKS = 16; temp_file tmp("bcache_t", 1); - int fd = open(tmp.get_path().c_str(), O_RDWR | O_DIRECT, 0666); + file_utils::file_descriptor fd(tmp.get_path().c_str(), O_RDWR | O_DIRECT); uint64_t bs = 8; block_cache bc(fd, bs, 64, (bs << SECTOR_SHIFT) * NR_BLOCKS); From 57cae67ff138d269032caa1689d79e793c23070b Mon Sep 17 00:00:00 2001 From: Ming-Hung Tsai Date: Sun, 23 Feb 2020 13:00:55 +0800 Subject: [PATCH 5/8] [unit-tests] fix googletest compile flags googletest now requires c++11 compiler --- unit-tests/Makefile.in | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/unit-tests/Makefile.in b/unit-tests/Makefile.in index 46bc347..c464de1 100644 --- a/unit-tests/Makefile.in +++ b/unit-tests/Makefile.in @@ -36,9 +36,9 @@ GMOCK_DEPS=\ lib/libgmock.a: $(GMOCK_DEPS) @echo " [CXX] gtest" @mkdir -p lib - $(V)g++ $(GMOCK_INCLUDES) -I$(GMOCK_DIR)/googletest -c $(GMOCK_DIR)/googletest/src/gtest-all.cc + $(V)g++ $(GMOCK_INCLUDES) -I$(GMOCK_DIR)/googletest -std=c++11 -c $(GMOCK_DIR)/googletest/src/gtest-all.cc @echo " [CXX] gmock" - $(V)g++ $(GMOCK_INCLUDES) -I$(GMOCK_DIR)/googlemock -c $(GMOCK_DIR)/googlemock/src/gmock-all.cc + $(V)g++ $(GMOCK_INCLUDES) -I$(GMOCK_DIR)/googlemock -std=c++11 -c $(GMOCK_DIR)/googlemock/src/gmock-all.cc @echo " [AR] $<" $(V)ar -rv lib/libgmock.a gtest-all.o gmock-all.o > /dev/null 2>&1 From cb0a77e2ae7a35a6409067e04805d0364b9139f8 Mon Sep 17 00:00:00 2001 From: Ming-Hung Tsai Date: Fri, 28 Feb 2020 22:18:35 +0800 Subject: [PATCH 6/8] [block-manager] remove unused copy-assignment operator block_cache::block is non-copyable, and so are the containing structures. --- persistent-data/block.h | 4 ---- persistent-data/block.tcc | 24 ------------------------ 2 files changed, 28 deletions(-) diff --git a/persistent-data/block.h b/persistent-data/block.h index d5127c2..393c60f 100644 --- a/persistent-data/block.h +++ b/persistent-data/block.h @@ -64,8 +64,6 @@ namespace persistent_data { read_ref(read_ref const &rhs); virtual ~read_ref(); - read_ref const &operator =(read_ref const &rhs); - block_address get_location() const; void const *data() const; @@ -82,8 +80,6 @@ namespace persistent_data { write_ref(write_ref const &rhs); ~write_ref(); - write_ref const &operator =(write_ref const &rhs); - using read_ref::data; void *data(); diff --git a/persistent-data/block.tcc b/persistent-data/block.tcc index e743360..792c658 100644 --- a/persistent-data/block.tcc +++ b/persistent-data/block.tcc @@ -47,18 +47,6 @@ namespace persistent_data { b_.put(); } - template - typename block_manager::read_ref const & - block_manager::read_ref::operator =(read_ref const &rhs) - { - if (this != &rhs) { - b_ = rhs.b_; - b_.get(); - } - - return *this; - } - template block_address block_manager::read_ref::get_location() const @@ -112,18 +100,6 @@ namespace persistent_data { } } - template - typename block_manager::write_ref const & - block_manager::write_ref::operator =(write_ref const &rhs) - { - if (&rhs != this) { - read_ref::operator =(rhs); - ref_count_ = rhs.ref_count_; - if (ref_count_) - (*ref_count_)++; - } - } - template void * block_manager::write_ref::data() From 7a1c6dc4bf0b9383bca39171b185341a0afdb014 Mon Sep 17 00:00:00 2001 From: Ming-Hung Tsai Date: Wed, 31 Oct 2018 18:10:51 +0800 Subject: [PATCH 7/8] [space-map-disk] improve performance of finding a free bitmap entry * Simply test the raw bitmap entries against zero, to avoid time-consumed reference-count value extraction. * Improve the way of iterating entries inside a bitmap: Extract 64-bit of bitmap entries at once, and use bitwise shift to iterate through the entries. --- persistent-data/space-maps/disk.cc | 35 +++++++++++++++++++++++++++--- 1 file changed, 32 insertions(+), 3 deletions(-) diff --git a/persistent-data/space-maps/disk.cc b/persistent-data/space-maps/disk.cc index 4492a3c..7d2e466 100644 --- a/persistent-data/space-maps/disk.cc +++ b/persistent-data/space-maps/disk.cc @@ -153,11 +153,27 @@ namespace { } boost::optional find_free(unsigned begin, unsigned end) { + begin = max(begin, ie_.none_free_before_); + if (begin >= end) + return boost::optional(); + read_ref rr = tm_.read_lock(ie_.blocknr_, validator_); void const *bits = bitmap_data(rr); - for (unsigned i = max(begin, ie_.none_free_before_); i < end; i++) - if (__lookup_raw(bits, i) == 0) - return boost::optional(i); + + // specify the search range inside the bitmap, in 64-bit unit + le64 const *w = reinterpret_cast(bits); + le64 const *le64_begin = w + (begin >> 5); // w + div_down(begin, 32) + le64 const *le64_end = w + ((end + 31) >> 5); // w + div_up(end, 32) + + for (le64 const *ptr = le64_begin; ptr < le64_end; ptr++) { + // specify the search range among a 64-bit of entries + unsigned entry_begin = (ptr == le64_begin) ? (begin & 0x1F) : 0; + unsigned entry_end = ((ptr == le64_end - 1) && (end & 0x1F)) ? + (end & 0x1F) : 32; + int i; + if ((i = find_free_entry(ptr, entry_begin, entry_end)) >= 0) + return ((ptr - w) << 5) + i; + } return boost::optional(); } @@ -198,6 +214,19 @@ namespace { return result; } + // find a free entry (a 2-bit pair) among the specified range in the input bits + int find_free_entry(le64 const* bits, unsigned entry_begin, unsigned entry_end) { + uint64_t v = to_cpu(*bits); + v >>= (entry_begin * 2); + for (; entry_begin < entry_end; entry_begin++) { + if (!(v & 0x3)) { + return entry_begin; + } + v = v >> 2; + } + return -1; + } + transaction_manager &tm_; bcache::validator::ptr validator_; From 3ed39253701797332db8a2a03c288c4fc7feafb0 Mon Sep 17 00:00:00 2001 From: Arusekk Date: Wed, 25 Mar 2020 15:58:02 +0100 Subject: [PATCH 8/8] Fix compilation with uClibc Closes #12 --- base/application.cc | 4 ++-- caching/cache_writeback.cc | 6 +++--- thin-provisioning/metadata_dumper.cc | 16 ++++++++-------- thin-provisioning/thin_dump.cc | 2 +- thin-provisioning/thin_ls.cc | 8 ++++---- 5 files changed, 18 insertions(+), 18 deletions(-) diff --git a/base/application.cc b/base/application.cc index 3c5202c..61338ff 100644 --- a/base/application.cc +++ b/base/application.cc @@ -24,12 +24,12 @@ command::die(string const &msg) exit(1); } -uint64_t +::uint64_t command::parse_uint64(string const &str, string const &desc) { try { // FIXME: check trailing garbage is handled - return lexical_cast(str); + return lexical_cast<::uint64_t>(str); } catch (...) { ostringstream out; diff --git a/caching/cache_writeback.cc b/caching/cache_writeback.cc index 4438433..1f7fa59 100644 --- a/caching/cache_writeback.cc +++ b/caching/cache_writeback.cc @@ -193,10 +193,10 @@ namespace { if (call_count++ % 128) return; - uint64_t scanned = stats_.blocks_scanned * 100 / cache_blocks_; - uint64_t copied = safe_div(stats_.blocks_completed * 100, + ::uint64_t scanned = stats_.blocks_scanned * 100 / cache_blocks_; + ::uint64_t copied = safe_div(stats_.blocks_completed * 100, stats_.blocks_needed, 100ull); - uint64_t percent = min(scanned, copied); + ::uint64_t percent = min<::uint64_t>(scanned, copied); monitor_.update_percent(percent); } diff --git a/thin-provisioning/metadata_dumper.cc b/thin-provisioning/metadata_dumper.cc index 3fc3b30..be4b666 100644 --- a/thin-provisioning/metadata_dumper.cc +++ b/thin-provisioning/metadata_dumper.cc @@ -232,8 +232,8 @@ namespace { block_address b; unsigned values; - uint64_t key_low; - uint64_t key_high; + ::uint64_t key_low; + ::uint64_t key_high; //set devices; uint32_t age; map time_counts; @@ -477,7 +477,7 @@ namespace { // values refer to blocks, so we should have infos for them. auto n = to_node(rr); - uint64_t key_low = 0; + ::uint64_t key_low = 0; unsigned values = 0; for (unsigned i = 0; i < n.get_nr_entries(); i++) { @@ -541,7 +541,7 @@ namespace { info.nr_mappings += n.value_at(i).mapped_blocks_; } - } else if (vsize == sizeof(uint64_t)) { + } else if (vsize == sizeof(::uint64_t)) { auto n = to_node(rr); if (n.get_nr_entries()) { @@ -658,7 +658,7 @@ namespace { } private: - void start_mapping(uint64_t origin_block, block_time const &bt) { + void start_mapping(::uint64_t origin_block, block_time const &bt) { origin_start_ = origin_block; dest_start_ = bt.block_; time_ = bt.time_; @@ -677,7 +677,7 @@ namespace { } } - void add_mapping(uint64_t origin_block, block_time const &bt) { + void add_mapping(::uint64_t origin_block, block_time const &bt) { if (!in_range_) start_mapping(origin_block, bt); @@ -748,7 +748,7 @@ namespace { } private: - void emit_mappings(uint64_t dev_id, block_address subtree_root) { + void emit_mappings(::uint64_t dev_id, block_address subtree_root) { mapping_emit_visitor me(e_); // Since we're not mutating the btrees we don't need a real space map @@ -918,7 +918,7 @@ thin_provisioning::metadata_repair(block_manager<>::ptr bm, emitter::ptr e, over //---------------------------------------------------------------- void -thin_provisioning::metadata_dump_subtree(metadata::ptr md, emitter::ptr e, bool repair, uint64_t subtree_root) { +thin_provisioning::metadata_dump_subtree(metadata::ptr md, emitter::ptr e, bool repair, ::uint64_t subtree_root) { mapping_emit_visitor me(e); single_mapping_tree tree(*md->tm_, subtree_root, mapping_tree_detail::block_time_ref_counter(md->data_sm_)); diff --git a/thin-provisioning/thin_dump.cc b/thin-provisioning/thin_dump.cc index 1b9cee2..e049202 100644 --- a/thin-provisioning/thin_dump.cc +++ b/thin-provisioning/thin_dump.cc @@ -145,7 +145,7 @@ thin_dump_cmd::run(int argc, char **argv) const char shortopts[] = "hm::o:f:rV"; char *end_ptr; block_address metadata_snap = 0; - uint64_t dev_id; + ::uint64_t dev_id; struct flags flags; const struct option longopts[] = { diff --git a/thin-provisioning/thin_ls.cc b/thin-provisioning/thin_ls.cc index 03b8d63..fe608a7 100644 --- a/thin-provisioning/thin_ls.cc +++ b/thin-provisioning/thin_ls.cc @@ -236,9 +236,9 @@ namespace { } }; - void pass1(metadata::ptr md, mapping_set &mappings, uint64_t dev_id) { + void pass1(metadata::ptr md, mapping_set &mappings, ::uint64_t dev_id) { dev_tree::key k = {dev_id}; - optional dev_root = md->mappings_top_level_->lookup(k); + optional<::uint64_t> dev_root = md->mappings_top_level_->lookup(k); if (!dev_root) throw runtime_error("couldn't find mapping tree root"); @@ -252,9 +252,9 @@ namespace { } - block_address count_exclusives(metadata::ptr md, mapping_set const &mappings, uint64_t dev_id) { + block_address count_exclusives(metadata::ptr md, mapping_set const &mappings, ::uint64_t dev_id) { dev_tree::key k = {dev_id}; - optional dev_root = md->mappings_top_level_->lookup(k); + optional<::uint64_t> dev_root = md->mappings_top_level_->lookup(k); if (!dev_root) throw runtime_error("couldn't find mapping tree root");