Skip to contentSkip to conversation

block: scsi: introduce and use BLK_STS_OFFLINE

PATCH Superseded Song Liu 4 participants 12 messages Feb 3, 2022 – Feb 4, 2022

Subsystems (2)
SCSI Block layer

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Song Liu

author
#0 lore raw

cover block: scsi: introduce and use BLK_STS_OFFLINE

We have a use case where HDDs are regularly power on/off to perserve power. When a drive is being removed, we often see errors like

   [  172.803279] I/O error, dev sda, sector 3137184

These messages are confusing for automations that grep dmesg, as they look very similar to real HDD error.

Solve this issue with a new block state BLK_STS_OFFLINE. After the change, the error message looks like

   [  172.803279] device offline error, dev sda, sector 3137184

so that the automations won't confuse them with real I/O error.

Song Liu (2):
block: introduce BLK_STS_OFFLINE
scsi: use BLK_STS_OFFLINE for not fully online devices

 block/blk-core.c          | 1 +
 drivers/scsi/scsi_lib.c   | 2 +-
 include/linux/blk_types.h | 7 +++++++
 3 files changed, 9 insertions(+), 1 deletion(-)

--
2.30.2

author
#1 lore raw

PATCH 1/2 block: introduce BLK_STS_OFFLINE

Currently, drivers reports BLK_STS_IOERR for devices that are not full online or being removed. This behavior could cause confusion for users, as they are not really I/O errors from the device.

Solve this issue with a new state BLK_STS_OFFLINE, which reports "device offline error" in dmesg instead of "I/O error".

Signed-off-by Song Liu <[email protected]>
---
 block/blk-core.c          | 1 +
 include/linux/blk_types.h | 7 +++++++
 2 files changed, 8 insertions(+)
block/blk-core.c +1 -0
index 61f6a0dc4511..24035dd2eef1 100644
@@ -164,6 +164,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_OFFLINE]	= { -EIO,	"device offline" },
 
 	/* device mapper special case, should not leak out: */
 	[BLK_STS_DM_REQUEUE]	= { -EREMCHG, "dm internal retry" },
include/linux/blk_types.h +7 -0
index fe065c394fff..5561e58d158a 100644
@@ -153,6 +153,13 @@ typedef u8 __bitwise blk_status_t;
  */
 #define BLK_STS_ZONE_ACTIVE_RESOURCE	((__force blk_status_t)16)
 
+/*
+ * BLK_STS_OFFLINE is returned from the driver when the target device is offline
+ * or is being taken offline. This could help differentiate the case where a
+ * device is intentionally being shut down from a real I/O error.
+ */
+#define BLK_STS_OFFLINE		((__force blk_status_t)17)
+
 /**
  * blk_path_error - returns true if error may be path related
  * @error: status the request was completed with
Signature · 1 line
2.30.2
author
#2 lore raw

Re: [PATCH 1/2] block: introduce BLK_STS_OFFLINE

CC linux-block (it was a typo in the original email)

Quoted 47 lines · Song Liu

Currently, drivers reports BLK_STS_IOERR for devices that are not full online or being removed. This behavior could cause confusion for users, as they are not really I/O errors from the device.

Solve this issue with a new state BLK_STS_OFFLINE, which reports "device offline error" in dmesg instead of "I/O error".

Signed-off-by Song Liu <[email protected]>
---
 block/blk-core.c          | 1 +
 include/linux/blk_types.h | 7 +++++++
 2 files changed, 8 insertions(+)
block/blk-core.c +1 -0
index 61f6a0dc4511..24035dd2eef1 100644
@@ -164,6 +164,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_OFFLINE]       = { -EIO,       "device offline" },
 
        /* device mapper special case, should not leak out: */
        [BLK_STS_DM_REQUEUE]    = { -EREMCHG, "dm internal retry" },
include/linux/blk_types.h +7 -1
index fe065c394fff..5561e58d158a 100644
@@ -153,6 +153,13 @@ typedef u8 __bitwise blk_status_t;
  */
 #define BLK_STS_ZONE_ACTIVE_RESOURCE   ((__force blk_status_t)16)
 
