[2/2] btrfs: Add error string for EUCLEAN
diff mbox series

Message ID 20190406031426.22436-2-wqu@suse.com
State New
Headers show
Series
  • [1/2] block: Add new BLK_STS_SELFTEST status
Related show

Commit Message

Qu Wenruo April 6, 2019, 3:14 a.m. UTC
Since we're going to support write time tree checker, it's possible that
transaction get aborted due to tree-checker, also due to new
BLK_STS_SELFTEST bit, we can distinguish real EIO error and EUCLEAN
error.

Now add error string for EUCLEAN too.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/super.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Darrick J. Wong April 6, 2019, 4:50 a.m. UTC | #1
On Sat, Apr 06, 2019 at 11:14:26AM +0800, Qu Wenruo wrote:
> Since we're going to support write time tree checker, it's possible that
> transaction get aborted due to tree-checker, also due to new
> BLK_STS_SELFTEST bit, we can distinguish real EIO error and EUCLEAN
> error.
> 
> Now add error string for EUCLEAN too.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  fs/btrfs/super.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> index 120e4340792a..a4cbbd7861d3 100644
> --- a/fs/btrfs/super.c
> +++ b/fs/btrfs/super.c
> @@ -86,6 +86,9 @@ const char *btrfs_decode_error(int errno)
>  	case -ENOENT:
>  		errstr = "No such entry";
>  		break;
> +	case -EUCLEAN:
> +		errstr = "Selftest failure";
> +		break;

EUCLEAN usually spits out "Structure needs cleaning" in userspace (and
ext4 & xfs have been using it for years to complain about corrupt data),
so why diverge here?

Wait... does "tree-checker" mean online metadata checking, and 'selftest
failure' is what gets spit out when it finds some metadata it doesn't
like?

--D

>  	}
>  
>  	return errstr;
> -- 
> 2.21.0
>
Qu Wenruo April 6, 2019, 5:05 a.m. UTC | #2
On 2019/4/6 下午12:50, Darrick J. Wong wrote:
> On Sat, Apr 06, 2019 at 11:14:26AM +0800, Qu Wenruo wrote:
>> Since we're going to support write time tree checker, it's possible that
>> transaction get aborted due to tree-checker, also due to new
>> BLK_STS_SELFTEST bit, we can distinguish real EIO error and EUCLEAN
>> error.
>>
>> Now add error string for EUCLEAN too.
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>  fs/btrfs/super.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
>> index 120e4340792a..a4cbbd7861d3 100644
>> --- a/fs/btrfs/super.c
>> +++ b/fs/btrfs/super.c
>> @@ -86,6 +86,9 @@ const char *btrfs_decode_error(int errno)
>>  	case -ENOENT:
>>  		errstr = "No such entry";
>>  		break;
>> +	case -EUCLEAN:
>> +		errstr = "Selftest failure";
>> +		break;
> 
> EUCLEAN usually spits out "Structure needs cleaning" in userspace (and
> ext4 & xfs have been using it for years to complain about corrupt data),
> so why diverge here?

Because the original "structure needs cleaning" doesn't really show the
meaning we're using.

We really means, something wrong happened, thus I prefer something like
selftest failure.

And, the idea of adding -EUCLEAN, is to replace the original "unknown"
output, if you're sticking with the original error string, we still need
that branch.

> 
> Wait... does "tree-checker" mean online metadata checking, and 'selftest
> failure' is what gets spit out when it finds some metadata it doesn't
> like?

Yep.

Thanks,
Qu

> 
> --D
> 
>>  	}
>>  
>>  	return errstr;
>> -- 
>> 2.21.0
>>
Nikolay Borisov April 6, 2019, 6:20 a.m. UTC | #3
On 6.04.19 г. 8:05 ч., Qu Wenruo wrote:
> 
> 
> On 2019/4/6 下午12:50, Darrick J. Wong wrote:
>> On Sat, Apr 06, 2019 at 11:14:26AM +0800, Qu Wenruo wrote:
>>> Since we're going to support write time tree checker, it's possible that
>>> transaction get aborted due to tree-checker, also due to new
>>> BLK_STS_SELFTEST bit, we can distinguish real EIO error and EUCLEAN
>>> error.
>>>
>>> Now add error string for EUCLEAN too.
>>>
>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>> ---
>>>  fs/btrfs/super.c | 3 +++
>>>  1 file changed, 3 insertions(+)
>>>
>>> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
>>> index 120e4340792a..a4cbbd7861d3 100644
>>> --- a/fs/btrfs/super.c
>>> +++ b/fs/btrfs/super.c
>>> @@ -86,6 +86,9 @@ const char *btrfs_decode_error(int errno)
>>>  	case -ENOENT:
>>>  		errstr = "No such entry";
>>>  		break;
>>> +	case -EUCLEAN:
>>> +		errstr = "Selftest failure";
>>> +		break;
>>
>> EUCLEAN usually spits out "Structure needs cleaning" in userspace (and
>> ext4 & xfs have been using it for years to complain about corrupt data),
>> so why diverge here?
> 
> Because the original "structure needs cleaning" doesn't really show the
> meaning we're using.

