linux-btrfs.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/3] btrfs-progs: check: Introduce optional argument for -b|--backup
@ 2019-10-21  9:37 Qu Wenruo
  2019-10-21  9:37 ` [PATCH 1/3] btrfs-progs: utils-lib: Use error() to replace fprintf(stderr, "ERROR: ") Qu Wenruo
                   ` (3 more replies)
  0 siblings, 4 replies; 7+ messages in thread
From: Qu Wenruo @ 2019-10-21  9:37 UTC (permalink / raw)
  To: linux-btrfs

Before this patchset, if we want to use backup roots, it's only possible
to let btrfs-check to automatically choose the backup.

If user want to use a specified backup, it can only use -r|--tree-root
option along with backup roots dump from "btrfs ins dump-super".

This patchset will introduce optional argument for -b|--backup, so user
can specify which backup to use by providing the generation difference
(-3, -2, -1).

If the optional argument is not provided, the default value is -1, and
the behavior should be pretty much the same.

Qu Wenruo (3):
  btrfs-progs: utils-lib: Use error() to replace fprintf(stderr, "ERROR:
    ")
  btrfs-progs: disk-io: Handle backup root more correctly
  btrfs-progs: check: Introduce optional argument for -b|--backup

 Documentation/btrfs-check.asciidoc |  6 ++--
 check/main.c                       | 33 +++++++++++++++---
 common/utils.h                     |  1 +
 ctree.h                            |  8 +++++
 disk-io.c                          | 55 ++++++++++++++++++++++++------
 disk-io.h                          | 33 +++++++++++-------
 utils-lib.c                        | 25 +++++++++++---
 7 files changed, 127 insertions(+), 34 deletions(-)

-- 
2.23.0


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH 1/3] btrfs-progs: utils-lib: Use error() to replace fprintf(stderr, "ERROR: ")
  2019-10-21  9:37 [PATCH 0/3] btrfs-progs: check: Introduce optional argument for -b|--backup Qu Wenruo
@ 2019-10-21  9:37 ` Qu Wenruo
  2019-10-21  9:48   ` Johannes Thumshirn
  2019-10-21  9:37 ` [PATCH 2/3] btrfs-progs: disk-io: Handle backup root more correctly Qu Wenruo
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 7+ messages in thread
From: Qu Wenruo @ 2019-10-21  9:37 UTC (permalink / raw)
  To: linux-btrfs

This saves several lines of code.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 utils-lib.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/utils-lib.c b/utils-lib.c
index c2b6097f5df9..0202dd7677f0 100644
--- a/utils-lib.c
+++ b/utils-lib.c
@@ -23,8 +23,7 @@ u64 arg_strtou64(const char *str)
 
 	value = strtoull(str, &ptr_parse_end, 0);
 	if (ptr_parse_end && *ptr_parse_end != '\0') {
-		fprintf(stderr, "ERROR: %s is not a valid numeric value.\n",
-			str);
+		error("%s is not a valid numeric value.", str);
 		exit(1);
 	}
 
@@ -33,12 +32,11 @@ u64 arg_strtou64(const char *str)
 	 * unexpected number to us, so let's do the check ourselves.
 	 */
 	if (str[0] == '-') {
-		fprintf(stderr, "ERROR: %s: negative value is invalid.\n",
-			str);
+		error("%s: negative value is invalid.", str);
 		exit(1);
 	}
 	if (value == ULLONG_MAX) {
-		fprintf(stderr, "ERROR: %s is too large.\n", str);
+		error("%s is too large.", str);
 		exit(1);
 	}
 	return value;
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH 2/3] btrfs-progs: disk-io: Handle backup root more correctly
  2019-10-21  9:37 [PATCH 0/3] btrfs-progs: check: Introduce optional argument for -b|--backup Qu Wenruo
  2019-10-21  9:37 ` [PATCH 1/3] btrfs-progs: utils-lib: Use error() to replace fprintf(stderr, "ERROR: ") Qu Wenruo
