diff mbox

[v2,3/6] scsi_io_completion_nz_result function added

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

Commit Message

Douglas Gilbert March 18, 2018, 8:59 p.m. UTC
Break out several interwined paths when cmd->result is non zero and
place them in scsi_io_completion_nz_result helper function. The logic
is not changed.

Signed-off-by: Douglas Gilbert <dgilbert@interlog.com>
---
A reviewer requested the original helper function's two return values
be reduced to one: the blk_stat variable. This required a hack to
differentiate the default setting of blk_stat (BLK_STS_OK) from the case
when the helper assigns BLK_STS_OK as the return value. The hack is to
return the otherwise unused BLK_STS_NOTSUPP value as an indication that
the helper didn't change anything.

 drivers/scsi/scsi_lib.c | 134 +++++++++++++++++++++++++++++-------------------
 1 file changed, 82 insertions(+), 52 deletions(-)

Comments

Johannes Thumshirn March 19, 2018, 8:59 a.m. UTC | #1
Looks good,
Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
Bart Van Assche March 26, 2018, 11:43 p.m. UTC | #2
On Sun, 2018-03-18 at 21:59 +0100, Douglas Gilbert wrote:
> Break out several interwined paths when cmd->result is non zero and

                    ^^^^^^^^^^
                    intertwined?
> place them in scsi_io_completion_nz_result helper function. The logic

> is not changed.

> [ ... ]

> A reviewer requested the original helper function's two return values

> be reduced to one: the blk_stat variable. This required a hack to

> differentiate the default setting of blk_stat (BLK_STS_OK) from the case

> when the helper assigns BLK_STS_OK as the return value. The hack is to

> return the otherwise unused BLK_STS_NOTSUPP value as an indication that

> the helper didn't change anything.


That sounds like bad advice to me ... I think the constructs in the current
version of this patch may lead to future maintainability problems. E.g. the
special value BLK_STS_NOTSUPP occurs in two different places. If anyone ever
wants to use another special value BLK_STS_NOTSUPP will have to be changed
into something else at two different locations. Additionally, if
__scsi_error_from_host_byte() ever would be modified to return BLK_STS_NOTSUPP
then the code introduced by this patch will break in a subtle way and it will
be really hard to figure out what went wrong and why something went wrong.

The approach of this patch however looks fine to me. scsi_io_completion() is
too long so I think it's certainly a good idea to split it into multiple
functions.

Thanks,

Bart.
diff mbox

Patch

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index a4da5773d42d..9731d0d3cc80 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -751,6 +751,77 @@  static blk_status_t __scsi_error_from_host_byte(struct scsi_cmnd *cmd,
 	}
 }
 
+/*
+ * Helper for scsi_io_completion() when cmd->result is non-zero. Returns
+ * BLK_STS_NOTSUPP if this function does not change blk_status .
+ */
+static blk_status_t scsi_io_completion_nz_result(struct scsi_cmnd *cmd,
+						 int result)
+{
+	bool sense_valid;
+	bool about_current;
+	/* __scsi_error_from_host_byte() does not return BLK_STS_NOTSUPP */
+	blk_status_t blk_stat = BLK_STS_NOTSUPP;
+	struct request *req = cmd->request;
+	struct scsi_sense_hdr sshdr;
+
+	sense_valid = scsi_command_normalize_sense(cmd, &sshdr);
+	about_current = sense_valid ? !scsi_sense_is_deferred(&sshdr) : true;
+
+	if (blk_rq_is_passthrough(req)) {
+		if (sense_valid) {
+			/*
+			 * SG_IO wants current and deferred errors
+			 */
+			scsi_req(req)->sense_len =
+				min(8 + cmd->sense_buffer[7],
+				    SCSI_SENSE_BUFFERSIZE);
+		}
+		if (about_current)
+			blk_stat = __scsi_error_from_host_byte(cmd, result);
+	} else if (blk_rq_bytes(req) == 0 && about_current) {
+		/*
+		 * Flush commands do not transfers any data, and thus cannot use
+		 * good_bytes != blk_rq_bytes(req) as the signal for an error.
+		 * This sets blk_stat explicitly for the problem case.
+		 */
+		blk_stat = __scsi_error_from_host_byte(cmd, result);
+	}
+	/*
+	 * Recovered errors need reporting, but they're always treated as
+	 * success, so fiddle the result code here.  For passthrough requests
+	 * 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)) {
+		bool do_print = true;
+		/*
+		 * if ATA PASS-THROUGH INFORMATION AVAILABLE [0x0, 0x1d]
+		 * 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))
+			do_print = false;
+		else if (req->rq_flags & RQF_QUIET)
+			do_print = false;
+		if (do_print)
+			scsi_print_sense(cmd);
+		/* for passthrough, blk_stat may be set */
+		blk_stat = 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.
+	 */
+	if (status_byte(result) && scsi_status_is_good(result))
+		blk_stat = BLK_STS_OK;
+
+	return blk_stat;
+}
+
 /*
  * Function:    scsi_io_completion()
  *
@@ -794,26 +865,23 @@  void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
 	      ACTION_DELAYED_RETRY} action;
 	unsigned long wait_for = (cmd->allowed + 1) * req->timeout;
 
-	if (result) {
+	if (result) {	/* does not necessarily mean there is an error */
 		sense_valid = scsi_command_normalize_sense(cmd, &sshdr);
 		if (sense_valid)
 			sense_current = !scsi_sense_is_deferred(&sshdr);
+		blk_stat = scsi_io_completion_nz_result(cmd, result);
+		if (blk_stat == BLK_STS_OK)
+			result = 0;	/* suppress error processing now */
+		/*
+		 * if scsi_io_completion_nz_result() made no change,
+		 * re-instate initial blk_stat value.
+		 */
+		if (blk_stat == BLK_STS_NOTSUPP)
+			blk_stat = BLK_STS_OK;
+
 	}
 
 	if (blk_rq_is_passthrough(req)) {
-		if (result) {
-			if (sense_valid) {
-				/*
-				 * SG_IO wants current and deferred errors
-				 */
-				scsi_req(req)->sense_len =
-					min(8 + cmd->sense_buffer[7],
-					    SCSI_SENSE_BUFFERSIZE);
-			}
-			if (sense_current)
-				blk_stat = __scsi_error_from_host_byte(cmd,
-								       result);
-		}
 		/*
 		 * __scsi_error_from_host_byte may have reset the host_byte
 		 */
@@ -831,13 +899,6 @@  void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
 				BUG();
 			return;
 		}
-	} else if (blk_rq_bytes(req) == 0 && result && sense_current) {
-		/*
-		 * Flush commands do not transfers any data, and thus cannot use
-		 * good_bytes != blk_rq_bytes(req) as the signal for an error.
-		 * This sets blk_stat explicitly for the problem case.
-		 */
-		blk_stat = __scsi_error_from_host_byte(cmd, result);
 	}
 
 	/* no bidi support for !blk_rq_is_passthrough yet */
@@ -851,37 +912,6 @@  void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
 		"%u sectors total, %d bytes done.\n",
 		blk_rq_sectors(req), good_bytes));
 
-	/*
-	 * Recovered errors need reporting, but they're always treated as
-	 * success, so fiddle the result code here.  For passthrough requests
-	 * 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, blk_stat may be set */
-		blk_stat = 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.
-	 */
-	if (status_byte(result) && scsi_status_is_good(result)) {
-		result = 0;
-		blk_stat = BLK_STS_OK;
-	}
-
 	/*
 	 * Next deal with any sectors which we were able to correctly
 	 * handle. Failed, zero length commands always need to drop down