+/*
+ * BLK_STS_OFFLINE is returned from the driver when the target device is offline
+ * or is being taken offline. This could help differentiate the case where a
+ * device is intentionally being shut down from a real I/O error.
+ */
+#define BLK_STS_OFFLINE                ((__force blk_status_t)17)
+
 /**
  * blk_path_error - returns true if error may be path related
  * @error: status the request was completed with
--

2.30.2

#3 lore raw

Re: [PATCH 1/2] block: introduce BLK_STS_OFFLINE

Quoted 37 lines · Song Liu

CC linux-block (it was a typo in the original email)

Quoted 34 lines · Song Liu

Currently, drivers reports BLK_STS_IOERR for devices that are not full online or being removed. This behavior could cause confusion for users, as they are not really I/O errors from the device.

Solve this issue with a new state BLK_STS_OFFLINE, which reports "device offline error" in dmesg instead of "I/O error".

Signed-off-by Song Liu <[email protected]>
---
  block/blk-core.c          | 1 +
  include/linux/blk_types.h | 7 +++++++
  2 files changed, 8 insertions(+)
block/blk-core.c +1 -0
index 61f6a0dc4511..24035dd2eef1 100644
@@ -164,6 +164,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_OFFLINE]       = { -EIO,       "device offline" },
 
         /* device mapper special case, should not leak out: */
         [BLK_STS_DM_REQUEUE]    = { -EREMCHG, "dm internal retry" },
include/linux/blk_types.h +0 -0
index fe065c394fff..5561e58d158a 100644
@@ -153,6 +153,13 @@ typedef u8 __bitwise blk_status_t;
   */
  #define BLK_STS_ZONE_ACTIVE_RESOURCE   ((__force blk_status_t)16)
Quoted 13 lines
Quoted 11 lines
include/linux/blk_types.h +7 -1
+/*
+ * BLK_STS_OFFLINE is returned from the driver when the target device is offline
+ * or is being taken offline. This could help differentiate the case where a
+ * device is intentionally being shut down from a real I/O error.
+ */
+#define BLK_STS_OFFLINE                ((__force blk_status_t)17)
+
  /**
   * blk_path_error - returns true if error may be path related
   * @error: status the request was completed with
--
Quoted 2 lines

2.30.2

Please do not overload EIO here.
EIO already is a catch-all error if we don't know any better, but for the 'device offline' case we do (or rather should). Please map it onto 'ENODEV' or 'ENXIO'.

Cheers,

Hannes

Signature · 4 lines
Dr. Hannes Reinecke                Kernel Storage Architect
[email protected]                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer
maintainer
#4 lore raw

Re: [PATCH 1/2] block: introduce BLK_STS_OFFLINE

Quoted 47 lines · Hannes Reinecke
Quoted 46 lines · Song Liu

CC linux-block (it was a typo in the original email)

Quoted 43 lines · Song Liu

Currently, drivers reports BLK_STS_IOERR for devices that are not full online or being removed. This behavior could cause confusion for users, as they are not really I/O errors from the device.

Solve this issue with a new state BLK_STS_OFFLINE, which reports "device offline error" in dmesg instead of "I/O error".

Signed-off-by Song Liu <[email protected]>
---
  block/blk-core.c          | 1 +
  include/linux/blk_types.h | 7 +++++++
  2 files changed, 8 insertions(+)
block/blk-core.c +1 -0
index 61f6a0dc4511..24035dd2eef1 100644
@@ -164,6 +164,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_OFFLINE]       = { -EIO,       "device offline" },
 
         /* device mapper special case, should not leak out: */
         [BLK_STS_DM_REQUEUE]    = { -EREMCHG, "dm internal retry" },
include/linux/blk_types.h +7 -0
index fe065c394fff..5561e58d158a 100644
@@ -153,6 +153,13 @@ typedef u8 __bitwise blk_status_t;
   */
  #define BLK_STS_ZONE_ACTIVE_RESOURCE   ((__force blk_status_t)16)
 