@ 2019-10-21  9:37 ` Qu Wenruo
  2019-10-21  9:37 ` [PATCH 3/3] btrfs-progs: check: Introduce optional argument for -b|--backup Qu Wenruo
  2019-11-15 12:32 ` [PATCH 0/3] " David Sterba
  3 siblings, 0 replies; 7+ messages in thread
From: Qu Wenruo @ 2019-10-21  9:37 UTC (permalink / raw)
  To: linux-btrfs

Current backup root handling has extra check on super generation:

static int find_best_backup_root(struct btrfs_super_block *super)
{
	u64 orig_gen = btrfs_super_generation(super);
...
		if (btrfs_backup_tree_root_gen(backup) != orig_gen &&
 		    btrfs_backup_tree_root_gen(backup) > gen) {
 			best_index = i;
 			gen = btrfs_backup_tree_root_gen(backup);

This check is to ensure we don't get backup root with current
generation, but it can still return backup root newer than current root.

So for the following super:
generation:		10
backup[0] generation:	8
backup[1] generation:	9
backup[2] generation:	10
backup[3] generation:	11

If we're calling find_best_backup_root() then we can pick up slot 3
which is newer than current generation.

This patch introduce a new parameter for find_best_backup_root() to
specify the max generation.

So with above superblock, calling find_best_backup_root(sb, sb_gen - 1)
will ensure we get slot 1, other than slot 3.
This also affects how we update backup roots.

Furthermore, due to the change in the return value,
find_best_backup_root() can now return -1 to indicates error (no valid
backup found), so change callers to co-operate.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 disk-io.c | 34 ++++++++++++++++++++++++++--------
 1 file changed, 26 insertions(+), 8 deletions(-)

diff --git a/disk-io.c b/disk-io.c
index be44eead5cef..36db1be264cd 100644
--- a/disk-io.c
+++ b/disk-io.c
@@ -845,17 +845,22 @@ int btrfs_check_fs_compatibility(struct btrfs_super_block *sb,
 	return 0;
 }
 
-static int find_best_backup_root(struct btrfs_super_block *super)
+/*
+ * Find the newest backup slot whose generation <= @max_gen
+ *
+ * Can return <0 for error, indicating no valid backup slot for @max_gen.
+ */
+static int find_best_backup_root(struct btrfs_super_block *super,
+				 u64 max_gen)
 {
 	struct btrfs_root_backup *backup;
-	u64 orig_gen = btrfs_super_generation(super);
 	u64 gen = 0;
-	int best_index = 0;
+	int best_index = -1;
 	int i;
 
 	for (i = 0; i < BTRFS_NUM_BACKUP_ROOTS; i++) {
 		backup = super->super_roots + i;
-		if (btrfs_backup_tree_root_gen(backup) != orig_gen &&
+		if (btrfs_backup_tree_root_gen(backup) <= max_gen &&
 		    btrfs_backup_tree_root_gen(backup) > gen) {
 			best_index = i;
 			gen = btrfs_backup_tree_root_gen(backup);
@@ -908,9 +913,10 @@ int btrfs_setup_all_roots(struct btrfs_fs_info *fs_info, u64 root_tree_bytenr,
 		root_tree_bytenr = btrfs_super_root(sb);
 	} else if (flags & OPEN_CTREE_BACKUP_ROOT) {
 		struct btrfs_root_backup *backup;
-		int index = find_best_backup_root(sb);
-		if (index >= BTRFS_NUM_BACKUP_ROOTS) {
-			fprintf(stderr, "Invalid backup root number\n");
+		int index = find_best_backup_root(sb,
+					btrfs_super_generation(sb) - 1);
+		if (index < 0) {
+			error("can't find any valid backup root");
 			return -EIO;
 		}
 		backup = fs_info->super_copy->super_roots + index;
@@ -1707,10 +1713,22 @@ static int write_dev_supers(struct btrfs_fs_info *fs_info,
 static void backup_super_roots(struct btrfs_fs_info *info)
 {
 	struct btrfs_root_backup *root_backup;
+	u64 current_gen = btrfs_super_generation(info->super_copy);
 	int next_backup;
 	int last_backup;
 
-	last_backup = find_best_backup_root(info->super_copy);
+	last_backup = find_best_backup_root(info->super_copy, current_gen - 1);
+	/* No older backups, retry current gen */
+	if (last_backup < 0) {
+		last_backup = find_best_backup_root(info->super_copy,
+						    current_gen);
+		/*
+		 * Still failed, means no valid backup root at all, restart
+		 * from slot 0.
+		 */
+		if (last_backup < 0)
+			last_backup = 0;
+	}
 	next_backup = (last_backup + 1) % BTRFS_NUM_BACKUP_ROOTS;
 
 	/* just overwrite the last backup if we're at the same generation */
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH 3/3] btrfs-progs: check: Introduce optional argument for -b|--backup
  2019-10-21  9:37 [PATCH 0/3] btrfs-progs: check: Introduce optional argument for -b|--backup Qu Wenruo
  2019-10-21  9:37 ` [PATCH 1/3] btrfs-progs: utils-lib: Use error() to replace fprintf(stderr, "ERROR: ") Qu Wenruo
  2019-10-21  9:37 ` [PATCH 2/3] btrfs-progs: disk-io: Handle backup root more correctly Qu Wenruo
@ 2019-10-21  9:37 ` Qu Wenruo
  2019-11-15 12:32 ` [PATCH 0/3] " David Sterba
  3 siblings, 0 replies; 7+ messages in thread
From: Qu Wenruo @ 2019-10-21  9:37 UTC (permalink / raw)
  To: linux-btrfs

Add an optional argument, <gen_diff> for -b|--backup.

This optional argument allow user to specify the generation difference
to search for the best backup root.

The value values are: -3, -2, -1.

To co-operate with this change, the following modifications are made:
- Man page and help string update

- New OPEN_CTREE flags and helpers are introduced
  OPEN_CTREE_BACKUP_GEN_DIFF[123] are introduced to replace old single
  bit OPEN_CTREE_BACKUP_ROOT.
  New helpers backup_gen_diff_to_cflags() and
  cflags_to_backup_gen_diff() are introduced to do the convert.
  So that we can keep the old open_ctree() interface without introducing
  new parameters.

- New arg_strol() helper to get negative int

- New btrfs_fs_info::backup_gen_diff member
  Now btrfs_setup_all_roots() uses that member to search backup roots.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 Documentation/btrfs-check.asciidoc |  6 ++++--
 check/main.c                       | 33 ++++++++++++++++++++++++++----
 common/utils.h                     |  1 +
 ctree.h                            |  8 ++++++++
 disk-io.c                          | 25 ++++++++++++++++++----
 disk-io.h                          | 33 ++++++++++++++++++------------
 utils-lib.c                        | 17 +++++++++++++++
 7 files changed, 100 insertions(+), 23 deletions(-)

diff --git a/Documentation/btrfs-check.asciidoc b/Documentation/btrfs-check.asciidoc
index b963eae5cdce..84a4241069cf 100644
--- a/Documentation/btrfs-check.asciidoc
+++ b/Documentation/btrfs-check.asciidoc
@@ -41,8 +41,10 @@ filesystem, similarly the run time.
 SAFE OR ADVISORY OPTIONS
 ------------------------
 
--b|--backup::
-use the first valid set of backup roots stored in the superblock
+-b[<gen_diff>] | --backup[=<gen_diff>]::
+use the newest valid set of backup roots which is no older than 'generation + <gen_diff>'
++
+'<gen_diff>' is optional, if not given, default value is -1. Valid values are -3, -2 ,-1.
 +
 This can be combined with '--super' if some of the superblocks are damaged.
 
diff --git a/check/main.c b/check/main.c
index c2d0f3949c5e..46f3e3d4c5b5 100644
--- a/check/main.c
+++ b/check/main.c
@@ -9773,7 +9773,9 @@ static const char * const cmd_check_usage[] = {
 	"Options:",
 	"  starting point selection:",
 	"       -s|--super <superblock>     use this superblock copy",
-	"       -b|--backup                 use the first valid backup root copy",
+	"       -b|--backup[=<gen_diff>]    use the backup root with <gen_diff>",
+	"                                   <gen_diff> is an optional parameter,",
+	"                                   valid values are -3, -2, -1 (default).",
 	"       -r|--tree-root <bytenr>     use the given bytenr for the tree root",
 	"       --chunk-root <bytenr>       use the given bytenr for the chunk tree root",
 	"  operation modes:",
@@ -9799,6 +9801,17 @@ static const char * const cmd_check_usage[] = {
 	NULL
 };
 
+static unsigned backup_gen_diff_to_cflags(int backup_gen_diff)
+{
+	ASSERT(-3 <= backup_gen_diff && backup_gen_diff <= -1);
+
+	if (backup_gen_diff == -3)
+		return OPEN_CTREE_BACKUP_GEN_DIFF3;
+	if (backup_gen_diff == -2)
+		return OPEN_CTREE_BACKUP_GEN_DIFF2;
+	return OPEN_CTREE_BACKUP_GEN_DIFF1;
+}
+
 static int cmd_check(const struct cmd_struct *cmd, int argc, char **argv)
 {
 	struct cache_tree root_cache;
@@ -9818,6 +9831,7 @@ static int cmd_check(const struct cmd_struct *cmd, int argc, char **argv)
 	int qgroup_report = 0;
 	int qgroups_repaired = 0;
 	int qgroup_report_ret;
+	int backup_gen_diff;
 	unsigned ctree_flags = OPEN_CTREE_EXCLUSIVE;
 	int force = 0;
 
@@ -9838,7 +9852,7 @@ static int cmd_check(const struct cmd_struct *cmd, int argc, char **argv)
 				GETOPT_VAL_INIT_EXTENT },
 			{ "check-data-csum", no_argument, NULL,
 				GETOPT_VAL_CHECK_CSUM },
-			{ "backup", no_argument, NULL, 'b' },
+			{ "backup", optional_argument, NULL, 'b' },
 			{ "subvol-extents", required_argument, NULL, 'E' },
 			{ "qgroup-report", no_argument, NULL, 'Q' },
 			{ "tree-root", required_argument, NULL, 'r' },
@@ -9853,13 +9867,24 @@ static int cmd_check(const struct cmd_struct *cmd, int argc, char **argv)
 			{ NULL, 0, NULL, 0}
 		};
 
-		c = getopt_long(argc, argv, "as:br:pE:Q", long_options, NULL);
+		c = getopt_long(argc, argv, "as:b::r:pE:Q", long_options, NULL);
 		if (c < 0)
 			break;
 		switch(c) {
 			case 'a': /* ignored */ break;
 			case 'b':
-				ctree_flags |= OPEN_CTREE_BACKUP_ROOT;
+				if (optarg) {
+					backup_gen_diff = arg_strtol(optarg);
+				} else
+					backup_gen_diff = -1;
+				if (!(-3 <= backup_gen_diff &&
+				      backup_gen_diff <= -1)) {
+					error(
+					"backup_gen_diff must be in [-3, -1]");
+					exit(1);
+				}
+				ctree_flags |= backup_gen_diff_to_cflags(
+						backup_gen_diff);
 				break;
 			case 's':
 				num = arg_strtou64(optarg);
diff --git a/common/utils.h b/common/utils.h
index 7867fb0a35d9..26bc2b081356 100644
--- a/common/utils.h
+++ b/common/utils.h
@@ -68,6 +68,7 @@ const char *pretty_size_mode(u64 size, unsigned mode);
 u64 parse_size(char *s);
 u64 parse_qgroupid(const char *p);
 u64 arg_strtou64(const char *str);
+int long arg_strtol(const char *str);
 int open_file_or_dir(const char *fname, DIR **dirstream);
 int open_file_or_dir3(const char *fname, DIR **dirstream, int open_flags);
 void close_file_or_dir(int fd, DIR *dirstream);
diff --git a/ctree.h b/ctree.h
index 0d12563b7261..5f83d9631678 100644
--- a/ctree.h
+++ b/ctree.h
@@ -1176,6 +1176,14 @@ struct btrfs_fs_info {
 	unsigned int finalize_on_close:1;
 
 	int transaction_aborted;
+	/*
+	 * The generation diff for backup root use.
+	 * Valid values are [-3, 0]
+	 * [-3, -1]:	Use backup root whose generation is no newer than
+	 *		current_gen + diff
+	 * 0:		Don't use backup root at all.
+	 */
+	int backup_gen_diff;
 
 	int (*free_extent_hook)(struct btrfs_fs_info *fs_info,
 				u64 bytenr, u64 num_bytes, u64 parent,
diff --git a/disk-io.c b/disk-io.c
index 36db1be264cd..4186a6544dce 100644
--- a/disk-io.c
+++ b/disk-io.c
@@ -909,14 +909,18 @@ int btrfs_setup_all_roots(struct btrfs_fs_info *fs_info, u64 root_tree_bytenr,
 	btrfs_setup_root(root, fs_info, BTRFS_ROOT_TREE_OBJECTID);
 	generation = btrfs_super_generation(sb);
 
-	if (!root_tree_bytenr && !(flags & OPEN_CTREE_BACKUP_ROOT)) {
+	if (!root_tree_bytenr && !fs_info->backup_gen_diff) {
 		root_tree_bytenr = btrfs_super_root(sb);
-	} else if (flags & OPEN_CTREE_BACKUP_ROOT) {
+	} else if (fs_info->backup_gen_diff) {
 		struct btrfs_root_backup *backup;
 		int index = find_best_backup_root(sb,
-					btrfs_super_generation(sb) - 1);
+					btrfs_super_generation(sb) +
+					fs_info->backup_gen_diff);
 		if (index < 0) {
-			error("can't find any valid backup root");
+			error(
+		"can't find valid backup root with no newer than gen %llu",
+			      btrfs_super_generation(sb) +
+			      fs_info->backup_gen_diff);
 			return -EIO;
 		}
 		backup = fs_info->super_copy->super_roots + index;
@@ -1150,6 +1154,17 @@ int btrfs_setup_chunk_tree_and_device_map(struct btrfs_fs_info *fs_info,
 	return 0;
 }
 
+static int cflags_to_backup_gen_diff(unsigned flags)
+{
+	if (flags & OPEN_CTREE_BACKUP_GEN_DIFF3)
+		return -3;
+	if (flags & OPEN_CTREE_BACKUP_GEN_DIFF2)
+		return -2;
+	if (flags & OPEN_CTREE_BACKUP_GEN_DIFF1)
+		return -1;
+	return 0;
+}
+
 static struct btrfs_fs_info *__open_ctree_fd(int fp, const char *path,
 					     u64 sb_bytenr,
 					     u64 root_tree_bytenr,
@@ -1261,6 +1276,8 @@ static struct btrfs_fs_info *__open_ctree_fd(int fp, const char *path,
 			   btrfs_header_chunk_tree_uuid(eb),
 			   BTRFS_UUID_SIZE);
 
+	fs_info->backup_gen_diff = cflags_to_backup_gen_diff(flags);
+
 	ret = btrfs_setup_all_roots(fs_info, root_tree_bytenr, flags);
 	if (ret && !(flags & __OPEN_CTREE_RETURN_CHUNK_ROOT) &&
 	    !fs_info->ignore_chunk_tree_error)
diff --git a/disk-io.h b/disk-io.h
index 7b5c3806ba98..cebedf8ff408 100644
--- a/disk-io.h
+++ b/disk-io.h
@@ -34,25 +34,23 @@ enum btrfs_open_ctree_flags {
 	OPEN_CTREE_WRITES		= (1U << 0),
 	/* Allow to open filesystem with some broken tree roots (eg log root) */
 	OPEN_CTREE_PARTIAL		= (1U << 1),
-	/* If primary root pinters are invalid, try backup copies */
-	OPEN_CTREE_BACKUP_ROOT		= (1U << 2),
 	/* Allow reading all superblock copies if the primary is damaged */
-	OPEN_CTREE_RECOVER_SUPER	= (1U << 3),
+	OPEN_CTREE_RECOVER_SUPER	= (1U << 2),
 	/* Restoring filesystem image */
-	OPEN_CTREE_RESTORE		= (1U << 4),
+	OPEN_CTREE_RESTORE		= (1U << 3),
 	/* Do not read block groups (extent tree) */
-	OPEN_CTREE_NO_BLOCK_GROUPS	= (1U << 5),
+	OPEN_CTREE_NO_BLOCK_GROUPS	= (1U << 4),
 	/* Open all devices in O_EXCL mode */
-	OPEN_CTREE_EXCLUSIVE		= (1U << 6),
+	OPEN_CTREE_EXCLUSIVE		= (1U << 5),
 	/* Do not scan devices */
-	OPEN_CTREE_NO_DEVICES		= (1U << 7),
+	OPEN_CTREE_NO_DEVICES		= (1U << 6),
 	/*
 	 * Don't print error messages if bytenr or checksums do not match in
 	 * tree block headers. Turn on by OPEN_CTREE_SUPPRESS_ERROR
 	 */
-	OPEN_CTREE_SUPPRESS_CHECK_BLOCK_ERRORS	= (1U << 8),
+	OPEN_CTREE_SUPPRESS_CHECK_BLOCK_ERRORS	= (1U << 7),
 	/* Return the chunk root */
-	__OPEN_CTREE_RETURN_CHUNK_ROOT	= (1U << 9),
+	__OPEN_CTREE_RETURN_CHUNK_ROOT	= (1U << 8),
 	OPEN_CTREE_CHUNK_ROOT_ONLY	= OPEN_CTREE_PARTIAL +
 					  OPEN_CTREE_SUPPRESS_CHECK_BLOCK_ERRORS +
 					  __OPEN_CTREE_RETURN_CHUNK_ROOT,
@@ -63,7 +61,7 @@ enum btrfs_open_ctree_flags {
 	 */
 
 	/* Ignore UUID mismatches */
-	OPEN_CTREE_IGNORE_FSID_MISMATCH	= (1U << 10),
+	OPEN_CTREE_IGNORE_FSID_MISMATCH	= (1U << 9),
 
 	/*
 	 * Allow open_ctree_fs_info() to return an incomplete fs_info with
@@ -71,20 +69,29 @@ enum btrfs_open_ctree_flags {
 	 * It's useful when chunks are corrupted.
 	 * Makes no sense for open_ctree variants returning btrfs_root.
 	 */
-	OPEN_CTREE_IGNORE_CHUNK_TREE_ERROR = (1U << 11),
+	OPEN_CTREE_IGNORE_CHUNK_TREE_ERROR = (1U << 10),
 
 	/*
 	 * Allow to open fs with temporary superblock (BTRFS_MAGIC_PARTIAL),
 	 * such fs contains very basic tree layout, just able to be opened.
 	 * Such temporary super is used for mkfs or convert.
 	 */
-	OPEN_CTREE_TEMPORARY_SUPER = (1U << 12),
+	OPEN_CTREE_TEMPORARY_SUPER = (1U << 11),
 
 	/*
 	 * Invalidate the free space tree (i.e., clear the FREE_SPACE_TREE_VALID
 	 * compat_ro bit).
 	 */
-	OPEN_CTREE_INVALIDATE_FST = (1U << 13),
+	OPEN_CTREE_INVALIDATE_FST = (1U << 12),
+
+	/*
+	 * If primary root pinters are invalid, try backup copies
+	 * The tailing number is the generation diff, for better
+	 * backup root control.
+	 */
+	OPEN_CTREE_BACKUP_GEN_DIFF1	= (1U << 13),
+	OPEN_CTREE_BACKUP_GEN_DIFF2	= (1U << 14),
+	OPEN_CTREE_BACKUP_GEN_DIFF3	= (1U << 15),
 };
 
 /*
diff --git a/utils-lib.c b/utils-lib.c
index 0202dd7677f0..2eb2bc8bbd0d 100644
--- a/utils-lib.c
+++ b/utils-lib.c
@@ -42,6 +42,23 @@ u64 arg_strtou64(const char *str)
 	return value;
 }
 
+int long arg_strtol(const char *str)
+{
+	int long value;
+	char *ptr_parse_end = NULL;
+
+	value = strtol(str, &ptr_parse_end, 0);
+	if (ptr_parse_end && *ptr_parse_end != '\0') {
+		error("%s is not a valid numeric value.", str);
+		exit(1);
+	}
+	if (errno == ERANGE) {
+		error("%s is out of valid range.", str);
+		exit(1);
+	}
+	return value;
+}
+
 /*
  * For a given:
  * - file or directory return the containing tree root id
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/3] btrfs-progs: utils-lib: Use error() to replace fprintf(stderr, "ERROR: ")
  2019-10-21  9:37 ` [PATCH 1/3] btrfs-progs: utils-lib: Use error() to replace fprintf(stderr, "ERROR: ") Qu Wenruo
