[PATCH 1/3] SCSI: rearrange code in scsi_io_completion

[PATCH 1/3] SCSI: rearrange code in scsi_io_completion

[Alan Stern]

This patch (as1142) consolidates some repetitious code in
scsi_io_completion().  Multiple calls to scsi_end_request() and to
scsi_requeue_command() are combined together and moved to the end of
the function.

This is simple mechanical code motion, with no change in behavior.

Signed-off-by: Alan Stern <stern@rowland.harvard.edu>

---

Index: usb-2.6/drivers/scsi/scsi_lib.c
===================================================================
--- usb-2.6.orig/drivers/scsi/scsi_lib.c
+++ usb-2.6/drivers/scsi/scsi_lib.c
@@ -910,9 +910,6 @@ void scsi_io_completion(struct scsi_cmnd
 		return;
 	this_count = blk_rq_bytes(req);
 
-	/* good_bytes = 0, or (inclusive) there were leftovers and
-	 * result = 0, so scsi_end_request couldn't retry.
-	 */
 	if (sense_valid && !sense_deferred) {
 		switch (sshdr.sense_key) {
 		case UNIT_ATTENTION:
@@ -921,18 +918,15 @@ void scsi_io_completion(struct scsi_cmnd
 				 * and quietly refuse further access.
 				 */
 				cmd->device->changed = 1;
-				scsi_end_request(cmd, -EIO, this_count, 1);
-				return;
+				goto retry2;
 			} else {
 				/* Must have been a power glitch, or a
 				 * bus reset.  Could not have been a
 				 * media change, so we just retry the
 				 * request and see what happens.
 				 */
-				scsi_requeue_command(q, cmd);
-				return;
+				goto requeue;
 			}
-			break;
 		case ILLEGAL_REQUEST:
 			/* If we had an ILLEGAL REQUEST returned, then
 			 * we may have performed an unsupported
@@ -950,17 +944,14 @@ void scsi_io_completion(struct scsi_cmnd
 				/* This will cause a retry with a
 				 * 6-byte command.
 				 */
-				scsi_requeue_command(q, cmd);
+				goto requeue;
 			} else if (sshdr.asc == 0x10) /* DIX */
-				scsi_end_request(cmd, -EIO, this_count, 0);
+				goto fail;
 			else
-				scsi_end_request(cmd, -EIO, this_count, 1);
-			return;
+				goto retry2;
 		case ABORTED_COMMAND:
-			if (sshdr.asc == 0x10) { /* DIF */
-				scsi_end_request(cmd, -EIO, this_count, 0);
-				return;
-			}
+			if (sshdr.asc == 0x10)  /* DIF */
+				goto fail;
 			break;
 		case NOT_READY:
 			/* If the device is in the process of becoming
@@ -975,8 +966,7 @@ void scsi_io_completion(struct scsi_cmnd
 				case 0x07: /* operation in progress */
 				case 0x08: /* Long write in progress */
 				case 0x09: /* self test in progress */
-					scsi_requeue_command(q, cmd);
-					return;
+					goto requeue;
 				default:
 					break;
 				}
@@ -985,9 +975,7 @@ void scsi_io_completion(struct scsi_cmnd
 				scsi_cmd_print_sense_hdr(cmd,
 							 "Device not ready",
 							 &sshdr);
-
-			scsi_end_request(cmd, -EIO, this_count, 1);
-			return;
+			goto retry2;
 		case VOLUME_OVERFLOW:
 			if (!(req->cmd_flags & REQ_QUIET)) {
 				scmd_printk(KERN_INFO, cmd,
@@ -996,8 +984,7 @@ void scsi_io_completion(struct scsi_cmnd
 				scsi_print_sense("", cmd);
 			}
 			/* See SSC3rXX or current. */
-			scsi_end_request(cmd, -EIO, this_count, 1);
-			return;
+			goto retry2;
 		default:
 			break;
 		}
@@ -1007,8 +994,7 @@ void scsi_io_completion(struct scsi_cmnd
 		 * reasons.  Just retry the request and see what
 		 * happens.
 		 */
-		scsi_requeue_command(q, cmd);
-		return;
+		goto requeue;
 	}
 	if (result) {
 		if (!(req->cmd_flags & REQ_QUIET)) {
@@ -1016,8 +1002,18 @@ void scsi_io_completion(struct scsi_cmnd
 			if (driver_byte(result) & DRIVER_SENSE)
 				scsi_print_sense("", cmd);
 		}
+		goto fail;
 	}
-	scsi_end_request(cmd, -EIO, this_count, !result);
+
+ retry2:
+	scsi_end_request(cmd, -EIO, this_count, 1);
+	return;
+ fail:
+	scsi_end_request(cmd, -EIO, this_count, 0);
+	return;
+ requeue:
+	scsi_requeue_command(q, cmd);
+	return;
 }
 
 static int scsi_init_sgtable(struct request *req, struct scsi_data_buffer *sdb,

Re: [PATCH 1/3] SCSI: rearrange code in scsi_io_completion

[James Bottomley]

On Mon, 2008-09-29 at 17:11 -0400, Alan Stern wrote:
> This patch (as1142) consolidates some repetitious code in
> scsi_io_completion().  Multiple calls to scsi_end_request() and to
> scsi_requeue_command() are combined together and moved to the end of
> the function.
> 
> This is simple mechanical code motion, with no change in behavior.

I don't really think this is the right approach, since the retry case
needs to be split apart again.

The only time scsi_requeue_command() needs to be called is if the
request completes successfully but has leftovers.  The reason is that
the command will be different next time around, so it has to be
re-prepared.  In all the other cases, the same command can be reused.
This will have the knock on effect of not resetting the timers or the
counters, so it has to be done carefully.

Of the three requeue cases:

UNIT_ATTENTION needs immediate retry
NOT_READY needs delayed retry
ILLEGAL_REQUEST with cmd switch (assuming we still do it) needs
immediate retry
DID_RESET is arguable either way, but probably needs delayed.

immediate requeue is done by:

scsi_queue_insert(cmd, SCSI_MLQUEUE_EH_RETRY);

And delayed by

scsi_queue_insert(cmd, SCSI_MLQUEUE_DEVICE_BUSY);

James

Re: [PATCH 1/3] SCSI: rearrange code in scsi_io_completion

[Alan Stern]

On Tue, 30 Sep 2008, James Bottomley wrote:

> I don't really think this is the right approach, since the retry case
> needs to be split apart again.
> 
> The only time scsi_requeue_command() needs to be called is if the
> request completes successfully but has leftovers.  The reason is that
> the command will be different next time around, so it has to be
> re-prepared.  In all the other cases, the same command can be reused.
> This will have the knock on effect of not resetting the timers or the
> counters, so it has to be done carefully.

All right.  (Incidentally, do you happen to know the derivation of 
"knock on effect"?  The American form, "side effect", seems more 
self-explanatory.)

> Of the three requeue cases:
> 
> UNIT_ATTENTION needs immediate retry
> NOT_READY needs delayed retry
> ILLEGAL_REQUEST with cmd switch (assuming we still do it) needs
> immediate retry

If the command is switched from 10-byte to 6-byte form, won't it need 
to be re-prepared?

> DID_RESET is arguable either way, but probably needs delayed.
> 
> immediate requeue is done by:
> 
> scsi_queue_insert(cmd, SCSI_MLQUEUE_EH_RETRY);
> 
> And delayed by
> 
> scsi_queue_insert(cmd, SCSI_MLQUEUE_DEVICE_BUSY);

Easily fixed.  And it looks like neither of these needs a call to
scsi_next_command(), right?

Alan Stern

Re: [PATCH 1/3] SCSI: rearrange code in scsi_io_completion

[James Bottomley]

On Tue, 2008-09-30 at 11:08 -0400, Alan Stern wrote:
> On Tue, 30 Sep 2008, James Bottomley wrote:
> 
> > I don't really think this is the right approach, since the retry case
> > needs to be split apart again.
> > 
> > The only time scsi_requeue_command() needs to be called is if the
> > request completes successfully but has leftovers.  The reason is that
> > the command will be different next time around, so it has to be
> > re-prepared.  In all the other cases, the same command can be reused.
> > This will have the knock on effect of not resetting the timers or the
> > counters, so it has to be done carefully.
> 
> All right.  (Incidentally, do you happen to know the derivation of 
> "knock on effect"?  The American form, "side effect", seems more 
> self-explanatory.)

The etymology is probably from Rugby: a knock on takes the ball further
than allowed by the rules, usually as an unintended consequence of some
other action.

> > Of the three requeue cases:
> > 
> > UNIT_ATTENTION needs immediate retry
> > NOT_READY needs delayed retry
> > ILLEGAL_REQUEST with cmd switch (assuming we still do it) needs
> > immediate retry
> 
> If the command is switched from 10-byte to 6-byte form, won't it need 
> to be re-prepared?

Yes, sorry, that one needs a re-prepare requeue.

> > DID_RESET is arguable either way, but probably needs delayed.
> > 
> > immediate requeue is done by:
> > 
> > scsi_queue_insert(cmd, SCSI_MLQUEUE_EH_RETRY);
> > 
> > And delayed by
> > 
> > scsi_queue_insert(cmd, SCSI_MLQUEUE_DEVICE_BUSY);
> 
> Easily fixed.  And it looks like neither of these needs a call to
> scsi_next_command(), right?

Right, which is a nice side effect.

James

Re: [PATCH 1/3] SCSI: rearrange code in scsi_io_completion

[Mike Anderson]

James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
> On Tue, 2008-09-30 at 11:08 -0400, Alan Stern wrote:
> > On Tue, 30 Sep 2008, James Bottomley wrote:
> > 
> > > I don't really think this is the right approach, since the retry case
> > > needs to be split apart again.
> > > 
> > > The only time scsi_requeue_command() needs to be called is if the
> > > request completes successfully but has leftovers.  The reason is that
> > > the command will be different next time around, so it has to be
> > > re-prepared.  In all the other cases, the same command can be reused.
> > > This will have the knock on effect of not resetting the timers or the
> > > counters, so it has to be done carefully.
> > 
> > All right.  (Incidentally, do you happen to know the derivation of 
> > "knock on effect"?  The American form, "side effect", seems more 
> > self-explanatory.)
> 
> The etymology is probably from Rugby: a knock on takes the ball further
> than allowed by the rules, usually as an unintended consequence of some
> other action.
> 
> > > Of the three requeue cases:
> > > 
> > > UNIT_ATTENTION needs immediate retry
> > > NOT_READY needs delayed retry
> > > ILLEGAL_REQUEST with cmd switch (assuming we still do it) needs
> > > immediate retry
> > 
> > If the command is switched from 10-byte to 6-byte form, won't it need 
> > to be re-prepared?
> 
> Yes, sorry, that one needs a re-prepare requeue.
> 
> > > DID_RESET is arguable either way, but probably needs delayed.
> > > 
> > > immediate requeue is done by:
> > > 
> > > scsi_queue_insert(cmd, SCSI_MLQUEUE_EH_RETRY);
> > > 
> > > And delayed by
> > > 
> > > scsi_queue_insert(cmd, SCSI_MLQUEUE_DEVICE_BUSY);
> > 
> > Easily fixed.  And it looks like neither of these needs a call to
> > scsi_next_command(), right?
> 
> Right, which is a nice side effect.

scsi_finish_command is only called from scsi_eh_flush_done_q (newer
patches moves this to scsi_attempt_requeue_command) and scsi_softirq_done.
scsi_io_completion is only called from scsi_finish_command. In
scsi_softirq_done we just called scsi_decide_disposition to map the
result. Could some (or all) of the sense mapping be moved to
scsi_decide_disposition? It seems incorrect to decode this same data in
more than one location plus in some cases could prevent device handlers
from the full control they need. Is there some path or behavior that I am
missing?

Also since previous mid retry changes are related to retry behavior borrowing
this thread for a related request....

James, It would be good if you have time to look at the repost of mid
retry changes and if they are ok would you consider applying them plus
these changes.  It would be good to also have a refresh of
scsi-post-merge-2.6 tree with Jens tree. I am running testing now, but my
tree needed a lot of merging assistance and it would be good to know what
others are testing is the same.
http://thread.gmane.org/gmane.linux.scsi/44715

Since this change plus Mike C's and mine effect retry / completion
behavior it would be good to test these changes all together.

Thanks,

-andmike
--
Michael Anderson
andmike@linux.vnet.ibm.com

Re: [PATCH 1/3] SCSI: rearrange code in scsi_io_completion

[James Bottomley]

On Tue, 2008-09-30 at 10:11 -0700, Mike Anderson wrote:
> James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
> > On Tue, 2008-09-30 at 11:08 -0400, Alan Stern wrote:
> > > On Tue, 30 Sep 2008, James Bottomley wrote:
> > > 
> > > > I don't really think this is the right approach, since the retry case
> > > > needs to be split apart again.
> > > > 
> > > > The only time scsi_requeue_command() needs to be called is if the
> > > > request completes successfully but has leftovers.  The reason is that
> > > > the command will be different next time around, so it has to be
> > > > re-prepared.  In all the other cases, the same command can be reused.
> > > > This will have the knock on effect of not resetting the timers or the
> > > > counters, so it has to be done carefully.
> > > 
> > > All right.  (Incidentally, do you happen to know the derivation of 
> > > "knock on effect"?  The American form, "side effect", seems more 
> > > self-explanatory.)
> > 
> > The etymology is probably from Rugby: a knock on takes the ball further
> > than allowed by the rules, usually as an unintended consequence of some
> > other action.
> > 
> > > > Of the three requeue cases:
> > > > 
> > > > UNIT_ATTENTION needs immediate retry
> > > > NOT_READY needs delayed retry
> > > > ILLEGAL_REQUEST with cmd switch (assuming we still do it) needs
> > > > immediate retry
> > > 
> > > If the command is switched from 10-byte to 6-byte form, won't it need 
> > > to be re-prepared?
> > 
> > Yes, sorry, that one needs a re-prepare requeue.
> > 
> > > > DID_RESET is arguable either way, but probably needs delayed.
> > > > 
> > > > immediate requeue is done by:
> > > > 
> > > > scsi_queue_insert(cmd, SCSI_MLQUEUE_EH_RETRY);
> > > > 
> > > > And delayed by
> > > > 
> > > > scsi_queue_insert(cmd, SCSI_MLQUEUE_DEVICE_BUSY);
> > > 
> > > Easily fixed.  And it looks like neither of these needs a call to
> > > scsi_next_command(), right?
> > 
> > Right, which is a nice side effect.
> 
> scsi_finish_command is only called from scsi_eh_flush_done_q (newer
> patches moves this to scsi_attempt_requeue_command) and scsi_softirq_done.
> scsi_io_completion is only called from scsi_finish_command. In
> scsi_softirq_done we just called scsi_decide_disposition to map the
> result. Could some (or all) of the sense mapping be moved to
> scsi_decide_disposition? It seems incorrect to decode this same data in
> more than one location plus in some cases could prevent device handlers
> from the full control they need. Is there some path or behavior that I am
> missing?

Well, it already is done in scsi_decide_disposition() with a call to
scsi_check_sense().

However, there's a slight problem in that some senses need to be
interpreted differently depending on what the ULD is which leads to the
current 3 tier system (before in decide disposition, during in ULD done
and after in scsi_io_completion).

The discriminators in the done functions do look to be duplicates, so I
suppose they could be collapsed.

> Also since previous mid retry changes are related to retry behavior borrowing
> this thread for a related request....
> 
> James, It would be good if you have time to look at the repost of mid
> retry changes and if they are ok would you consider applying them plus
> these changes.

They're on my list ... it's just there's rather a lot of them and I was
hoping someone else would get there first ...

>   It would be good to also have a refresh of
> scsi-post-merge-2.6 tree with Jens tree. I am running testing now, but my
> tree needed a lot of merging assistance and it would be good to know what
> others are testing is the same.
> http://thread.gmane.org/gmane.linux.scsi/44715

Um, it shouldn't need any merging assistance ... it rebases into
linux-next just fine.  Are you sure you're doing the right thing.

To get this tree, you need to set it up as a remote

git remote add -f scsi-postmerge git://git.kernel.org//pub/scm/linux/kernel/git/jejb/scsi-post-merge-2.6.git

in a tree you want to use it in (must contain at least scsi-misc and
Jens block#for-2.6.28) and rebase the postmerge tree into your current

git rebase --onto master scsi-postmerge/merge-base scsi-postmerge/master

This will create a temporary branch for you to play with (git branch
will show 

* (no branch)
  master

).  If you play around with it and find it to be OK, you can create a
new branch for it

git branch <new branch name>
git checkout <new branch name>

And the temporary branch will become permanent (you can even force your
temporary to become master with git branch -f master)

> Since this change plus Mike C's and mine effect retry / completion
> behavior it would be good to test these changes all together.

James

Re: [PATCH 1/3] SCSI: rearrange code in scsi_io_completion

[Alan Stern]

On Tue, 30 Sep 2008, James Bottomley wrote:

> However, there's a slight problem in that some senses need to be
> interpreted differently depending on what the ULD is which leads to the
> current 3 tier system (before in decide disposition, during in ULD done
> and after in scsi_io_completion).
> 
> The discriminators in the done functions do look to be duplicates, so I
> suppose they could be collapsed.

Related to this in a minor way is my patch from September 25, which
removes from sd_done() the code to update the use_10_for_rw and
use_10_for_ms flags:

	http://marc.info/?l=linux-scsi&m=122236940613587&w=2

This change has now been confirmed to fix a problem that previously 
caused a disk drive to be unusable:

https://lists.one-eyed-alien.net/pipermail/usb-storage/2008-September/004022.html

Alan Stern

Re: [PATCH 1/3] SCSI: rearrange code in scsi_io_completion

[Alan Stern]

On Tue, 30 Sep 2008, James Bottomley wrote:

> > > Of the three requeue cases:
> > > 
> > > UNIT_ATTENTION needs immediate retry
> > > NOT_READY needs delayed retry
> > > ILLEGAL_REQUEST with cmd switch (assuming we still do it) needs
> > > immediate retry
> > 
> > If the command is switched from 10-byte to 6-byte form, won't it need 
> > to be re-prepared?
> 
> Yes, sorry, that one needs a re-prepare requeue.
> 
> > > DID_RESET is arguable either way, but probably needs delayed.
> > > 
> > > immediate requeue is done by:
> > > 
> > > scsi_queue_insert(cmd, SCSI_MLQUEUE_EH_RETRY);
> > > 
> > > And delayed by
> > > 
> > > scsi_queue_insert(cmd, SCSI_MLQUEUE_DEVICE_BUSY);
> > 
> > Easily fixed.  And it looks like neither of these needs a call to
> > scsi_next_command(), right?
> 
> Right, which is a nice side effect.

What about the logging code near the end of scsi_io_completion(), the
part which conditionally does scsi_print_result() and possibly
scsi_print_sense()?

The conditions under which those routines get called are pretty
obscure.  It would be nicer to make them more transparent.

For example, do the following rules make sense?

	For retries/requeues there's no need to print anything.
	(Excepting of course the calls to scsi_cmd_print_sense_hdr() 
	under NOT_READY and scsi_print_sense() under VOLUME_OVERFLOW.)

	For failures, we might as well always run the logging code.

Are there types of failure for which we really don't want to print 
anything?  For example, what about UNIT ATTENTION for media changes or 
the DIX/DIF failures (should those both be DIF?)?

Alan Stern

Re: [PATCH 1/3] SCSI: rearrange code in scsi_io_completion

[Martin K. Petersen]

>>>>> "Alan" == Alan Stern <stern@rowland.harvard.edu> writes:

Alan> Are there types of failure for which we really don't want to
Alan> print anything?  For example, what about UNIT ATTENTION for
Alan> media changes or the DIX/DIF failures (should those both be
Alan> DIF?)?

ILLEGAL REQUEST + 10/[123] indicates that the HBA rejected the request
(DIX).

ABORTED COMMAND + 10/[123] indicates that the device found a
corruption error (DIF).

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH 1/3] SCSI: rearrange code in scsi_io_completion

[Alan Stern]

On Tue, 30 Sep 2008, Martin K. Petersen wrote:

> >>>>> "Alan" == Alan Stern <stern@rowland.harvard.edu> writes:
> 
> Alan> Are there types of failure for which we really don't want to
> Alan> print anything?  For example, what about UNIT ATTENTION for
> Alan> media changes or the DIX/DIF failures (should those both be
> Alan> DIF?)?
> 
> ILLEGAL REQUEST + 10/[123] indicates that the HBA rejected the request
> (DIX).
> 
> ABORTED COMMAND + 10/[123] indicates that the device found a
> corruption error (DIF).

That answer the parenthetical question.  Do you know the answer to the 
main question?  That is, when either of these events occurs do we want 
to skip adding an error message to the system log?

Alan Stern

Re: [PATCH 1/3] SCSI: rearrange code in scsi_io_completion

[Douglas Gilbert]

Alan Stern wrote:
> On Tue, 30 Sep 2008, Martin K. Petersen wrote:
> 
>>>>>>> "Alan" == Alan Stern <stern@rowland.harvard.edu> writes:
>> Alan> Are there types of failure for which we really don't want to
>> Alan> print anything?  For example, what about UNIT ATTENTION for
>> Alan> media changes or the DIX/DIF failures (should those both be
>> Alan> DIF?)?
>>
>> ILLEGAL REQUEST + 10/[123] indicates that the HBA rejected the request
>> (DIX).
>>
>> ABORTED COMMAND + 10/[123] indicates that the device found a
>> corruption error (DIF).
> 
> That answer the parenthetical question.  Do you know the answer to the 
> main question?  That is, when either of these events occurs do we want 
> to skip adding an error message to the system log?

I know that SAS maps certain types of transport errors to
"ABORTED COMMAND" (see
http://www.t10.org/ftp/t10/drafts/sas2/sas2r14e.pdf
section 10.2.3) and they should be retried, especially the
timeouts.

For example, if there are 20 disks hanging off a SAS expander
with a 4 wide link back to the host, then issuing big and
very frequent READs to those 20 disks should see some
"ABORTED COMMAND"s.

Doug Gilbert