[PATCH] SCSI: don't do bogus retries

[PATCH] SCSI: don't do bogus retries

[Alan Stern]

This patch (as1143) changes the bogus "retry" paths in
scsi_io_completion() into straightforward failures.  It's not clear
why the retries were present to begin with.

The patch also changes the "failure" path; now instead of calling
scsi_end_request() it directly calls end_dequeued_request() followed
by scsi_next_command().

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
@@ -852,7 +852,6 @@ static void scsi_end_bidi_request(struct
 void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
 {
 	int result = cmd->result;
-	int this_count;
 	struct request_queue *q = cmd->device->request_queue;
 	struct request *req = cmd->request;
 	int error = 0;
@@ -908,7 +907,6 @@ void scsi_io_completion(struct scsi_cmnd
 	 */
 	if (scsi_end_request(cmd, error, good_bytes, result == 0) == NULL)
 		return;
-	this_count = blk_rq_bytes(req);
 
 	if (sense_valid && !sense_deferred) {
 		switch (sshdr.sense_key) {
@@ -918,7 +916,7 @@ void scsi_io_completion(struct scsi_cmnd
 				 * and quietly refuse further access.
 				 */
 				cmd->device->changed = 1;
-				goto retry2;
+				goto fail;
 			} else {
 				/* Must have been a power glitch, or a
 				 * bus reset.  Could not have been a
@@ -948,7 +946,7 @@ void scsi_io_completion(struct scsi_cmnd
 			} else if (sshdr.asc == 0x10) /* DIX */
 				goto fail;
 			else
-				goto retry2;
+				goto fail;
 		case ABORTED_COMMAND:
 			if (sshdr.asc == 0x10)  /* DIF */
 				goto fail;
@@ -975,7 +973,7 @@ void scsi_io_completion(struct scsi_cmnd
 				scsi_cmd_print_sense_hdr(cmd,
 							 "Device not ready",
 							 &sshdr);
-			goto retry2;
+			goto fail;
 		case VOLUME_OVERFLOW:
 			if (!(req->cmd_flags & REQ_QUIET)) {
 				scmd_printk(KERN_INFO, cmd,
@@ -984,7 +982,7 @@ void scsi_io_completion(struct scsi_cmnd
 				scsi_print_sense("", cmd);
 			}
 			/* See SSC3rXX or current. */
-			goto retry2;
+			goto fail;
 		default:
 			break;
 		}
@@ -1002,14 +1000,10 @@ void scsi_io_completion(struct scsi_cmnd
 			if (driver_byte(result) & DRIVER_SENSE)
 				scsi_print_sense("", cmd);
 		}
-		goto fail;
 	}
-
- retry2:
-	scsi_end_request(cmd, -EIO, this_count, 1);
-	return;
  fail:
-	scsi_end_request(cmd, -EIO, this_count, 0);
+	end_dequeued_request(req, -EIO);
+	scsi_next_command(cmd);
 	return;
  requeue:
 	scsi_requeue_command(q, cmd);

Re: [PATCH] SCSI: don't do bogus retries

[Kiyoshi Ueda]

Hi Alan,

On Mon, 29 Sep 2008 17:12:28 -0400 (EDT), Alan Stern wrote:
> The patch also changes the "failure" path; now instead of calling
> scsi_end_request() it directly calls end_dequeued_request() followed
> by scsi_next_command().
...
> @@ -908,7 +907,6 @@ void scsi_io_completion(struct scsi_cmnd
>  	 */
>  	if (scsi_end_request(cmd, error, good_bytes, result == 0) == NULL)
>  		return;
> -	this_count = blk_rq_bytes(req);
>  
>  	if (sense_valid && !sense_deferred) {
>  		switch (sshdr.sense_key) {
...
> @@ -1002,14 +1000,10 @@ void scsi_io_completion(struct scsi_cmnd
>  			if (driver_byte(result) & DRIVER_SENSE)
>  				scsi_print_sense("", cmd);
>  		}
> -		goto fail;
>  	}
> -
> - retry2:
> -	scsi_end_request(cmd, -EIO, this_count, 1);
> -	return;
>   fail:
> -	scsi_end_request(cmd, -EIO, this_count, 0);
> +	end_dequeued_request(req, -EIO);
> +	scsi_next_command(cmd);
>  	return;
>   requeue:
>  	scsi_requeue_command(q, cmd);

I think you should use blk_end_request(req, -EIO, blk_rq_bytes(rq))
here instead of end_dequeued_request(req, -EIO).

end_dequeued_request() needs to be called with queue lock held,
but I can't see any queue lock in your patches.
Also, if you add the queue lock and still use end_dequeued_request(),
__end_that_request_first(), which completes BIOs in the request,
is called with the queue lock held, though it doesn't require queue
lock actually.  So that might cause some performance regressions.

Thanks,
Kiyoshi Ueda

Re: [PATCH] SCSI: don't do bogus retries

[Alan Stern]

On Tue, 30 Sep 2008, Kiyoshi Ueda wrote:

> I think you should use blk_end_request(req, -EIO, blk_rq_bytes(rq))
> here instead of end_dequeued_request(req, -EIO).
> 
> end_dequeued_request() needs to be called with queue lock held,
> but I can't see any queue lock in your patches.
> Also, if you add the queue lock and still use end_dequeued_request(),
> __end_that_request_first(), which completes BIOs in the request,
> is called with the queue lock held, though it doesn't require queue
> lock actually.  So that might cause some performance regressions.

Thanks for your comments.

It sure would be nice if the locking requirements of the block layer
were documented properly!  The only description appears to be in a
comment in include/linux/blkdev.h, and it is woefully incomplete.

(There also appear to be far too many redundant entry points, but never 
mind that...)

I'll update the patch as you suggest.

Alan Stern

Re: [PATCH] SCSI: don't do bogus retries

[James Bottomley]

On Tue, 2008-09-30 at 10:14 -0400, Kiyoshi Ueda wrote:
> I think you should use blk_end_request(req, -EIO, blk_rq_bytes(rq))
> here instead of end_dequeued_request(req, -EIO).
> 
> end_dequeued_request() needs to be called with queue lock held,
> but I can't see any queue lock in your patches.
> Also, if you add the queue lock and still use end_dequeued_request(),
> __end_that_request_first(), which completes BIOs in the request,
> is called with the queue lock held, though it doesn't require queue
> lock actually.  So that might cause some performance regressions.

Yes, I was just getting around to noticing this.  Several other issues
spring immediately to mind

     1. Why are there both end_dequeued_request and end_queued_request?
        They both (by design since we tried to make the end cases the
        same) do the same thing
     2. Why don't they follow the block naming convention (they should
        begin blk_rq_ to indicate they're functions in block and act on
        requests).
     3. The block convention is for unlocked variants to be preceded by
        two underscores (although not universally adhered to).

James

Re: [PATCH] SCSI: don't do bogus retries

[Kiyoshi Ueda]

Hi James, Jens,

On Tue, 30 Sep 2008 09:46:38 -0500, James Bottomley wrote:
> On Tue, 2008-09-30 at 10:14 -0400, Kiyoshi Ueda wrote:
> > I think you should use blk_end_request(req, -EIO, blk_rq_bytes(rq))
> > here instead of end_dequeued_request(req, -EIO).
> > 
> > end_dequeued_request() needs to be called with queue lock held,
> > but I can't see any queue lock in your patches.
> > Also, if you add the queue lock and still use end_dequeued_request(),
> > __end_that_request_first(), which completes BIOs in the request,
> > is called with the queue lock held, though it doesn't require queue
> > lock actually.  So that might cause some performance regressions.
> 
> Yes, I was just getting around to noticing this.  Several other issues
> spring immediately to mind
> 
>      1. Why are there both end_dequeued_request and end_queued_request?
>         They both (by design since we tried to make the end cases the
>         same) do the same thing

They originally differed a little bit, but when blk_end_request
interfaces were introduced, they became exactly same:
    http://marc.info/?t=120008891100007&r=1&w=2

So I tried to kill them:
    http://marc.info/?t=120008891100007&r=1&w=2
But Jens suggested keep them for a while at the time.

Is it the time now to kill them, Jens?

Thanks,
Kiyoshi Ueda

Re: [PATCH] SCSI: don't do bogus retries

[Jens Axboe]

On Tue, Sep 30 2008, Kiyoshi Ueda wrote:
> Hi James, Jens,
> 
> On Tue, 30 Sep 2008 09:46:38 -0500, James Bottomley wrote:
> > On Tue, 2008-09-30 at 10:14 -0400, Kiyoshi Ueda wrote:
> > > I think you should use blk_end_request(req, -EIO, blk_rq_bytes(rq))
> > > here instead of end_dequeued_request(req, -EIO).
> > > 
> > > end_dequeued_request() needs to be called with queue lock held,
> > > but I can't see any queue lock in your patches.
> > > Also, if you add the queue lock and still use end_dequeued_request(),
> > > __end_that_request_first(), which completes BIOs in the request,
> > > is called with the queue lock held, though it doesn't require queue
> > > lock actually.  So that might cause some performance regressions.
> > 
> > Yes, I was just getting around to noticing this.  Several other issues
> > spring immediately to mind
> > 
> >      1. Why are there both end_dequeued_request and end_queued_request?
> >         They both (by design since we tried to make the end cases the
> >         same) do the same thing
> 
> They originally differed a little bit, but when blk_end_request
> interfaces were introduced, they became exactly same:
>     http://marc.info/?t=120008891100007&r=1&w=2
> 
> So I tried to kill them:
>     http://marc.info/?t=120008891100007&r=1&w=2
> But Jens suggested keep them for a while at the time.
> 
> Is it the time now to kill them, Jens?

We can kill them for 2.6.28. If you feel like it, send me a patch
against the for-2.6.28 branch of the block git repo.

-- 
Jens Axboe