+/*
+ * BLK_STS_OFFLINE is returned from the driver when the target device is offline
+ * or is being taken offline. This could help differentiate the case where a
+ * device is intentionally being shut down from a real I/O error.
+ */
+#define BLK_STS_OFFLINE                ((__force blk_status_t)17)
+
  /**
   * blk_path_error - returns true if error may be path related
Quoted 8 lines
Quoted 4 lines
Quoted 4 lines
include/linux/blk_types.h +0 -1
   * @error: status the request was completed with
--

2.30.2

Please do not overload EIO here.
EIO already is a catch-all error if we don't know any better, but for the 'device offline' case we do (or rather should).
Please map it onto 'ENODEV' or 'ENXIO'.

It's deliberately EIO as not to force a change in behavior. I don't mind using something else, but that should be a separate change then.

Signature · 1 line
Jens Axboe
#5 lore raw

Re: [PATCH 1/2] block: introduce BLK_STS_OFFLINE

Hi Hannes and Jens,

Quoted 59 lines · Jens Axboe
Quoted 47 lines · Hannes Reinecke
Quoted 46 lines · Song Liu

CC linux-block (it was a typo in the original email)

Quoted 43 lines · Song Liu

Currently, drivers reports BLK_STS_IOERR for devices that are not full online or being removed. This behavior could cause confusion for users, as they are not really I/O errors from the device.

Solve this issue with a new state BLK_STS_OFFLINE, which reports "device offline error" in dmesg instead of "I/O error".

Signed-off-by Song Liu <[email protected]>
---
 block/blk-core.c          | 1 +
 include/linux/blk_types.h | 7 +++++++
 2 files changed, 8 insertions(+)
block/blk-core.c +1 -0
index 61f6a0dc4511..24035dd2eef1 100644
@@ -164,6 +164,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_OFFLINE]       = { -EIO,       "device offline" },
 
        /* device mapper special case, should not leak out: */
        [BLK_STS_DM_REQUEUE]    = { -EREMCHG, "dm internal retry" },
include/linux/blk_types.h +7 -0
index fe065c394fff..5561e58d158a 100644
@@ -153,6 +153,13 @@ typedef u8 __bitwise blk_status_t;
  */
 #define BLK_STS_ZONE_ACTIVE_RESOURCE   ((__force blk_status_t)16)
 
+/*
+ * BLK_STS_OFFLINE is returned from the driver when the target device is offline
+ * or is being taken offline. This could help differentiate the case where a
+ * device is intentionally being shut down from a real I/O error.
+ */
+#define BLK_STS_OFFLINE                ((__force blk_status_t)17)
+
 /**
  * blk_path_error - returns true if error may be path related
Quoted 8 lines
Quoted 4 lines
Quoted 4 lines
include/linux/blk_types.h +0 -1
  * @error: status the request was completed with
--

2.30.2

Please do not overload EIO here.
EIO already is a catch-all error if we don't know any better, but for the 'device offline' case we do (or rather should).
Please map it onto 'ENODEV' or 'ENXIO'.

Quoted 2 lines

It's deliberately EIO as not to force a change in behavior. I don't mind using something else, but that should be a separate change then.

Thanks for these feedbacks. Shall I send v2 with an extra patch that changes EIO to ENODEV/ENXIO? Or shall we do that in a follow up patch? Also, any preference between ENODEV and ENXIO?

Thanks,
Song

maintainer
#6 lore raw

Re: [PATCH 1/2] block: introduce BLK_STS_OFFLINE

Quoted 61 lines · Song Liu

Hi Hannes and Jens,

Quoted 59 lines · Jens Axboe
Quoted 55 lines · Hannes Reinecke
Quoted 37 lines · Song Liu

CC linux-block (it was a typo in the original email)

Quoted 34 lines · Song Liu

Currently, drivers reports BLK_STS_IOERR for devices that are not full online or being removed. This behavior could cause confusion for users, as they are not really I/O errors from the device.

Solve this issue with a new state BLK_STS_OFFLINE, which reports "device offline error" in dmesg instead of "I/O error".

Signed-off-by Song Liu <[email protected]>
---
 block/blk-core.c          | 1 +
 include/linux/blk_types.h | 7 +++++++
 2 files changed, 8 insertions(+)
block/blk-core.c +1 -0
index 61f6a0dc4511..24035dd2eef1 100644
@@ -164,6 +164,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_OFFLINE]       = { -EIO,       "device offline" },
 
        /* device mapper special case, should not leak out: */
        [BLK_STS_DM_REQUEUE]    = { -EREMCHG, "dm internal retry" },