On the contrary, structure needs cleaning means the filesystem has
detected a consistency error. How that detection has happened shouldn't
burden the user, the important fact is it has. Selftest is somewhat
cryptic, I'd suggest at least "verification error" or just go with
canonical "Structure needs cleaning".

> 
> We really means, something wrong happened, thus I prefer something like
> selftest failure.
> 
> And, the idea of adding -EUCLEAN, is to replace the original "unknown"
> output, if you're sticking with the original error string, we still need
> that branch.
> 
>>
>> Wait... does "tree-checker" mean online metadata checking, and 'selftest
>> failure' is what gets spit out when it finds some metadata it doesn't
>> like?
> 
> Yep.
> 
> Thanks,
> Qu
> 
>>
>> --D
>>
>>>  	}
>>>  
>>>  	return errstr;
>>> -- 
>>> 2.21.0
>>>
>
Qu Wenruo April 6, 2019, 7:56 a.m. UTC | #4
On 2019/4/6 下午2:20, Nikolay Borisov wrote:
>
>
> On 6.04.19 г. 8:05 ч., Qu Wenruo wrote:
>>
>>
>> On 2019/4/6 下午12:50, Darrick J. Wong wrote:
>>> On Sat, Apr 06, 2019 at 11:14:26AM +0800, Qu Wenruo wrote:
>>>> Since we're going to support write time tree checker, it's possible that
>>>> transaction get aborted due to tree-checker, also due to new
>>>> BLK_STS_SELFTEST bit, we can distinguish real EIO error and EUCLEAN
>>>> error.
>>>>
>>>> Now add error string for EUCLEAN too.
>>>>
>>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>>> ---
>>>>  fs/btrfs/super.c | 3 +++
>>>>  1 file changed, 3 insertions(+)
>>>>
>>>> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
>>>> index 120e4340792a..a4cbbd7861d3 100644
>>>> --- a/fs/btrfs/super.c
>>>> +++ b/fs/btrfs/super.c
>>>> @@ -86,6 +86,9 @@ const char *btrfs_decode_error(int errno)
>>>>  	case -ENOENT:
>>>>  		errstr = "No such entry";
>>>>  		break;
>>>> +	case -EUCLEAN:
>>>> +		errstr = "Selftest failure";
>>>> +		break;
>>>
>>> EUCLEAN usually spits out "Structure needs cleaning" in userspace (and
>>> ext4 & xfs have been using it for years to complain about corrupt data),
>>> so why diverge here?
>>
>> Because the original "structure needs cleaning" doesn't really show the
>> meaning we're using.
>
> On the contrary, structure needs cleaning means the filesystem has
> detected a consistency error. How that detection has happened shouldn't
> burden the user, the important fact is it has. Selftest is somewhat
> cryptic, I'd suggest at least "verification error" or just go with
> canonical "Structure needs cleaning".

OK, I'll go that way.

Thanks for the hint,
Qu

>
>>
>> We really means, something wrong happened, thus I prefer something like
>> selftest failure.
>>
>> And, the idea of adding -EUCLEAN, is to replace the original "unknown"
>> output, if you're sticking with the original error string, we still need
>> that branch.
>>
>>>
>>> Wait... does "tree-checker" mean online metadata checking, and 'selftest
>>> failure' is what gets spit out when it finds some metadata it doesn't
>>> like?
>>
>> Yep.
>>
>> Thanks,
>> Qu
>>
>>>
>>> --D
>>>
>>>>  	}
>>>>
>>>>  	return errstr;
>>>> --
>>>> 2.21.0
>>>>
>>

Patch
diff mbox series

diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 120e4340792a..a4cbbd7861d3 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -86,6 +86,9 @@  const char *btrfs_decode_error(int errno)
 	case -ENOENT:
 		errstr = "No such entry";
 		break;
+	case -EUCLEAN:
+		errstr = "Selftest failure";
+		break;
 	}
 
 	return errstr;