Re: [RFC PATCH 2/5] break sd_done sense processing out to own function

[Douglas Gilbert <dgilbert@xxxxxxxxxxxx>]

On 2018-08-06 01:41 AM, Damien Le Moal wrote:
> On 2018/08/06 13:51, Douglas Gilbert wrote:
> > Break out the sense key handling in the sd_done() (response) path into
> > its own function. Note that the sense key only needs to be inspected
> > when a SCSI status of Check Condition is returned.
>
> It looks like sd_done_sense() is called for any command that has sense data.
> Check Condition or not. This should be clarified.

Yes. It is only recent SAM documents that have reduced the SCSI
statuses that yield sense data to just CHECK CONDITION. Also as
the code shows, just setting DRIVER_SENSE in the result (irrespective
of the SCSI status) will cause the sense data to be checked.

So that can be expended.

Doug Gilbert

> Other than that, this looks OK to me.
>
> > Signed-off-by: Douglas Gilbert <dgilbert@xxxxxxxxxxxx>
> > ---
> >
> > No speed up here, just a clean up. There could possibly be a speed
> > improvement (not observed in tests) from lessening the cache footprint
> > of the sd_done() function which is on the fast path.
> >
> >
> >   drivers/scsi/sd.c | 112 ++++++++++++++++++++++++----------------------
> >   1 file changed, 59 insertions(+), 53 deletions(-)
> >
> > diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> > index b17b8c66881d..4b1402633c36 100644
> > --- a/drivers/scsi/sd.c
> > +++ b/drivers/scsi/sd.c
> > @@ -1898,6 +1898,62 @@ static unsigned int sd_completed_bytes(struct scsi_cmnd *scmd)
> >   	return min(good_bytes, transferred);
> >   }
> >   
> > +/* Helper for scsi_done() when there is a sense buffer */
> > +static int sd_done_sense(struct scsi_cmnd *SCpnt, unsigned int good_bytes,
> > +			 struct scsi_sense_hdr *sshdrp)
> > +{
> > +	struct request *req = SCpnt->request;
> > +	struct scsi_disk *sdkp = scsi_disk(SCpnt->request->rq_disk);
> > +
> > +	switch (sshdrp->sense_key) {
> > +	case HARDWARE_ERROR:
> > +	case MEDIUM_ERROR:
> > +		return sd_completed_bytes(SCpnt);
> > +	case RECOVERED_ERROR:
> > +		return scsi_bufflen(SCpnt);
> > +	case NO_SENSE:
> > +		/* This indicates a false check condition, so ignore it.  An
> > +		 * unknown amount of data was transferred so treat it as an
> > +		 * error.
> > +		 */
> > +		SCpnt->result = 0;
> > +		memset(SCpnt->sense_buffer, 0, SCSI_SENSE_BUFFERSIZE);
> > +		break;
> > +	case ABORTED_COMMAND:
> > +		if (sshdrp->asc == 0x10)  /* DIF: Target detected corruption */
> > +			good_bytes = sd_completed_bytes(SCpnt);
> > +		break;
> > +	case ILLEGAL_REQUEST:
> > +		switch (sshdrp->asc) {
> > +		case 0x10:	/* DIX: Host detected corruption */
> > +			good_bytes = sd_completed_bytes(SCpnt);
> > +			break;
> > +		case 0x20:	/* INVALID COMMAND OPCODE */
> > +		case 0x24:	/* INVALID FIELD IN CDB */
> > +			switch (SCpnt->cmnd[0]) {
> > +			case UNMAP:
> > +				sd_config_discard(sdkp, SD_LBP_DISABLE);
> > +				break;
> > +			case WRITE_SAME_16:
> > +			case WRITE_SAME:
> > +				if (SCpnt->cmnd[1] & 8) { /* UNMAP */
> > +					sd_config_discard(sdkp, SD_LBP_DISABLE);
> > +				} else {
> > +					sdkp->device->no_write_same = 1;
> > +					sd_config_write_same(sdkp);
> > +					req->__data_len = blk_rq_bytes(req);
> > +					req->rq_flags |= RQF_QUIET;
> > +				}
> > +				break;
> > +			}
> > +		}
> > +		break;
> > +	default:
> > +		break;
> > +	}
> > +	return good_bytes;
> > +}
> > +
> >   /**
> >    *	sd_done - bottom half handler: called when the lower level
> >    *	driver has completed (successfully or otherwise) a scsi command.
> > @@ -1964,60 +2020,10 @@ static int sd_done(struct scsi_cmnd *SCpnt)
> >   	}
> >   	sdkp->medium_access_timed_out = 0;
> >   
> > -	if (driver_byte(result) != DRIVER_SENSE &&
> > -	    (!sense_valid || sense_deferred))
> > -		goto out;
> > +	if (unlikely(driver_byte(result) == DRIVER_SENSE ||
> > +		     (sense_valid && !sense_deferred)))
> > +		good_bytes = sd_done_sense(SCpnt, good_bytes, &sshdr);
> >   
> > -	switch (sshdr.sense_key) {
> > -	case HARDWARE_ERROR:
> > -	case MEDIUM_ERROR:
> > -		good_bytes = sd_completed_bytes(SCpnt);
> > -		break;
> > -	case RECOVERED_ERROR:
> > -		good_bytes = scsi_bufflen(SCpnt);
> > -		break;
> > -	case NO_SENSE:
> > -		/* This indicates a false check condition, so ignore it.  An
> > -		 * unknown amount of data was transferred so treat it as an
> > -		 * error.
> > -		 */
> > -		SCpnt->result = 0;
> > -		memset(SCpnt->sense_buffer, 0, SCSI_SENSE_BUFFERSIZE);
> > -		break;
> > -	case ABORTED_COMMAND:
> > -		if (sshdr.asc == 0x10)  /* DIF: Target detected corruption */
> > -			good_bytes = sd_completed_bytes(SCpnt);
> > -		break;
> > -	case ILLEGAL_REQUEST:
> > -		switch (sshdr.asc) {
> > -		case 0x10:	/* DIX: Host detected corruption */
> > -			good_bytes = sd_completed_bytes(SCpnt);
> > -			break;
> > -		case 0x20:	/* INVALID COMMAND OPCODE */
> > -		case 0x24:	/* INVALID FIELD IN CDB */
> > -			switch (SCpnt->cmnd[0]) {
> > -			case UNMAP:
> > -				sd_config_discard(sdkp, SD_LBP_DISABLE);
> > -				break;
> > -			case WRITE_SAME_16:
> > -			case WRITE_SAME:
> > -				if (SCpnt->cmnd[1] & 8) { /* UNMAP */
> > -					sd_config_discard(sdkp, SD_LBP_DISABLE);
> > -				} else {
> > -					sdkp->device->no_write_same = 1;
> > -					sd_config_write_same(sdkp);
> > -					req->__data_len = blk_rq_bytes(req);
> > -					req->rq_flags |= RQF_QUIET;
> > -				}
> > -				break;
> > -			}
> > -		}
> > -		break;
> > -	default:
> > -		break;
> > -	}
> > -
> > - out:
> >   	if (sd_is_zoned(sdkp))
> >   		sd_zbc_complete(SCpnt, good_bytes, &sshdr);
> >