[v2,1/2] block: Add new BLK_STS_SELFTEST status
diff mbox series

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

Commit Message

Qu Wenruo April 6, 2019, 10:37 a.m. UTC
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(+)

Comments

Christoph Hellwig April 7, 2019, 7:04 a.m. UTC | #1
> +	[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.
Qu Wenruo April 7, 2019, 7:08 a.m. UTC | #2
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
Christoph Hellwig April 8, 2019, 6:32 a.m. UTC | #3
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.

Patch
diff mbox series

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