linux-btrfs.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 00/10] Enhance btrfs-find-root and open_ctree() to provide better chance on damaged btrfs.
@ 2015-01-29  9:07 Qu Wenruo
  2015-01-29  9:07 ` [PATCH v3 01/10] btrfs-progs: Cleanup check_tree_block() function and split the output to print_tree_block_err() function Qu Wenruo
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Qu Wenruo @ 2015-01-29  9:07 UTC (permalink / raw)
  To: linux-btrfs

The patchset first enhance btrfs-find-root command and then use it to
enhance open_ctree to provide a better chance to open heavily damaged
btrfs.

Patch 1~9 are all enhancement/cleanup for btrfs-find-root in the following
concepts.
1) Reuse existing infrastructure.
Use existing or slightly modified infrastructure other than
copy-n-modify codes.

2) Enhanced root search logic
The old root search logic have many problems, like ignore newer root
with smaller level and use wrong generation/level for searching.

The new logic will keep a per-generation record to deal the tree search,
and use different level/generation for different tree.

3) Make the find-root infrastructure exported to other commands.
Allow other btrfs-progs components to use find-root infrastructure, e.g.
open_ctree can use it if all primary/backup tree roots are corrupted.

Patch 10 is enhancement for open_ctree to use find-root infrastructure.

Also, since only 2 patches is modified(although other part is slightly
modified to match the change), to avoid mail bombing, I created the pull
request on github and only send the first 2 patches with cover-letter.
https://github.com/kdave/btrfs-progs/pull/5

---
changelog:
v2:
   Split patch into logically independent parts.
v3:
   Change fs_info->suppress_error to fs_info->suppress_tree_err to avoid
   naming confusion.
   Cleanup the check_tree_block() output and provide better output in
   print_tree_block_err().
---

Qu Wenruo (10):
  btrfs-progs: Cleanup check_tree_block() function and split the output
    to     print_tree_block_err() function.
  btrfs-progs: Add support to suppress tree block csum error output.
  btrfs-progs: Add new btrfs_open_ctree_flags CHUNK_ONLY.
  btrfs-progs: Add new find-root.[ch] infrastructure.
  btrfs-progs: Switch btrfs-find-root to use the new open_ctree flags.
  btrfs-progs: Add better search generation judgment for
    btrfs-find-root.
  btrfs-progs: Switch btrfs-find-root to use the find-root
    infrastructure.
  btrfs-progs: Cleanup unneeded btrfs-find-root codes.
  btrfs-progs: Add new option for btrfs-find-root to search through all
    the metadata extents.
  btrfs-progs: Allow open_ctree use backup tree root or search it
    automatically if primary tree root is corrupted.

 Documentation/btrfs-find-root.txt |   2 +
 Makefile                          |   2 +-
 btrfs-find-root.c                 | 379 +++++++++++++-------------------------
 ctree.h                           |   2 +
 disk-io.c                         | 172 ++++++++++++++---
 disk-io.h                         |  10 +
 find-root.c                       | 138 ++++++++++++++
 find-root.h                       |  84 +++++++++
 8 files changed, 514 insertions(+), 275 deletions(-)
 create mode 100644 find-root.c
 create mode 100644 find-root.h

-- 
2.2.2


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

* [PATCH v3 01/10] btrfs-progs: Cleanup check_tree_block() function and split the output to print_tree_block_err() function.
  2015-01-29  9:07 [PATCH v3 00/10] Enhance btrfs-find-root and open_ctree() to provide better chance on damaged btrfs Qu Wenruo
@ 2015-01-29  9:07 ` Qu Wenruo
  2015-01-29  9:07 ` [PATCH v3 02/10] btrfs-progs: Add support to suppress tree block csum error output Qu Wenruo
  2015-02-10 14:48 ` [PATCH v3 00/10] Enhance btrfs-find-root and open_ctree() to provide better chance on damaged btrfs David Sterba
  2 siblings, 0 replies; 9+ messages in thread
