BUG in handling of last_sector_bug flag

BUG in handling of last_sector_bug flag

[Alan Stern]

Antonio reported a problem in

	http://marc.info/?l=linux-usb&m=121802760208717&w=2

and I have traced it to a bad interaction between the last_sector_bug 
flag in sd.c and error reporting in the midlayer.

The last_sector_bug flag is set for devices which can't handle 
multi-sector accesses at the end of the medium.  It causes such 
accesses to be broken up into multiple single-sector accesses.  For 
example, a read request for the last 8 sectors of a drive would be 
turned into eight read requests, each for a single sector.

The problem arises when one of those single-sector requests fails with
an I/O error.  In the example above, the total length of the original
request was 4096 bytes.  But scsi_io_completion() is called with 
good_bytes = 0 and this_count gets set to 512, the number of bytes in 
the failed command.

At the end of the function, scsi_end_request() is called with error =
-EIO, bytes = 512, and requeue = 0.  This results in a call to

	blk_end_request(req, -EIO, 512);

and the remainder of the request is left hanging out to dry.  It never 
is requeued, it never completes, and the caller hangs.

This suggests that we need to change the call to scsi_end_request() at 
the end of scsi_io_completion().  If result is nonzero then nothing 
will be requeued, so this_count should be replaced with the total 
number of bytes remaining in the request.  Does that sound reasonable?

If anyone would like to recreate the conditions leading to this 
problem, there's a description of how to do it in this email:

	http://marc.info/?l=linux-kernel&m=121805565105096&w=2

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: BUG in handling of last_sector_bug flag

[Alan Jenkins]

Alan Stern wrote:
> Antonio reported a problem in
>
> 	http://marc.info/?l=linux-usb&m=121802760208717&w=2
>
> and I have traced it to a bad interaction between the last_sector_bug 
> flag in sd.c and error reporting in the midlayer.
>
> The last_sector_bug flag is set for devices which can't handle 
> multi-sector accesses at the end of the medium.  It causes such 
> accesses to be broken up into multiple single-sector accesses.  For 
> example, a read request for the last 8 sectors of a drive would be 
> turned into eight read requests, each for a single sector.
>
> The problem arises when one of those single-sector requests fails with
> an I/O error.  In the example above, the total length of the original
> request was 4096 bytes.  But scsi_io_completion() is called with 
> good_bytes = 0 and this_count gets set to 512, the number of bytes in 
> the failed command.
>
> At the end of the function, scsi_end_request() is called with error =
> -EIO, bytes = 512, and requeue = 0.  This results in a call to
>
> 	blk_end_request(req, -EIO, 512);
>
> and the remainder of the request is left hanging out to dry.  It never 
> is requeued, it never completes, and the caller hangs.
>
> This suggests that we need to change the call to scsi_end_request() at 
> the end of scsi_io_completion().  If result is nonzero then nothing 
> will be requeued, so this_count should be replaced with the total 
> number of bytes remaining in the request.  Does that sound reasonable?
>
> If anyone would like to recreate the conditions leading to this 
> problem, there's a description of how to do it in this email:
>
> 	http://marc.info/?l=linux-kernel&m=121805565105096&w=2
>   
It sounds a bit like the hangs that happened with my buggy card reader. 
My card reader returned errors because of the IO patterns from the first
version of the last_sector_bug flag.  But instead of reads returning
with errors, I just ended up with hung processes.

It's great to see this tracked down.  Keep me CC'd and I'll test
whatever patch you come up with.  I can simulate the old last_sector_bug
behaviour by changing SD_LAST_BUGGY_SECTORS to 1 (instead of 8).

Alan
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: BUG in handling of last_sector_bug flag

[Alan Stern]

On Tue, 12 Aug 2008, Alan Jenkins wrote:

> > This suggests that we need to change the call to scsi_end_request() at 
> > the end of scsi_io_completion().  If result is nonzero then nothing 
> > will be requeued, so this_count should be replaced with the total 
> > number of bytes remaining in the request.  Does that sound reasonable?

> It sounds a bit like the hangs that happened with my buggy card reader. 
> My card reader returned errors because of the IO patterns from the first
> version of the last_sector_bug flag.  But instead of reads returning
> with errors, I just ended up with hung processes.
> 
> It's great to see this tracked down.  Keep me CC'd and I'll test
> whatever patch you come up with.  I can simulate the old last_sector_bug
> behaviour by changing SD_LAST_BUGGY_SECTORS to 1 (instead of 8).

Well, here's a patch that fixes the problem.  However I'm not certain
it's the right thing to do.  (The first hunk has nothing to do with
this; it is just a comment change.  I simply can't stand the existing
comment -- it is wrong in at least three respects.)

James, what do you think?

