Re: [PATCH v2 02/35] scsi: Allow passthrough to override what errors to retry

[Martin Wilck <mwilck@suse.com>]

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)