Skip to content

Commit

Permalink
[thin_check] Change the policy of --clear-needs-check-flag to prevent…
Browse files Browse the repository at this point in the history 
… error recurrence

- Disallow clearing the needs_check flag if there's any error,
  i.e., the metadata must be fully examined, and the result must
  be NO_ERROR.
- Disallow combining --clear-needs-check with -m, --super-blocks-only,
  --skip-mappings, --override-mapping-root, or --ignore-non-fatal-errors.
  • Loading branch information
@mingnus
mingnus committed Jul 30, 2020
1 parent 49efa06 commit b278f4f
Show file tree
Hide file tree
Showing 3 changed files with 43 additions and 40 deletions.
43 changes: 37 additions & 6 deletions thin-provisioning/metadata_checker.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
// with thin-provisioning-tools. If not, see
// <http://www.gnu.org/licenses/>.

#include "base/error_state.h"
#include "base/nested_output.h"
#include "persistent-data/file_utils.h"
#include "persistent-data/space-maps/core.h"
Expand Down Expand Up @@ -437,8 +438,30 @@ namespace {
return true;
}

error_state get_error() const {
return err_;
bool clear_needs_check_flag() {
if (!verify_preconditions_before_fixing()) {
out_ << "metadata has not been fully examined" << end_message();
return false;
}

if (err_ != NO_ERROR)
return false;

block_manager::ptr bm = open_bm(path_, block_manager::READ_WRITE);
superblock_detail::superblock sb = read_superblock(bm);
sb.set_needs_check_flag(false);
write_superblock(bm, sb);

out_ << "cleared needs_check flag" << end_message();

return true;
}

bool get_status() const {
if (options_.ignore_non_fatal_)
return (err_ == FATAL) ? false : true;

return (err_ == NO_ERROR) ? true : false;
}

private:
Expand Down Expand Up @@ -529,7 +552,8 @@ check_options::check_options()
data_mapping_opts_(DATA_MAPPING_LEVEL2),
sm_opts_(SPACE_MAP_FULL),
ignore_non_fatal_(false),
fix_metadata_leaks_(false) {
fix_metadata_leaks_(false),
clear_needs_check_(false) {
}

void check_options::set_superblock_only() {
Expand Down Expand Up @@ -559,8 +583,12 @@ void check_options::set_fix_metadata_leaks() {
fix_metadata_leaks_ = true;
}

void check_options::set_clear_needs_check() {
clear_needs_check_ = true;
}

bool check_options::check_conformance() {
if (fix_metadata_leaks_) {
if (fix_metadata_leaks_ || clear_needs_check_) {
if (ignore_non_fatal_) {
cerr << "cannot perform fix by ignoring non-fatal errors" << endl;
return false;
Expand Down Expand Up @@ -588,7 +616,7 @@ bool check_options::check_conformance() {

//----------------------------------------------------------------

base::error_state
bool
thin_provisioning::check_metadata(std::string const &path,
check_options const &check_opts,
output_options output_opts)
Expand All @@ -598,8 +626,11 @@ thin_provisioning::check_metadata(std::string const &path,
checker.check();
if (check_opts.fix_metadata_leaks_)
checker.fix_metadata_leaks();
if (check_opts.fix_metadata_leaks_ ||
check_opts.clear_needs_check_)
checker.clear_needs_check_flag();

return checker.get_error();
return checker.get_status();
}

//----------------------------------------------------------------
5 changes: 3 additions & 2 deletions thin-provisioning/metadata_checker.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
#ifndef METADATA_CHECKER_H
#define METADATA_CHECKER_H

#include "base/error_state.h"
#include "block-cache/block_cache.h"
#include "persistent-data/block.h"

Expand Down Expand Up @@ -47,21 +46,23 @@ namespace thin_provisioning {
void set_metadata_snap();
void set_ignore_non_fatal();
void set_fix_metadata_leaks();
void set_clear_needs_check();

bool use_metadata_snap_;
data_mapping_options data_mapping_opts_;
space_map_options sm_opts_;
boost::optional<bcache::block_address> override_mapping_root_;
bool ignore_non_fatal_;
bool fix_metadata_leaks_;
bool clear_needs_check_;
};

enum output_options {
OUTPUT_NORMAL,
OUTPUT_QUIET,
};

base::error_state
bool
check_metadata(std::string const &path,
check_options const &check_opts,
output_options output_opts);
Expand Down
35 changes: 3 additions & 32 deletions thin-provisioning/thin_check.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,27 +43,13 @@ using namespace thin_provisioning;
namespace {
struct flags {
flags()
: ignore_non_fatal_errors(false),
quiet(false),
clear_needs_check_flag_on_success(false) {
: quiet(false) {
}

check_options check_opts;

bool ignore_non_fatal_errors;

bool quiet;
bool clear_needs_check_flag_on_success;
};

void clear_needs_check(string const &path) {
block_manager::ptr bm = open_bm(path, block_manager::READ_WRITE);

superblock_detail::superblock sb = read_superblock(bm);
sb.set_needs_check_flag(false);
write_superblock(bm, sb);
}

// Returns 0 on success, 1 on failure (this gets returned directly
// by main).
int check(string const &path, flags fs) {
Expand All @@ -77,15 +63,7 @@ namespace {
}

output_options output_opts = !fs.quiet ? OUTPUT_NORMAL : OUTPUT_QUIET;
error_state err = check_metadata(path, fs.check_opts, output_opts);

if (fs.ignore_non_fatal_errors)
success = (err == FATAL) ? false : true;
else
success = (err == NO_ERROR) ? true : false;

if (success && fs.clear_needs_check_flag_on_success)
clear_needs_check(path);
success = check_metadata(path, fs.check_opts, output_opts);

} catch (std::exception &e) {
if (!fs.quiet)
Expand Down Expand Up @@ -173,13 +151,12 @@ thin_check_cmd::run(int argc, char **argv)

case 3:
// ignore-non-fatal-errors
fs.ignore_non_fatal_errors = true;
fs.check_opts.set_ignore_non_fatal();
break;

case 4:
// clear needs-check flag
fs.clear_needs_check_flag_on_success = true;
fs.check_opts.set_clear_needs_check();
break;

case 5:
Expand All @@ -198,12 +175,6 @@ thin_check_cmd::run(int argc, char **argv)
}
}

if (fs.clear_needs_check_flag_on_success && fs.check_opts.use_metadata_snap_) {
cerr << "--metadata-snap cannot be combined with --clear-needs-check-flag." << endl;
usage(cerr);
exit(1);
}

if (!fs.check_opts.check_conformance()) {
usage(cerr);
exit(1);
Expand Down

2 comments on commit b278f4f

@XeCycle
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ArchWiki has a hint that (vaguely) recommends setting --skip-mappings together; exit(1) here becomes a hard error. Since thin_check is run as part of LVM initialization this can cause boot to fail; actually I was dropped to initrd rescue shell last night. What about just printing a warning and switch off --clear-needs-check-flag?

Btw at first I thought metadata volume became full and tried some inappropriate repairing commands; those did not cause data loss but I am still in a nasty state to recover from. I'm afraid there may be other affected users.

@mingnus
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I apologize for the inconvenience caused. Actually, using --clear-needs-check-flag together with --skip-mappings is not a recommended approach, since you're not able to check for potential defects in metadata, that's why the commit was made. For now, I think the good way for most users is maintaining backward compatibility (i.e., cancel the limitation), until most distro documents are correctly updated. I've sent a pull request for this issue.

Switching off --clear-needs-check-flag might not work as expected, since a flagged metadata will lead to read-only pool, and I'm afraid that people don't aware that --skip-mappings causes NEEDS_CHECK flag uncleared, even though the warning message is printed.

Please sign in to comment.