[PATCH 1/4] Btrfs-progs: fsck: only allow partial opening under repair mode

[PATCH 1/4] Btrfs-progs: fsck: only allow partial opening under repair mode

[Wang Shilong]

Signed-off-by: Wang Shilong <wangsl.fnst@cn.fujitsu.com>
---
 cmds-check.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/cmds-check.c b/cmds-check.c
index db7df80..0e4e042 100644
--- a/cmds-check.c
+++ b/cmds-check.c
@@ -6810,8 +6810,7 @@ int cmd_check(int argc, char **argv)
 	int option_index = 0;
 	int init_csum_tree = 0;
 	int qgroup_report = 0;
-	enum btrfs_open_ctree_flags ctree_flags =
-		OPEN_CTREE_PARTIAL | OPEN_CTREE_EXCLUSIVE;
+	enum btrfs_open_ctree_flags ctree_flags = OPEN_CTREE_EXCLUSIVE;
 
 	while(1) {
 		int c;
@@ -6877,6 +6876,10 @@ int cmd_check(int argc, char **argv)
 		goto err_out;
 	}
 
+	/* only allow partial opening under repair mode */
+	if (repair)
+		ctree_flags |= OPEN_CTREE_PARTIAL;
+
 	info = open_ctree_fs_info(argv[optind], bytenr, 0, ctree_flags);
 	if (!info) {
 		fprintf(stderr, "Couldn't open file system\n");
-- 
1.9.0

[PATCH 2/4] Btrfs-progs: fsck: disallow partial opening if critical roots corrupted

[Wang Shilong]

If btrfs tree root is corrupted, fsck will hit the following segmentation.

enabling repair mode
Check tree block failed, want=29376512, have=0
Check tree block failed, want=29376512, have=0
Check tree block failed, want=29376512, have=0
Check tree block failed, want=29376512, have=0
Check tree block failed, want=29376512, have=0
read block failed check_tree_block
Couldn't read tree root
Checking filesystem on /dev/sda9
UUID: 0e1a754d-04a5-4256-ae79-0f769751803e
Critical roots corrupted, unable to fsck the FS
Segmentation fault (core dumped)

In btrfs_setup_all_roots(), we could tolerate some trees(extent tree, csum tree)
corrupted, and we have did careful check inside that function, it will
return NULL if critial roots corrupt(for example tree root).

The problem is that we check @OPEN_CTREE_PARTIAL flag again after
calling btrfs_setup_all_roots() which will successfully return
@fs_info though critial roots corrupted.

Fix this problem by removing @OPEN_CTREE_PARTIAL flag check outsize
btrfs_setup_all_roots().

Signed-off-by: Wang Shilong <wangsl.fnst@cn.fujitsu.com>
---
 disk-io.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/disk-io.c b/disk-io.c
index 58f3f07..63e153d 100644
--- a/disk-io.c
+++ b/disk-io.c
@@ -1134,13 +1134,10 @@ static struct btrfs_fs_info *__open_ctree_fd(int fp, const char *path,
 
 	ret = btrfs_setup_all_roots(fs_info, root_tree_bytenr, flags);
 	if (ret)
-		goto out_failed;
+		goto out_chunk;
 
 	return fs_info;
 
-out_failed:
-	if (flags & OPEN_CTREE_PARTIAL)
-		return fs_info;
 out_chunk:
 	btrfs_release_all_roots(fs_info);
 	btrfs_cleanup_all_caches(fs_info);
-- 
1.9.0

[PATCH 3/4] Btrfs-progs: fsck: deal with corrupted csum root

[Wang Shilong]

If checksum root is corrupted, fsck will get segmentation. This
is because if we fail to load checksum root, root's node is NULL which
cause NULL pointer deferences later.

To fix this problem, we just did something like extent tree rebuilding.
Allocate a new one and clear uptodate flag. We will do sanity check
before fsck going on.

Signed-off-by: Wang Shilong <wangsl.fnst@cn.fujitsu.com>
---
 cmds-check.c | 5 +++++
 disk-io.c    | 7 +++++++
 2 files changed, 12 insertions(+)

diff --git a/cmds-check.c b/cmds-check.c
index 0e4e042..55e7753 100644
--- a/cmds-check.c
+++ b/cmds-check.c
@@ -6963,6 +6963,11 @@ int cmd_check(int argc, char **argv)
 		ret = -EIO;
 		goto close_out;
 	}
+	if (!extent_buffer_uptodate(info->csum_root->node)) {
+		fprintf(stderr, "Checksum root corrupted, run to rerun with --init-csum-tree option\n");
+		ret = -EIO;
+		goto close_out;
+	}
 
 	fprintf(stderr, "checking extents\n");
 	ret = check_chunks_and_extents(root);
diff --git a/disk-io.c b/disk-io.c
index 63e153d..bbfd8e7 100644
--- a/disk-io.c
+++ b/disk-io.c
@@ -914,6 +914,13 @@ int btrfs_setup_all_roots(struct btrfs_fs_info *fs_info, u64 root_tree_bytenr,
 		printk("Couldn't setup csum tree\n");
 		if (!(flags & OPEN_CTREE_PARTIAL))
 			return -EIO;
+		/* do the same thing as extent tree rebuilding */
+		fs_info->csum_root->node =
+			btrfs_find_create_tree_block(fs_info->extent_root, 0,
+						     leafsize);
+		if (!fs_info->csum_root->node)
+			return -ENOMEM;
+		clear_extent_buffer_uptodate(NULL, fs_info->csum_root->node);
 	}
 	fs_info->csum_root->track_dirty = 1;
 
-- 
1.9.0

[PATCH 4/4] Btrfs-progs: fsck: fix wrong check for btrfs_read_fs_root()

[Wang Shilong]

When encountering a corrupted fs root node, fsck hit following message:

Check tree block failed, want=29360128, have=0
Check tree block failed, want=29360128, have=0
Check tree block failed, want=29360128, have=0
Check tree block failed, want=29360128, have=0
Check tree block failed, want=29360128, have=0
read block failed check_tree_block
Checking filesystem on /dev/sda9
UUID: 0d295d80-bae2-45f2-a106-120dbfd0e173
checking extents
Segmentation fault (core dumped)

This is because in btrfs_setup_all_roots(), we check
btrfs_read_fs_root() return value by verifing whether it is
NULL pointer, this is wrong since btrfs_read_fs_root() return
PTR_ERR(ret), fix it.

Signed-off-by: Wang Shilong <wangsl.fnst@cn.fujitsu.com>
---
 btrfs-calc-size.c | 2 +-
 disk-io.c         | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/btrfs-calc-size.c b/btrfs-calc-size.c
index a832ee0..5eabdfc 100644
--- a/btrfs-calc-size.c
+++ b/btrfs-calc-size.c
@@ -326,7 +326,7 @@ static int calc_root_size(struct btrfs_root *tree_root, struct btrfs_key *key,
 	int size_fail = 0;
 
 	root = btrfs_read_fs_root(tree_root->fs_info, key);
-	if (!root) {
+	if (IS_ERR(root)) {
 		fprintf(stderr, "Failed to read root %Lu\n", key->objectid);
 		return 1;
 	}
diff --git a/disk-io.c b/disk-io.c
index bbfd8e7..c1c5b70 100644
--- a/disk-io.c
+++ b/disk-io.c
@@ -946,7 +946,7 @@ int btrfs_setup_all_roots(struct btrfs_fs_info *fs_info, u64 root_tree_bytenr,
 	key.offset = (u64)-1;
 	fs_info->fs_root = btrfs_read_fs_root(fs_info, &key);
 
-	if (!fs_info->fs_root)
+	if (IS_ERR(fs_info->fs_root))
 		return -EIO;
 	return 0;
 }
-- 
1.9.0

Re: [PATCH 1/4] Btrfs-progs: fsck: only allow partial opening under repair mode

[Eric Sandeen]

The subject and the comment say what this change does, but
that's obvious from reading the code.  Nothing says *why*
the change has been made.  What does this fix, and how does
it fix it?

Can you add/update the commit log so that some reader in the future
(or for that matter, a reviewer in the present) will have an idea
about the reason for this change?  What was the failure case, what
was the failure mode, why does this change fix it, etc.

Thanks,
-Eric 

On 5/28/14, 6:20 AM, Wang Shilong wrote:
> Signed-off-by: Wang Shilong <wangsl.fnst@cn.fujitsu.com>
> ---
>  cmds-check.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/cmds-check.c b/cmds-check.c
> index db7df80..0e4e042 100644
> --- a/cmds-check.c
> +++ b/cmds-check.c
> @@ -6810,8 +6810,7 @@ int cmd_check(int argc, char **argv)
>  	int option_index = 0;
>  	int init_csum_tree = 0;
>  	int qgroup_report = 0;
> -	enum btrfs_open_ctree_flags ctree_flags =
> -		OPEN_CTREE_PARTIAL | OPEN_CTREE_EXCLUSIVE;
> +	enum btrfs_open_ctree_flags ctree_flags = OPEN_CTREE_EXCLUSIVE;
>  
>  	while(1) {
>  		int c;
> @@ -6877,6 +6876,10 @@ int cmd_check(int argc, char **argv)
>  		goto err_out;
>  	}
>  
> +	/* only allow partial opening under repair mode */
> +	if (repair)
> +		ctree_flags |= OPEN_CTREE_PARTIAL;
> +
>  	info = open_ctree_fs_info(argv[optind], bytenr, 0, ctree_flags);
>  	if (!info) {
>  		fprintf(stderr, "Couldn't open file system\n");
> 

Re: [PATCH 1/4] Btrfs-progs: fsck: only allow partial opening under repair mode

[Shilong Wang]

2014-05-28 21:56 GMT+08:00 Eric Sandeen <sandeen@redhat.com>:
> The subject and the comment say what this change does, but
> that's obvious from reading the code.  Nothing says *why*
> the change has been made.  What does this fix, and how does
> it fix it?

Yup, the reason that we allow partial opening is that sometimes,
we may have a corrupted extent tree(for example), but for fsck repair case,
the broken tree maybe rebuilt.

So if users only want to do check but not repaired, this patch will make fsck
return failure as soon as possible and tell users that some critial roots have
been corrupted...

Let's come to your comments, Eric, you are absolutely right, i was a little
lazy sometimes...

I would add necessay changelog to describe why we need this patch,
thanks for your comments.

Regards,
Wang
>
> Can you add/update the commit log so that some reader in the future
> (or for that matter, a reviewer in the present) will have an idea
> about the reason for this change?  What was the failure case, what
> was the failure mode, why does this change fix it, etc.
>
> Thanks,
> -Eric
>
> On 5/28/14, 6:20 AM, Wang Shilong wrote:
>> Signed-off-by: Wang Shilong <wangsl.fnst@cn.fujitsu.com>
>> ---
>>  cmds-check.c | 7 +++++--
>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/cmds-check.c b/cmds-check.c
>> index db7df80..0e4e042 100644
>> --- a/cmds-check.c
>> +++ b/cmds-check.c
>> @@ -6810,8 +6810,7 @@ int cmd_check(int argc, char **argv)
>>       int option_index = 0;
>>       int init_csum_tree = 0;
>>       int qgroup_report = 0;
>> -     enum btrfs_open_ctree_flags ctree_flags =
>> -             OPEN_CTREE_PARTIAL | OPEN_CTREE_EXCLUSIVE;
>> +     enum btrfs_open_ctree_flags ctree_flags = OPEN_CTREE_EXCLUSIVE;
>>
>>       while(1) {
>>               int c;
>> @@ -6877,6 +6876,10 @@ int cmd_check(int argc, char **argv)
>>               goto err_out;
>>       }
>>
>> +     /* only allow partial opening under repair mode */
>> +     if (repair)
>> +             ctree_flags |= OPEN_CTREE_PARTIAL;
>> +
>>       info = open_ctree_fs_info(argv[optind], bytenr, 0, ctree_flags);
>>       if (!info) {
>>               fprintf(stderr, "Couldn't open file system\n");
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] Btrfs-progs: fsck: only allow partial opening under repair mode

[Eric Sandeen]

On 5/28/14, 9:10 AM, Shilong Wang wrote:
> 2014-05-28 21:56 GMT+08:00 Eric Sandeen <sandeen@redhat.com>:
>> The subject and the comment say what this change does, but
>> that's obvious from reading the code.  Nothing says *why*
>> the change has been made.  What does this fix, and how does
>> it fix it?
> 
> Yup, the reason that we allow partial opening is that sometimes,
> we may have a corrupted extent tree(for example), but for fsck repair case,
> the broken tree maybe rebuilt.
> 
> So if users only want to do check but not repaired, this patch will make fsck
> return failure as soon as possible and tell users that some critial roots have
> been corrupted...
> 
> Let's come to your comments, Eric, you are absolutely right, i was a little
> lazy sometimes...

Sometimes I am too.  ;)

> I would add necessay changelog to describe why we need this patch,
> thanks for your comments.

thanks.  Perhaps it's obvious to people more familiar with the code, but
I think it's always helpful to be a bit more descriptive; things that make
perfect sense right now can sometimes be confusing a year or so later.

Thanks,
-Eric

> Regards,
> Wang
>>
>> Can you add/update the commit log so that some reader in the future
>> (or for that matter, a reviewer in the present) will have an idea
>> about the reason for this change?  What was the failure case, what
>> was the failure mode, why does this change fix it, etc.
>>
>> Thanks,
>> -Eric
>>
>> On 5/28/14, 6:20 AM, Wang Shilong wrote:
>>> Signed-off-by: Wang Shilong <wangsl.fnst@cn.fujitsu.com>
>>> ---
>>>  cmds-check.c | 7 +++++--
>>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/cmds-check.c b/cmds-check.c
>>> index db7df80..0e4e042 100644
>>> --- a/cmds-check.c
>>> +++ b/cmds-check.c
>>> @@ -6810,8 +6810,7 @@ int cmd_check(int argc, char **argv)
>>>       int option_index = 0;
>>>       int init_csum_tree = 0;
>>>       int qgroup_report = 0;
>>> -     enum btrfs_open_ctree_flags ctree_flags =
>>> -             OPEN_CTREE_PARTIAL | OPEN_CTREE_EXCLUSIVE;
>>> +     enum btrfs_open_ctree_flags ctree_flags = OPEN_CTREE_EXCLUSIVE;
>>>
>>>       while(1) {
>>>               int c;
>>> @@ -6877,6 +6876,10 @@ int cmd_check(int argc, char **argv)
>>>               goto err_out;
>>>       }
>>>
>>> +     /* only allow partial opening under repair mode */
>>> +     if (repair)
>>> +             ctree_flags |= OPEN_CTREE_PARTIAL;
>>> +
>>>       info = open_ctree_fs_info(argv[optind], bytenr, 0, ctree_flags);
>>>       if (!info) {
>>>               fprintf(stderr, "Couldn't open file system\n");
>>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 1/4] Btrfs-progs: fsck: only allow partial opening under repair mode

[David Sterba]

On Wed, May 28, 2014 at 07:20:38PM +0800, Wang Shilong wrote:
> --- a/cmds-check.c
> +++ b/cmds-check.c
> @@ -6810,8 +6810,7 @@ int cmd_check(int argc, char **argv)
>  	int option_index = 0;
>  	int init_csum_tree = 0;
>  	int qgroup_report = 0;
> -	enum btrfs_open_ctree_flags ctree_flags =
> -		OPEN_CTREE_PARTIAL | OPEN_CTREE_EXCLUSIVE;
> +	enum btrfs_open_ctree_flags ctree_flags = OPEN_CTREE_EXCLUSIVE;
>  
>  	while(1) {
>  		int c;
> @@ -6877,6 +6876,10 @@ int cmd_check(int argc, char **argv)
>  		goto err_out;
>  	}
>  
> +	/* only allow partial opening under repair mode */
> +	if (repair)
> +		ctree_flags |= OPEN_CTREE_PARTIAL;

I'm curious why. The usual way is to run fsck, look at errors and call
with --repair eventually, expecting the repair mode do fix what's
fixable.

Now this would not return the same set of errors in the non-repair mode?

This of course depends on the damage of the filesystem, but I think we
should try to let it continue as far as possible and then stop. This
probably means extra checks of the data structures before use, but this
a good pattern for fsck anyway.

> +
>  	info = open_ctree_fs_info(argv[optind], bytenr, 0, ctree_flags);
>  	if (!info) {
>  		fprintf(stderr, "Couldn't open file system\n");

Re: [PATCH 2/4] Btrfs-progs: fsck: disallow partial opening if critical roots corrupted

[Qu Wenruo]

-------- Original Message --------
Subject: [PATCH 2/4] Btrfs-progs: fsck: disallow partial opening if 
critical roots corrupted
From: Wang Shilong <wangsl.fnst@cn.fujitsu.com>
To: linux-btrfs@vger.kernel.org
Date: 2014年05月28日 19:20
> If btrfs tree root is corrupted, fsck will hit the following segmentation.
>
> enabling repair mode
> Check tree block failed, want=29376512, have=0
> Check tree block failed, want=29376512, have=0
> Check tree block failed, want=29376512, have=0
> Check tree block failed, want=29376512, have=0
> Check tree block failed, want=29376512, have=0
> read block failed check_tree_block
> Couldn't read tree root
> Checking filesystem on /dev/sda9
> UUID: 0e1a754d-04a5-4256-ae79-0f769751803e
> Critical roots corrupted, unable to fsck the FS
> Segmentation fault (core dumped)
>
> In btrfs_setup_all_roots(), we could tolerate some trees(extent tree, csum tree)
> corrupted, and we have did careful check inside that function, it will
> return NULL if critial roots corrupt(for example tree root).
>
> The problem is that we check @OPEN_CTREE_PARTIAL flag again after
> calling btrfs_setup_all_roots() which will successfully return
> @fs_info though critial roots corrupted.
>
> Fix this problem by removing @OPEN_CTREE_PARTIAL flag check outsize
> btrfs_setup_all_roots().
>
> Signed-off-by: Wang Shilong <wangsl.fnst@cn.fujitsu.com>
> ---
>   disk-io.c | 5 +----
>   1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/disk-io.c b/disk-io.c
> index 58f3f07..63e153d 100644
> --- a/disk-io.c
> +++ b/disk-io.c
> @@ -1134,13 +1134,10 @@ static struct btrfs_fs_info *__open_ctree_fd(int fp, const char *path,
>   
>   	ret = btrfs_setup_all_roots(fs_info, root_tree_bytenr, flags);
>   	if (ret)
> -		goto out_failed;
> +		goto out_chunk;
>   
>   	return fs_info;
>   
> -out_failed:
> -	if (flags & OPEN_CTREE_PARTIAL)
> -		return fs_info;
>   out_chunk:
>   	btrfs_release_all_roots(fs_info);
>   	btrfs_cleanup_all_caches(fs_info);
Tested-by: Qu Wenruo <quwenruo@cn.fujitsu.com>

This patch fixed a lot of segfault when checking fsfuzzed btrfs image.

Thanks,
Qu