[PATCH 1/2] block: Add new BLK_STS_SELFTEST status

[PATCH 1/2] block: Add new BLK_STS_SELFTEST status

[Qu Wenruo]

There are a lot of end_io hooks which can do some self test at
read/write time.

Normally such selftest should return -EUCLEAN to indicate something
unexpected.

However there is no BLK_STS_* bit for that, this makes every selftest
error to be interpreted to EIO, which lowers the severity.

This patch will add a new BLK_STS_SELFTEST, to allow the error from
endio to be passed to filesystem.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 block/blk-core.c          | 1 +
 include/linux/blk_types.h | 3 +++
 2 files changed, 4 insertions(+)

diff --git a/block/blk-core.c b/block/blk-core.c
index 4673ebe42255..66967642a5ef 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -135,6 +135,7 @@ static const struct {
 	[BLK_STS_RESOURCE]	= { -ENOMEM,	"kernel resource" },
 	[BLK_STS_DEV_RESOURCE]	= { -EBUSY,	"device resource" },
 	[BLK_STS_AGAIN]		= { -EAGAIN,	"nonblocking retry" },
+	[BLK_STS_SELFTEST]	= { -EUCLEAN,	"filesystem selftest" },
 
 	/* device mapper special case, should not leak out: */
 	[BLK_STS_DM_REQUEUE]	= { -EREMCHG, "dm internal retry" },
diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
index 791fee35df88..5bba24c3753c 100644
--- a/include/linux/blk_types.h
+++ b/include/linux/blk_types.h
@@ -63,6 +63,9 @@ typedef u8 __bitwise blk_status_t;
  */
 #define BLK_STS_DEV_RESOURCE	((__force blk_status_t)13)
 
+/* Filesystem selftest error */
+#define BLK_STS_SELFTEST	((__force blk_status_t)14)
+
 /**
  * blk_path_error - returns true if error may be path related
  * @error: status the request was completed with
-- 
2.21.0

[PATCH 2/2] btrfs: Add error string for EUCLEAN

[Qu Wenruo]

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;
 	}
 
 	return errstr;
-- 
2.21.0

Re: [PATCH 2/2] btrfs: Add error string for EUCLEAN

[Darrick J. Wong]

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
> 

Re: [PATCH 2/2] btrfs: Add error string for EUCLEAN

[Qu Wenruo]

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



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
>>


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

Re: [PATCH 2/2] btrfs: Add error string for EUCLEAN

[Nikolay Borisov]

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
>>>
> 

Re: [PATCH 2/2] btrfs: Add error string for EUCLEAN

[Qu Wenruo]

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
>>>>
>>

Re: [PATCH 1/2] block: Add new BLK_STS_SELFTEST status

[Nikolay Borisov]

On 6.04.19 г. 6:14 ч., Qu Wenruo wrote:
> There are a lot of end_io hooks which can do some self test at
> read/write time.
> 
> Normally such selftest should return -EUCLEAN to indicate something
> unexpected.
> 
> However there is no BLK_STS_* bit for that, this makes every selftest
> error to be interpreted to EIO, which lowers the severity.
> 
> This patch will add a new BLK_STS_SELFTEST, to allow the error from
> endio to be passed to filesystem.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  block/blk-core.c          | 1 +
>  include/linux/blk_types.h | 3 +++
>  2 files changed, 4 insertions(+)
> 
> diff --git a/block/blk-core.c b/block/blk-core.c
> index 4673ebe42255..66967642a5ef 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -135,6 +135,7 @@ static const struct {
>  	[BLK_STS_RESOURCE]	= { -ENOMEM,	"kernel resource" },
>  	[BLK_STS_DEV_RESOURCE]	= { -EBUSY,	"device resource" },
>  	[BLK_STS_AGAIN]		= { -EAGAIN,	"nonblocking retry" },
> +	[BLK_STS_SELFTEST]	= { -EUCLEAN,	"filesystem selftest" },

SELFTEST is ambiguous, I personally relate it to the selftests that some
subsystems have. This should be changed to STS_CLEANUP or STS_CORRUPT or
some such.

>  
>  	/* device mapper special case, should not leak out: */
>  	[BLK_STS_DM_REQUEUE]	= { -EREMCHG, "dm internal retry" },
> diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
> index 791fee35df88..5bba24c3753c 100644
> --- a/include/linux/blk_types.h
> +++ b/include/linux/blk_types.h
> @@ -63,6 +63,9 @@ typedef u8 __bitwise blk_status_t;
>   */
>  #define BLK_STS_DEV_RESOURCE	((__force blk_status_t)13)
>  
> +/* Filesystem selftest error */
> +#define BLK_STS_SELFTEST	((__force blk_status_t)14)
> +
>  /**
>   * blk_path_error - returns true if error may be path related
>   * @error: status the request was completed with
> 

Re: [PATCH 1/2] block: Add new BLK_STS_SELFTEST status

[Qu Wenruo]

On 2019/4/6 下午4:41, Nikolay Borisov wrote:
>
>
> On 6.04.19 г. 6:14 ч., Qu Wenruo wrote:
>> There are a lot of end_io hooks which can do some self test at
>> read/write time.
>>
>> Normally such selftest should return -EUCLEAN to indicate something
>> unexpected.
>>
>> However there is no BLK_STS_* bit for that, this makes every selftest
>> error to be interpreted to EIO, which lowers the severity.
>>
>> This patch will add a new BLK_STS_SELFTEST, to allow the error from
>> endio to be passed to filesystem.
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>  block/blk-core.c          | 1 +
>>  include/linux/blk_types.h | 3 +++
>>  2 files changed, 4 insertions(+)
>>
>> diff --git a/block/blk-core.c b/block/blk-core.c
>> index 4673ebe42255..66967642a5ef 100644
>> --- a/block/blk-core.c
>> +++ b/block/blk-core.c
>> @@ -135,6 +135,7 @@ static const struct {
>>  	[BLK_STS_RESOURCE]	= { -ENOMEM,	"kernel resource" },
>>  	[BLK_STS_DEV_RESOURCE]	= { -EBUSY,	"device resource" },
>>  	[BLK_STS_AGAIN]		= { -EAGAIN,	"nonblocking retry" },
>> +	[BLK_STS_SELFTEST]	= { -EUCLEAN,	"filesystem selftest" },
>
> SELFTEST is ambiguous, I personally relate it to the selftests that some
> subsystems have. This should be changed to STS_CLEANUP or STS_CORRUPT or
> some such.

Considering my poor naming schemes, what about sticking to EUCLEAN
across all of these?
BLK_STS_UCLEAN	-ECULEAN "Structure needs clean up"

Thanks,
Qu

>
>>
>>  	/* device mapper special case, should not leak out: */
>>  	[BLK_STS_DM_REQUEUE]	= { -EREMCHG, "dm internal retry" },
>> diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
>> index 791fee35df88..5bba24c3753c 100644
>> --- a/include/linux/blk_types.h
>> +++ b/include/linux/blk_types.h
>> @@ -63,6 +63,9 @@ typedef u8 __bitwise blk_status_t;
>>   */
>>  #define BLK_STS_DEV_RESOURCE	((__force blk_status_t)13)
>>
>> +/* Filesystem selftest error */
>> +#define BLK_STS_SELFTEST	((__force blk_status_t)14)
>> +
>>  /**
>>   * blk_path_error - returns true if error may be path related
>>   * @error: status the request was completed with
>>

Re: [PATCH 1/2] block: Add new BLK_STS_SELFTEST status

[Nikolay Borisov]

On 6.04.19 г. 12:18 ч., Qu Wenruo wrote:
> 
> 
> On 2019/4/6 下午4:41, Nikolay Borisov wrote:
>>
>>
>> On 6.04.19 г. 6:14 ч., Qu Wenruo wrote:
>>> There are a lot of end_io hooks which can do some self test at
>>> read/write time.
>>>
>>> Normally such selftest should return -EUCLEAN to indicate something
>>> unexpected.
>>>
>>> However there is no BLK_STS_* bit for that, this makes every selftest
>>> error to be interpreted to EIO, which lowers the severity.
>>>
>>> This patch will add a new BLK_STS_SELFTEST, to allow the error from
>>> endio to be passed to filesystem.
>>>
>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>> ---
>>>  block/blk-core.c          | 1 +
>>>  include/linux/blk_types.h | 3 +++
>>>  2 files changed, 4 insertions(+)
>>>
>>> diff --git a/block/blk-core.c b/block/blk-core.c
>>> index 4673ebe42255..66967642a5ef 100644
>>> --- a/block/blk-core.c
>>> +++ b/block/blk-core.c
>>> @@ -135,6 +135,7 @@ static const struct {
>>>  	[BLK_STS_RESOURCE]	= { -ENOMEM,	"kernel resource" },
>>>  	[BLK_STS_DEV_RESOURCE]	= { -EBUSY,	"device resource" },
>>>  	[BLK_STS_AGAIN]		= { -EAGAIN,	"nonblocking retry" },
>>> +	[BLK_STS_SELFTEST]	= { -EUCLEAN,	"filesystem selftest" },
>>
>> SELFTEST is ambiguous, I personally relate it to the selftests that some
>> subsystems have. This should be changed to STS_CLEANUP or STS_CORRUPT or
>> some such.
> 
> Considering my poor naming schemes, what about sticking to EUCLEAN
> across all of these?
> BLK_STS_UCLEAN	-ECULEAN "Structure needs clean up"

Sounds good!
> 
> Thanks,
> Qu
> 
>>
>>>
>>>  	/* device mapper special case, should not leak out: */
>>>  	[BLK_STS_DM_REQUEUE]	= { -EREMCHG, "dm internal retry" },
>>> diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
>>> index 791fee35df88..5bba24c3753c 100644
>>> --- a/include/linux/blk_types.h
>>> +++ b/include/linux/blk_types.h
>>> @@ -63,6 +63,9 @@ typedef u8 __bitwise blk_status_t;
>>>   */
>>>  #define BLK_STS_DEV_RESOURCE	((__force blk_status_t)13)
>>>
>>> +/* Filesystem selftest error */
>>> +#define BLK_STS_SELFTEST	((__force blk_status_t)14)
>>> +
>>>  /**
>>>   * blk_path_error - returns true if error may be path related
>>>   * @error: status the request was completed with
>>>
>