On Thu, 2017-02-23 at 11:27 +0100, Hannes Reinecke wrote: > 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. > > 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(-) > > diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c > index f2cafae..481ea1b 100644 > --- a/drivers/scsi/scsi_error.c > +++ b/drivers/scsi/scsi_error.c > @@ -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); 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. > list_add_tail(&scmd->eh_entry, &shost->eh_cmd_q); > shost->host_failed++; > scsi_eh_wakeup(shost); > diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c > index be535d4..cd9f290 100644 > --- a/drivers/scsi/sd.c > +++ b/drivers/scsi/sd.c > @@ -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 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; > > /* > * If the device keeps failing read/write commands but TEST UNIT > diff --git a/drivers/scsi/sd.h b/drivers/scsi/sd.h > index 4dac35e..19e0bab 100644 > --- a/drivers/scsi/sd.h > +++ b/drivers/scsi/sd.h > @@ -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. > u8 media_present; > u8 write_prot; > u8 protection_type;/* Data Integrity Field */ > diff --git a/include/scsi/scsi.h b/include/scsi/scsi.h > index a1e1930..b6c750f 100644 > --- a/include/scsi/scsi.h > +++ b/include/scsi/scsi.h > @@ -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. > > /* > * Midlevel queue return values. Reviewed-by: Ewan D. Milne <[email protected]>
