Re: [PATCH v12 01/20] scsi: Allow passthrough to request scsi-ml retries

John Garry
2023-11-16T10:54:12Z

On 14/11/2023 01:37, Mike Christie wrote:
> For passthrough we don't retry any error we get a check condition for.

nit: For passthrough we don't retry any error which we get a check
condition for.

> This results in a lot of callers driving their own retries for all UAs,
> specific UAs, NOT_READY, specific sense values or any type of failure.
>
> 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
> a lot of their sense parsing and retry handling.
>
> Signed-off-by: Mike Christie <michael.christie@oracle.com>

Looks ok to me, so feel free to pick up:
Reviewed-by: John Garry <john.g.garry@oracle.com>

> ---
> drivers/scsi/scsi_lib.c | 92 ++++++++++++++++++++++++++++++++++++++
> include/scsi/scsi_device.h | 48 ++++++++++++++++++++
> 2 files changed, 140 insertions(+)
>
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index cf3864f72093..dee43c6f7ad0 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -184,6 +184,92 @@ void scsi_queue_insert(struct scsi_cmnd *cmd, int reason)
> __scsi_queue_insert(cmd, reason, true);
> }
>
> +void scsi_reset_failures(struct scsi_failures *failures)

nit: maybe scsi_reset_failures_retries as the name, to be more specific

> +{
> + struct scsi_failure *failure;
> +
> + failures->total_retries = 0;
> +
> + for (failure = failures->failure_definitions; failure->result;
> + failure++)
> + failure->retries = 0;
> +}
> +EXPORT_SYMBOL_GPL(scsi_reset_failures);
> +
> +/**
> + * scsi_check_passthrough - Determine if passthrough scsi_cmnd needs a retry.
> + * @scmd: scsi_cmnd to check.
> + * @failures: scsi_failures struct that lists failures to check for.
> + *
> + * Returns -EAGAIN if the caller should retry else 0.
> + */
> +static int scsi_check_passthrough(struct scsi_cmnd *scmd,
> + struct scsi_failures *failures)
> +{
> + struct scsi_failure *failure;
> + struct scsi_sense_hdr sshdr;
> + enum sam_status status;
> +
> + if (!failures)
> + return 0;
> +
> + for (failure = failures->failure_definitions; failure->result;
> + failure++) {
> + if (failure->result == SCMD_FAILURE_RESULT_ANY)
> + goto maybe_retry;
> +
> + if (host_byte(scmd->result) &&
> + host_byte(scmd->result) == host_byte(failure->result))
> + goto maybe_retry;
> +
> + status = status_byte(scmd->result);
> + if (!status)
> + continue;
> +
> + if (failure->result == SCMD_FAILURE_STAT_ANY &&
> + !scsi_status_is_good(scmd->result))
> + goto maybe_retry;
> +
> + if (status != status_byte(failure->result))
> + continue;
> +
> + if (status_byte(failure->result) != SAM_STAT_CHECK_CONDITION ||
> + failure->sense == SCMD_FAILURE_SENSE_ANY)
> + goto maybe_retry;
> +
> + if (!scsi_command_normalize_sense(scmd, &sshdr))
> + return 0;
> +
> + 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 0;
> +
> +maybe_retry:
> + if (failure->allowed) {
> + if (failure->allowed == SCMD_FAILURE_NO_LIMIT ||
> + ++failure->retries <= failure->allowed)
> + return -EAGAIN;
> + } else {
> + if (failures->total_allowed == SCMD_FAILURE_NO_LIMIT ||
> + ++failures->total_retries <= failures->total_allowed)
> + return -EAGAIN;
> + }
> +
> + return 0;
> +}
> +
> /**
> * scsi_execute_cmd - insert request and wait for the result
> * @sdev: scsi_device
> @@ -214,6 +300,7 @@ int scsi_execute_cmd(struct scsi_device *sdev, const unsigned char *cmd,
> args->sense_len != SCSI_SENSE_BUFFERSIZE))
> return -EINVAL;
>
> +retry:
> req = scsi_alloc_request(sdev->request_queue, opf, args->req_flags);
> if (IS_ERR(req))
> return PTR_ERR(req);
> @@ -237,6 +324,11 @@ int scsi_execute_cmd(struct scsi_device *sdev, const unsigned char *cmd,
> */
> blk_execute_rq(req, true);
>
> + if (scsi_check_passthrough(scmd, args->failures) == -EAGAIN) {
> + blk_mq_free_request(req);
> + goto retry;
> + }
> +
> /*
> * Some devices (USB mass-storage in particular) may transfer
> * garbage data together with a residue indicating that the data
> diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
> index 10480eb582b2..c92d6d9e644e 100644
> --- a/include/scsi/scsi_device.h
> +++ b/include/scsi/scsi_device.h
> @@ -483,6 +483,52 @@ extern int scsi_is_sdev_device(const struct device *);
> extern int scsi_is_target_device(const struct device *);
> extern void scsi_sanitize_inquiry_string(unsigned char *s, int len);
>
> +/*
> + * scsi_execute_cmd users can set scsi_failure.result to have
> + * scsi_check_passthrough fail/retry a command. scsi_failure.result can be a
> + * specific host byte or message code, or SCMD_FAILURE_RESULT_ANY can be used
> + * to match any host or message code.
> + */
> +#define SCMD_FAILURE_RESULT_ANY 0x7fffffff
> +/*
> + * Set scsi_failure.result to SCMD_FAILURE_STAT_ANY to fail/retry any failure
> + * scsi_status_is_good returns false for.
> + */
> +#define SCMD_FAILURE_STAT_ANY 0xff
> +/*
> + * The following can be set to the scsi_failure sense, asc and ascq fields to
> + * match on any sense, ASC, or ASCQ value.
> + */
> +#define SCMD_FAILURE_SENSE_ANY 0xff
> +#define SCMD_FAILURE_ASC_ANY 0xff
> +#define SCMD_FAILURE_ASCQ_ANY 0xff
> +/* Always retry a matching failure. */
> +#define SCMD_FAILURE_NO_LIMIT -1
> +
> +struct scsi_failure {
> + int result;
> + u8 sense;
> + u8 asc;
> + u8 ascq;
> +
> + /*
> + * Number of attempts allowed for this failure. It does not count
> + * for the total_allowed.
> + */
> + s8 allowed;
> + s8 retries;
> +};
> +
> +struct scsi_failures {
> + /*
> + * If the failure does not have a specific limit in the scsi_failure
> + * then this limit is followed.
> + */
> + int total_allowed;
> + int total_retries;
> + struct scsi_failure *failure_definitions;
> +};
> +
> /* Optional arguments to scsi_execute_cmd */
> struct scsi_exec_args {
> unsigned char *sense; /* sense buffer */
> @@ -491,12 +537,14 @@ struct scsi_exec_args {
> blk_mq_req_flags_t req_flags; /* BLK_MQ_REQ flags */
> int scmd_flags; /* SCMD flags */
> int *resid; /* residual length */
> + struct scsi_failures *failures; /* failures to retry */
> };
>
> int scsi_execute_cmd(struct scsi_device *sdev, const unsigned char *cmd,
> blk_opf_t opf, void *buffer, unsigned int bufflen,
> int timeout, int retries,
> const struct scsi_exec_args *args);
> +void scsi_reset_failures(struct scsi_failures *failures);
>
> extern void sdev_disable_disk_events(struct scsi_device *sdev);
> extern void sdev_enable_disk_events(struct scsi_device *sdev);