Re: [PATCH v3 09/18] scsi: sd: handle read/write CDL timeout failures

[Niklas Cassel <Niklas.Cassel@wdc.com>]

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
>