scsi_error: count medium access timeout only once per EH run
PATCH 1/1 scsi_error: count medium access timeout only once per EH run
The current medium access timeout counter will be increased for each command, so if there are enough failed commands we'll hit the medium access timeout for even a single failure. Fix this by making the timeout per EH run, ie the counter will only be increased once per device and EH run.
--- drivers/scsi/scsi_error.c | 2 ++ drivers/scsi/sd.c | 16 ++++++++++++++-- drivers/scsi/sd.h | 1 + include/scsi/scsi.h | 1 + 4 files changed, 18 insertions(+), 2 deletions(-)
drivers/scsi/scsi_error.c +2 -0
@@ -58,6 +58,7 @@static int scsi_eh_try_stu (struct scsi_cmnd *scmd);static int scsi_try_to_abort_cmd (struct scsi_host_template *,struct scsi_cmnd *); +static int scsi_eh_action (struct scsi_cmnd *scmd,int rtn);/* called with shost->host_lock held */ void scsi_eh_wakeup (struct Scsi_Host *shost)
@@ -249,6 +250,7 @@ int scsi_eh_scmd_add(struct scsi_cmnd *scmd, int eh_flag)if (scmd->eh_eflags &SCSI_EH_ABORT_SCHEDULED ) eh_flag &= ~SCSI_EH_CANCEL_CMD ; scmd->eh_eflags |= eh_flag; +scsi_eh_action (scmd,NEEDS_RESET );list_add_tail (&scmd->eh_entry, &shost->eh_cmd_q); shost->host_failed++;scsi_eh_wakeup (shost);
drivers/scsi/sd.c +14 -2
@@ -1696,12 +1696,21 @@ static int sd_pr_clear(struct block_device *bdev, u64 key)* the eh command is passed in eh_disp. We're looking for devices that * fail medium access commands but are OK with non access commands like * test unit ready (so wrongly see the device as having a successful -* recovery) +* recovery). +* We have to be careful to count a medium access failure only once +* per SCSI EH run; there might be several timed out commands which +* will cause the 'max_medium_access_timeouts' counter to trigger +* after the first SCSI EH run already and set the device to offline. **/ static int sd_eh_action (struct scsi_cmnd *scmd,int eh_disp) {struct scsi_disk *sdkp =scsi_disk (scmd->request->rq_disk); +if (eh_disp ==NEEDS_RESET ) { +/* New SCSI EH run, reset gate variable */ + sdkp->medium_access_reset =0 ; +return eh_disp; + } if (!scsi_device_online(scmd->device) || !scsi_medium_access_command(scmd) || host_byte(scmd->result) != DID_TIME_OUT ||
@@ -1715,7 +1724,10 @@ static int sd_eh_action(struct scsi_cmnd *scmd, int eh_disp)* process of recovering or has it suffered an internal failure * that prevents access to the storage medium. */ - sdkp->medium_access_timed_out++; +if (!sdkp->medium_access_reset) { + sdkp->medium_access_timed_out++; + sdkp->medium_access_reset++; + }/* * If the device keeps failing read/write commands but TEST UNIT
drivers/scsi/sd.h +1 -0
@@ -85,6 +85,7 @@ struct scsi_disk {unsigned int physical_block_size;unsigned int max_medium_access_timeouts;unsigned int medium_access_timed_out; +unsigned int medium_access_reset;u8 media_present;u8 write_prot;u8 protection_type;/* Data Integrity Field */
include/scsi/scsi.h +1 -0
@@ -185,6 +185,7 @@ static inline int scsi_is_wlun(u64 lun)#define TIMEOUT_ERROR 0x2007#define SCSI_RETURN_NOT_HANDLED 0x2008#define FAST_IO_FAIL 0x2009 +#define NEEDS_RESET 0x2010/* * Midlevel queue return values.
Signature · 1 line
1.8.5.6
----- Original Message -----
Quoted 113 lines
From: "Hannes Reinecke" <[email protected]>
To: "Martin K. Petersen" <[email protected]>
Cc: "Christoph Hellwig" <[email protected]>, "James Bottomley" <[email protected]>, [email protected], "Hannes Reinecke" <[email protected]>, "Ewan Milne" <[email protected]>, "Lawrence Oberman" <[email protected]>, "Benjamin Block" <[email protected]>, "Steffen Maier" <[email protected]>, "Hannes Reinecke" <[email protected]>
Sent: Thursday, February 23, 2017 5:27:19 AM
Subject: [PATCH]scsi_error: count medium access timeout only once per EH runThe current medium access timeout counter will be increased for each command, so if there are enough failed commands we'll hit the medium access timeout for even a single failure. Fix this by making the timeout per EH run, ie the counter will only be increased once per device and EH run.
Cc Ewan Milne <[email protected]>Cc Lawrence Oberman <[email protected]>Cc Benjamin Block <[email protected]>Cc Steffen Maier <[email protected]>Signed-off-by Hannes Reinecke <[email protected]>--- drivers/scsi/scsi_error.c | 2 ++ drivers/scsi/sd.c | 16 ++++++++++++++-- drivers/scsi/sd.h | 1 + include/scsi/scsi.h | 1 + 4 files changed, 18 insertions(+), 2 deletions(-)drivers/scsi/scsi_error.c +2
-0@@ -58,6 +58,7 @@static int scsi_eh_try_stu (struct scsi_cmnd *scmd);static int scsi_try_to_abort_cmd (struct scsi_host_template *,struct scsi_cmnd *); +static int scsi_eh_action (struct scsi_cmnd *scmd,int rtn);/* called with shost->host_lock held */ void scsi_eh_wakeup (struct Scsi_Host *shost)@@ -249,6 +250,7 @@ int scsi_eh_scmd_add(struct scsi_cmnd *scmd, int eh_flag)if (scmd->eh_eflags &SCSI_EH_ABORT_SCHEDULED ) eh_flag &= ~SCSI_EH_CANCEL_CMD ; scmd->eh_eflags |= eh_flag; +scsi_eh_action (scmd,NEEDS_RESET );list_add_tail (&scmd->eh_entry, &shost->eh_cmd_q); shost->host_failed++;scsi_eh_wakeup (shost);drivers/scsi/sd.c +10
-1@@ -1696,12 +1696,21 @@ static int sd_pr_clear(struct block_device *bdev, u64key) * the eh command is passed in eh_disp. We're looking for devices that * fail medium access commands but are OK with non access commands like * test unit ready (so wrongly see the device as having a successful -* recovery) +* recovery). +* We have to be careful to count a medium access failure only once +* per SCSI EH run; there might be several timed out commands which +* will cause the 'max_medium_access_timeouts' counter to trigger +* after the first SCSI EH run already and set the device to offline. **/ static int sd_eh_action (struct scsi_cmnd *scmd,int eh_disp) {struct scsi_disk *sdkp =scsi_disk (scmd->request->rq_disk); +if (eh_disp ==NEEDS_RESET ) { +/* New SCSI EH run, reset gate variable */ + sdkp->medium_access_reset =0 ; +return eh_disp; + } if (!scsi_device_online(scmd->device) || !scsi_medium_access_command(scmd) ||drivers/scsi/sd.c +4
-1host_byte(scmd->result) != DID_TIME_OUT ||@@ -1715,7 +1724,10 @@ static int sd_eh_action(struct scsi_cmnd *scmd, inteh_disp) * process of recovering or has it suffered an internal failure * that prevents access to the storage medium. */ - sdkp->medium_access_timed_out++; +if (!sdkp->medium_access_reset) { + sdkp->medium_access_timed_out++; + sdkp->medium_access_reset++; + }/* drivers/scsi/sd.c +0
-0* If the device keeps failing read/write commands but TEST UNIT drivers/scsi/sd.h +1
-0@@ -85,6 +85,7 @@ struct scsi_disk {unsigned int physical_block_size;unsigned int max_medium_access_timeouts;unsigned int medium_access_timed_out; +unsigned int medium_access_reset;u8 media_present;u8 write_prot;u8 protection_type;/* Data Integrity Field */ include/scsi/scsi.h +1
-1@@ -185,6 +185,7 @@ static inline int scsi_is_wlun(u64 lun)#define TIMEOUT_ERROR 0x2007#define SCSI_RETURN_NOT_HANDLED 0x2008#define FAST_IO_FAIL 0x2009 +#define NEEDS_RESET 0x2010/* * Midlevel queue return values. --
Quoted 3 lines
1.8.5.6
Hello Hannes
This makes sense to me what you are doing here.
I will also wait for Ewan to weigh in but I wonder if we should make a simple change. Maybe good to clarify the RESET here by simply changing the name.
Change
+#define NEEDS_RESET 0x2010
to
+#define MAX_MEDIUM_ERROR_NEEDS_RESET
Of course then also change
+ if (eh_disp == NEEDS_RESET) {
to
+ if (eh_disp == MAX_MEDIUM_ERROR_NEEDS_RESET) {
Quoted 5 lines · Hannes Reinecke
The current medium access timeout counter will be increased for each command, so if there are enough failed commands we'll hit the medium access timeout for even a single failure. Fix this by making the timeout per EH run, ie the counter will only be increased once per device and EH run.
So, this is good, the current implementation has a flaw in that under certain conditions, a device will get offlined immediately, (i.e. if there are a few medium access commands pending, and they all timeout), which isn't what was intended.
It means, of course, that we will no longer detect cases like:
<timeout>, <timeout>, SUCCESS, SUCCESS, SUCCESS, <timeout>
as separate medium access timeouts, but I think the original intent of Martin's change wasn't to operate on such a short time-scale, am I right, Martin?
I made a few notes on the coding/implementation (below), but that doesn't affect the functional change. We should definitely change what we have now, it is causing people problems.
Quoted 26 lines
Cc Ewan Milne <[email protected]>Cc Lawrence Oberman <[email protected]>Cc Benjamin Block <[email protected]>Cc Steffen Maier <[email protected]>Signed-off-by Hannes Reinecke <[email protected]>--- drivers/scsi/scsi_error.c | 2 ++ drivers/scsi/sd.c | 16 ++++++++++++++-- drivers/scsi/sd.h | 1 + include/scsi/scsi.h | 1 + 4 files changed, 18 insertions(+), 2 deletions(-)drivers/scsi/scsi_error.c +1
-0@@ -58,6 +58,7 @@static int scsi_eh_try_stu (struct scsi_cmnd *scmd);static int scsi_try_to_abort_cmd (struct scsi_host_template *,struct scsi_cmnd *); +static int scsi_eh_action (struct scsi_cmnd *scmd,int rtn);/* called with shost->host_lock held */ void scsi_eh_wakeup (struct Scsi_Host *shost)
drivers/scsi/scsi_error.c int scsi_eh_scmd_add(struct scsi_cmnd *scmd, int eh_flag) · line 253
Quoted 4 lines
drivers/scsi/scsi_error.c +1
-0@@ -249,6 +250,7 @@ int scsi_eh_scmd_add(struct scsi_cmnd *scmd, int eh_flag)if (scmd->eh_eflags &SCSI_EH_ABORT_SCHEDULED ) eh_flag &= ~SCSI_EH_CANCEL_CMD ; scmd->eh_eflags |= eh_flag; +scsi_eh_action (scmd,NEEDS_RESET );
So here we are overloading the eh_disp argument with a flag to reset the medium_access_reset variable. James changed the calling sequence of this function already to remove arguments, we could just add another boolean parameter "reset". scsi_driver.eh_action() would need it too.
Quoted 31 lines
drivers/scsi/scsi_error.c +0
-0list_add_tail (&scmd->eh_entry, &shost->eh_cmd_q); shost->host_failed++;scsi_eh_wakeup (shost);drivers/scsi/sd.c +10
-1@@ -1696,12 +1696,21 @@ static int sd_pr_clear(struct block_device *bdev, u64 key)* the eh command is passed in eh_disp. We're looking for devices that * fail medium access commands but are OK with non access commands like * test unit ready (so wrongly see the device as having a successful -* recovery) +* recovery). +* We have to be careful to count a medium access failure only once +* per SCSI EH run; there might be several timed out commands which +* will cause the 'max_medium_access_timeouts' counter to trigger +* after the first SCSI EH run already and set the device to offline. **/ static int sd_eh_action (struct scsi_cmnd *scmd,int eh_disp) {struct scsi_disk *sdkp =scsi_disk (scmd->request->rq_disk); +if (eh_disp ==NEEDS_RESET ) { +/* New SCSI EH run, reset gate variable */ + sdkp->medium_access_reset =0 ; +return eh_disp; + } if (!scsi_device_online(scmd->device) || !scsi_medium_access_command(scmd) || host_byte(scmd->result) != DID_TIME_OUT ||
drivers/scsi/sd.c static int sd_eh_action(struct scsi_cmnd *scmd, int eh_disp) · line 1730
Quoted 8 lines
drivers/scsi/sd.c +4
-1@@ -1715,7 +1724,10 @@ static int sd_eh_action(struct scsi_cmnd *scmd, int eh_disp)* process of recovering or has it suffered an internal failure * that prevents access to the storage medium. */ - sdkp->medium_access_timed_out++; +if (!sdkp->medium_access_reset) { + sdkp->medium_access_timed_out++; + sdkp->medium_access_reset++; + }
If we only increment sdkp->medium_access_reset when it was 0, then it will only have the values 0 and 1, and does not need to have the full unsigned int precision. A single bit field is sufficient, in which case the code would be: sdkp->medium_access_reset = 1;
drivers/scsi/sd.h struct scsi_disk { · line 88
Quoted 12 lines
drivers/scsi/sd.c +0
-0/* * If the device keeps failing read/write commands but TEST UNIT drivers/scsi/sd.h +1
-0@@ -85,6 +85,7 @@ struct scsi_disk {unsigned int physical_block_size;unsigned int max_medium_access_timeouts;unsigned int medium_access_timed_out; +unsigned int medium_access_reset;
This could be an unsigned int : 1 with the other single bit fields at the end of the structure, with the change above.
include/scsi/scsi.h static inline int scsi_is_wlun(u64 lun) · line 188
Quoted 12 lines
drivers/scsi/sd.h +0
-0u8 media_present;u8 write_prot;u8 protection_type;/* Data Integrity Field */ include/scsi/scsi.h +1
-0@@ -185,6 +185,7 @@ static inline int scsi_is_wlun(u64 lun)#define TIMEOUT_ERROR 0x2007#define SCSI_RETURN_NOT_HANDLED 0x2008#define FAST_IO_FAIL 0x2009 +#define NEEDS_RESET 0x2010
See above, this is overloading the use of the parameter.
include/scsi/scsi.h static inline int scsi_is_wlun(u64 lun) · line 191
Quoted 3 lines
include/scsi/scsi.h +0
-0/* * Midlevel queue return values.
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Ewan" == Ewan D Milne <[email protected]> writes:
Ewan,
Ewan> So, this is good, the current implementation has a flaw in that Ewan> under certain conditions, a device will get offlined immediately, Ewan> (i.e. if there are a few medium access commands pending, and they Ewan> all timeout), which isn't what was intended.
Yeah. That was OK for my use case. I was trying to prevent the server from going into a tail spin. There was no chance of recovering the disk.
But ideally we'd be offlining based on how many times we retry the same medium access command.
Ewan> as separate medium access timeouts, but I think the original Ewan> intent of Martin's change wasn't to operate on such a short Ewan> time-scale, am I right, Martin?
On the device that begat my original patch, SPC command responses were handled by the SAS controller firmware on behalf of all discovered devices. Regardless of whether said drives were still alive or not.
Medium Access commands, however, would always get passed on to the physical drive for processing. So when a drive went pining for the fjords, TUR would always succeed whereas reads and writes would time out.
Signature · 1 line
Martin K. Petersen Oracle Linux Engineering
Quoted 47 lines · Ewan D. Milne
Quoted 5 lines · Hannes Reinecke
The current medium access timeout counter will be increased for each command, so if there are enough failed commands we'll hit the medium access timeout for even a single failure. Fix this by making the timeout per EH run, ie the counter will only be increased once per device and EH run.
So, this is good, the current implementation has a flaw in that under certain conditions, a device will get offlined immediately, (i.e. if there are a few medium access commands pending, and they all timeout), which isn't what was intended.
It means, of course, that we will no longer detect cases like:
<timeout>, <timeout>, SUCCESS, SUCCESS, SUCCESS, <timeout>
as separate medium access timeouts, but I think the original intent of Martin's change wasn't to operate on such a short time-scale, am I right, Martin?
I made a few notes on the coding/implementation (below), but that doesn't affect the functional change. We should definitely change what we have now, it is causing people problems.
Quoted 23 lines
Cc Ewan Milne <[email protected]>Cc Lawrence Oberman <[email protected]>Cc Benjamin Block <[email protected]>Cc Steffen Maier <[email protected]>Signed-off-by Hannes Reinecke <[email protected]>--- drivers/scsi/scsi_error.c | 2 ++ drivers/scsi/sd.c | 16 ++++++++++++++-- drivers/scsi/sd.h | 1 + include/scsi/scsi.h | 1 + 4 files changed, 18 insertions(+), 2 deletions(-)drivers/scsi/scsi_error.c +1
-0@@ -58,6 +58,7 @@static int scsi_eh_try_stu (struct scsi_cmnd *scmd);static int scsi_try_to_abort_cmd (struct scsi_host_template *,struct scsi_cmnd *); +static int scsi_eh_action (struct scsi_cmnd *scmd,int rtn);
Quoted 13 lines
drivers/scsi/scsi_error.c int scsi_eh_scmd_add(struct scsi_cmnd *scmd, int eh_flag) · line 253
Quoted 7 lines
drivers/scsi/scsi_error.c +1
-0/* called with shost->host_lock held */ void scsi_eh_wakeup (struct Scsi_Host *shost)@@ -249,6 +250,7 @@ int scsi_eh_scmd_add(struct scsi_cmnd *scmd, int eh_flag)if (scmd->eh_eflags &SCSI_EH_ABORT_SCHEDULED ) eh_flag &= ~SCSI_EH_CANCEL_CMD ; scmd->eh_eflags |= eh_flag; +scsi_eh_action (scmd,NEEDS_RESET );So here we are overloading the
eh_dispargument with a flag to reset themedium_access_resetvariable. James changed the calling sequence of this function already to remove arguments, we could just add another boolean parameter "reset".scsi_driver.eh_action()would need it too.
Sure, I could be doing it.
Using a separate 'eh_disp' variable has the added benefit that it doesn't break the kABI :-)
But yeah, I modify that.
Quoted 36 lines
Quoted 36 lines
drivers/scsi/scsi_error.c +0
-0list_add_tail (&scmd->eh_entry, &shost->eh_cmd_q); shost->host_failed++;scsi_eh_wakeup (shost);drivers/scsi/sd.c +11
-2@@ -1696,12 +1696,21 @@ static int sd_pr_clear(struct block_device *bdev, u64 key)* the eh command is passed in eh_disp. We're looking for devices that * fail medium access commands but are OK with non access commands like * test unit ready (so wrongly see the device as having a successful -* recovery) +* recovery). +* We have to be careful to count a medium access failure only once +* per SCSI EH run; there might be several timed out commands which +* will cause the 'max_medium_access_timeouts' counter to trigger +* after the first SCSI EH run already and set the device to offline. **/ static int sd_eh_action (struct scsi_cmnd *scmd,int eh_disp) {struct scsi_disk *sdkp =scsi_disk (scmd->request->rq_disk); +if (eh_disp ==NEEDS_RESET ) { +/* New SCSI EH run, reset gate variable */ + sdkp->medium_access_reset =0 ; +return eh_disp; + } if (!scsi_device_online(scmd->device) || !scsi_medium_access_command(scmd) || host_byte(scmd->result) != DID_TIME_OUT ||@@ -1715,7 +1724,10 @@ static int sd_eh_action(struct scsi_cmnd *scmd, int eh_disp)* process of recovering or has it suffered an internal failure * that prevents access to the storage medium. */ - sdkp->medium_access_timed_out++; +if (!sdkp->medium_access_reset) {
Quoted 9 lines
drivers/scsi/sd.c static int sd_eh_action(struct scsi_cmnd *scmd, int eh_disp) · line 1730
Quoted 3 lines
drivers/scsi/sd.c +3
-0+ sdkp->medium_access_timed_out++; + sdkp->medium_access_reset++; + }If we only increment
sdkp->medium_access_resetwhen it was 0, then it will only have the values 0 and 1, and does not need to have the full unsigned int precision. A single bit field is sufficient, in which case the code would be:sdkp->medium_access_reset= 1;
Okay, can do that.
Cheers,
Hannes
Signature · 4 lines
Dr. Hannes Reinecke zSeries & Storage [email protected] +49 911 74053 688 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)