Alan Stern



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
@@ -909,8 +909,8 @@ void scsi_io_completion(struct scsi_cmnd
 	if (scsi_end_request(cmd, error, good_bytes, result == 0) == NULL)
 		return;
 
-	/* good_bytes = 0, or (inclusive) there were leftovers and
-	 * result = 0, so scsi_end_request couldn't retry.
+	/* There are leftovers and result != 0, so scsi_end_request couldn't
+	 * requeue the remainder and we have to retry it.
 	 */
 	if (sense_valid && !sense_deferred) {
 		switch (sshdr.sense_key) {
@@ -1015,6 +1015,10 @@ void scsi_io_completion(struct scsi_cmnd
 			if (driver_byte(result) & DRIVER_SENSE)
 				scsi_print_sense("", cmd);
 		}
+
+		/* Fail the entire remainder of the request. */
+		this_count = (blk_pc_request(req) ? req->data_len :
+				(req->hard_nr_sectors << 9));
 	}
 	scsi_end_request(cmd, -EIO, this_count, !result);
 }

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: BUG in handling of last_sector_bug flag

[Boaz Harrosh]

Alan Stern wrote:
> On Tue, 12 Aug 2008, Alan Jenkins wrote:
> 
>>> This suggests that we need to change the call to scsi_end_request() at 
>>> the end of scsi_io_completion().  If result is nonzero then nothing 
>>> will be requeued, so this_count should be replaced with the total 
>>> number of bytes remaining in the request.  Does that sound reasonable?
> 
>> It sounds a bit like the hangs that happened with my buggy card reader. 
>> My card reader returned errors because of the IO patterns from the first
>> version of the last_sector_bug flag.  But instead of reads returning
>> with errors, I just ended up with hung processes.
>>
>> It's great to see this tracked down.  Keep me CC'd and I'll test
>> whatever patch you come up with.  I can simulate the old last_sector_bug
>> behaviour by changing SD_LAST_BUGGY_SECTORS to 1 (instead of 8).
> 
> Well, here's a patch that fixes the problem.  However I'm not certain
> it's the right thing to do.  (The first hunk has nothing to do with
> this; it is just a comment change.  I simply can't stand the existing
> comment -- it is wrong in at least three respects.)
> 
> James, what do you think?
> 
> Alan Stern
> 
> 
> 
> 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
> @@ -909,8 +909,8 @@ void scsi_io_completion(struct scsi_cmnd
>  	if (scsi_end_request(cmd, error, good_bytes, result == 0) == NULL)
>  		return;
>  
> -	/* good_bytes = 0, or (inclusive) there were leftovers and
> -	 * result = 0, so scsi_end_request couldn't retry.
> +	/* There are leftovers and result != 0, so scsi_end_request couldn't
> +	 * requeue the remainder and we have to retry it.
>  	 */
>  	if (sense_valid && !sense_deferred) {
>  		switch (sshdr.sense_key) {
> @@ -1015,6 +1015,10 @@ void scsi_io_completion(struct scsi_cmnd
>  			if (driver_byte(result) & DRIVER_SENSE)
>  				scsi_print_sense("", cmd);
>  		}
> +
> +		/* Fail the entire remainder of the request. */
> +		this_count = (blk_pc_request(req) ? req->data_len :
> +				(req->hard_nr_sectors << 9));
This one has an export
+		this_count = blk_rq_bytes(req);
>  	}
>  	scsi_end_request(cmd, -EIO, this_count, !result);
>  }
> 

What you are doing here is changing the semantics of what used to
work. Now I'm not saying it's a bad thing, but You must audit all
scsi ULDs to make sure they are not now broken because of  the new
assumptions. Let me explain.

This is what happens now (before the patch):
A request comes in with some size. And gets prepared by ULD.
If the ULD does not like the size for some reason it will
chunk off scsi_bufflen and will submit the command with
bufflen != req->size. The midlayer and drivers will only
respond to bufflen. Until ...

Until this loop magic in scsi_io_completion(). Since the
request is not done it will be requeued, the ULD will inspect
new size, adjust scsi_bufflen and so on until done.

In case of an error the request goes back to the ULD and the ULD
should decide what to do with the reminder. scsi-ml until now
did not see any-other size but scsi_bufflen. So in theory it is
sd.c job to check for errors on split up requests and decide if
to complete the reminder or re-submit. The scsi-ml did not make
that policy.

If scsi-ml decides that it wants to set a new error policy here, then
you should audit other ULDs to make sure they did not rely on the
old behavior.

sd could check for errors in it's drv->done(cmd) function. and return
the reminder of the request size in case of error. look at
scsi.c::scsi_finish_command(). ULD has control here.

my $0.017
Boaz
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: BUG in handling of last_sector_bug flag

[Alan Stern]

On Tue, 12 Aug 2008, Boaz Harrosh wrote:

> What you are doing here is changing the semantics of what used to
> work.

Yes, and I don't want to do that unless the current code is truly
wrong.  That's why I wasn't certain this was the right thing to do.

>  Now I'm not saying it's a bad thing, but You must audit all
> scsi ULDs to make sure they are not now broken because of  the new
> assumptions. Let me explain.
> 
> This is what happens now (before the patch):
> A request comes in with some size. And gets prepared by ULD.
> If the ULD does not like the size for some reason it will
> chunk off scsi_bufflen and will submit the command with
> bufflen != req->size. The midlayer and drivers will only
> respond to bufflen. Until ...
> 
> Until this loop magic in scsi_io_completion(). Since the
> request is not done it will be requeued, the ULD will inspect
> new size, adjust scsi_bufflen and so on until done.
> 
> In case of an error the request goes back to the ULD and the ULD
> should decide what to do with the reminder.

I don't understand this point.  _Every_ non-BLOCK_PC request
automatically goes back to the ULD via the ->done callback, not only
those which get an error.  This callback does not get to decide what to
do with the remainder of the request, as far as I can tell; all it can
do is return the number of bytes actually handled.

> scsi-ml until now
> did not see any-other size but scsi_bufflen. So in theory it is
> sd.c job to check for errors on split up requests and decide if
> to complete the reminder or re-submit. The scsi-ml did not make
> that policy.

How can sd.c decide whether or not to resubmit?  It doesn't know 
whether scsi_io_completion() will go ahead and requeue the request 
anyway.  And besides, you can't resubmit the request until the current 
portion has been sent back to the block layer, which doesn't happen 
until after ->done returns.

> If scsi-ml decides that it wants to set a new error policy here, then
> you should audit other ULDs to make sure they did not rely on the
> old behavior.
> 
> sd could check for errors in it's drv->done(cmd) function. and return
> the reminder of the request size in case of error. look at
> scsi.c::scsi_finish_command(). ULD has control here.

The ->done function does not return the remainder of the request size; 
it returns good_bytes, which is the number of bytes that were handled.

I agree that since the decision to split the request up into multiple 
commands was made in sd.c, the decision of what to do about the 
remainder should also be made in sd.c.  However the current structure 
of the code doesn't seem to allow this to happen.  Requeue decisions 
are all made in scsi_io_completion() or scsi_end_request().

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: BUG in handling of last_sector_bug flag

[Boaz Harrosh]

Alan Stern wrote:
> On Tue, 12 Aug 2008, Boaz Harrosh wrote:
> 
>> What you are doing here is changing the semantics of what used to
>> work.
> 
> Yes, and I don't want to do that unless the current code is truly
> wrong.  That's why I wasn't certain this was the right thing to do.
> 
>>  Now I'm not saying it's a bad thing, but You must audit all
>> scsi ULDs to make sure they are not now broken because of  the new
>> assumptions. Let me explain.
>>
>> This is what happens now (before the patch):
>> A request comes in with some size. And gets prepared by ULD.
>> If the ULD does not like the size for some reason it will
>> chunk off scsi_bufflen and will submit the command with
>> bufflen != req->size. The midlayer and drivers will only
>> respond to bufflen. Until ...
>>
>> Until this loop magic in scsi_io_completion(). Since the
>> request is not done it will be requeued, the ULD will inspect
>> new size, adjust scsi_bufflen and so on until done.
>>
>> In case of an error the request goes back to the ULD and the ULD
>> should decide what to do with the reminder.
> 
> I don't understand this point.  _Every_ non-BLOCK_PC request
> automatically goes back to the ULD via the ->done callback, not only
> those which get an error.  This callback does not get to decide what to
> do with the remainder of the request, as far as I can tell; all it can
> do is return the number of bytes actually handled.
> 
>> scsi-ml until now
>> did not see any-other size but scsi_bufflen. So in theory it is
>> sd.c job to check for errors on split up requests and decide if
>> to complete the reminder or re-submit. The scsi-ml did not make
>> that policy.
> 
> How can sd.c decide whether or not to resubmit?  It doesn't know 
> whether scsi_io_completion() will go ahead and requeue the request 
> anyway.  And besides, you can't resubmit the request until the current 
> portion has been sent back to the block layer, which doesn't happen 
> until after ->done returns.
> 
>> If scsi-ml decides that it wants to set a new error policy here, then
>> you should audit other ULDs to make sure they did not rely on the
>> old behavior.
>>
>> sd could check for errors in it's drv->done(cmd) function. and return
>> the reminder of the request size in case of error. look at
>> scsi.c::scsi_finish_command(). ULD has control here.
> 
> The ->done function does not return the remainder of the request size; 
> it returns good_bytes, which is the number of bytes that were handled.
> 
> I agree that since the decision to split the request up into multiple 
> commands was made in sd.c, the decision of what to do about the 
> remainder should also be made in sd.c.  However the current structure 
> of the code doesn't seem to allow this to happen.  Requeue decisions 
> are all made in scsi_io_completion() or scsi_end_request().
> 
> Alan Stern
> 
I agree with all of what you said. Even if drv->done(cmd) would magically
guess what scsi_io_completion() is going to do and will return not good_bytes
(It's just a name) but lets call it bytes_to_complete. It will still be a
bug in the requeue case.

But now that I stared at the code harder. Current code is a complete bug.
the: "this_count = scsi_bufflen()" Has nothing to do with anything. 
Maybe it was meant to be: "this_count = scsi_bufflen() - good_bytes"
That is the count left over after the first complete. But after we
have completed good_bytes, the second complete is never scsi_bufflen().
(It used to work because most of the times it is bigger then the reminder
but then we can just use: "this_count = ~0")

Also what you did is not enough. What if the error is one of the known cases
above, where the "complete and return" is inside the case. They will hang also. 
Here is what I think should be:

---
git diff --stat -p drivers/scsi/
 drivers/scsi/scsi_lib.c |    7 +++----
 1 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index ff5d56b..36995e5 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -852,7 +852,7 @@ static void scsi_end_bidi_request(struct scsi_cmnd *cmd)
 void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
 {
 	int result = cmd->result;
-	int this_count = scsi_bufflen(cmd);
+	int this_count;
 	struct request_queue *q = cmd->device->request_queue;
 	struct request *req = cmd->request;
 	int error = 0;
@@ -909,9 +909,8 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
 	if (scsi_end_request(cmd, error, good_bytes, result == 0) == NULL)
 		return;
 
-	/* good_bytes = 0, or (inclusive) there were leftovers and
-	 * result = 0, so scsi_end_request couldn't retry.
-	 */
+	this_count = blk_rq_bytes(req);
+
 	if (sense_valid && !sense_deferred) {
 		switch (sshdr.sense_key) {
 		case UNIT_ATTENTION:

Re: BUG in handling of last_sector_bug flag

[Alan Stern]

On Wed, 13 Aug 2008, Boaz Harrosh wrote:

> But now that I stared at the code harder. Current code is a complete bug.
> the: "this_count = scsi_bufflen()" Has nothing to do with anything. 
> Maybe it was meant to be: "this_count = scsi_bufflen() - good_bytes"
> That is the count left over after the first complete. But after we
> have completed good_bytes, the second complete is never scsi_bufflen().
> (It used to work because most of the times it is bigger then the reminder
> but then we can just use: "this_count = ~0")
> 
> Also what you did is not enough. What if the error is one of the known cases
> above, where the "complete and return" is inside the case. They will hang also. 
> Here is what I think should be:
> 
> ---
> git diff --stat -p drivers/scsi/
>  drivers/scsi/scsi_lib.c |    7 +++----
>  1 files changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index ff5d56b..36995e5 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -852,7 +852,7 @@ static void scsi_end_bidi_request(struct scsi_cmnd *cmd)
>  void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
>  {
>  	int result = cmd->result;
> -	int this_count = scsi_bufflen(cmd);
> +	int this_count;
>  	struct request_queue *q = cmd->device->request_queue;
>  	struct request *req = cmd->request;
>  	int error = 0;
> @@ -909,9 +909,8 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
>  	if (scsi_end_request(cmd, error, good_bytes, result == 0) == NULL)
>  		return;
>  
> -	/* good_bytes = 0, or (inclusive) there were leftovers and
> -	 * result = 0, so scsi_end_request couldn't retry.
> -	 */
> +	this_count = blk_rq_bytes(req);
> +
>  	if (sense_valid && !sense_deferred) {
>  		switch (sshdr.sense_key) {
>  		case UNIT_ATTENTION:

Is this really right?  Consider the subcases that follow.  For example, 
in the UNIT ATTENTION case we have

			scsi_end_request(cmd, -EIO, this_count, 1);

With this_count equal to blk_rq_bytes(req), there will be no leftovers 
and so nothing will be requeued.  Shouldn't it really be

			scsi_end_request(cmd, -EIO, 0, 1);

The same holds for all the other places scsi_end_request() is called 
with requeue equal to 1, including the final call (if result == 0).  
This suggests that this_count should be eliminated entirely and each 
of those calls be replaced by either

			goto retry;
or
			goto fail;

together with:

 retry:
	scsi_end_request(cmd, -EIO, 0, 1);
	return;
 fail:
	scsi_end_request(cmd, -EIO, blk_rq_bytes(req), 0);
	return;

Alan Stern

PS: In my copy of the code, scsi_end_request() doesn't use
blk_rq_bytes().  Has this been updated in the SCSI development tree?

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: BUG in handling of last_sector_bug flag

[Boaz Harrosh]

Alan Stern wrote:
> On Wed, 13 Aug 2008, Boaz Harrosh wrote:
> 
>> But now that I stared at the code harder. Current code is a complete bug.
>> the: "this_count = scsi_bufflen()" Has nothing to do with anything. 
>> Maybe it was meant to be: "this_count = scsi_bufflen() - good_bytes"
>> That is the count left over after the first complete. But after we
>> have completed good_bytes, the second complete is never scsi_bufflen().
>> (It used to work because most of the times it is bigger then the reminder
>> but then we can just use: "this_count = ~0")
>>
>> Also what you did is not enough. What if the error is one of the known cases
>> above, where the "complete and return" is inside the case. They will hang also. 
>> Here is what I think should be:
>>
>> ---
>> git diff --stat -p drivers/scsi/
>>  drivers/scsi/scsi_lib.c |    7 +++----
>>  1 files changed, 3 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
>> index ff5d56b..36995e5 100644
>> --- a/drivers/scsi/scsi_lib.c
>> +++ b/drivers/scsi/scsi_lib.c
>> @@ -852,7 +852,7 @@ static void scsi_end_bidi_request(struct scsi_cmnd *cmd)
>>  void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
>>  {
>>  	int result = cmd->result;
>> -	int this_count = scsi_bufflen(cmd);
>> +	int this_count;
>>  	struct request_queue *q = cmd->device->request_queue;
>>  	struct request *req = cmd->request;
>>  	int error = 0;
>> @@ -909,9 +909,8 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
>>  	if (scsi_end_request(cmd, error, good_bytes, result == 0) == NULL)
>>  		return;
>>  
>> -	/* good_bytes = 0, or (inclusive) there were leftovers and
>> -	 * result = 0, so scsi_end_request couldn't retry.
>> -	 */
>> +	this_count = blk_rq_bytes(req);
>> +
>>  	if (sense_valid && !sense_deferred) {
>>  		switch (sshdr.sense_key) {
>>  		case UNIT_ATTENTION:
> 
> Is this really right?  Consider the subcases that follow.  For example, 
> in the UNIT ATTENTION case we have
> 
> 			scsi_end_request(cmd, -EIO, this_count, 1);
> 
> With this_count equal to blk_rq_bytes(req), there will be no leftovers 
> and so nothing will be requeued.  Shouldn't it really be
> 
> 			scsi_end_request(cmd, -EIO, 0, 1);
> 
> The same holds for all the other places scsi_end_request() is called 
> with requeue equal to 1, including the final call (if result == 0).  
> This suggests that this_count should be eliminated entirely and each 
> of those calls be replaced by either
> 
> 			goto retry;
> or
> 			goto fail;
> 
> together with:
> 
>  retry:
> 	scsi_end_request(cmd, -EIO, 0, 1);
> 	return;
>  fail:
> 	scsi_end_request(cmd, -EIO, blk_rq_bytes(req), 0);
> 	return;
> 

OK Yes, you are absolutely right. Please send a proposal and we'll 
stare at it for a while.

One thing I don't like is the now blk_end_request(req, -EIO, bytes=0)
inside scsi_end_request(). What's the point of calling that with 0 bytes?
Maybe fix that too.

> Alan Stern
> 
> PS: In my copy of the code, scsi_end_request() doesn't use
> blk_rq_bytes().  Has this been updated in the SCSI development tree?
> 

No it does not. I was just commenting on the new code. If you fix up
scsi_end_request() you should fix this too.

Thanks for doing this. This is a long outstanding nagging bug. It was
encountered before.

Boaz
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: BUG in handling of last_sector_bug flag

[Boaz Harrosh]

Boaz Harrosh wrote:
> Alan Stern wrote:
>> On Wed, 13 Aug 2008, Boaz Harrosh wrote:
>>
>>> But now that I stared at the code harder. Current code is a complete bug.
>>> the: "this_count = scsi_bufflen()" Has nothing to do with anything. 
>>> Maybe it was meant to be: "this_count = scsi_bufflen() - good_bytes"
>>> That is the count left over after the first complete. But after we
>>> have completed good_bytes, the second complete is never scsi_bufflen().
>>> (It used to work because most of the times it is bigger then the reminder
>>> but then we can just use: "this_count = ~0")
>>>
>>> Also what you did is not enough. What if the error is one of the known cases
>>> above, where the "complete and return" is inside the case. They will hang also. 
>>> Here is what I think should be:
>>>
>>> ---
>>> git diff --stat -p drivers/scsi/
>>>  drivers/scsi/scsi_lib.c |    7 +++----
>>>  1 files changed, 3 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
>>> index ff5d56b..36995e5 100644
>>> --- a/drivers/scsi/scsi_lib.c
>>> +++ b/drivers/scsi/scsi_lib.c
>>> @@ -852,7 +852,7 @@ static void scsi_end_bidi_request(struct scsi_cmnd *cmd)
>>>  void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
>>>  {
>>>  	int result = cmd->result;
>>> -	int this_count = scsi_bufflen(cmd);
>>> +	int this_count;
>>>  	struct request_queue *q = cmd->device->request_queue;
>>>  	struct request *req = cmd->request;
>>>  	int error = 0;
>>> @@ -909,9 +909,8 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
>>>  	if (scsi_end_request(cmd, error, good_bytes, result == 0) == NULL)
>>>  		return;
>>>  
>>> -	/* good_bytes = 0, or (inclusive) there were leftovers and
>>> -	 * result = 0, so scsi_end_request couldn't retry.
>>> -	 */
>>> +	this_count = blk_rq_bytes(req);
>>> +
>>>  	if (sense_valid && !sense_deferred) {
>>>  		switch (sshdr.sense_key) {
>>>  		case UNIT_ATTENTION:
>> Is this really right?  Consider the subcases that follow.  For example, 
>> in the UNIT ATTENTION case we have
>>
>> 			scsi_end_request(cmd, -EIO, this_count, 1);
>>
>> With this_count equal to blk_rq_bytes(req), there will be no leftovers 
>> and so nothing will be requeued.  Shouldn't it really be
>>
>> 			scsi_end_request(cmd, -EIO, 0, 1);
>>
>> The same holds for all the other places scsi_end_request() is called 
>> with requeue equal to 1, including the final call (if result == 0).  
>> This suggests that this_count should be eliminated entirely and each 
>> of those calls be replaced by either
>>
>> 			goto retry;
>> or
>> 			goto fail;
>>
>> together with:
>>
>>  retry:
>> 	scsi_end_request(cmd, -EIO, 0, 1);
>> 	return;
>>  fail:
>> 	scsi_end_request(cmd, -EIO, blk_rq_bytes(req), 0);
>> 	return;
>>
> 
> OK Yes, you are absolutely right. Please send a proposal and we'll 
> stare at it for a while.
> 
> One thing I don't like is the now blk_end_request(req, -EIO, bytes=0)
> inside scsi_end_request(). What's the point of calling that with 0 bytes?
> Maybe fix that too.
> 
>> Alan Stern
>>
>> PS: In my copy of the code, scsi_end_request() doesn't use
>> blk_rq_bytes().  Has this been updated in the SCSI development tree?
>>
> 
> No it does not. I was just commenting on the new code. If you fix up
> scsi_end_request() you should fix this too.
> 
> Thanks for doing this. This is a long outstanding nagging bug. It was
> encountered before.
> 
> Boaz
> 

One more thing I do not understand is: inside scsi_io_completion()
in some places it will requeue with: "scsi_end_request(cmd, -EIO, this_count, 1)"
and at other places it will directly requeue with: "scsi_requeue_command(q, cmd);"
The difference being in the later case not calling scsi_next_command(cmd);

Do you understand why?

Boaz

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: BUG in handling of last_sector_bug flag

[Alan Stern]

On Wed, 13 Aug 2008, Boaz Harrosh wrote:

> OK Yes, you are absolutely right. Please send a proposal and we'll 
> stare at it for a while.
> 
> One thing I don't like is the now blk_end_request(req, -EIO, bytes=0)
> inside scsi_end_request(). What's the point of calling that with 0 bytes?
> Maybe fix that too.
> 
> > Alan Stern
> > 
> > PS: In my copy of the code, scsi_end_request() doesn't use
> > blk_rq_bytes().  Has this been updated in the SCSI development tree?
> > 
> 
> No it does not. I was just commenting on the new code. If you fix up
> scsi_end_request() you should fix this too.
> 
> Thanks for doing this. This is a long outstanding nagging bug. It was
> encountered before.

> One more thing I do not understand is: inside scsi_io_completion()
> in some places it will requeue with: "scsi_end_request(cmd, -EIO, this_count, 1)"
> and at other places it will directly requeue with: "scsi_requeue_command(q, cmd);"
> The difference being in the later case not calling scsi_next_command(cmd);
> 
> Do you understand why?

I'm not sure.  Maybe it's part of the original conceptual bug about
calling blk_end_request(req, error, this_count).  The only other
difference between the two approaches is that scsi_end_request() won't
requeue if blk_noretry_request is true -- it will just terminate the 
request.

Here's the new patch, addressing all your points.  I have not tested 
it.

Alan Stern


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
@@ -674,16 +674,12 @@ static struct scsi_cmnd *scsi_end_reques
 	 * If there are blocks left over at the end, set up the command
 	 * to queue the remainder of them.
 	 */
-	if (blk_end_request(req, error, bytes)) {
-		int leftover = (req->hard_nr_sectors << 9);
-
-		if (blk_pc_request(req))
-			leftover = req->data_len;
+	if (bytes == 0 || blk_end_request(req, error, bytes)) {
 
 		/* kill remainder if no retrys */
-		if (error && blk_noretry_request(req))
-			blk_end_request(req, error, leftover);
-		else {
+		if (error && blk_noretry_request(req)) {
+			blk_end_request(req, error, blk_rq_bytes(req));
+		} else {
 			if (requeue) {
 				/*
 				 * Bleah.  Leftovers again.  Stick the
@@ -852,7 +848,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 = scsi_bufflen(cmd);
 	struct request_queue *q = cmd->device->request_queue;
 	struct request *req = cmd->request;
 	int error = 0;
@@ -909,9 +904,6 @@ void scsi_io_completion(struct scsi_cmnd
 	if (scsi_end_request(cmd, error, good_bytes, result == 0) == NULL)
 		return;
 
-	/* 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:
@@ -920,8 +912,7 @@ 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 retry;
 			} else {
 				/* Must have been a power glitch, or a
 				 * bus reset.  Could not have been a
@@ -951,15 +942,13 @@ void scsi_io_completion(struct scsi_cmnd
 				 */
 				scsi_requeue_command(q, cmd);
 			} 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);
+				goto retry;
 			return;
 		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
@@ -985,8 +974,7 @@ void scsi_io_completion(struct scsi_cmnd
 							 "Device not ready",
 							 &sshdr);
 
-			scsi_end_request(cmd, -EIO, this_count, 1);
-			return;
+			goto retry;
 		case VOLUME_OVERFLOW:
 			if (!(req->cmd_flags & REQ_QUIET)) {
 				scmd_printk(KERN_INFO, cmd,
@@ -995,8 +983,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 retry;
 		default:
 			break;
 		}
@@ -1015,8 +1002,13 @@ 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);
+ retry:
+	scsi_end_request(cmd, -EIO, 0, 1);
+	return;
+ fail:
+	scsi_end_request(cmd, -EIO, blk_rq_bytes(req), 0);
 }
 
 static int scsi_init_sgtable(struct request *req, struct scsi_data_buffer *sdb,

Re: BUG in handling of last_sector_bug flag

[Alan Stern]

On Wed, 13 Aug 2008, Boaz Harrosh wrote:

> One thing I don't like is the now blk_end_request(req, -EIO, bytes=0)
> inside scsi_end_request(). What's the point of calling that with 0 bytes?
> Maybe fix that too.

While testing the patch, I learned what the point is.  :-)

Actually it's very simple; you just have to remember that not all 
requests are BLOCK_FS type.  Other types of request can indeed have a 
transfer length of 0, and we want to end those requests normally.

So this version of the patch works better than the earlier one.  In 
principle there doesn't seem to be any reason not to call 
blk_end_request with bytes = 0, but I left the test in there.

Alan, you might want to test this version and see how well it works for 
you.

Alan Stern



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
@@ -674,16 +674,13 @@ static struct scsi_cmnd *scsi_end_reques
 	 * If there are blocks left over at the end, set up the command
 	 * to queue the remainder of them.
 	 */
-	if (blk_end_request(req, error, bytes)) {
-		int leftover = (req->hard_nr_sectors << 9);
-
-		if (blk_pc_request(req))
-			leftover = req->data_len;
+	if (unlikely(bytes == 0 && blk_fs_request(req)) ||
+			blk_end_request(req, error, bytes)) {
 
 		/* kill remainder if no retrys */
-		if (error && blk_noretry_request(req))
-			blk_end_request(req, error, leftover);
-		else {
+		if (error && blk_noretry_request(req)) {
+			blk_end_request(req, error, blk_rq_bytes(req));
+		} else {
 			if (requeue) {
 				/*
 				 * Bleah.  Leftovers again.  Stick the
@@ -852,7 +849,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 = scsi_bufflen(cmd);
 	struct request_queue *q = cmd->device->request_queue;
 	struct request *req = cmd->request;
 	int error = 0;
@@ -909,9 +905,6 @@ void scsi_io_completion(struct scsi_cmnd
 	if (scsi_end_request(cmd, error, good_bytes, result == 0) == NULL)
 		return;
 
-	/* 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:
@@ -920,8 +913,7 @@ 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 retry;
 			} else {
 				/* Must have been a power glitch, or a
 				 * bus reset.  Could not have been a
@@ -951,15 +943,13 @@ void scsi_io_completion(struct scsi_cmnd
 				 */
 				scsi_requeue_command(q, cmd);
 			} 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);
+				goto retry;
 			return;
 		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
@@ -985,8 +975,7 @@ void scsi_io_completion(struct scsi_cmnd
 							 "Device not ready",
 							 &sshdr);
 
-			scsi_end_request(cmd, -EIO, this_count, 1);
-			return;
+			goto retry;
 		case VOLUME_OVERFLOW:
 			if (!(req->cmd_flags & REQ_QUIET)) {
 				scmd_printk(KERN_INFO, cmd,
@@ -995,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 retry;
 		default:
 			break;
 		}
@@ -1015,8 +1003,13 @@ 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);
+ retry:
+	scsi_end_request(cmd, -EIO, 0, 1);
+	return;
+ fail:
+	scsi_end_request(cmd, -EIO, blk_rq_bytes(req), 0);
 }
 
 static int scsi_init_sgtable(struct request *req, struct scsi_data_buffer *sdb,

Re: BUG in handling of last_sector_bug flag

[Alan Jenkins]

Alan Stern wrote:
> On Wed, 13 Aug 2008, Boaz Harrosh wrote:
>
>   
>> One thing I don't like is the now blk_end_request(req, -EIO, bytes=0)
>> inside scsi_end_request(). What's the point of calling that with 0 bytes?
>> Maybe fix that too.
>>     
>
> While testing the patch, I learned what the point is.  :-)
>
> Actually it's very simple; you just have to remember that not all 
> requests are BLOCK_FS type.  Other types of request can indeed have a 
> transfer length of 0, and we want to end those requests normally.
>
> So this version of the patch works better than the earlier one.  In 
> principle there doesn't seem to be any reason not to call 
> blk_end_request with bytes = 0, but I left the test in there.
>
> Alan, you might want to test this version and see how well it works for 
> you.
>   
Yes, that fixes it!

I applied your patch and reduced SD_LAST_BUGGY_SECTORS to 1.  As
expected, when I inserted my USB cardreader the kernel log shows
"uncorrected read errors", as /lib/udev/vol_id triggers the hardware
bug.  But vol_id doesn't hang any more.  udev goes on to create the
device node, I get the KDE popup and can successfully mount the device.

Great work, thanks.

Alan
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: BUG in handling of last_sector_bug flag

[Antonio Ospite]

[-- Attachment #1: Type: text/plain, Size: 1564 bytes --]

On Thu, 14 Aug 2008 15:41:28 -0400 (EDT)
Alan Stern <stern@rowland.harvard.edu> wrote:

> On Wed, 13 Aug 2008, Boaz Harrosh wrote:
> 
> > One thing I don't like is the now blk_end_request(req, -EIO, bytes=0)
> > inside scsi_end_request(). What's the point of calling that with 0 bytes?
> > Maybe fix that too.
> 
> While testing the patch, I learned what the point is.  :-)
> 
> Actually it's very simple; you just have to remember that not all 
> requests are BLOCK_FS type.  Other types of request can indeed have a 
> transfer length of 0, and we want to end those requests normally.
> 
> So this version of the patch works better than the earlier one.  In 
> principle there doesn't seem to be any reason not to call 
> blk_end_request with bytes = 0, but I left the test in there.
> 
> Alan, you might want to test this version and see how well it works for 
> you.
> 
> Alan Stern
>

Thanks Alan,

the latest patch fixes the problem for me as well, just great.

Now here's the log I have, there are still Medium errors, but I think
that's ok, I also had them with 2.6.24 after all.
http://shell.studenti.unina.it/~ospite/tmp/linux-usb/usb-mass-storage+Alan_Stern_patch-2.6.27-rc3-WORKING.log

Best regards,
   Antonio Ospite

-- 
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing in e-mail?

  Web site: http://www.studenti.unina.it/~ospite
Public key: http://www.studenti.unina.it/~ospite/aopubkey.asc

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: BUG in handling of last_sector_bug flag

[Antonio Ospite]

[-- Attachment #1: Type: text/plain, Size: 1313 bytes --]

On Thu, 14 Aug 2008 15:41:28 -0400 (EDT)
Alan Stern <stern@rowland.harvard.edu> wrote:

> On Wed, 13 Aug 2008, Boaz Harrosh wrote:
> 
> > One thing I don't like is the now blk_end_request(req, -EIO, bytes=0)
> > inside scsi_end_request(). What's the point of calling that with 0 bytes?
> > Maybe fix that too.
> 
> While testing the patch, I learned what the point is.  :-)
> 
> Actually it's very simple; you just have to remember that not all 
> requests are BLOCK_FS type.  Other types of request can indeed have a 
> transfer length of 0, and we want to end those requests normally.
> 
> So this version of the patch works better than the earlier one.  In 
> principle there doesn't seem to be any reason not to call 
> blk_end_request with bytes = 0, but I left the test in there.
> 
> Alan, you might want to test this version and see how well it works for 
> you.
> 
> Alan Stern
> 

Hi,

is the fix going to be merged? I haven't seen it in rc4-git series.

Thanks,
   Antonio

-- 
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing in e-mail?

  Web site: http://www.studenti.unina.it/~ospite
Public key: http://www.studenti.unina.it/~ospite/aopubkey.asc

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: BUG in handling of last_sector_bug flag

[Alan Stern]

On Tue, 26 Aug 2008, Antonio Ospite wrote:

> Hi,
> 
> is the fix going to be merged? I haven't seen it in rc4-git series.

Sorry, it slipped my mind.  I'll submit it shortly.  However it may not 
be accepted right away, since it affects a very important and difficult 
part of the SCSI stack.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: BUG in handling of last_sector_bug flag

[James Bottomley]

On Wed, 2008-08-27 at 10:21 -0400, Alan Stern wrote:
> On Tue, 26 Aug 2008, Antonio Ospite wrote:
> 
> > Hi,
> > 
> > is the fix going to be merged? I haven't seen it in rc4-git series.
> 
> Sorry, it slipped my mind.  I'll submit it shortly.  However it may not 
> be accepted right away, since it affects a very important and difficult 
> part of the SCSI stack.

Um, it was my impression that the failure to retry hang problem was, in
fact, fixed in USB by this:

commit 580da34847488b404218d1d7f53b156f245f5555
Author: Alan Stern <stern@rowland.harvard.edu>
Date:   Tue Aug 5 13:05:17 2008 -0400

    Fix USB storage hang on command abort

What bug is still manifesting with the last_sector_bug flag that still
needs fixing?

James

Re: BUG in handling of last_sector_bug flag

[Alan Stern]

On Wed, 27 Aug 2008, James Bottomley wrote:

> On Wed, 2008-08-27 at 10:21 -0400, Alan Stern wrote:
> > On Tue, 26 Aug 2008, Antonio Ospite wrote:
> > 
> > > Hi,
> > > 
> > > is the fix going to be merged? I haven't seen it in rc4-git series.
> > 
> > Sorry, it slipped my mind.  I'll submit it shortly.  However it may not 
> > be accepted right away, since it affects a very important and difficult 
> > part of the SCSI stack.
> 
> Um, it was my impression that the failure to retry hang problem was, in
> fact, fixed in USB by this:
> 
> commit 580da34847488b404218d1d7f53b156f245f5555
> Author: Alan Stern <stern@rowland.harvard.edu>
> Date:   Tue Aug 5 13:05:17 2008 -0400
> 
>     Fix USB storage hang on command abort

No, that commit fixed a totally different bug.

> What bug is still manifesting with the last_sector_bug flag that still
> needs fixing?

This bug occurs when the last_sector_bug flag causes a multi-sector 
request request to be broken up into multiple single-sector commands.  
If one of those commands fails then the remainder of the request never 
completes, because of bad communication between scsi_io_completion() 
and scsi_end_request().  The calling program then hangs in the kernel, 
waiting for the request to finish.

I suppose there are two possible approaches when this happens.  We 
could either fail the entire request then and there, or else just fail 
the portion corresponding to the last command and then continue on to 
try the remainder.

The existing code implicitly takes the first approach by falling
through to the end of scsi_io_completion() with result nonzero.  But 
this_count is set to the size of the current command instead of the 
remaining size of the request.  So only part of the request gets 
completed, and the request isn't added back to the queue for retrying.  
Thus it never fully completes.

Alan Stern

[PATCH] Fix handling of failed requests in scsi_io_completion

[Alan Stern]

This patch (as1133) fixes a bug in the interaction between
scsi_end_request() and scsi_io_completion().  The bug was triggered by
recent changes to accomodate USB drives can't access their last few
sectors using multi-sector transfers (the so-called last_sector_bug
flag).

The bug shows up when a multi-sector I/O request has been broken up
into single-sector commands.  If one of those commands gets an error,
the remainder of the request never gets completed.  This is because
the "bytes" value passed to scsi_end_request() is completely wrong; it
is the transfer size of the current command, not the total transfer
size of the request.

Changes made in the patch include:

	Use the blk_rq_bytes() routine to get the remaining size
	of a request instead of calculating it by hand.

	Don't bother to call blk_end_request() for a BLK_TYPE_FS
	request when the number of bytes transferred is 0.  (This
	test isn't really needed; it is a possibly premature
	optimization.)

	Remove a comment in scsi_io_completion() that has bothered
	me for a long time.  Although it's less than two lines long,
	it contains at least three major mistakes.

	Replace a bunch of repetitious calls to scsi_end_request() for
	-EIO errors by either "goto retry" or "goto fail", depending
	on whether or not the individual request should be retried.

	In the "retry" case, don't claim that any additional bytes
	have been transferred since the last call to
	scsi_end_request().  They haven't.

	In the "fail" case, pass the total number of bytes remaining
	in the request (i.e., blk_rq_bytes(req)) so that the entire
	request will complete.

Signed-off-by: Alan Stern <stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz@public.gmane.org>

---

This patch could be broken up into several simpler patches, of which 
all but the last would be uncontroversial.  But first I'd like to get 
some feedback concerning the overall correctness.

Alan Stern



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
@@ -674,16 +674,13 @@ static struct scsi_cmnd *scsi_end_reques
 	 * If there are blocks left over at the end, set up the command
 	 * to queue the remainder of them.
 	 */
-	if (blk_end_request(req, error, bytes)) {
-		int leftover = (req->hard_nr_sectors << 9);
-
-		if (blk_pc_request(req))
-			leftover = req->data_len;
+	if (unlikely(bytes == 0 && blk_fs_request(req)) ||
+			blk_end_request(req, error, bytes)) {
 
 		/* kill remainder if no retrys */
-		if (error && blk_noretry_request(req))
-			blk_end_request(req, error, leftover);
-		else {
+		if (error && blk_noretry_request(req)) {
+			blk_end_request(req, error, blk_rq_bytes(req));
+		} else {
 			if (requeue) {
 				/*
 				 * Bleah.  Leftovers again.  Stick the
@@ -852,7 +849,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 = scsi_bufflen(cmd);
 	struct request_queue *q = cmd->device->request_queue;
 	struct request *req = cmd->request;
 	int error = 0;
@@ -909,9 +905,6 @@ void scsi_io_completion(struct scsi_cmnd
 	if (scsi_end_request(cmd, error, good_bytes, result == 0) == NULL)
 		return;
 
-	/* 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:
@@ -920,8 +913,7 @@ 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 retry;
 			} else {
 				/* Must have been a power glitch, or a
 				 * bus reset.  Could not have been a
@@ -951,15 +943,13 @@ void scsi_io_completion(struct scsi_cmnd
 				 */
 				scsi_requeue_command(q, cmd);
 			} 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);
+				goto retry;
 			return;
 		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
@@ -985,8 +975,7 @@ void scsi_io_completion(struct scsi_cmnd
 							 "Device not ready",
 							 &sshdr);
 
-			scsi_end_request(cmd, -EIO, this_count, 1);
-			return;
+			goto retry;
 		case VOLUME_OVERFLOW:
 			if (!(req->cmd_flags & REQ_QUIET)) {
 				scmd_printk(KERN_INFO, cmd,
@@ -995,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 retry;
 		default:
 			break;
 		}
@@ -1015,8 +1003,13 @@ 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);
+ retry:
+	scsi_end_request(cmd, -EIO, 0, 1);
+	return;
+ fail:
+	scsi_end_request(cmd, -EIO, blk_rq_bytes(req), 0);
 }
 
 static int scsi_init_sgtable(struct request *req, struct scsi_data_buffer *sdb,

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Fix handling of failed requests in scsi_io_completion

[Alan Stern]

On Wed, 27 Aug 2008, Alan Stern wrote:

> This patch (as1133) fixes a bug in the interaction between
> scsi_end_request() and scsi_io_completion().  The bug was triggered by
> recent changes to accomodate USB drives can't access their last few
> sectors using multi-sector transfers (the so-called last_sector_bug
> flag).
> 
> The bug shows up when a multi-sector I/O request has been broken up
> into single-sector commands.  If one of those commands gets an error,
> the remainder of the request never gets completed.  This is because
> the "bytes" value passed to scsi_end_request() is completely wrong; it
> is the transfer size of the current command, not the total transfer
> size of the request.

There hasn't been any reply to this patch submission.  For reference,
the original message is at

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

Has it been accepted, rejected, or did it fall through the cracks?

Alan Stern

Re: [PATCH] Fix handling of failed requests in scsi_io_completion

[Antonio Ospite]

[-- Attachment #1: Type: text/plain, Size: 1586 bytes --]

On Fri, 5 Sep 2008 15:35:13 -0400 (EDT)
Alan Stern <stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz@public.gmane.org> wrote:

> On Wed, 27 Aug 2008, Alan Stern wrote:
> 
> > This patch (as1133) fixes a bug in the interaction between
> > scsi_end_request() and scsi_io_completion().  The bug was triggered by
> > recent changes to accomodate USB drives can't access their last few
> > sectors using multi-sector transfers (the so-called last_sector_bug
> > flag).
> > 
> > The bug shows up when a multi-sector I/O request has been broken up
> > into single-sector commands.  If one of those commands gets an error,
> > the remainder of the request never gets completed.  This is because
> > the "bytes" value passed to scsi_end_request() is completely wrong; it
> > is the transfer size of the current command, not the total transfer
> > size of the request.
> 
> There hasn't been any reply to this patch submission.  For reference,
> the original message is at
> 
> 	http://marc.info/?l=linux-scsi&m=121986305201166&w=2
> 
> Has it been accepted, rejected, or did it fall through the cracks?
> 
> Alan Stern
>

Hi again,

anything new about this patch? Alan, I'll keep reminding about it from
time to time, until it is accepted or rejected.

Thanks,
   Antonio

-- 
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing in e-mail?

  Web site: http://www.studenti.unina.it/~ospite
Public key: http://www.studenti.unina.it/~ospite/aopubkey.asc

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] Fix handling of failed requests in scsi_io_completion

[Alan Stern]

On Fri, 19 Sep 2008, Antonio Ospite wrote:

> On Fri, 5 Sep 2008 15:35:13 -0400 (EDT)
> Alan Stern <stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz@public.gmane.org> wrote:
> 
> > On Wed, 27 Aug 2008, Alan Stern wrote:
> > 
> > > This patch (as1133) fixes a bug in the interaction between
> > > scsi_end_request() and scsi_io_completion().  The bug was triggered by
> > > recent changes to accomodate USB drives can't access their last few
> > > sectors using multi-sector transfers (the so-called last_sector_bug
> > > flag).
> > > 
> > > The bug shows up when a multi-sector I/O request has been broken up
> > > into single-sector commands.  If one of those commands gets an error,
> > > the remainder of the request never gets completed.  This is because
> > > the "bytes" value passed to scsi_end_request() is completely wrong; it
> > > is the transfer size of the current command, not the total transfer
> > > size of the request.
> > 
> > There hasn't been any reply to this patch submission.  For reference,
> > the original message is at
> > 
> > 	http://marc.info/?l=linux-scsi&m=121986305201166&w=2
> > 
> > Has it been accepted, rejected, or did it fall through the cracks?
> > 
> > Alan Stern
> >
> 
> Hi again,
> 
> anything new about this patch? Alan, I'll keep reminding about it from
> time to time, until it is accepted or rejected.

All I can tell you is that I asked James Bottomley at the Linux Kernel
Summit, and he said he has been too busy to look at it but it's got an
"!" on his to-do list.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Fix handling of failed requests in scsi_io_completion

[James Bottomley]

On Fri, 2008-09-19 at 11:53 -0400, Alan Stern wrote:
> On Fri, 19 Sep 2008, Antonio Ospite wrote:
> 
> > On Fri, 5 Sep 2008 15:35:13 -0400 (EDT)
> > Alan Stern <stern@rowland.harvard.edu> wrote:
> > 
> > > On Wed, 27 Aug 2008, Alan Stern wrote:
> > > 
> > > > This patch (as1133) fixes a bug in the interaction between
> > > > scsi_end_request() and scsi_io_completion().  The bug was triggered by
> > > > recent changes to accomodate USB drives can't access their last few
> > > > sectors using multi-sector transfers (the so-called last_sector_bug
> > > > flag).
> > > > 
> > > > The bug shows up when a multi-sector I/O request has been broken up
> > > > into single-sector commands.  If one of those commands gets an error,
> > > > the remainder of the request never gets completed.  This is because
> > > > the "bytes" value passed to scsi_end_request() is completely wrong; it
> > > > is the transfer size of the current command, not the total transfer
> > > > size of the request.
> > > 
> > > There hasn't been any reply to this patch submission.  For reference,
> > > the original message is at
> > > 
> > > 	http://marc.info/?l=linux-scsi&m=121986305201166&w=2
> > > 
> > > Has it been accepted, rejected, or did it fall through the cracks?
> > > 
> > > Alan Stern
> > >
> > 
> > Hi again,
> > 
> > anything new about this patch? Alan, I'll keep reminding about it from
> > time to time, until it is accepted or rejected.
> 
> All I can tell you is that I asked James Bottomley at the Linux Kernel
> Summit, and he said he has been too busy to look at it but it's got an
> "!" on his to-do list.

OK, I looked at it.  Sorry about the delay; I wanted to audit the users
to make sure we didn't over complete.  There's lots more than just the
last sector bug that relies on partial completions doing request
chunking.  I don't like your bytes == 0 micro optimisation, but other
than that, it looks fine.

However, there's too much code movement in this for a bug fix that has
to go into a late -rc and the stable series.  I think the patch below
represents the smallest and simplest (and thus least error prone) fix,
doesn't it (the rest can go into scsi-misc as an enhancement)?

With regard to what the mid-layer is doing, for the error cases where we
send enough bytes to complete the request fully, I think the requeue
looks superfluous (it's never going to get into that code in
scsi_end_request), The other strange bit is that I can't find any error
cases where we don't complete everything ... i.e. the partial complete
and requeue never applies, so we should probably just be using
end_dequeued_request().  I also think we could junk scsi_end_request()
in faviour of blk_end_request and do the blk_noretry_request() check
inline in scsi_io_completion, after we try to complete but before we
might retry.

James

---

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index ff5d56b..62307bd 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -852,7 +852,7 @@ static void scsi_end_bidi_request(struct scsi_cmnd *cmd)
 void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
 {
 	int result = cmd->result;
-	int this_count = scsi_bufflen(cmd);
+	int this_count;
 	struct request_queue *q = cmd->device->request_queue;
 	struct request *req = cmd->request;
 	int error = 0;
@@ -908,6 +908,7 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
 	 */
 	if (scsi_end_request(cmd, error, good_bytes, result == 0) == NULL)
 		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.

Re: [PATCH] Fix handling of failed requests in scsi_io_completion

[Alan Stern]

On Sat, 20 Sep 2008, James Bottomley wrote:

> OK, I looked at it.  Sorry about the delay; I wanted to audit the users
> to make sure we didn't over complete.  There's lots more than just the
> last sector bug that relies on partial completions doing request
> chunking.  I don't like your bytes == 0 micro optimisation, but other
> than that, it looks fine.

I'm perfectly happy to remove that micro optimization.  Also it would 
be good to break all the code motion out into a separate preliminary 
patch, apart from the functional changes.  (The code motion in itself 
is a worthwhile cleanup, IMO.)

> However, there's too much code movement in this for a bug fix that has
> to go into a late -rc and the stable series.  I think the patch below
> represents the smallest and simplest (and thus least error prone) fix,
> doesn't it (the rest can go into scsi-misc as an enhancement)?

Your patch doesn't seem quite right.  Comments below.

As regards too much change for this late in -rc...  This bug affects 
only a handful of devices, so far as we've seen.  It wouldn't hurt to 
delay the fix until 2.6.27.stable.

> With regard to what the mid-layer is doing, for the error cases where we
> send enough bytes to complete the request fully, I think the requeue
> looks superfluous (it's never going to get into that code in
> scsi_end_request),

Agreed.

> The other strange bit is that I can't find any error
> cases where we don't complete everything ... i.e. the partial complete
> and requeue never applies, so we should probably just be using
> end_dequeued_request().

These cases can arise from the "last-sector bug" stuff.  As an example,
suppose a program tries to read the last 2 sectors of the device.  The
"last-sector bug" code breaks the request in two; the first command
will read only the second-to-last sector.  If this command fails with
an error, you're in exactly this case -- an error in which less than
the entire request has been completed.  Depending on the exact nature 
of the error, we will choose between failing the entire request or 
failing just the second-to-last sector (and requeuing the command in 
order to try reading the last sector).

>  I also think we could junk scsi_end_request()
> in faviour of blk_end_request and do the blk_noretry_request() check
> inline in scsi_io_completion, after we try to complete but before we
> might retry.

Possibly.  With the patch in place, scsi_end_request() gets called in 
only three places, one of which doesn't attempt to do any retries.  But 
that still leaves two places which might attempt it, so the retry logic 
would have to be inlined twice.

> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -852,7 +852,7 @@ static void scsi_end_bidi_request(struct scsi_cmnd *cmd)
>  void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
>  {
>  	int result = cmd->result;
> -	int this_count = scsi_bufflen(cmd);
> +	int this_count;
>  	struct request_queue *q = cmd->device->request_queue;
>  	struct request *req = cmd->request;
>  	int error = 0;
> @@ -908,6 +908,7 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
>  	 */
>  	if (scsi_end_request(cmd, error, good_bytes, result == 0) == NULL)
>  		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.

The problem is what happens in all the "retry" paths that follow.  
They end up doing this:

		scsi_end_request(cmd, -EIO, this_count, 1);

when in fact they should do this:

		scsi_end_request(cmd, -EIO, 0, 1);

(where the -EIO value is ignored).

By passing this_count as the third argument, they are telling the block
layer that the entire remainder of the request completed with an error.  
Hence blk_end_request() will return 0, scsi_end_request() won't take
its "blocks left over at the end" pathway, and nothing will be
requeued.

Alan Stern

Re: [PATCH] Fix handling of failed requests in scsi_io_completion

[James Bottomley]

On Sat, 2008-09-20 at 13:06 -0400, Alan Stern wrote:
> On Sat, 20 Sep 2008, James Bottomley wrote:
> 
> > OK, I looked at it.  Sorry about the delay; I wanted to audit the users
> > to make sure we didn't over complete.  There's lots more than just the
> > last sector bug that relies on partial completions doing request
> > chunking.  I don't like your bytes == 0 micro optimisation, but other
> > than that, it looks fine.
> 
> I'm perfectly happy to remove that micro optimization.  Also it would 
> be good to break all the code motion out into a separate preliminary 
> patch, apart from the functional changes.  (The code motion in itself 
> is a worthwhile cleanup, IMO.)
> 
> > However, there's too much code movement in this for a bug fix that has
> > to go into a late -rc and the stable series.  I think the patch below
> > represents the smallest and simplest (and thus least error prone) fix,
> > doesn't it (the rest can go into scsi-misc as an enhancement)?
> 
> Your patch doesn't seem quite right.  Comments below.
> 
> As regards too much change for this late in -rc...  This bug affects 
> only a handful of devices, so far as we've seen.  It wouldn't hurt to 
> delay the fix until 2.6.27.stable.
> 
> > With regard to what the mid-layer is doing, for the error cases where we
> > send enough bytes to complete the request fully, I think the requeue
> > looks superfluous (it's never going to get into that code in
> > scsi_end_request),
> 
> Agreed.
> 
> > The other strange bit is that I can't find any error
> > cases where we don't complete everything ... i.e. the partial complete
> > and requeue never applies, so we should probably just be using
> > end_dequeued_request().
> 
> These cases can arise from the "last-sector bug" stuff.  As an example,
> suppose a program tries to read the last 2 sectors of the device.  The
> "last-sector bug" code breaks the request in two; the first command
> will read only the second-to-last sector.  If this command fails with
> an error, you're in exactly this case -- an error in which less than
> the entire request has been completed.  Depending on the exact nature 
> of the error, we will choose between failing the entire request or 
> failing just the second-to-last sector (and requeuing the command in 
> order to try reading the last sector).

No ... I understand how they arise:  They arise from any transaction
where the transaction length is less than the request length ... that's
not just the last sector bug stuff.

What I mean is that I can't find an error case that's currently shown
scsi_end_request(cmd, -EIO, this_count, 1) where requeuing after
completing only the currently attempted transfer is valid.  If this had
all been done as a single transaction, we'd have killed everything at
this point.  Just because we split the request into multiple
transactions doesn't mean we should go back around and try a new
transaction after we hit an error.

> >  I also think we could junk scsi_end_request()
> > in faviour of blk_end_request and do the blk_noretry_request() check
> > inline in scsi_io_completion, after we try to complete but before we
> > might retry.
> 
> Possibly.  With the patch in place, scsi_end_request() gets called in 
> only three places, one of which doesn't attempt to do any retries.  But 
> that still leaves two places which might attempt it, so the retry logic 
> would have to be inlined twice.
> 
> > --- a/drivers/scsi/scsi_lib.c
> > +++ b/drivers/scsi/scsi_lib.c
> > @@ -852,7 +852,7 @@ static void scsi_end_bidi_request(struct scsi_cmnd *cmd)
> >  void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
> >  {
> >  	int result = cmd->result;
> > -	int this_count = scsi_bufflen(cmd);
> > +	int this_count;
> >  	struct request_queue *q = cmd->device->request_queue;
> >  	struct request *req = cmd->request;
> >  	int error = 0;
> > @@ -908,6 +908,7 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
> >  	 */
> >  	if (scsi_end_request(cmd, error, good_bytes, result == 0) == NULL)
> >  		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.
> 
> The problem is what happens in all the "retry" paths that follow.  
> They end up doing this:
> 
> 		scsi_end_request(cmd, -EIO, this_count, 1);
> 
> when in fact they should do this:
> 
> 		scsi_end_request(cmd, -EIO, 0, 1);
> 
> (where the -EIO value is ignored).

Actually, no ... that's just equivalent to scsi_requeue_command(q, cmd)
which is done at several places in the code (correctly) for sense errors
that imply the whole lot should be retried (actually, it's assuming
good_bytes is zero).

> By passing this_count as the third argument, they are telling the block
> layer that the entire remainder of the request completed with an error.  
> Hence blk_end_request() will return 0, scsi_end_request() won't take
> its "blocks left over at the end" pathway, and nothing will be
> requeued.

OK, so look at the current code in scsi_io_completion where we call
scsi_end_request(cmd, -EIO, this_count, 1):

UNIT ATTENTION for removable medium (means medium changed)
ILLEGAL REQUEST where there's no command resize fallback
NOT READY for unknown (non retryable) reasons
VOLUME OVERFLOW

In none of these cases do we want any form of requeuing, we want to kill
the entire request.

James


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Fix handling of failed requests in scsi_io_completion

[Alan Stern]

On Sat, 20 Sep 2008, James Bottomley wrote:

> What I mean is that I can't find an error case that's currently shown
> scsi_end_request(cmd, -EIO, this_count, 1) where requeuing after
> completing only the currently attempted transfer is valid.  If this had
> all been done as a single transaction, we'd have killed everything at
> this point.  Just because we split the request into multiple
> transactions doesn't mean we should go back around and try a new
> transaction after we hit an error.

Yes, that makes sense.

> > They end up doing this:
> > 
> > 		scsi_end_request(cmd, -EIO, this_count, 1);
> > 
> > when in fact they should do this:
> > 
> > 		scsi_end_request(cmd, -EIO, 0, 1);
> > 
> > (where the -EIO value is ignored).
> 
> Actually, no ... that's just equivalent to scsi_requeue_command(q, cmd)
> which is done at several places in the code (correctly) for sense errors
> that imply the whole lot should be retried (actually, it's assuming
> good_bytes is zero).

Except that those places don't do the blk_noretry_request test.  Should 
they?  And even if they do, what's to prevent an infinite retry loop?

> OK, so look at the current code in scsi_io_completion where we call
> scsi_end_request(cmd, -EIO, this_count, 1):
> 
> UNIT ATTENTION for removable medium (means medium changed)
> ILLEGAL REQUEST where there's no command resize fallback
> NOT READY for unknown (non retryable) reasons
> VOLUME OVERFLOW
> 
> In none of these cases do we want any form of requeuing, we want to kill
> the entire request.

I take your point.  Which leads to the question: Why was the code ever 
calling scsi_end_request(cmd, -EIO, this_count, 1) in the first place?  
Apparently all of these paths should be setting the last argument to 0, 
always.

Alan Stern

Re: [PATCH] Fix handling of failed requests in scsi_io_completion

[James Bottomley]

On Sat, 2008-09-20 at 16:49 -0400, Alan Stern wrote:
> On Sat, 20 Sep 2008, James Bottomley wrote:
> 
> > What I mean is that I can't find an error case that's currently shown
> > scsi_end_request(cmd, -EIO, this_count, 1) where requeuing after
> > completing only the currently attempted transfer is valid.  If this had
> > all been done as a single transaction, we'd have killed everything at
> > this point.  Just because we split the request into multiple
> > transactions doesn't mean we should go back around and try a new
> > transaction after we hit an error.
> 
> Yes, that makes sense.
> 
> > > They end up doing this:
> > > 
> > > 		scsi_end_request(cmd, -EIO, this_count, 1);
> > > 
> > > when in fact they should do this:
> > > 
> > > 		scsi_end_request(cmd, -EIO, 0, 1);
> > > 
> > > (where the -EIO value is ignored).
> > 
> > Actually, no ... that's just equivalent to scsi_requeue_command(q, cmd)
> > which is done at several places in the code (correctly) for sense errors
> > that imply the whole lot should be retried (actually, it's assuming
> > good_bytes is zero).
> 
> Except that those places don't do the blk_noretry_request test.  Should 
> they?  And even if they do, what's to prevent an infinite retry loop?

Well, that's another oddity ... the retries occur at a lower level
(scsi_decide_disposition) ... there's no reason to make that check if
all the error paths complete everything.  Now if we find an error where
we should be moving on to the next transaction, then we'd need to do
that test.  However, I think I've demonstrated so far that there isn't
one.  I also don't think we want to make the test for the obviously
retryable conditions like UNIT_ATTENTION because that will cause paths
to flip on irrelevant AEN conditions.

> > OK, so look at the current code in scsi_io_completion where we call
> > scsi_end_request(cmd, -EIO, this_count, 1):
> > 
> > UNIT ATTENTION for removable medium (means medium changed)
> > ILLEGAL REQUEST where there's no command resize fallback
> > NOT READY for unknown (non retryable) reasons
> > VOLUME OVERFLOW
> > 
> > In none of these cases do we want any form of requeuing, we want to kill
> > the entire request.
> 
> I take your point.  Which leads to the question: Why was the code ever 
> calling scsi_end_request(cmd, -EIO, this_count, 1) in the first place?  
> Apparently all of these paths should be setting the last argument to 0, 
> always.

Yes, that was the question I asked in my first reply (where I said the
requeue looks superfluous).  I think the answer is that there's no
point ... that's why I advocated simply eliminating scsi_end_request()
in favour of either scsi_requeue_request/blk_run_queue or
end_dequeued_request().

So are you happy with the simple fix proposal ... I think rearranging
this code needs more debate and discussion.

James

Re: [PATCH] Fix handling of failed requests in scsi_io_completion

[Boaz Harrosh]

James Bottomley wrote:
>>>
>>> In none of these cases do we want any form of requeuing, we want to kill
>>> the entire request.
>> I take your point.  Which leads to the question: Why was the code ever 
>> calling scsi_end_request(cmd, -EIO, this_count, 1) in the first place?  
>> Apparently all of these paths should be setting the last argument to 0, 
>> always.
> 
> Yes, that was the question I asked in my first reply (where I said the
> requeue looks superfluous).  I think the answer is that there's no
> point ... that's why I advocated simply eliminating scsi_end_request()
> in favour of either scsi_requeue_request/blk_run_queue or
> end_dequeued_request().
> 
> So are you happy with the simple fix proposal ... I think rearranging
> this code needs more debate and discussion.
> 
> James
> 

It looks to me that your 2-liners is a much deeper change then Alen's
original patch, with it's big code movements. So it might be a candidate
for a next release but not as a late -rc bug fix. This is because Allen's
patch is just a mechanical code change, that we can carefully follow and
verify to be equivalent code, minus the bug. Your patch changes behavior
of existing code and should be tested more carefully. What changes is your
retry behavior. I am pretty convinced of what you said, about all these retrys
been impossible/not-wanted, and I think your final deep proposal is the right
one, I wanted to clean this code up and get rid of scsi_end_request lots of
times in the passed. But for a late -rc bug fix it feels very dangerous. I have
observed more then one situation where retrys are a part of a normal IO and
I would like more time to test all the implications.

I have reviewed Allen's patch very carefully several times, and have been running
with it since he posted it. So:
 Reviewed-by: Boaz Harrosh <bharrosh@panasas.com> 
 Tested-by: Boaz Harrosh <bharrosh@panasas.com>

If you'll dig into the email-thread You'll see that I sent something
exactly like yours, at the very beginning, but Alen convinced me that it
is more dangerous, and should be in a following patch.

Thanks
Boaz
 

Re: [PATCH] Fix handling of failed requests in scsi_io_completion

[Alan Stern]

On Sat, 20 Sep 2008, James Bottomley wrote:

> > Except that those places don't do the blk_noretry_request test.  Should 
> > they?  And even if they do, what's to prevent an infinite retry loop?
> 
> Well, that's another oddity ... the retries occur at a lower level
> (scsi_decide_disposition) ... there's no reason to make that check if
> all the error paths complete everything.

Are you referring to the NEEDS_RETRY case in scsi_softirq_done?  Yet
another looping mechanism!  This one requeues the scsi_cmnd, whereas I
was talking about requeuing the request.  However either one could in
theory lead to unending retries.

For example, suppose a buggy device (without removable media) always
replies with UNIT ATTENTION without making any forward progress.  
We'll just call scsi_requeue_command each time and get stuck.

>  Now if we find an error where
> we should be moving on to the next transaction, then we'd need to do
> that test.  However, I think I've demonstrated so far that there isn't
> one.  I also don't think we want to make the test for the obviously
> retryable conditions like UNIT_ATTENTION because that will cause paths
> to flip on irrelevant AEN conditions.

To be honest, I don't know what sort of requests get marked as
non-retryable in the block layer.  Maybe you're right and we don't need 
to worry about them.

But I'm still concerned about the possibility of getting stuck doing
the same command or request over and over.  Both structures have a
"retries" field, but I'm not clear on how/where they get used.


> > I take your point.  Which leads to the question: Why was the code ever 
> > calling scsi_end_request(cmd, -EIO, this_count, 1) in the first place?  
> > Apparently all of these paths should be setting the last argument to 0, 
> > always.
> 
> Yes, that was the question I asked in my first reply (where I said the
> requeue looks superfluous).  I think the answer is that there's no
> point ... that's why I advocated simply eliminating scsi_end_request()
> in favour of either scsi_requeue_request/blk_run_queue or

You mean just scsi_requeue_command?  There is no scsi_requeue_request.

> end_dequeued_request().

Yes.  Those new "goto"s in scsi_io_completion could be rearranged to 
end up calling either scsi_requeue_command or end_dequeued_request.  
That still leaves the initial call to scsi_end_request; I guess this is 
where you would move that routine's contents into scsi_io_completion.

> So are you happy with the simple fix proposal ... I think rearranging
> this code needs more debate and discussion.

I tested your simple fix, and it does indeed solve the problem of tasks
hanging because of an uncompleted request.  In view of Boaz's concerns,
should this change be postponed until 2.6.27.stable so that it can get
wider testing?

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Fix handling of failed requests in scsi_io_completion

[James Bottomley]

On Sun, 2008-09-21 at 15:55 +0300, Boaz Harrosh wrote:
> James Bottomley wrote:
> >>>
> >>> In none of these cases do we want any form of requeuing, we want to kill
> >>> the entire request.
> >> I take your point.  Which leads to the question: Why was the code ever 
> >> calling scsi_end_request(cmd, -EIO, this_count, 1) in the first place?  
> >> Apparently all of these paths should be setting the last argument to 0, 
> >> always.
> > 
> > Yes, that was the question I asked in my first reply (where I said the
> > requeue looks superfluous).  I think the answer is that there's no
> > point ... that's why I advocated simply eliminating scsi_end_request()
> > in favour of either scsi_requeue_request/blk_run_queue or
> > end_dequeued_request().
> > 
> > So are you happy with the simple fix proposal ... I think rearranging
> > this code needs more debate and discussion.
> > 
> > James
> > 
> 
> It looks to me that your 2-liners is a much deeper change then Alen's
> original patch, with it's big code movements. So it might be a candidate
> for a next release but not as a late -rc bug fix. This is because Allen's
> patch is just a mechanical code change, that we can carefully follow and
> verify to be equivalent code, minus the bug. Your patch changes behavior
> of existing code and should be tested more carefully. What changes is your
> retry behavior.

Both patches change the retry behaviour for multi transaction requests.

>  I am pretty convinced of what you said, about all these retrys
> been impossible/not-wanted, and I think your final deep proposal is the right
> one, I wanted to clean this code up and get rid of scsi_end_request lots of
> times in the passed. But for a late -rc bug fix it feels very dangerous. I have
> observed more then one situation where retrys are a part of a normal IO and
> I would like more time to test all the implications.

OK, so point out that test case in this retry code.  Bearing in mind
that the behaviour in question only applies to the multi-transaction
commands, which aren't the normal case.  I think the principle that if
we could have completed the command in one transaction then we should
behave identically for the multi-transaction one is correct.

> I have reviewed Allen's patch very carefully several times, and have been running
> with it since he posted it. So:
>  Reviewed-by: Boaz Harrosh <bharrosh-C4P08NqkoRlBDgjK7y7TUQ@public.gmane.org> 
>  Tested-by: Boaz Harrosh <bharrosh-C4P08NqkoRlBDgjK7y7TUQ@public.gmane.org>

Hmm, I think you'll find with the original patch that you can generate
infinite retries with volume overflow because of the retry behaviour
change.  The same is true of any immutable error condition that goes
through the retry: path.

> If you'll dig into the email-thread You'll see that I sent something
> exactly like yours, at the very beginning, but Alen convinced me that it
> is more dangerous, and should be in a following patch.

Like I said, for a single transaction we kill everything after the error
point.  That should be the same for multiple transaction commands
because the behaviour on error shouldn't depend on how the command was
split.

James


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Fix handling of failed requests in scsi_io_completion

[James Bottomley]

On Sun, 2008-09-21 at 12:30 -0400, Alan Stern wrote:
> On Sat, 20 Sep 2008, James Bottomley wrote:
> 
> > > Except that those places don't do the blk_noretry_request test.  Should 
> > > they?  And even if they do, what's to prevent an infinite retry loop?
> > 
> > Well, that's another oddity ... the retries occur at a lower level
> > (scsi_decide_disposition) ... there's no reason to make that check if
> > all the error paths complete everything.
> 
> Are you referring to the NEEDS_RETRY case in scsi_softirq_done?  Yet
> another looping mechanism!  This one requeues the scsi_cmnd, whereas I
> was talking about requeuing the request.  However either one could in
> theory lead to unending retries.
> 
> For example, suppose a buggy device (without removable media) always
> replies with UNIT ATTENTION without making any forward progress.  
> We'll just call scsi_requeue_command each time and get stuck.

That's a separate bug from the current one ... fortunately one I don't
think we've actually seen manifest.

> >  Now if we find an error where
> > we should be moving on to the next transaction, then we'd need to do
> > that test.  However, I think I've demonstrated so far that there isn't
> > one.  I also don't think we want to make the test for the obviously
> > retryable conditions like UNIT_ATTENTION because that will cause paths
> > to flip on irrelevant AEN conditions.
> 
> To be honest, I don't know what sort of requests get marked as
> non-retryable in the block layer.  Maybe you're right and we don't need 
> to worry about them.

They tend to be device mapper ones.  Anything that wants a fast failure
to do path switch over for instance.

> But I'm still concerned about the possibility of getting stuck doing
> the same command or request over and over.  Both structures have a
> "retries" field, but I'm not clear on how/where they get used.

Block relies on the lower layers for retry ... it just transmits the
status, so we get to fix it.

> > > I take your point.  Which leads to the question: Why was the code ever 
> > > calling scsi_end_request(cmd, -EIO, this_count, 1) in the first place?  
> > > Apparently all of these paths should be setting the last argument to 0, 
> > > always.
> > 
> > Yes, that was the question I asked in my first reply (where I said the
> > requeue looks superfluous).  I think the answer is that there's no
> > point ... that's why I advocated simply eliminating scsi_end_request()
> > in favour of either scsi_requeue_request/blk_run_queue or
> 
> You mean just scsi_requeue_command?  There is no scsi_requeue_request.

Yes.

> > end_dequeued_request().
> 
> Yes.  Those new "goto"s in scsi_io_completion could be rearranged to 
> end up calling either scsi_requeue_command or end_dequeued_request.  
> That still leaves the initial call to scsi_end_request; I guess this is 
> where you would move that routine's contents into scsi_io_completion.
> 
> > So are you happy with the simple fix proposal ... I think rearranging
> > this code needs more debate and discussion.
> 
> I tested your simple fix, and it does indeed solve the problem of tasks
> hanging because of an uncompleted request.  In view of Boaz's concerns,
> should this change be postponed until 2.6.27.stable so that it can get
> wider testing?

We can ... I think it's safe enough though given it only affects
multiple transaction commands.

James


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Fix handling of failed requests in scsi_io_completion

[Alan Stern]

On Sun, 21 Sep 2008, James Bottomley wrote:

> > For example, suppose a buggy device (without removable media) always
> > replies with UNIT ATTENTION without making any forward progress.  
> > We'll just call scsi_requeue_command each time and get stuck.
> 
> That's a separate bug from the current one ... fortunately one I don't
> think we've actually seen manifest.

> > But I'm still concerned about the possibility of getting stuck doing
> > the same command or request over and over.  Both structures have a
> > "retries" field, but I'm not clear on how/where they get used.
> 
> Block relies on the lower layers for retry ... it just transmits the
> status, so we get to fix it.

Okay -- I'll keep it in the back of my mind for later...

> > To be honest, I don't know what sort of requests get marked as
> > non-retryable in the block layer.  Maybe you're right and we don't need 
> > to worry about them.
> 
> They tend to be device mapper ones.  Anything that wants a fast failure
> to do path switch over for instance.

If all the retry paths in scsi_io_completion jump to a common location, 
it will be easy to add the test there.

> > I tested your simple fix, and it does indeed solve the problem of tasks
> > hanging because of an uncompleted request.  In view of Boaz's concerns,
> > should this change be postponed until 2.6.27.stable so that it can get
> > wider testing?
> 
> We can ... I think it's safe enough though given it only affects
> multiple transaction commands.

The decision's yours.  Let me know when and in which tree it is merged,
so I can start writing some patches for a more-complete fix.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Fix handling of failed requests in scsi_io_completion

[Boaz Harrosh]

Alan Stern wrote:
> On Sun, 21 Sep 2008, James Bottomley wrote:
> 
>>> For example, suppose a buggy device (without removable media) always
>>> replies with UNIT ATTENTION without making any forward progress.  
>>> We'll just call scsi_requeue_command each time and get stuck.
>> That's a separate bug from the current one ... fortunately one I don't
>> think we've actually seen manifest.
> 
>>> But I'm still concerned about the possibility of getting stuck doing
>>> the same command or request over and over.  Both structures have a
>>> "retries" field, but I'm not clear on how/where they get used.
>> Block relies on the lower layers for retry ... it just transmits the
>> status, so we get to fix it.
> 
> Okay -- I'll keep it in the back of my mind for later...
> 
>>> To be honest, I don't know what sort of requests get marked as
>>> non-retryable in the block layer.  Maybe you're right and we don't need 
>>> to worry about them.
>> They tend to be device mapper ones.  Anything that wants a fast failure
>> to do path switch over for instance.
> 
> If all the retry paths in scsi_io_completion jump to a common location, 
> it will be easy to add the test there.
> 
>>> I tested your simple fix, and it does indeed solve the problem of tasks
>>> hanging because of an uncompleted request.  In view of Boaz's concerns,
>>> should this change be postponed until 2.6.27.stable so that it can get
>>> wider testing?
>> We can ... I think it's safe enough though given it only affects
>> multiple transaction commands.
> 

James, I see your point. Thanks. I have one small request. Please kill
the comment Just below the: "this_count = blk_rq_bytes(req);"

Now it is totally wrong, it used to be mostly wrong before.

(See here: http://www.spinics.net/lists/linux-scsi/msg28763.html)

> The decision's yours.  Let me know when and in which tree it is merged,
> so I can start writing some patches for a more-complete fix.
> 
> Alan Stern
> 

Boaz