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

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

[Qu Wenruo]

There are quite a lot of filesystems doing their verification work done
at endio hook or hook before submitting bio.

Normally such verification 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_UCLEAN, to allow -EUCLEAN to be
converted to BLK_STS_UCLEAN, and then converted back to -EUCLEAN without
losing anything.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
changelog:
v2:
- Use BLK_STS_UCLEAN to replace the previous stupid naming scheme.
---
 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..a51c0a13fd5e 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_UCLEAN]	= { -EUCLEAN,	"structure needs cleaning" },
 
 	/* 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..df0c470147c1 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)
 
+/* Normally filesystem layer generated error */
+#define BLK_STS_UCLEAN		((__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 v2 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_UCLEAN bit, we can distinguish real EIO error and EUCLEAN
error.

So adding new string for EUCLEAN to make the "unknown" reason to an easy
to read one.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
changelog:
v2:
- Use the original error message from EUCLEAN.
---
 fs/btrfs/super.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 120e4340792a..f69a6696f59d 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 = "Structure needs cleaning";
+		break;
 	}
 
 	return errstr;
-- 
2.21.0

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

[Christoph Hellwig]

> +	[BLK_STS_UCLEAN]	= { -EUCLEAN,	"structure needs cleaning" },

The subject line doesn't mention this new error code.  That being said
while this sounds slightly less bad than the original name it still
sounds weird..

The various filesystems really use EFSCORRUPTED which is just mapped
to EUCLEAN, so maybe this really should be

	[BLK_STS_FSCORRUPTED]	=
		{ -EUCLEAN, "file system corruption detected" },

But then again I really wonder why you need to pass this information
through a blk_status_t to start with.  In general these kinds of error
should be passed through file system specific errno fields.

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

[Qu Wenruo]

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



On 2019/4/7 下午3:04, Christoph Hellwig wrote:
>> +	[BLK_STS_UCLEAN]	= { -EUCLEAN,	"structure needs cleaning" },
> 
> The subject line doesn't mention this new error code.  That being said
> while this sounds slightly less bad than the original name it still
> sounds weird..
> 
> The various filesystems really use EFSCORRUPTED which is just mapped
> to EUCLEAN, so maybe this really should be
> 
> 	[BLK_STS_FSCORRUPTED]	=
> 		{ -EUCLEAN, "file system corruption detected" },
> 
> But then again I really wonder why you need to pass this information
> through a blk_status_t to start with.  In general these kinds of error
> should be passed through file system specific errno fields.
> 

For functions called in endio hook, we return blk_status_t.
Or for case like hook before submitting bio, we set bio->bi_status to
record it.

Yes, it's possible to restore such info into fs specific structure, but
why not reuse the bi_status we all use and love?

Thanks,
Qu


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

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

[Christoph Hellwig]

On Sun, Apr 07, 2019 at 03:08:42PM +0800, Qu Wenruo wrote:
> > But then again I really wonder why you need to pass this information
> > through a blk_status_t to start with.  In general these kinds of error
> > should be passed through file system specific errno fields.
> > 
> 
> For functions called in endio hook, we return blk_status_t.
> Or for case like hook before submitting bio, we set bio->bi_status to
> record it.

Ok.  Good enough explanation, please add it to the changelog.