Hello Hannes,
On Fri, Jan 27, 2023 at 04:34:59PM +0100, Hannes Reinecke wrote:
> > @@ -691,6 +708,15 @@ enum scsi_disposition scsi_check_sense(struct scsi_cmnd *scmd)
> > }
> > return SUCCESS;
> > + case COMPLETED:
> > + if (sshdr.asc == 0x55 && sshdr.ascq == 0x0a) {
> > + set_scsi_ml_byte(scmd, SCSIML_STAT_DL_TIMEOUT);
> > + req->cmd_flags |= REQ_FAILFAST_DEV;
> > + req->rq_flags |= RQF_QUIET;
> > + return SUCCESS;
>
> You can kill this line, will be done anyway.
Thanks, we will fix in next revision.
>
> > + }
> > + return SUCCESS;
> > +
> > default:
> > return SUCCESS;
> > }
> > @@ -785,6 +811,14 @@ static enum scsi_disposition scsi_eh_completed_normally(struct scsi_cmnd *scmd)
> > switch (get_status_byte(scmd)) {
> > case SAM_STAT_GOOD:
> > scsi_handle_queue_ramp_up(scmd->device);
> > + if (scmd->sense_buffer && SCSI_SENSE_VALID(scmd))
> > + /*
> > + * If we have sense data, call scsi_check_sense() in
> > + * order to set the correct SCSI ML byte (if any).
> > + * No point in checking the return value, since the
> > + * command has already completed successfully.
> > + */
> > + scsi_check_sense(scmd);
>
> I am every so slightly nervous here.
> We never checked the sense code for GOOD status, so heaven knows if there
> are devices out there which return something here.
Well, right now we have had quite the opposite problem, to even allow
sense data while not being a CHECK_COMMAND, since they have been very
intertwined historically.
That alone makes me seriously doubt that this is going to be a problem.
But if there is a device that returns sense data when it shouldn't,
that is obviously a spec violation, and I assume that we would deal
with it in the same way we usually do, i.e. a device that does not
follow the spec has to be quirked. However, right now, I think that
it is sheer speculation that such a device even exists.
> And you have checked that we've cleared the sense code before submitting (or
> retrying, even), right?
Yes, scsi_queue_rq() does an unconditionall memset(cmd->sense_buffer, ...).
A retried command will call scsi_queue_insert() -> blk_mq_requeue_request(),
which will eventually reach blk_mq_dispatch_rq_list(), which will again lead
to scsi_queue_insert() getting called (which will memset the sense_buffer).
>
> > fallthrough;
> > case SAM_STAT_COMMAND_TERMINATED:
> > return SUCCESS;
> > @@ -1807,6 +1841,10 @@ bool scsi_noretry_cmd(struct scsi_cmnd *scmd)
> > return !!(req->cmd_flags & REQ_FAILFAST_DRIVER);
> > }
> > + /* Never retry commands aborted due to a duration limit timeout */
> > + if (scsi_ml_byte(scmd->result) == SCSIML_STAT_DL_TIMEOUT)
> > + return true;
> > +
> > if (!scsi_status_is_check_condition(scmd->result))
> > return false;
> > @@ -1966,6 +2004,14 @@ enum scsi_disposition scsi_decide_disposition(struct scsi_cmnd *scmd)
> > if (scmd->cmnd[0] == REPORT_LUNS)
> > scmd->device->sdev_target->expecting_lun_change = 0;
> > scsi_handle_queue_ramp_up(scmd->device);
> > + if (scmd->sense_buffer && SCSI_SENSE_VALID(scmd))
> > + /*
> > + * If we have sense data, call scsi_check_sense() in
> > + * order to set the correct SCSI ML byte (if any).
> > + * No point in checking the return value, since the
> > + * command has already completed successfully.
> > + */
> > + scsi_check_sense(scmd);
> > fallthrough;
> > case SAM_STAT_COMMAND_TERMINATED:
> > return SUCCESS;
> > diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> > index e1a021dd4da2..406952e72a68 100644
> > --- a/drivers/scsi/scsi_lib.c
> > +++ b/drivers/scsi/scsi_lib.c
> > @@ -600,6 +600,8 @@ static blk_status_t scsi_result_to_blk_status(int result)
> > return BLK_STS_MEDIUM;
> > case SCSIML_STAT_TGT_FAILURE:
> > return BLK_STS_TARGET;
> > + case SCSIML_STAT_DL_TIMEOUT:
> > + return BLK_STS_DURATION_LIMIT;
> > }
> > switch (host_byte(result)) {
> > @@ -797,6 +799,8 @@ static void scsi_io_completion_action(struct scsi_cmnd *cmd, int result)
> > blk_stat = BLK_STS_ZONE_OPEN_RESOURCE;
> > }
> > break;
> > + case COMPLETED:
> > + fallthrough;
> > default:
> > action = ACTION_FAIL;
> > break;
> > diff --git a/drivers/scsi/scsi_priv.h b/drivers/scsi/scsi_priv.h
> > index 74324fba4281..f42388ecb024 100644
> > --- a/drivers/scsi/scsi_priv.h
> > +++ b/drivers/scsi/scsi_priv.h
> > @@ -27,6 +27,7 @@ enum scsi_ml_status {
> > SCSIML_STAT_NOSPC = 0x02, /* Space allocation on the dev failed */
> > SCSIML_STAT_MED_ERROR = 0x03, /* Medium error */
> > SCSIML_STAT_TGT_FAILURE = 0x04, /* Permanent target failure */
> > + SCSIML_STAT_DL_TIMEOUT = 0x05, /* Command Duration Limit timeout */
> > };
> > static inline u8 scsi_ml_byte(int result)
>
> Cheers,
>
> Hannes
>
