| Message ID | 20220929025407.119804-3-michael.christie@oracle.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Allow scsi_execute users to control retries | expand |
On Wed, 2022-09-28 at 21:53 -0500, Mike Christie wrote: > For passthrough, we don't retry any error we get a check condition > for. > This results in a lot of callers driving their own retries for those > types > of errors and retrying all errors, and there has been a request to > retry > specific host byte errors. > > This adds the core code to allow passthrough users to specify what > errors > they want scsi-ml to retry for them. We can then convert users to > drop > their sense parsing and retry handling. > > Signed-off-by: Mike Christie <michael.christie@oracle.com> I like the general approach. A few remarks below. > --- > drivers/scsi/scsi_error.c | 63 > +++++++++++++++++++++++++++++++++++++++ > drivers/scsi/scsi_lib.c | 1 + > include/scsi/scsi_cmnd.h | 26 +++++++++++++++- > 3 files changed, 89 insertions(+), 1 deletion(-) > > diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c > index 3f630798d1eb..4bf7b65bc63d 100644 > --- a/drivers/scsi/scsi_error.c > +++ b/drivers/scsi/scsi_error.c > @@ -1831,6 +1831,63 @@ bool scsi_noretry_cmd(struct scsi_cmnd *scmd) > return false; > } > > +static enum scsi_disposition scsi_check_passthrough(struct scsi_cmnd > *scmd) > +{ > + struct scsi_failure *failure; > + struct scsi_sense_hdr sshdr; > + enum scsi_disposition ret; > + int i = 0; > + > + if (!scmd->result || !scmd->failures) > + return SCSI_RETURN_NOT_HANDLED; > + > + while (1) { > + failure = &scmd->failures[i++]; Style nit: a for loop would express the logic better, IMO. > + if (!failure->result) > + break; > + > + if (failure->result == SCMD_FAILURE_ANY) > + goto maybe_retry; > + > + if (host_byte(scmd->result) & host_byte(failure- > >result)) { > + goto maybe_retry; Using "&" here needs explanation. The host byte is not a bit mask. > + } else if (get_status_byte(scmd) & > + __get_status_byte(failure->result)) { See above. > + if (get_status_byte(scmd) != > SAM_STAT_CHECK_CONDITION || > + failure->sense == SCMD_FAILURE_SENSE_ANY) > + goto maybe_retry; > + > + ret = scsi_start_sense_processing(scmd, > &sshdr); > + if (ret == NEEDS_RETRY) > + goto maybe_retry; > + else if (ret != SUCCESS) > + return ret; > + > + if (failure->sense != sshdr.sense_key) > + continue; > + > + if (failure->asc == SCMD_FAILURE_ASC_ANY) > + goto maybe_retry; > + > + if (failure->asc != sshdr.asc) > + continue; > + > + if (failure->ascq == SCMD_FAILURE_ASCQ_ANY || > + failure->ascq == sshdr.ascq) > + goto maybe_retry; > + } > + } > + > + return SCSI_RETURN_NOT_HANDLED; > + > +maybe_retry: > + if (failure->allowed == SCMD_FAILURE_NO_LIMIT || > + ++failure->retries <= failure->allowed) > + return NEEDS_RETRY; > + > + return SUCCESS; > +} > + > /** > * scsi_decide_disposition - Disposition a cmd on return from LLD. > * @scmd: SCSI cmd to examine. > @@ -1859,6 +1916,12 @@ enum scsi_disposition > scsi_decide_disposition(struct scsi_cmnd *scmd) > return SUCCESS; > } > > + if (blk_rq_is_passthrough(scsi_cmd_to_rq(scmd))) { > + rtn = scsi_check_passthrough(scmd); > + if (rtn != SCSI_RETURN_NOT_HANDLED) > + return rtn; > + } > + > /* > * first check the host byte, to see if there is anything in > there > * that would indicate what we need to do. > diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c > index 497efc0da259..56aefe38d69b 100644 > --- a/drivers/scsi/scsi_lib.c > +++ b/drivers/scsi/scsi_lib.c > @@ -1608,6 +1608,7 @@ static blk_status_t scsi_prepare_cmd(struct > request *req) > > /* Usually overridden by the ULP */ > cmd->allowed = 0; > + cmd->failures = NULL; > memset(cmd->cmnd, 0, sizeof(cmd->cmnd)); > return scsi_cmd_to_driver(cmd)->init_command(cmd); > } > diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h > index bac55decf900..9fb85932d5b9 100644 > --- a/include/scsi/scsi_cmnd.h > +++ b/include/scsi/scsi_cmnd.h > @@ -65,6 +65,23 @@ enum scsi_cmnd_submitter { > SUBMITTED_BY_SCSI_RESET_IOCTL = 2, > } __packed; > > +#define SCMD_FAILURE_NONE 0 > +#define SCMD_FAILURE_ANY -1 > +#define SCMD_FAILURE_SENSE_ANY 0xff > +#define SCMD_FAILURE_ASC_ANY 0xff > +#define SCMD_FAILURE_ASCQ_ANY 0xff > +#define SCMD_FAILURE_NO_LIMIT -1 > + > +struct scsi_failure { > + int result; > + u8 sense; > + u8 asc; > + u8 ascq; > + > + s8 allowed; > + s8 retries; > +}; > + > struct scsi_cmnd { > struct scsi_device *device; > struct list_head eh_entry; /* entry for the host > eh_abort_list/eh_cmd_q */ > @@ -85,6 +102,8 @@ struct scsi_cmnd { > > int retries; > int allowed; > + /* optional array of failures that passthrough users want > retried */ > + struct scsi_failure *failures; > > unsigned char prot_op; > unsigned char prot_type; > @@ -330,9 +349,14 @@ static inline void set_status_byte(struct > scsi_cmnd *cmd, char status) > cmd->result = (cmd->result & 0xffffff00) | status; > } > > +static inline u8 __get_status_byte(int result) > +{ > + return result & 0xff; > +} > + > static inline u8 get_status_byte(struct scsi_cmnd *cmd) > { > - return cmd->result & 0xff; > + return __get_status_byte(cmd->result); > } > > static inline void set_host_byte(struct scsi_cmnd *cmd, char status)
On 9/29/22 6:00 AM, Martin Wilck wrote: >> + if (!failure->result) >> + break; >> + >> + if (failure->result == SCMD_FAILURE_ANY) >> + goto maybe_retry; >> + >> + if (host_byte(scmd->result) & host_byte(failure- >>> result)) { >> + goto maybe_retry; > > Using "&" here needs explanation. The host byte is not a bit mask. > >> + } else if (get_status_byte(scmd) & >> + __get_status_byte(failure->result)) { > > See above. > I had a brain far for host/status bytes. Will fix throughout the set.
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c index 3f630798d1eb..4bf7b65bc63d 100644 --- a/drivers/scsi/scsi_error.c +++ b/drivers/scsi/scsi_error.c @@ -1831,6 +1831,63 @@ bool scsi_noretry_cmd(struct scsi_cmnd *scmd) return false; } +static enum scsi_disposition scsi_check_passthrough(struct scsi_cmnd *scmd) +{ + struct scsi_failure *failure; + struct scsi_sense_hdr sshdr; + enum scsi_disposition ret; + int i = 0; + + if (!scmd->result || !scmd->failures) + return SCSI_RETURN_NOT_HANDLED; + + while (1) { + failure = &scmd->failures[i++]; + if (!failure->result) + break; + + if (failure->result == SCMD_FAILURE_ANY) + goto maybe_retry; + + if (host_byte(scmd->result) & host_byte(failure->result)) { + goto maybe_retry; + } else if (get_status_byte(scmd) & + __get_status_byte(failure->result)) { + if (get_status_byte(scmd) != SAM_STAT_CHECK_CONDITION || + failure->sense == SCMD_FAILURE_SENSE_ANY) + goto maybe_retry; + + ret = scsi_start_sense_processing(scmd, &sshdr); + if (ret == NEEDS_RETRY) + goto maybe_retry; + else if (ret != SUCCESS) + return ret; + + if (failure->sense != sshdr.sense_key) + continue; + + if (failure->asc == SCMD_FAILURE_ASC_ANY) + goto maybe_retry; + + if (failure->asc != sshdr.asc) + continue; + + if (failure->ascq == SCMD_FAILURE_ASCQ_ANY || + failure->ascq == sshdr.ascq) + goto maybe_retry; + } + } + + return SCSI_RETURN_NOT_HANDLED; + +maybe_retry: + if (failure->allowed == SCMD_FAILURE_NO_LIMIT || + ++failure->retries <= failure->allowed) + return NEEDS_RETRY; + + return SUCCESS; +} + /** * scsi_decide_disposition - Disposition a cmd on return from LLD. * @scmd: SCSI cmd to examine. @@ -1859,6 +1916,12 @@ enum scsi_disposition scsi_decide_disposition(struct scsi_cmnd *scmd) return SUCCESS; } + if (blk_rq_is_passthrough(scsi_cmd_to_rq(scmd))) { + rtn = scsi_check_passthrough(scmd); + if (rtn != SCSI_RETURN_NOT_HANDLED) + return rtn; + } + /* * first check the host byte, to see if there is anything in there * that would indicate what we need to do. diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c index 497efc0da259..56aefe38d69b 100644 --- a/drivers/scsi/scsi_lib.c +++ b/drivers/scsi/scsi_lib.c @@ -1608,6 +1608,7 @@ static blk_status_t scsi_prepare_cmd(struct request *req) /* Usually overridden by the ULP */ cmd->allowed = 0; + cmd->failures = NULL; memset(cmd->cmnd, 0, sizeof(cmd->cmnd)); return scsi_cmd_to_driver(cmd)->init_command(cmd); } diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h index bac55decf900..9fb85932d5b9 100644 --- a/include/scsi/scsi_cmnd.h +++ b/include/scsi/scsi_cmnd.h @@ -65,6 +65,23 @@ enum scsi_cmnd_submitter { SUBMITTED_BY_SCSI_RESET_IOCTL = 2, } __packed; +#define SCMD_FAILURE_NONE 0 +#define SCMD_FAILURE_ANY -1 +#define SCMD_FAILURE_SENSE_ANY 0xff +#define SCMD_FAILURE_ASC_ANY 0xff +#define SCMD_FAILURE_ASCQ_ANY 0xff +#define SCMD_FAILURE_NO_LIMIT -1 + +struct scsi_failure { + int result; + u8 sense; + u8 asc; + u8 ascq; + + s8 allowed; + s8 retries; +}; + struct scsi_cmnd { struct scsi_device *device; struct list_head eh_entry; /* entry for the host eh_abort_list/eh_cmd_q */ @@ -85,6 +102,8 @@ struct scsi_cmnd { int retries; int allowed; + /* optional array of failures that passthrough users want retried */ + struct scsi_failure *failures; unsigned char prot_op; unsigned char prot_type; @@ -330,9 +349,14 @@ static inline void set_status_byte(struct scsi_cmnd *cmd, char status) cmd->result = (cmd->result & 0xffffff00) | status; } +static inline u8 __get_status_byte(int result) +{ + return result & 0xff; +} + static inline u8 get_status_byte(struct scsi_cmnd *cmd) { - return cmd->result & 0xff; + return __get_status_byte(cmd->result); } static inline void set_host_byte(struct scsi_cmnd *cmd, char status)
For passthrough, we don't retry any error we get a check condition for. This results in a lot of callers driving their own retries for those types of errors and retrying all errors, and there has been a request to retry specific host byte errors. This adds the core code to allow passthrough users to specify what errors they want scsi-ml to retry for them. We can then convert users to drop their sense parsing and retry handling. Signed-off-by: Mike Christie <michael.christie@oracle.com> --- drivers/scsi/scsi_error.c | 63 +++++++++++++++++++++++++++++++++++++++ drivers/scsi/scsi_lib.c | 1 + include/scsi/scsi_cmnd.h | 26 +++++++++++++++- 3 files changed, 89 insertions(+), 1 deletion(-)