[v2,4/6] scsi_io_completion_action helper added

Commit Message

Douglas Gilbert March 18, 2018, 8:59 p.m. UTC

Place scsi_io_completion()'s complex error processing associated with a
local enumeration into a static helper function. That enumeration's
values start with "ACTION_" so use the suffix "_action" in the helper
function's name.

Signed-off-by: Douglas Gilbert <[e-mail]>
Reviewed-by: Johannes Thumshirn <[e-mail]>
---
Replicate the ACTION_REPREP case code since the former 'goto requeue'
would now need to jump into another function which is not permitted.

 drivers/scsi/scsi_lib.c | 322 ++++++++++++++++++++++++++----------------------
 1 file changed, 172 insertions(+), 150 deletions(-)

Comments

Bart Van Assche March 27, 2018, 12:13 a.m. UTC | #1

On Sun, 2018-03-18 at 21:59 +0100, Douglas Gilbert wrote:
> +	/* sense not about current command is termed: deferred */


Do we really need comments that explain the SCSI specs? If such a comment is
added I think it should be added above the definition of scsi_sense_is_deferred()
together with a reference to the "Sense data" section in SPC.

> +	if (result == 0) {

> +		/*

> +		 * Unprep the request and put it back at the head of the

> +		 * queue. A new command will be prepared and issued.

> +		 * This block is the same as case ACTION_REPREP in

> +		 * scsi_io_completion_action() above.

>  		 */

> -		if (q->mq_ops) {

> +		if (q->mq_ops)

>  			scsi_mq_requeue_cmd(cmd);

> -		} else {

> +		else {

>  			scsi_release_buffers(cmd);

>  			scsi_requeue_command(q, cmd);

>  		}


Have these changes been verified with checkpatch? Checkpatch should have reported
the following about the above chunk of code: Unbalanced braces around else statement.
Additionally, have you considered to introduce a new function instead of duplicating
existing code?

Otherwise this patch looks fine to me.

Thanks,

Bart.

Douglas Gilbert March 27, 2018, 1:36 p.m. UTC | #2

On 2018-03-26 08:13 PM, Bart Van Assche wrote:
> On Sun, 2018-03-18 at 21:59 +0100, Douglas Gilbert wrote:
>> +	/* sense not about current command is termed: deferred */
> 
> Do we really need comments that explain the SCSI specs? If such a comment is
> added I think it should be added above the definition of scsi_sense_is_deferred()
> together with a reference to the "Sense data" section in SPC.
> 
>> +	if (result == 0) {
>> +		/*
>> +		 * Unprep the request and put it back at the head of the
>> +		 * queue. A new command will be prepared and issued.
>> +		 * This block is the same as case ACTION_REPREP in
>> +		 * scsi_io_completion_action() above.
>>   		 */
>> -		if (q->mq_ops) {
>> +		if (q->mq_ops)
>>   			scsi_mq_requeue_cmd(cmd);
>> -		} else {
>> +		else {
>>   			scsi_release_buffers(cmd);
>>   			scsi_requeue_command(q, cmd);
>>   		}
> 
> Have these changes been verified with checkpatch? Checkpatch should have reported
> the following about the above chunk of code: Unbalanced braces around else statement.

Yes they were, did you check them? If so, with what command line options?
Since with no options <mkp-4.17/scsi-queue>/scripts/checkpatch.pl returns
clean for all patches in this set.

> Additionally, have you considered to introduce a new function instead of duplicating
> existing code?

Yes, that could be done.

> Otherwise this patch looks fine to me.

Doug Gilbert

Bart Van Assche March 27, 2018, 3:09 p.m. UTC | #3

On Tue, 2018-03-27 at 09:36 -0400, Douglas Gilbert wrote:
> On 2018-03-26 08:13 PM, Bart Van Assche wrote:

> > On Sun, 2018-03-18 at 21:59 +0100, Douglas Gilbert wrote:

> > > +	/* sense not about current command is termed: deferred */

> > 

> > Do we really need comments that explain the SCSI specs? If such a comment is

> > added I think it should be added above the definition of scsi_sense_is_deferred()

> > together with a reference to the "Sense data" section in SPC.

> > 

> > > +	if (result == 0) {

> > > +		/*

> > > +		 * Unprep the request and put it back at the head of the

> > > +		 * queue. A new command will be prepared and issued.

> > > +		 * This block is the same as case ACTION_REPREP in

> > > +		 * scsi_io_completion_action() above.

> > >   		 */

> > > -		if (q->mq_ops) {

> > > +		if (q->mq_ops)

> > >   			scsi_mq_requeue_cmd(cmd);

> > > -		} else {

> > > +		else {

> > >   			scsi_release_buffers(cmd);

> > >   			scsi_requeue_command(q, cmd);

> > >   		}

> > 

> > Have these changes been verified with checkpatch? Checkpatch should have reported

> > the following about the above chunk of code: Unbalanced braces around else statement.

> 

> Yes they were, did you check them? If so, with what command line options?

> Since with no options <mkp-4.17/scsi-queue>/scripts/checkpatch.pl returns

> clean for all patches in this set.


If checkpatch did not complain about this patch then I think that that
indicates a bug in checkpatch. The following excerpt from the kernel v4.16-rc7
checkpatch source code shows that checkpatch should complain about the above
changes:

# check for single line unbalanced braces
                if ($sline =~ /^.\s*\}\s*else\s*$/ ||
                    $sline =~ /^.\s*else\s*\{\s*$/) {
                        CHK("BRACES", "Unbalanced braces around else statement\n" . $herecurr);
                }

Anyway, I think the output of the following commands shows that balancing braces
is the preferred style in the Linux kernel:

$ git grep "$(printf "\t")else {" | wc -l
4971
$ git grep '} else {' | wc -l
61132

Thanks,

Bart.

Bart Van Assche March 27, 2018, 3:12 p.m. UTC | #4

On Sun, 2018-03-18 at 21:59 +0100, Douglas Gilbert wrote:
> Place scsi_io_completion()'s complex error processing associated with a

> local enumeration into a static helper function. That enumeration's

> values start with "ACTION_" so use the suffix "_action" in the helper

> function's name.


Please split this patch into two patches: one that introduces the new function
scsi_io_completion_action() without changing the logic of the code and a second
patch that introduces the "sense_valid_and_current" variable. I think that that
will make the changes easier to read and to verify.

Thanks,

Bart.

Patch

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 9731d0d3cc80..0e2dcdb63fcd 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -751,6 +751,168 @@  static blk_status_t __scsi_error_from_host_byte(struct scsi_cmnd *cmd,
 	}
 }
 
+/* Helper for scsi_io_completion() when special action required. */
+static void scsi_io_completion_action(struct scsi_cmnd *cmd, int result)
+{
+	struct request_queue *q = cmd->device->request_queue;
+	struct request *req = cmd->request;
+	int level = 0;
+	enum {ACTION_FAIL, ACTION_REPREP, ACTION_RETRY,
+	      ACTION_DELAYED_RETRY} action;
+	unsigned long wait_for = (cmd->allowed + 1) * req->timeout;
+	struct scsi_sense_hdr sshdr;
+	bool sense_valid_and_current = false;
+	blk_status_t blk_stat;		/* u8, BLK_STS_OK is only 0 */
+
+	/* sense not about current command is termed: deferred */
+	if (scsi_command_normalize_sense(cmd, &sshdr) &&
+	    !scsi_sense_is_deferred(&sshdr))
+		sense_valid_and_current = true;
+
+	blk_stat = __scsi_error_from_host_byte(cmd, result);
+
+	if (host_byte(result) == DID_RESET) {
+		/* Third party bus reset or reset for error recovery
+		 * reasons.  Just retry the command and see what
+		 * happens.
+		 */
+		action = ACTION_RETRY;
+	} else if (sense_valid_and_current) {
+		switch (sshdr.sense_key) {
+		case UNIT_ATTENTION:
+			if (cmd->device->removable) {
+				/* Detected disc change.  Set a bit
+				 * and quietly refuse further access.
+				 */
+				cmd->device->changed = 1;
+				action = ACTION_FAIL;
+			} else {
+				/* Must have been a power glitch, or a
+				 * bus reset.  Could not have been a
+				 * media change, so we just retry the
+				 * command and see what happens.
+				 */
+				action = ACTION_RETRY;
+			}
+			break;
+		case ILLEGAL_REQUEST:
+			/* If we had an ILLEGAL REQUEST returned, then
+			 * we may have performed an unsupported
+			 * command.  The only thing this should be
+			 * would be a ten byte read where only a six
+			 * byte read was supported.  Also, on a system
+			 * where READ CAPACITY failed, we may have
+			 * read past the end of the disk.
+			 */
+			if ((cmd->device->use_10_for_rw &&
+			    sshdr.asc == 0x20 && sshdr.ascq == 0x00) &&
+			    (cmd->cmnd[0] == READ_10 ||
+			     cmd->cmnd[0] == WRITE_10)) {
+				/* This will issue a new 6-byte command. */
+				cmd->device->use_10_for_rw = 0;
+				action = ACTION_REPREP;
+			} else if (sshdr.asc == 0x10) /* DIX */ {
+				action = ACTION_FAIL;
+				blk_stat = BLK_STS_PROTECTION;
+			/* INVALID COMMAND OPCODE or INVALID FIELD IN CDB */
+			} else if (sshdr.asc == 0x20 || sshdr.asc == 0x24) {
+				action = ACTION_FAIL;
+				blk_stat = BLK_STS_TARGET;
+			} else
+				action = ACTION_FAIL;
+			break;
+		case ABORTED_COMMAND:
+			action = ACTION_FAIL;
+			if (sshdr.asc == 0x10) /* DIF */
+				blk_stat = BLK_STS_PROTECTION;
+			break;
+		case NOT_READY:
+			/* If the device is in the process of becoming
+			 * ready, or has a temporary blockage, retry.
+			 */
+			if (sshdr.asc == 0x04) {
+				switch (sshdr.ascq) {
+				case 0x01: /* becoming ready */
+				case 0x04: /* format in progress */
+				case 0x05: /* rebuild in progress */
+				case 0x06: /* recalculation in progress */
+				case 0x07: /* operation in progress */
+				case 0x08: /* Long write in progress */
+				case 0x09: /* self test in progress */
+				case 0x14: /* space allocation in progress */
+					action = ACTION_DELAYED_RETRY;
+					break;
+				default:
+					action = ACTION_FAIL;
+					break;
+				}
+			} else
+				action = ACTION_FAIL;
+			break;
+		case VOLUME_OVERFLOW:
+			/* See SSC3rXX or current. */
+			action = ACTION_FAIL;
+			break;
+		default:
+			action = ACTION_FAIL;
+			break;
+		}
+	} else
+		action = ACTION_FAIL;
+
+	if (action != ACTION_FAIL &&
+	    time_before(cmd->jiffies_at_alloc + wait_for, jiffies))
+		action = ACTION_FAIL;
+
+	switch (action) {
+	case ACTION_FAIL:
+		/* Give up and fail the remainder of the request */
+		if (!(req->rq_flags & RQF_QUIET)) {
+			static DEFINE_RATELIMIT_STATE(_rs,
+					DEFAULT_RATELIMIT_INTERVAL,
+					DEFAULT_RATELIMIT_BURST);
+
+			if (unlikely(scsi_logging_level))
+				level =
+				     SCSI_LOG_LEVEL(SCSI_LOG_MLCOMPLETE_SHIFT,
+						    SCSI_LOG_MLCOMPLETE_BITS);
+
+			/*
+			 * if logging is enabled the failure will be printed
+			 * in scsi_log_completion(), so avoid duplicate messages
+			 */
+			if (!level && __ratelimit(&_rs)) {
+				scsi_print_result(cmd, NULL, FAILED);
+				if (driver_byte(result) & DRIVER_SENSE)
+					scsi_print_sense(cmd);
+				scsi_print_command(cmd);
+			}
+		}
+		if (!scsi_end_request(req, blk_stat, blk_rq_err_bytes(req), 0))
+			return;
+		/*FALLTHRU*/
+	case ACTION_REPREP:
+		/* Unprep the request and put it back at the head of the queue.
+		 * A new command will be prepared and issued.
+		 */
+		if (q->mq_ops)
+			scsi_mq_requeue_cmd(cmd);
+		else {
+			scsi_release_buffers(cmd);
+			scsi_requeue_command(q, cmd);
+		}
+		break;
+	case ACTION_RETRY:
+		/* Retry the same command immediately */
+		__scsi_queue_insert(cmd, SCSI_MLQUEUE_EH_RETRY, false);
+		break;
+	case ACTION_DELAYED_RETRY:
+		/* Retry the same command after a delay */
+		__scsi_queue_insert(cmd, SCSI_MLQUEUE_DEVICE_BUSY, false);
+		break;
+	}
+}
+
 /*
  * Helper for scsi_io_completion() when cmd->result is non-zero. Returns
  * BLK_STS_NOTSUPP if this function does not change blk_status .
@@ -857,18 +1019,8 @@  void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
 	struct request_queue *q = cmd->device->request_queue;
 	struct request *req = cmd->request;
 	blk_status_t blk_stat = BLK_STS_OK;	/* u8: BLK_STS_OK is only 0 */
-	struct scsi_sense_hdr sshdr;
-	bool sense_valid = false;
-	bool sense_current = true;	/* false implies "deferred sense" */
-	int level = 0;
-	enum {ACTION_FAIL, ACTION_REPREP, ACTION_RETRY,
-	      ACTION_DELAYED_RETRY} action;
-	unsigned long wait_for = (cmd->allowed + 1) * req->timeout;
 
 	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 */
@@ -935,151 +1087,21 @@  void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
 	 * If there had been no error, but we have leftover bytes in the
 	 * requeues just queue the command up again.
 	 */
-	if (result == 0)
-		goto requeue;
-
-	blk_stat = __scsi_error_from_host_byte(cmd, result);
-
-	if (host_byte(result) == DID_RESET) {
-		/* Third party bus reset or reset for error recovery
-		 * reasons.  Just retry the command and see what
-		 * happens.
-		 */
-		action = ACTION_RETRY;
-	} else if (sense_valid && sense_current) {
-		switch (sshdr.sense_key) {
-		case UNIT_ATTENTION:
-			if (cmd->device->removable) {
-				/* Detected disc change.  Set a bit
-				 * and quietly refuse further access.
-				 */
-				cmd->device->changed = 1;
-				action = ACTION_FAIL;
-			} else {
-				/* Must have been a power glitch, or a
-				 * bus reset.  Could not have been a
-				 * media change, so we just retry the
-				 * command and see what happens.
-				 */
-				action = ACTION_RETRY;
-			}
-			break;
-		case ILLEGAL_REQUEST:
-			/* If we had an ILLEGAL REQUEST returned, then
-			 * we may have performed an unsupported
-			 * command.  The only thing this should be
-			 * would be a ten byte read where only a six
-			 * byte read was supported.  Also, on a system
-			 * where READ CAPACITY failed, we may have
-			 * read past the end of the disk.
-			 */
-			if ((cmd->device->use_10_for_rw &&
-			    sshdr.asc == 0x20 && sshdr.ascq == 0x00) &&
-			    (cmd->cmnd[0] == READ_10 ||
-			     cmd->cmnd[0] == WRITE_10)) {
-				/* This will issue a new 6-byte command. */
-				cmd->device->use_10_for_rw = 0;
-				action = ACTION_REPREP;
-			} else if (sshdr.asc == 0x10) /* DIX */ {
-				action = ACTION_FAIL;
-				blk_stat = BLK_STS_PROTECTION;
-			/* INVALID COMMAND OPCODE or INVALID FIELD IN CDB */
-			} else if (sshdr.asc == 0x20 || sshdr.asc == 0x24) {
-				action = ACTION_FAIL;
-				blk_stat = BLK_STS_TARGET;
-			} else
-				action = ACTION_FAIL;
-			break;
-		case ABORTED_COMMAND:
-			action = ACTION_FAIL;
-			if (sshdr.asc == 0x10) /* DIF */
-				blk_stat = BLK_STS_PROTECTION;
-			break;
-		case NOT_READY:
-			/* If the device is in the process of becoming
-			 * ready, or has a temporary blockage, retry.
-			 */
-			if (sshdr.asc == 0x04) {
-				switch (sshdr.ascq) {
-				case 0x01: /* becoming ready */
-				case 0x04: /* format in progress */
-				case 0x05: /* rebuild in progress */
-				case 0x06: /* recalculation in progress */
-				case 0x07: /* operation in progress */
-				case 0x08: /* Long write in progress */
-				case 0x09: /* self test in progress */
-				case 0x14: /* space allocation in progress */
-					action = ACTION_DELAYED_RETRY;
-					break;
-				default:
-					action = ACTION_FAIL;
-					break;
-				}
-			} else
-				action = ACTION_FAIL;
-			break;
-		case VOLUME_OVERFLOW:
-			/* See SSC3rXX or current. */
-			action = ACTION_FAIL;
-			break;
-		default:
-			action = ACTION_FAIL;
-			break;
-		}
-	} else
-		action = ACTION_FAIL;
-
-	if (action != ACTION_FAIL &&
-	    time_before(cmd->jiffies_at_alloc + wait_for, jiffies))
-		action = ACTION_FAIL;
-
-	switch (action) {
-	case ACTION_FAIL:
-		/* Give up and fail the remainder of the request */
-		if (!(req->rq_flags & RQF_QUIET)) {
-			static DEFINE_RATELIMIT_STATE(_rs,
-					DEFAULT_RATELIMIT_INTERVAL,
-					DEFAULT_RATELIMIT_BURST);
-
-			if (unlikely(scsi_logging_level))
-				level = SCSI_LOG_LEVEL(SCSI_LOG_MLCOMPLETE_SHIFT,
-						       SCSI_LOG_MLCOMPLETE_BITS);
-
-			/*
-			 * if logging is enabled the failure will be printed
-			 * in scsi_log_completion(), so avoid duplicate messages
-			 */
-			if (!level && __ratelimit(&_rs)) {
-				scsi_print_result(cmd, NULL, FAILED);
-				if (driver_byte(result) & DRIVER_SENSE)
-					scsi_print_sense(cmd);
-				scsi_print_command(cmd);
-			}
-		}
-		if (!scsi_end_request(req, blk_stat, blk_rq_err_bytes(req), 0))
-			return;
-		/*FALLTHRU*/
-	case ACTION_REPREP:
-	requeue:
-		/* Unprep the request and put it back at the head of the queue.
-		 * A new command will be prepared and issued.
+	if (result == 0) {
+		/*
+		 * Unprep the request and put it back at the head of the
+		 * queue. A new command will be prepared and issued.
+		 * This block is the same as case ACTION_REPREP in
+		 * scsi_io_completion_action() above.
 		 */
-		if (q->mq_ops) {
+		if (q->mq_ops)
 			scsi_mq_requeue_cmd(cmd);
-		} else {
+		else {
 			scsi_release_buffers(cmd);
 			scsi_requeue_command(q, cmd);
 		}
-		break;
-	case ACTION_RETRY:
-		/* Retry the same command immediately */
-		__scsi_queue_insert(cmd, SCSI_MLQUEUE_EH_RETRY, false);
-		break;
-	case ACTION_DELAYED_RETRY:
-		/* Retry the same command after a delay */
-		__scsi_queue_insert(cmd, SCSI_MLQUEUE_DEVICE_BUSY, false);
-		break;
-	}
+	} else
+		scsi_io_completion_action(cmd, result);
 }
 
 static int scsi_init_sgtable(struct request *req, struct scsi_data_buffer *sdb)