@ 2019-10-21  9:48   ` Johannes Thumshirn
  0 siblings, 0 replies; 7+ messages in thread
From: Johannes Thumshirn @ 2019-10-21  9:48 UTC (permalink / raw)
  To: Qu Wenruo, linux-btrfs

Looks good,
Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>

-- 
Johannes Thumshirn                            SUSE Labs Filesystems
jthumshirn@suse.de                                +49 911 74053 689
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5
90409 Nürnberg
Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 0/3] btrfs-progs: check: Introduce optional argument for -b|--backup
  2019-10-21  9:37 [PATCH 0/3] btrfs-progs: check: Introduce optional argument for -b|--backup Qu Wenruo
                   ` (2 preceding siblings ...)
  2019-10-21  9:37 ` [PATCH 3/3] btrfs-progs: check: Introduce optional argument for -b|--backup Qu Wenruo
@ 2019-11-15 12:32 ` David Sterba
  2019-11-15 12:36   ` Qu Wenruo
  3 siblings, 1 reply; 7+ messages in thread
From: David Sterba @ 2019-11-15 12:32 UTC (permalink / raw)
  To: Qu Wenruo; +Cc: linux-btrfs

On Mon, Oct 21, 2019 at 05:37:52PM +0800, Qu Wenruo wrote:
> Before this patchset, if we want to use backup roots, it's only possible
> to let btrfs-check to automatically choose the backup.
> 
> If user want to use a specified backup, it can only use -r|--tree-root
> option along with backup roots dump from "btrfs ins dump-super".
> 
> This patchset will introduce optional argument for -b|--backup, so user
> can specify which backup to use by providing the generation difference
> (-3, -2, -1).

Please don't introduce the optional arguments. I think we've learned the
lesson with 'defrag -c' or balance -d/-m arguments. In this case a long
option would be fine as the backup roots is not something that's used
often. We can keep the --backup as "use first good" and add the more
specific selection.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 0/3] btrfs-progs: check: Introduce optional argument for -b|--backup
  2019-11-15 12:32 ` [PATCH 0/3] " David Sterba
