diff mbox

Make SCSI Status CONDITION MET equivalent to GOOD

Message ID 20180222014807.5329-1-dgilbert@interlog.com (mailing list archive)
State Superseded
Headers show

Commit Message

Douglas Gilbert Feb. 22, 2018, 1:48 a.m. UTC
The SCSI PRE-FETCH (10 or 16) command is present both on hard disks
and some SSDs. It is useful when the address of the next block(s) to
be read is known but it is not following the LBA of the current READ
(so read-ahead won't help). It returns two "good" SCSI Status values.
If the requested blocks have fitted (or will most likely fit (when
the IMMED bit is set)) into the disk's cache, it returns CONDITION
MET. If is didn't (or will not) fit then it returns GOOD status.

Currently the SCSI sub-system treats CONDITION MET as an error. It
has a purpose built inline function called scsi_status_is_good()
that doesn't check for CONDITION MET so it treats it as an error.

Two things may be done to handle this case:
   1) stop treating CONDITION MET as an error
   2) decide whether the block layer needs to know the difference
      between the two "good' statuses. If so, build a mechanism
      to convey that information

This patch addresses point 1).

ChangeLog:
  - expand scsi_status_is_good() to check for CONDITION MET
  - add another corner case in scsi_io_completion() adjacent
    to the one for the RECOVERED ERROR sense key case. That
    is another "non-error"
  - structure code so extra checks are only on the error
    path (i.e. when cmd->result is non-zero)

This patch is against mkp's 4.17/scsi-queue branch. It also applies
to lk 4.15.x where it was tested on a SAS SSD.


Signed-off-by: Douglas Gilbert <dgilbert@interlog.com>
---
 drivers/scsi/scsi_lib.c | 38 ++++++++++++++++++++++++++------------
 include/scsi/scsi.h     |  2 ++
 2 files changed, 28 insertions(+), 12 deletions(-)

Comments

Bart Van Assche Feb. 23, 2018, 10:19 p.m. UTC | #1
On Wed, 2018-02-21 at 20:48 -0500, Douglas Gilbert wrote:
> +	if (result) {

> +		if (sense_valid && (sshdr.sense_key == RECOVERED_ERROR)) {

> +			/* if ATA PASS-THROUGH INFORMATION AVAILABLE skip

> +			 * print since caller wants ATA registers. Only occurs

> +			 * on SCSI ATA PASS_THROUGH commands when CK_COND=1

> +			 */

> +			if ((sshdr.asc == 0x0) && (sshdr.ascq == 0x1d))

> +				;

> +			else if (!(req->rq_flags & RQF_QUIET))

> +				scsi_print_sense(cmd);

> +			result = 0;

> +			/* for passthrough error may be set */

> +			error = BLK_STS_OK;

> +	/*

> +	 * Another corner case: the SCSI status byte is non-zero but 'good'.

> +	 * Example: PRE-FETCH command returns SAM_STAT_CONDITION_MET when

> +	 * it is able to fit nominated LBs in its cache (and SAM_STAT_GOOD

> +	 * if it can't fit). Treat SAM_STAT_CONDITION_MET and the related

> +	 * intermediate statuses (both obsolete in SAM-4) as good.

> +	 */

> +		} else if (status_byte(result) &&

> +			   scsi_status_is_good(result)) {

> +			result = 0;

> +			/* for passthrough error may be set */

> +			error = BLK_STS_OK;

> +		}

>  	}


Please move the new comment block under "} else if" and indent it further to
the right such that neither gcc with W=1 nor smatch complain about incorrect
indentation.

Please also change the branching logic as follows to keep the indentation level
low:

	if (sense_valid && (sshdr.sense_key == RECOVERED_ERROR)) {
		[ ... ]
	} else if (status_byte(result) && scsi_status_is_good(result)) {
		[ ... ]
	}

Thanks,

Bart.
diff mbox

Patch

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index aea5a1ae318b..e25b288bc17c 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -842,18 +842,32 @@  void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
 	 * we already took a copy of the original into sreq->result which
 	 * is what gets returned to the user
 	 */
-	if (sense_valid && (sshdr.sense_key == RECOVERED_ERROR)) {
-		/* if ATA PASS-THROUGH INFORMATION AVAILABLE skip
-		 * print since caller wants ATA registers. Only occurs on
-		 * SCSI ATA PASS_THROUGH commands when CK_COND=1
-		 */
-		if ((sshdr.asc == 0x0) && (sshdr.ascq == 0x1d))
-			;
-		else if (!(req->rq_flags & RQF_QUIET))
-			scsi_print_sense(cmd);
-		result = 0;
-		/* for passthrough error may be set */
-		error = BLK_STS_OK;
+	if (result) {
+		if (sense_valid && (sshdr.sense_key == RECOVERED_ERROR)) {
+			/* if ATA PASS-THROUGH INFORMATION AVAILABLE skip
+			 * print since caller wants ATA registers. Only occurs
+			 * on SCSI ATA PASS_THROUGH commands when CK_COND=1
+			 */
+			if ((sshdr.asc == 0x0) && (sshdr.ascq == 0x1d))
+				;
+			else if (!(req->rq_flags & RQF_QUIET))
+				scsi_print_sense(cmd);
+			result = 0;
+			/* for passthrough error may be set */
+			error = BLK_STS_OK;
+	/*
+	 * Another corner case: the SCSI status byte is non-zero but 'good'.
+	 * Example: PRE-FETCH command returns SAM_STAT_CONDITION_MET when
+	 * it is able to fit nominated LBs in its cache (and SAM_STAT_GOOD
+	 * if it can't fit). Treat SAM_STAT_CONDITION_MET and the related
+	 * intermediate statuses (both obsolete in SAM-4) as good.
+	 */
+		} else if (status_byte(result) &&
+			   scsi_status_is_good(result)) {
+			result = 0;
+			/* for passthrough error may be set */
+			error = BLK_STS_OK;
+		}
 	}
 
 	/*
diff --git a/include/scsi/scsi.h b/include/scsi/scsi.h
index cb85eddb47ea..eb7853c1a23b 100644
--- a/include/scsi/scsi.h
+++ b/include/scsi/scsi.h
@@ -47,6 +47,8 @@  static inline int scsi_status_is_good(int status)
 	 */
 	status &= 0xfe;
 	return ((status == SAM_STAT_GOOD) ||
+		(status == SAM_STAT_CONDITION_MET) ||
+		/* Next two "intermediate" statuses are obsolete in SAM-4 */
 		(status == SAM_STAT_INTERMEDIATE) ||
 		(status == SAM_STAT_INTERMEDIATE_CONDITION_MET) ||
 		/* FIXME: this is obsolete in SAM-3 */