include/linux/blk_types.h +0 -0
index fe065c394fff..5561e58d158a 100644
@@ -153,6 +153,13 @@ typedef u8 __bitwise blk_status_t;
  */
 #define BLK_STS_ZONE_ACTIVE_RESOURCE   ((__force blk_status_t)16)
Quoted 13 lines
Quoted 11 lines
include/linux/blk_types.h +7 -1
+/*
+ * BLK_STS_OFFLINE is returned from the driver when the target device is offline
+ * or is being taken offline. This could help differentiate the case where a
+ * device is intentionally being shut down from a real I/O error.
+ */
+#define BLK_STS_OFFLINE                ((__force blk_status_t)17)
+
 /**
  * blk_path_error - returns true if error may be path related
  * @error: status the request was completed with
--
Quoted 2 lines

2.30.2

Please do not overload EIO here.
EIO already is a catch-all error if we don't know any better, but for the 'device offline' case we do (or rather should).
Please map it onto 'ENODEV' or 'ENXIO'.

Quoted 6 lines
Quoted 2 lines

It's deliberately EIO as not to force a change in behavior. I don't mind using something else, but that should be a separate change then.

Thanks for these feedbacks. Shall I send v2 with an extra patch that changes EIO to ENODEV/ENXIO? Or shall we do that in a follow up patch? Also, any preference between ENODEV and ENXIO?

Yeah I think so, and perhaps put a mention in this patch on why EIO is chosen to not change the user visible return value.

Signature · 1 line
Jens Axboe
Reviewed-by
#7 lore raw

Re: [PATCH 1/2] block: introduce BLK_STS_OFFLINE

In reply to Song Liu
Quoted 61 lines · Song Liu

Hi Hannes and Jens,

Quoted 59 lines · Jens Axboe
Quoted 55 lines · Hannes Reinecke
Quoted 37 lines · Song Liu

CC linux-block (it was a typo in the original email)

Quoted 34 lines · Song Liu

Currently, drivers reports BLK_STS_IOERR for devices that are not full online or being removed. This behavior could cause confusion for users, as they are not really I/O errors from the device.

Solve this issue with a new state BLK_STS_OFFLINE, which reports "device offline error" in dmesg instead of "I/O error".

Signed-off-by Song Liu <[email protected]>
---
  block/blk-core.c          | 1 +
  include/linux/blk_types.h | 7 +++++++
  2 files changed, 8 insertions(+)
block/blk-core.c +1 -0
index 61f6a0dc4511..24035dd2eef1 100644
@@ -164,6 +164,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_OFFLINE]       = { -EIO,       "device offline" },
 
         /* device mapper special case, should not leak out: */
         [BLK_STS_DM_REQUEUE]    = { -EREMCHG, "dm internal retry" },
include/linux/blk_types.h +0 -0
index fe065c394fff..5561e58d158a 100644
@@ -153,6 +153,13 @@ typedef u8 __bitwise blk_status_t;
   */
  #define BLK_STS_ZONE_ACTIVE_RESOURCE   ((__force blk_status_t)16)
Quoted 13 lines
Quoted 11 lines
include/linux/blk_types.h +7 -1
+/*
+ * BLK_STS_OFFLINE is returned from the driver when the target device is offline
+ * or is being taken offline. This could help differentiate the case where a
+ * device is intentionally being shut down from a real I/O error.
+ */
+#define BLK_STS_OFFLINE                ((__force blk_status_t)17)
+
  /**
   * blk_path_error - returns true if error may be path related
   * @error: status the request was completed with
--
Quoted 2 lines

2.30.2

Please do not overload EIO here.
EIO already is a catch-all error if we don't know any better, but for the 'device offline' case we do (or rather should).
Please map it onto 'ENODEV' or 'ENXIO'.

Quoted 7 lines
Quoted 2 lines

It's deliberately EIO as not to force a change in behavior. I don't mind using something else, but that should be a separate change then.

Thanks for these feedbacks. Shall I send v2 with an extra patch that changes EIO to ENODEV/ENXIO? Or shall we do that in a follow up patch? Also, any preference between ENODEV and ENXIO?

Please make it an addtional patch, and use ENODEV as a return value. For this patch you can add:

Reviewed-by Hannes Reinecke <[email protected]>

Cheers,

Hannes

Signature · 4 lines
Dr. Hannes Reinecke                Kernel Storage Architect
[email protected]                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer
author
#8 lore raw

PATCH 2/2 scsi: use BLK_STS_OFFLINE for not fully online devices

In reply to Song Liu

The new error message for such case looks like

[  172.809565] device offline error, dev sda, sector 3138208 ...

which will not be confused with regular I/O error (BLK_STS_IOERR).

Signed-off-by Song Liu <[email protected]>
---
 drivers/scsi/scsi_lib.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/scsi/scsi_lib.c +1 -1
index 0a70aa763a96..e30bc51578e9 100644
@@ -1276,7 +1276,7 @@ scsi_device_state_check(struct scsi_device *sdev, struct request *req)
 		 * power management commands.
 		 */
 		if (req && !(req->rq_flags & RQF_PM))