@ 2019-11-15 12:36   ` Qu Wenruo
  0 siblings, 0 replies; 7+ messages in thread
From: Qu Wenruo @ 2019-11-15 12:36 UTC (permalink / raw)
  To: dsterba, Qu Wenruo, linux-btrfs


[-- Attachment #1.1: Type: text/plain, Size: 1015 bytes --]



On 2019/11/15 下午8:32, David Sterba wrote:
> On Mon, Oct 21, 2019 at 05:37:52PM +0800, Qu Wenruo wrote:
>> Before this patchset, if we want to use backup roots, it's only possible
>> to let btrfs-check to automatically choose the backup.
>>
>> If user want to use a specified backup, it can only use -r|--tree-root
>> option along with backup roots dump from "btrfs ins dump-super".
>>
>> This patchset will introduce optional argument for -b|--backup, so user
>> can specify which backup to use by providing the generation difference
>> (-3, -2, -1).
> 
> Please don't introduce the optional arguments. I think we've learned the
> lesson with 'defrag -c' or balance -d/-m arguments. In this case a long
> option would be fine as the backup roots is not something that's used
> often. We can keep the --backup as "use first good" and add the more
> specific selection.
>

OK, I'll rename it to something like --backup-gen-diff, and get rid of
the minus geneartion part.

Thanks,
Qu


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2019-11-15 12:37 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-21  9:37 [PATCH 0/3] btrfs-progs: check: Introduce optional argument for -b|--backup Qu Wenruo
2019-10-21  9:37 ` [PATCH 1/3] btrfs-progs: utils-lib: Use error() to replace fprintf(stderr, "ERROR: ") Qu Wenruo
2019-10-21  9:48   ` Johannes Thumshirn
2019-10-21  9:37 ` [PATCH 2/3] btrfs-progs: disk-io: Handle backup root more correctly Qu Wenruo
2019-10-21  9:37 ` [PATCH 3/3] btrfs-progs: check: Introduce optional argument for -b|--backup Qu Wenruo
2019-11-15 12:32 ` [PATCH 0/3] " David Sterba
2019-11-15 12:36   ` Qu Wenruo

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).