diff mbox

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

Message ID 1401276041-18349-1-git-send-email-wangsl.fnst@cn.fujitsu.com
State Superseded, archived
Delegated to: David Sterba
Headers show

Commit Message

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

Comments

Eric Sandeen May 28, 2014, 1:56 p.m. UTC | #1
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");
> 

--
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
Wang Shilong May 28, 2014, 2:10 p.m. UTC | #2
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
--
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
Eric Sandeen May 28, 2014, 3:10 p.m. UTC | #3
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
> 

--
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
David Sterba June 2, 2014, 4:06 p.m. UTC | #4
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");
--
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
diff mbox

Patch

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");