[PATCH] btrfs-progs: Continue checking even we found something wrong in free space cache

[PATCH] btrfs-progs: Continue checking even we found something wrong in free space cache

[Qu Wenruo]

No need to abort checking, especially for RO check free space cache is
meaningless, the errors in fs/extent tree is more interesting for
developers.

So continue checking even something in free space cache is wrong.

Reported-by: Etienne Champetier <champetier.etienne@gmail.com>
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 check/main.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/check/main.c b/check/main.c
index b361cd7e26a0..4f720163221e 100644
--- a/check/main.c
+++ b/check/main.c
@@ -9885,7 +9885,6 @@ int cmd_check(int argc, char **argv)
 			error("errors found in free space tree");
 		else
 			error("errors found in free space cache");
-		goto out;
 	}
 
 	/*
-- 
2.18.0

Re: [PATCH] btrfs-progs: Continue checking even we found something wrong in free space cache

[David Sterba]

On Tue, Sep 04, 2018 at 08:41:28PM +0800, Qu Wenruo wrote:
> No need to abort checking, especially for RO check free space cache is
> meaningless, the errors in fs/extent tree is more interesting for
> developers.
> 
> So continue checking even something in free space cache is wrong.
> 
> Reported-by: Etienne Champetier <champetier.etienne@gmail.com>
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  check/main.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/check/main.c b/check/main.c
> index b361cd7e26a0..4f720163221e 100644
> --- a/check/main.c
> +++ b/check/main.c
> @@ -9885,7 +9885,6 @@ int cmd_check(int argc, char **argv)
>  			error("errors found in free space tree");
>  		else
>  			error("errors found in free space cache");

Please make it a warning and update the message so it says that it will
continue despite the errors found.

I noticed that error handling in check_space_cache is a bit fishy, some
errors are stored to 'ret' but ignored at the end of the function.

Re: [PATCH] btrfs-progs: Continue checking even we found something wrong in free space cache

[Qu Wenruo]

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



On 2018/9/11 下午10:48, David Sterba wrote:
> On Tue, Sep 04, 2018 at 08:41:28PM +0800, Qu Wenruo wrote:
>> No need to abort checking, especially for RO check free space cache is
>> meaningless, the errors in fs/extent tree is more interesting for
>> developers.
>>
>> So continue checking even something in free space cache is wrong.
>>
>> Reported-by: Etienne Champetier <champetier.etienne@gmail.com>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>  check/main.c | 1 -
>>  1 file changed, 1 deletion(-)
>>
>> diff --git a/check/main.c b/check/main.c
>> index b361cd7e26a0..4f720163221e 100644
>> --- a/check/main.c
>> +++ b/check/main.c
>> @@ -9885,7 +9885,6 @@ int cmd_check(int argc, char **argv)
>>  			error("errors found in free space tree");
>>  		else
>>  			error("errors found in free space cache");
> 
> Please make it a warning and update the message so it says that it will
> continue despite the errors found.

I'm fine to add warning, but isn't it the expected behavior?

In fact I'm quite surprised that when we found something wrong we just
abort checking.

It's never the case for file/extent tree check. We always report all
errors we found, not only the first error and exit.

Thanks,
Qu

> 
> I noticed that error handling in check_space_cache is a bit fishy, some
> errors are stored to 'ret' but ignored at the end of the function.
> 


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

Re: [PATCH] btrfs-progs: Continue checking even we found something wrong in free space cache

[David Sterba]

On Wed, Sep 12, 2018 at 07:43:27AM +0800, Qu Wenruo wrote:
> 
> 
> On 2018/9/11 下午10:48, David Sterba wrote:
> > On Tue, Sep 04, 2018 at 08:41:28PM +0800, Qu Wenruo wrote:
> >> No need to abort checking, especially for RO check free space cache is
> >> meaningless, the errors in fs/extent tree is more interesting for
> >> developers.
> >>
> >> So continue checking even something in free space cache is wrong.
> >>
> >> Reported-by: Etienne Champetier <champetier.etienne@gmail.com>
> >> Signed-off-by: Qu Wenruo <wqu@suse.com>
> >> ---
> >>  check/main.c | 1 -
> >>  1 file changed, 1 deletion(-)
> >>
> >> diff --git a/check/main.c b/check/main.c
> >> index b361cd7e26a0..4f720163221e 100644
> >> --- a/check/main.c
> >> +++ b/check/main.c
> >> @@ -9885,7 +9885,6 @@ int cmd_check(int argc, char **argv)
> >>  			error("errors found in free space tree");
> >>  		else
> >>  			error("errors found in free space cache");
> > 
> > Please make it a warning and update the message so it says that it will
> > continue despite the errors found.
> 
> I'm fine to add warning, but isn't it the expected behavior?
> 
> In fact I'm quite surprised that when we found something wrong we just
> abort checking.
> 
> It's never the case for file/extent tree check. We always report all
> errors we found, not only the first error and exit.

You're right, in context of 'check' this is the expected behaviour. So
let's keep error().