-			return BLK_STS_IOERR;
+			return BLK_STS_OFFLINE;
 		return BLK_STS_OK;
 	}
 }
Signature · 1 line
2.30.2
author
#9 lore raw

Re: [PATCH 2/2] scsi: use BLK_STS_OFFLINE for not fully online devices

CC linux-block (it was a typo in the original email)

Quoted 28 lines · Song Liu

The new error message for such case looks like

[  172.809565] device offline error, dev sda, sector 3138208 ...

which will not be confused with regular I/O error (BLK_STS_IOERR).

Signed-off-by Song Liu <[email protected]>
---
 drivers/scsi/scsi_lib.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/scsi/scsi_lib.c +1 -2
index 0a70aa763a96..e30bc51578e9 100644
@@ -1276,7 +1276,7 @@ scsi_device_state_check(struct scsi_device *sdev, struct request *req)
                 * power management commands.
                 */
                if (req && !(req->rq_flags & RQF_PM))
-                       return BLK_STS_IOERR;
+                       return BLK_STS_OFFLINE;
                return BLK_STS_OK;
        }
 }
--

2.30.2

Reviewed-by
#10 lore raw

Re: [PATCH 2/2] scsi: use BLK_STS_OFFLINE for not fully online devices

Quoted 15 lines · Song Liu

CC linux-block (it was a typo in the original email)

Quoted 12 lines · Song Liu

The new error message for such case looks like

[  172.809565] device offline error, dev sda, sector 3138208 ...

which will not be confused with regular I/O error (BLK_STS_IOERR).

Signed-off-by Song Liu <[email protected]>
---
  drivers/scsi/scsi_lib.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
Quoted 16 lines
Quoted 14 lines
drivers/scsi/scsi_lib.c +1 -2
index 0a70aa763a96..e30bc51578e9 100644
@@ -1276,7 +1276,7 @@ scsi_device_state_check(struct scsi_device *sdev, struct request *req)
                  * power management commands.
                  */
                 if (req && !(req->rq_flags & RQF_PM))
-                       return BLK_STS_IOERR;
+                       return BLK_STS_OFFLINE;
                 return BLK_STS_OK;
         }
  }
--
Quoted 2 lines

2.30.2

Reviewed-by Hannes Reinecke <[email protected]>

Cheers,

Hannes

Signature · 4 lines
Dr. Hannes Reinecke                Kernel Storage Architect
[email protected]                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer
author
#11 lore raw
In reply to Song Liu

CC linux-block (it was a typo in the original email)

Quoted 27 lines · Song Liu

We have a use case where HDDs are regularly power on/off to perserve power. When a drive is being removed, we often see errors like

   [  172.803279] I/O error, dev sda, sector 3137184

These messages are confusing for automations that grep dmesg, as they look very similar to real HDD error.

Solve this issue with a new block state BLK_STS_OFFLINE. After the change, the error message looks like

   [  172.803279] device offline error, dev sda, sector 3137184

so that the automations won't confuse them with real I/O error.

Song Liu (2):
block: introduce BLK_STS_OFFLINE
scsi: use BLK_STS_OFFLINE for not fully online devices

 block/blk-core.c          | 1 +
 drivers/scsi/scsi_lib.c   | 2 +-
 include/linux/blk_types.h | 7 +++++++
 3 files changed, 9 insertions(+), 1 deletion(-)

--
2.30.2