From: Qu Wenruo @ 2015-01-29  9:07 UTC (permalink / raw)
  To: linux-btrfs

Before this patch, check_tree_block() will print error on bytenr
mismatch but don't output error on fsid mismatch.

This patch will modify check_tree_block(), so it will only return errno
but not print error messages.
The error message will be output by print_tree_block_err() function.

Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
---
Changelog:
v3:
  Newly introduced.
---
 disk-io.c | 45 +++++++++++++++++++++++++++++++++++++--------
 1 file changed, 37 insertions(+), 8 deletions(-)

diff --git a/disk-io.c b/disk-io.c
index 7f03790..e14a143 100644
--- a/disk-io.c
+++ b/disk-io.c
@@ -22,6 +22,7 @@
 #include <sys/stat.h>
 #include <fcntl.h>
 #include <unistd.h>
+#include <uuid/uuid.h>
 #include "kerncompat.h"
 #include "radix-tree.h"
 #include "ctree.h"
@@ -33,17 +34,18 @@
 #include "print-tree.h"
 #include "rbtree-utils.h"
 
+/* specified errno for check_tree_block */
+#define EBYTENR		1
+#define EFSID		2
+
 static int check_tree_block(struct btrfs_root *root, struct extent_buffer *buf)
 {
 
 	struct btrfs_fs_devices *fs_devices;
-	int ret = 1;
+	int ret = -EFSID;
 
-	if (buf->start != btrfs_header_bytenr(buf)) {
-		printk("Check tree block failed, want=%Lu, have=%Lu\n",
-		       buf->start, btrfs_header_bytenr(buf));
-		return ret;
-	}
+	if (buf->start != btrfs_header_bytenr(buf))
+		return -EBYTENR;
 
 	fs_devices = root->fs_info->fs_devices;
 	while (fs_devices) {
@@ -58,6 +60,30 @@ static int check_tree_block(struct btrfs_root *root, struct extent_buffer *buf)
 	return ret;
 }
 
+static void print_tree_block_err(struct btrfs_root *root,
+				struct extent_buffer *eb,
+				int err)
+{
+	char fs_uuid[BTRFS_UUID_UNPARSED_SIZE] = {'\0'};
+	char found_uuid[BTRFS_UUID_UNPARSED_SIZE] = {'\0'};
+	u8 buf[BTRFS_UUID_SIZE];
+
+	switch (err) {
+	case -EFSID:
+		read_extent_buffer(eb, buf, btrfs_header_fsid(),
+				   BTRFS_UUID_SIZE);
+		uuid_unparse(buf, found_uuid);
+		uuid_unparse(root->fs_info->fsid, fs_uuid);
+		fprintf(stderr, "fsid mismatch, want=%s, have=%s\n",
+			fs_uuid, found_uuid);
+		break;
+	case -EBYTENR:
+		fprintf(stderr, "bytenr mismatch, want=%llu, have=%llu\n",
+			eb->start, btrfs_header_bytenr(eb));
+		break;
+	}
+}
+
 u32 btrfs_csum_data(struct btrfs_root *root, char *data, u32 seed, size_t len)
 {
 	return crc32c(seed, data, len);
@@ -280,7 +306,8 @@ struct extent_buffer *read_tree_block(struct btrfs_root *root, u64 bytenr,
 		}
 		if (ignore) {
 			if (check_tree_block(root, eb))
-				printk("read block failed check_tree_block\n");
+				print_tree_block_err(root, eb,
+						check_tree_block(root, eb));
 			else
 				printk("Csum didn't match\n");
 			break;
@@ -342,8 +369,10 @@ static int write_tree_block(struct btrfs_trans_handle *trans,
 		     struct btrfs_root *root,
 		     struct extent_buffer *eb)
 {
-	if (check_tree_block(root, eb))
+	if (check_tree_block(root, eb)) {
+		print_tree_block_err(root, eb, check_tree_block(root, eb));
 		BUG();
+	}
 
 	if (!btrfs_buffer_uptodate(eb, trans->transid))
 		BUG();
-- 
2.2.2


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

* [PATCH v3 02/10] btrfs-progs: Add support to suppress tree block csum error output.
  2015-01-29  9:07 [PATCH v3 00/10] Enhance btrfs-find-root and open_ctree() to provide better chance on damaged btrfs Qu Wenruo
  2015-01-29  9:07 ` [PATCH v3 01/10] btrfs-progs: Cleanup check_tree_block() function and split the output to print_tree_block_err() function Qu Wenruo
@ 2015-01-29  9:07 ` Qu Wenruo
  2015-02-10 14:48 ` [PATCH v3 00/10] Enhance btrfs-find-root and open_ctree() to provide better chance on damaged btrfs David Sterba
  2 siblings, 0 replies; 9+ messages in thread
From: Qu Wenruo @ 2015-01-29  9:07 UTC (permalink / raw)
  To: linux-btrfs

Add new open ctree flag OPEN_CTREE_SUPPRESS_ERROR to suppress tree block
csum error output.

Provides the basis for new btrfs-find-root and other enhancement on
btrfs offline tools output.

Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
---
changelog:
v2:
  None
v3:
  Use 'suppress_tree_error' to replace the old name 'suppress_error' to
  avoid confusion.
---
 ctree.h   |  2 ++
 disk-io.c | 19 +++++++++++++------
 disk-io.h |  2 ++
 3 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/ctree.h b/ctree.h
index c069a95..494c9ac 100644
--- a/ctree.h
+++ b/ctree.h
@@ -996,6 +996,7 @@ struct btrfs_fs_info {
 	unsigned int on_restoring:1;
 	unsigned int is_chunk_recover:1;
 	unsigned int quota_enabled:1;
+	unsigned int suppress_tree_err:1;
 
 	int (*free_extent_hook)(struct btrfs_trans_handle *trans,
 				struct btrfs_root *root,
@@ -1004,6 +1005,7 @@ struct btrfs_fs_info {
 				int refs_to_drop);
 	struct cache_tree *fsck_extent_cache;
 	struct cache_tree *corrupt_blocks;
+
 };
 
 /*
diff --git a/disk-io.c b/disk-io.c
index e14a143..3291071 100644
--- a/disk-io.c
+++ b/disk-io.c
@@ -141,6 +141,8 @@ int csum_tree_block(struct btrfs_root *root, struct extent_buffer *buf,
 {
 	u16 csum_size =
 		btrfs_super_csum_size(root->fs_info->super_copy);
+	if (verify && root->fs_info->suppress_tree_err)
+		return verify_tree_block_csum_silent(buf, csum_size);
 	return csum_tree_block_size(buf, csum_size, verify);
 }
 
@@ -291,8 +293,8 @@ struct extent_buffer *read_tree_block(struct btrfs_root *root, u64 bytenr,
 
 	while (1) {
 		ret = read_whole_eb(root->fs_info, eb, mirror_num);
-		if (ret == 0 && check_tree_block(root, eb) == 0 &&
-		    csum_tree_block(root, eb, 1) == 0 &&
+		if (ret == 0 && csum_tree_block(root, eb, 1) == 0 &&
+		    check_tree_block(root, eb) == 0 &&
 		    verify_parent_transid(eb->tree, eb, parent_transid, ignore)
 		    == 0) {
 			if (eb->flags & EXTENT_BAD_TRANSID &&
@@ -305,11 +307,14 @@ struct extent_buffer *read_tree_block(struct btrfs_root *root, u64 bytenr,
 			return eb;
 		}
 		if (ignore) {
-			if (check_tree_block(root, eb))
-				print_tree_block_err(root, eb,
+			if (check_tree_block(root, eb)) {
+				if (!root->fs_info->suppress_tree_err)
+					print_tree_block_err(root, eb,
 						check_tree_block(root, eb));
-			else
-				printk("Csum didn't match\n");
+			} else {
+				if (!root->fs_info->suppress_tree_err)
+					fprintf(stderr, "Csum didn't match\n");
+			}
 			break;
 		}
 		num_copies = btrfs_num_copies(&root->fs_info->mapping_tree,
@@ -1138,6 +1143,8 @@ static struct btrfs_fs_info *__open_ctree_fd(int fp, const char *path,
 	}
 	if (flags & OPEN_CTREE_RESTORE)
 		fs_info->on_restoring = 1;
+	if (flags & OPEN_CTREE_SUPPRESS_ERROR)
+		fs_info->suppress_tree_err = 1;
 
 	ret = btrfs_scan_fs_devices(fp, path, &fs_devices, sb_bytenr,
 				    (flags & OPEN_CTREE_RECOVER_SUPER));
diff --git a/disk-io.h b/disk-io.h
index f963a96..84f878a 100644
--- a/disk-io.h
+++ b/disk-io.h
@@ -33,6 +33,8 @@ enum btrfs_open_ctree_flags {
 	OPEN_CTREE_RESTORE		= (1 << 4),
 	OPEN_CTREE_NO_BLOCK_GROUPS	= (1 << 5),
 	OPEN_CTREE_EXCLUSIVE		= (1 << 6),
+	OPEN_CTREE_SUPPRESS_ERROR	= (1 << 7), /* Suppress tree block
+						       check error */
 };
 
 static inline u64 btrfs_sb_offset(int mirror)
-- 
2.2.2


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

* Re: [PATCH v3 00/10] Enhance btrfs-find-root and open_ctree() to provide better chance on damaged btrfs.
  2015-01-29  9:07 [PATCH v3 00/10] Enhance btrfs-find-root and open_ctree() to provide better chance on damaged btrfs Qu Wenruo
  2015-01-29  9:07 ` [PATCH v3 01/10] btrfs-progs: Cleanup check_tree_block() function and split the output to print_tree_block_err() function Qu Wenruo
  2015-01-29  9:07 ` [PATCH v3 02/10] btrfs-progs: Add support to suppress tree block csum error output Qu Wenruo
@ 2015-02-10 14:48 ` David Sterba
  2015-02-11  0:33   ` Qu Wenruo
  2 siblings, 1 reply; 9+ messages in thread
From: David Sterba @ 2015-02-10 14:48 UTC (permalink / raw)
  To: Qu Wenruo; +Cc: linux-btrfs

On Thu, Jan 29, 2015 at 05:07:15PM +0800, Qu Wenruo wrote:
> Also, since only 2 patches is modified(although other part is slightly
> modified to match the change), to avoid mail bombing, I created the pull
> request on github and only send the first 2 patches with cover-letter.
> https://github.com/kdave/btrfs-progs/pull/5

Sending the changed patches only is ok (if you point me at the rest of
the patches), but it's not necessary to open the github pull request.

The version to version changelogs are also stored in the commit
changelogs, that's a bit unexpected for a branch to be pulled.

I've been reviewing this patch series and am mostly ok with that so I'm
going to pull that soon.


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

* Re: [PATCH v3 00/10] Enhance btrfs-find-root and open_ctree() to provide better chance on damaged btrfs.
  2015-02-10 14:48 ` [PATCH v3 00/10] Enhance btrfs-find-root and open_ctree() to provide better chance on damaged btrfs David Sterba
@ 2015-02-11  0:33   ` Qu Wenruo
  2015-02-11 17:52     ` David Sterba
  0 siblings, 1 reply; 9+ messages in thread
From: Qu Wenruo @ 2015-02-11  0:33 UTC (permalink / raw)
  To: dsterba, linux-btrfs


-------- Original Message --------
Subject: Re: [PATCH v3 00/10] Enhance btrfs-find-root and open_ctree() 
to provide better chance on damaged btrfs.
From: David Sterba <dsterba@suse.cz>
To: Qu Wenruo <quwenruo@cn.fujitsu.com>
Date: 2015年02月10日 22:48
> On Thu, Jan 29, 2015 at 05:07:15PM +0800, Qu Wenruo wrote:
>> Also, since only 2 patches is modified(although other part is slightly
>> modified to match the change), to avoid mail bombing, I created the pull
>> request on github and only send the first 2 patches with cover-letter.
>> https://github.com/kdave/btrfs-progs/pull/5
> Sending the changed patches only is ok (if you point me at the rest of
> the patches), but it's not necessary to open the github pull request.
>
> The version to version changelogs are also stored in the commit
> changelogs, that's a bit unexpected for a branch to be pulled.
Oh, very sorry for this.
I was meant to save your time, but I forgot that pull branch won't emit 
the changelog like patches.

I'll take care next time.

Thanks,
Qu
>
> I've been reviewing this patch series and am mostly ok with that so I'm
> going to pull that soon.
>


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

* Re: [PATCH v3 00/10] Enhance btrfs-find-root and open_ctree() to provide better chance on damaged btrfs.
  2015-02-11  0:33   ` Qu Wenruo
@ 2015-02-11 17:52     ` David Sterba
  2015-02-12  1:36       ` Qu Wenruo
  0 siblings, 1 reply; 9+ messages in thread
From: David Sterba @ 2015-02-11 17:52 UTC (permalink / raw)
  To: Qu Wenruo; +Cc: dsterba, linux-btrfs

On Wed, Feb 11, 2015 at 08:33:03AM +0800, Qu Wenruo wrote:
> >> Also, since only 2 patches is modified(although other part is slightly
> >> modified to match the change), to avoid mail bombing, I created the pull
> >> request on github and only send the first 2 patches with cover-letter.
> >> https://github.com/kdave/btrfs-progs/pull/5
> > Sending the changed patches only is ok (if you point me at the rest of
> > the patches), but it's not necessary to open the github pull request.
> >
> > The version to version changelogs are also stored in the commit
> > changelogs, that's a bit unexpected for a branch to be pulled.
> Oh, very sorry for this.
> I was meant to save your time, but I forgot that pull branch won't emit 
> the changelog like patches.

Pulled except the last patch, and I've cleaned up some bits so please
have a look. It's basically what I'd tell you during a normal review but
now it was easier to do myself.

My concern about the patch "btrfs-progs: Allow open_ctree use backup
tree root or search it automatically if primary..." is the
'automatically' part. Falling to the backup roots should be IMO on
request. The tools should have (and some of them already do have)
commandline options to request a given backup root. That way the user
can try the default action and then decide if the backup roots are fine
for use.

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

* Re: [PATCH v3 00/10] Enhance btrfs-find-root and open_ctree() to provide better chance on damaged btrfs.
  2015-02-11 17:52     ` David Sterba
@ 2015-02-12  1:36       ` Qu Wenruo
  2015-02-12 13:16         ` David Sterba
  0 siblings, 1 reply; 9+ messages in thread
From: Qu Wenruo @ 2015-02-12  1:36 UTC (permalink / raw)
  To: dsterba, linux-btrfs


-------- Original Message --------
Subject: Re: [PATCH v3 00/10] Enhance btrfs-find-root and open_ctree() 
to provide better chance on damaged btrfs.
From: David Sterba <dsterba@suse.cz>
To: Qu Wenruo <quwenruo@cn.fujitsu.com>
Date: 2015年02月12日 01:52
> On Wed, Feb 11, 2015 at 08:33:03AM +0800, Qu Wenruo wrote:
>>>> Also, since only 2 patches is modified(although other part is slightly
>>>> modified to match the change), to avoid mail bombing, I created the pull
>>>> request on github and only send the first 2 patches with cover-letter.
>>>> https://github.com/kdave/btrfs-progs/pull/5
>>> Sending the changed patches only is ok (if you point me at the rest of
>>> the patches), but it's not necessary to open the github pull request.
>>>
>>> The version to version changelogs are also stored in the commit
>>> changelogs, that's a bit unexpected for a branch to be pulled.
>> Oh, very sorry for this.
>> I was meant to save your time, but I forgot that pull branch won't emit
>> the changelog like patches.
> Pulled except the last patch, and I've cleaned up some bits so please
> have a look. It's basically what I'd tell you during a normal review but
> now it was easier to do myself.
Thanks for merging and modifying them.
It seems that my naming sense is not so good and the new naming looks 
good for me, except some of them,
like OPEN_CTREE_SUPPRESS_CHECK_TREE_ERROR seems too long for me, but 
that's all right and doesn't
do any harm.

And it seems that the patch I send it still out of date and some naming 
changes in my v3 patch doesn't show in
it...

Sorry for taking  your time to change them.

>
> My concern about the patch "btrfs-progs: Allow open_ctree use backup
> tree root or search it automatically if primary..." is the
> 'automatically' part. Falling to the backup roots should be IMO on
> request. The tools should have (and some of them already do have)
> commandline options to request a given backup root. That way the user
> can try the default action and then decide if the backup roots are fine
> for use.
What about ask user to do such fallback method?
IMHO, such way should take a balance for average user and advanced user 
who can, for example, extract
the bad block and fix it manually without corruption.

Current btrfsck has a problem that doesn't give enough info on possible 
solutions.
The case is much like --init-(csum/extent)-tree, we provide such 
options, but when a tree block in extent
tree happens, the backref mismatch error messages won't really help to 
guide user to use --init-extent-tree
option.

How do you think about the ask-user method?

Thanks,
Qu




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

* Re: [PATCH v3 00/10] Enhance btrfs-find-root and open_ctree() to provide better chance on damaged btrfs.
  2015-02-12  1:36       ` Qu Wenruo
@ 2015-02-12 13:16         ` David Sterba
  2015-02-13  1:34           ` Qu Wenruo
  0 siblings, 1 reply; 9+ messages in thread
From: David Sterba @ 2015-02-12 13:16 UTC (permalink / raw)
  To: Qu Wenruo; +Cc: linux-btrfs

On Thu, Feb 12, 2015 at 09:36:01AM +0800, Qu Wenruo wrote:
> Subject: Re: [PATCH v3 00/10] Enhance btrfs-find-root and open_ctree() 
> to provide better chance on damaged btrfs.
> From: David Sterba <dsterba@suse.cz>
> To: Qu Wenruo <quwenruo@cn.fujitsu.com>
> Date: 2015年02月12日 01:52
> > On Wed, Feb 11, 2015 at 08:33:03AM +0800, Qu Wenruo wrote:
> >>>> Also, since only 2 patches is modified(although other part is slightly
> >>>> modified to match the change), to avoid mail bombing, I created the pull
> >>>> request on github and only send the first 2 patches with cover-letter.
> >>>> https://github.com/kdave/btrfs-progs/pull/5
> >>> Sending the changed patches only is ok (if you point me at the rest of
> >>> the patches), but it's not necessary to open the github pull request.
> >>>
> >>> The version to version changelogs are also stored in the commit
> >>> changelogs, that's a bit unexpected for a branch to be pulled.
> >> Oh, very sorry for this.
> >> I was meant to save your time, but I forgot that pull branch won't emit
> >> the changelog like patches.
> > Pulled except the last patch, and I've cleaned up some bits so please
> > have a look. It's basically what I'd tell you during a normal review but
> > now it was easier to do myself.
> Thanks for merging and modifying them.
> It seems that my naming sense is not so good and the new naming looks 
> good for me, except some of them,
> like OPEN_CTREE_SUPPRESS_CHECK_TREE_ERROR seems too long for me, but 
> that's all right and doesn't
> do any harm.

Yeah it's long and I'm not completely happy about that but I gave
preference to descriptiveness as we may want to add other fine tuning
flags to open_ctree. As the TODO you've added to the enum definition
says, we may split the flags and then cleanup as needed.

> And it seems that the patch I send it still out of date and some naming 
> changes in my v3 patch doesn't show in
> it...

I've used the v3 branch from github. Please send incremental fixes if I
missed something.

> > My concern about the patch "btrfs-progs: Allow open_ctree use backup
> > tree root or search it automatically if primary..." is the
> > 'automatically' part. Falling to the backup roots should be IMO on
> > request. The tools should have (and some of them already do have)
> > commandline options to request a given backup root. That way the user
> > can try the default action and then decide if the backup roots are fine
> > for use.
> What about ask user to do such fallback method?
> IMHO, such way should take a balance for average user and advanced user 
> who can, for example, extract
> the bad block and fix it manually without corruption.

Agreed this is about finding a good balance. However, here a regular
user could do more harm than good if the tool just "does something" and
does not stop if there's not a safe way forward.

> Current btrfsck has a problem that doesn't give enough info on possible 
> solutions.

Right, this should be improved in the long term. This requires
documentation of the internals and some level of knowledge even if the
tool provides options how to proceed.

> The case is much like --init-(csum/extent)-tree, we provide such 
> options, but when a tree block in extent
> tree happens, the backref mismatch error messages won't really help to 
> guide user to use --init-extent-tree
> option.
> 
> How do you think about the ask-user method?

The options are:

* exit if user input/decision is required
* pass command line options to checker to preset the behaviour
* add a specialized subcommand to fix a particular class of errors
* run checker in interactive mode that asks user if she/he wants to fix
  the problem, possibly how if there are more options

The superblock and root backup options are very handy in the analysis
phase, so one can see the differences in the output. I think the same
holds for the find-root command. We don't have a "cookbook" how to
analyze a broken filesystem. Your and my experience is probably
different in how to do the analysis but I believe we will find a common
ground and implement that into the tools and documentation.

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

* Re: [PATCH v3 00/10] Enhance btrfs-find-root and open_ctree() to provide better chance on damaged btrfs.
  2015-02-12 13:16         ` David Sterba
@ 2015-02-13  1:34           ` Qu Wenruo
  0 siblings, 0 replies; 9+ messages in thread
From: Qu Wenruo @ 2015-02-13  1:34 UTC (permalink / raw)
  To: dsterba, linux-btrfs


-------- Original Message --------
Subject: Re: [PATCH v3 00/10] Enhance btrfs-find-root and open_ctree() 
to provide better chance on damaged btrfs.
From: David Sterba <dsterba@suse.cz>
To: Qu Wenruo <quwenruo@cn.fujitsu.com>
Date: 2015年02月12日 21:16
> On Thu, Feb 12, 2015 at 09:36:01AM +0800, Qu Wenruo wrote:
>> Subject: Re: [PATCH v3 00/10] Enhance btrfs-find-root and open_ctree()
>> to provide better chance on damaged btrfs.
>> From: David Sterba <dsterba@suse.cz>
>> To: Qu Wenruo <quwenruo@cn.fujitsu.com>
>> Date: 2015年02月12日 01:52
>>> On Wed, Feb 11, 2015 at 08:33:03AM +0800, Qu Wenruo wrote:
>>>>>> Also, since only 2 patches is modified(although other part is slightly
>>>>>> modified to match the change), to avoid mail bombing, I created the pull
>>>>>> request on github and only send the first 2 patches with cover-letter.
>>>>>> https://github.com/kdave/btrfs-progs/pull/5
>>>>> Sending the changed patches only is ok (if you point me at the rest of
>>>>> the patches), but it's not necessary to open the github pull request.
>>>>>
>>>>> The version to version changelogs are also stored in the commit
>>>>> changelogs, that's a bit unexpected for a branch to be pulled.
>>>> Oh, very sorry for this.
>>>> I was meant to save your time, but I forgot that pull branch won't emit
>>>> the changelog like patches.
>>> Pulled except the last patch, and I've cleaned up some bits so please
>>> have a look. It's basically what I'd tell you during a normal review but
>>> now it was easier to do myself.
>> Thanks for merging and modifying them.
>> It seems that my naming sense is not so good and the new naming looks
>> good for me, except some of them,
>> like OPEN_CTREE_SUPPRESS_CHECK_TREE_ERROR seems too long for me, but
>> that's all right and doesn't
>> do any harm.
> Yeah it's long and I'm not completely happy about that but I gave
> preference to descriptiveness as we may want to add other fine tuning
> flags to open_ctree. As the TODO you've added to the enum definition
> says, we may split the flags and then cleanup as needed.
>
>> And it seems that the patch I send it still out of date and some naming
>> changes in my v3 patch doesn't show in
>> it...
> I've used the v3 branch from github. Please send incremental fixes if I
> missed something.
The modification seems better than mine, so that's completely OK for me.
No need for other patches.
>>> My concern about the patch "btrfs-progs: Allow open_ctree use backup
>>> tree root or search it automatically if primary..." is the
>>> 'automatically' part. Falling to the backup roots should be IMO on
>>> request. The tools should have (and some of them already do have)
>>> commandline options to request a given backup root. That way the user
>>> can try the default action and then decide if the backup roots are fine
>>> for use.
>> What about ask user to do such fallback method?
>> IMHO, such way should take a balance for average user and advanced user
>> who can, for example, extract
>> the bad block and fix it manually without corruption.
> Agreed this is about finding a good balance. However, here a regular
> user could do more harm than good if the tool just "does something" and
> does not stop if there's not a safe way forward.
>
>> Current btrfsck has a problem that doesn't give enough info on possible
>> solutions.
> Right, this should be improved in the long term. This requires
> documentation of the internals and some level of knowledge even if the
> tool provides options how to proceed.
>
>> The case is much like --init-(csum/extent)-tree, we provide such
>> options, but when a tree block in extent
>> tree happens, the backref mismatch error messages won't really help to
>> guide user to use --init-extent-tree
>> option.
>>
>> How do you think about the ask-user method?
> The options are:
>
> * exit if user input/decision is required
> * pass command line options to checker to preset the behaviour
> * add a specialized subcommand to fix a particular class of errors
> * run checker in interactive mode that asks user if she/he wants to fix
>    the problem, possibly how if there are more options
Thanks for all the options.
These options are better than my original just ask_user() method.
>
> The superblock and root backup options are very handy in the analysis
> phase, so one can see the differences in the output. I think the same
> holds for the find-root command. We don't have a "cookbook" how to
> analyze a broken filesystem. Your and my experience is probably
> different in how to do the analysis but I believe we will find a common
> ground and implement that into the tools and documentation.
This makes sense, I was only focused on the btrfsck --repair work, 
forgot that we can use btrfsck and btrfs-find-root
to do analysis before we do repair.

Considering analysis, I'd like a add a prompt if we failed to read tree 
root, guiding user to use the new
option like --search-root(which will first use backup and then 
find-root) or btrfs-find-root to find the best tree root.

Thanks,
Qu

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

end of thread, other threads:[~2015-02-13  1:34 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-01-29  9:07 [PATCH v3 00/10] Enhance btrfs-find-root and open_ctree() to provide better chance on damaged btrfs Qu Wenruo
2015-01-29  9:07 ` [PATCH v3 01/10] btrfs-progs: Cleanup check_tree_block() function and split the output to print_tree_block_err() function Qu Wenruo
2015-01-29  9:07 ` [PATCH v3 02/10] btrfs-progs: Add support to suppress tree block csum error output Qu Wenruo
2015-02-10 14:48 ` [PATCH v3 00/10] Enhance btrfs-find-root and open_ctree() to provide better chance on damaged btrfs David Sterba
2015-02-11  0:33   ` Qu Wenruo
2015-02-11 17:52     ` David Sterba
2015-02-12  1:36       ` Qu Wenruo
2015-02-12 13:16         ` David Sterba
2015-02-13  1:34           ` 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).