[PATCH] SCSI: make use of the residue value

[PATCH] SCSI: make use of the residue value

[Alan Stern]

This patch (as1036) causes the SCSI midlayer to take into account the
residue value provided by some low-level drivers.  There's at least
one situation (USB mass storage with the Bulk-only transport) where
the specification states that it is permissible for a device to
indicate some of the data was not transferred correctly merely by
setting the residue value, without issuing a Check Condition.

Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
CC: Boaz Harrosh <bharrosh@panasas.com>

---

Index: usb-2.6/drivers/scsi/scsi.c
===================================================================
--- usb-2.6.orig/drivers/scsi/scsi.c
+++ usb-2.6/drivers/scsi/scsi.c
@@ -732,6 +732,7 @@ void scsi_finish_command(struct scsi_cmn
 	struct Scsi_Host *shost = sdev->host;
 	struct scsi_driver *drv;
 	unsigned int good_bytes;
+	unsigned int non_residue_bytes;
 
 	scsi_device_unbusy(sdev);
 
@@ -758,11 +759,16 @@ void scsi_finish_command(struct scsi_cmn
 				"(result %x)\n", cmd->result));
 
 	good_bytes = scsi_bufflen(cmd);
+	non_residue_bytes = good_bytes - scsi_get_resid(cmd);
         if (cmd->request->cmd_type != REQ_TYPE_BLOCK_PC) {
 		drv = scsi_cmd_to_driver(cmd);
 		if (drv->done)
 			good_bytes = drv->done(cmd);
 	}
+
+	/* If the device sent a meaningful residue, accept it. */
+	if (unlikely(non_residue_bytes < good_bytes))
+		good_bytes = non_residue_bytes;
 	scsi_io_completion(cmd, good_bytes);
 }
 EXPORT_SYMBOL(scsi_finish_command);

Re: [PATCH] SCSI: make use of the residue value

[Alan Stern]

On 20 Feb 2008, Alan Stern wrote:

> This patch (as1036) causes the SCSI midlayer to take into account the
> residue value provided by some low-level drivers.  There's at least
> one situation (USB mass storage with the Bulk-only transport) where
> the specification states that it is permissible for a device to
> indicate some of the data was not transferred correctly merely by
> setting the residue value, without issuing a Check Condition.

After a week, there hasn't been any feedback on this patch.  Has it
been accepted?  Is there anything wrong with it?  Is it still on a
"to-look-at" queue?

Alan Stern

Re: [PATCH] SCSI: make use of the residue value

[James Bottomley]

On Wed, 2008-02-27 at 15:25 -0500, Alan Stern wrote:
> On 20 Feb 2008, Alan Stern wrote:
> 
> > This patch (as1036) causes the SCSI midlayer to take into account the
> > residue value provided by some low-level drivers.  There's at least
> > one situation (USB mass storage with the Bulk-only transport) where
> > the specification states that it is permissible for a device to
> > indicate some of the data was not transferred correctly merely by
> > setting the residue value, without issuing a Check Condition.
> 
> After a week, there hasn't been any feedback on this patch.  Has it
> been accepted?  Is there anything wrong with it?  Is it still on a
> "to-look-at" queue?

OK, I ran it through its paces, but it fails in testing.  A very fun
failure, actually, some disks fail to appear with udev.

The reason is they return a residue from the VPD inquiry.  What your
patch actually causes is the block layer to resubmit the command with
the residue and triggers an overrun error (because the length in the
command is now much longer than the data buffer).

The bottom line is that this patch won't work with variable length
commands like inquiry that always return a residue.

James

Re: [PATCH] SCSI: make use of the residue value

[Alan Stern]

On Fri, 7 Mar 2008, James Bottomley wrote:

> On Wed, 2008-02-27 at 15:25 -0500, Alan Stern wrote:
> > On 20 Feb 2008, Alan Stern wrote:
> > 
> > > This patch (as1036) causes the SCSI midlayer to take into account the
> > > residue value provided by some low-level drivers.  There's at least
> > > one situation (USB mass storage with the Bulk-only transport) where
> > > the specification states that it is permissible for a device to
> > > indicate some of the data was not transferred correctly merely by
> > > setting the residue value, without issuing a Check Condition.
> > 
> > After a week, there hasn't been any feedback on this patch.  Has it
> > been accepted?  Is there anything wrong with it?  Is it still on a
> > "to-look-at" queue?
> 
> OK, I ran it through its paces, but it fails in testing.  A very fun
> failure, actually, some disks fail to appear with udev.
> 
> The reason is they return a residue from the VPD inquiry.  What your
> patch actually causes is the block layer to resubmit the command with
> the residue and triggers an overrun error (because the length in the
> command is now much longer than the data buffer).

Can you give any more details?

	What was the transfer length of the original command?

	What were the actual data length and the residue value?

	What were the length and data-buffer size of the
	resubmitted command?

> The bottom line is that this patch won't work with variable length
> commands like inquiry that always return a residue.

You're saying that the amount of data returned is smaller that the 
amount requested because the data is variable length, right?

Under these circumstances the block layer should not resubmit anything.
That is, it should be smart enough to know that the "missing" data does
not in fact exist, as opposed to not being returned because of a
retryable device error.

Aren't requests for things like VPD distinguished from regular 
data-block accesses by a flag in the request structure already?  The 
block layer should take this flag into account when deciding whether to 
continue trying to transfer the "missing" data.  Maybe that needs to be 
fixed first.

Alan Stern

Re: [PATCH] SCSI: make use of the residue value

[James Bottomley]

On Sat, 2008-03-08 at 11:22 -0500, Alan Stern wrote:
> On Fri, 7 Mar 2008, James Bottomley wrote:
> 
> > On Wed, 2008-02-27 at 15:25 -0500, Alan Stern wrote:
> > > On 20 Feb 2008, Alan Stern wrote:
> > > 
> > > > This patch (as1036) causes the SCSI midlayer to take into account the
> > > > residue value provided by some low-level drivers.  There's at least
> > > > one situation (USB mass storage with the Bulk-only transport) where
> > > > the specification states that it is permissible for a device to
> > > > indicate some of the data was not transferred correctly merely by
> > > > setting the residue value, without issuing a Check Condition.
> > > 
> > > After a week, there hasn't been any feedback on this patch.  Has it
> > > been accepted?  Is there anything wrong with it?  Is it still on a
> > > "to-look-at" queue?
> > 
> > OK, I ran it through its paces, but it fails in testing.  A very fun
> > failure, actually, some disks fail to appear with udev.
> > 
> > The reason is they return a residue from the VPD inquiry.  What your
> > patch actually causes is the block layer to resubmit the command with
> > the residue and triggers an overrun error (because the length in the
> > command is now much longer than the data buffer).
> 
> Can you give any more details?
> 
> 	What was the transfer length of the original command?

Depends on how you instigate it.  If you use sg_inq it will be 252.

> 	What were the actual data length and the residue value?

Depends on the page and the drive.  Best way is 0x80 it also depends on
the driver (since a lot don't report a residue).  Mostly they seem to be
24 bytes.

> 	What were the length and data-buffer size of the
> 	resubmitted command?

The length remains the same, it's embedded in the command.  The failing
length reported at 2.

> > The bottom line is that this patch won't work with variable length
> > commands like inquiry that always return a residue.
> 
> You're saying that the amount of data returned is smaller that the 
> amount requested because the data is variable length, right?

Yes.

> Under these circumstances the block layer should not resubmit anything.
> That is, it should be smart enough to know that the "missing" data does
> not in fact exist, as opposed to not being returned because of a
> retryable device error.
> 
> Aren't requests for things like VPD distinguished from regular 
> data-block accesses by a flag in the request structure already?  The 
> block layer should take this flag into account when deciding whether to 
> continue trying to transfer the "missing" data.  Maybe that needs to be 
> fixed first.

I'm sure Jens will look at patches.

James

Re: [PATCH] SCSI: make use of the residue value

[Alan Stern]

On Sat, 8 Mar 2008, James Bottomley wrote:

> > > The bottom line is that this patch won't work with variable length
> > > commands like inquiry that always return a residue.
> > 
> > You're saying that the amount of data returned is smaller that the 
> > amount requested because the data is variable length, right?
> 
> Yes.
> 
> > Under these circumstances the block layer should not resubmit anything.
> > That is, it should be smart enough to know that the "missing" data does
> > not in fact exist, as opposed to not being returned because of a
> > retryable device error.
> > 
> > Aren't requests for things like VPD distinguished from regular 
> > data-block accesses by a flag in the request structure already?  The 
> > block layer should take this flag into account when deciding whether to 
> > continue trying to transfer the "missing" data.  Maybe that needs to be 
> > fixed first.
> 
> I'm sure Jens will look at patches.

Actually it looks as though the faulty logic is in scsi_end_request()  
and/or scsi_io_completion().  The interface between those two routines
is quite confusing in regard to how the "requeue" argument is used.

At the site of the first call to scsi_end_request(), the code says:

	/* A number of bytes were successfully read.  If there
	 * are leftovers and there is some kind of error
	 * (result != 0), retry the rest.
	 */
	if (scsi_end_request(cmd, 0, good_bytes, result == 0) == NULL)
		return;

	/* good_bytes = 0, or (inclusive) there were leftovers and
	 * result = 0, so scsi_end_request couldn't retry.
	 */

These comments do a wonderful job of misdirecting the reader and 
encouraging him to conflate "requeue" with "retry".

If there are leftovers but error is 0, it seems reasonable to requeue
the remainder only when blk_fs_request(req) is true.  Anything else 
could simply be a short, variable-length response.

Do you think adding something like this is the right way to go?  I'm 
not sure it will work correctly.  What does one pass to 
blk_end_request() to indicate that the remaining data can't be 
transferred because it doesn't exist?

Or do you think that when blk_fs_request(req) isn't true, then 
scsi_io_completion() should get to decide whether or not to retry 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
@@ -667,10 +667,15 @@ static struct scsi_cmnd *scsi_end_reques
 		if (blk_pc_request(req))
 			leftover = req->data_len;
 
-		/* kill remainder if no retrys */
-		if (error && blk_noretry_request(req))
+		/* Kill remainder if no retrys.  For request types
+		 * other than REQ_TYPE_FS, !error indicates a normal
+		 * variable-length response so the remainder should
+		 * not be requeued.
+		 */
+		if ((error && blk_noretry_request(req)) ||
+		    (!error && !blk_fs_request(req))) {
 			blk_end_request(req, error, leftover);
-		else {
+		} else {
 			if (requeue) {
 				/*
 				 * Bleah.  Leftovers again.  Stick the
@@ -888,15 +893,16 @@ void scsi_io_completion(struct scsi_cmnd
 	if (clear_errors)
 		req->errors = 0;
 
-	/* A number of bytes were successfully read.  If there
-	 * are leftovers and there is some kind of error
-	 * (result != 0), retry the rest.
+	/* A number of bytes were successfully read.  If there are
+	 * leftovers and no error (result == 0), simply requeue them.
+	 * But if this isn't possible, we must figure out whether to
+	 * retry the command.
 	 */
 	if (scsi_end_request(cmd, 0, good_bytes, result == 0) == NULL)
 		return;
 
 	/* good_bytes = 0, or (inclusive) there were leftovers and
-	 * result = 0, so scsi_end_request couldn't retry.
+	 * result != 0, so scsi_end_request couldn't requeue.
 	 */
 	if (sense_valid && !sense_deferred) {
 		switch (sshdr.sense_key) {

Re: [PATCH] SCSI: make use of the residue value

[James Bottomley]

On Sat, 2008-03-08 at 17:35 -0500, Alan Stern wrote:
> On Sat, 8 Mar 2008, James Bottomley wrote:
> 
> > > > The bottom line is that this patch won't work with variable length
> > > > commands like inquiry that always return a residue.
> > > 
> > > You're saying that the amount of data returned is smaller that the 
> > > amount requested because the data is variable length, right?
> > 
> > Yes.
> > 
> > > Under these circumstances the block layer should not resubmit anything.
> > > That is, it should be smart enough to know that the "missing" data does
> > > not in fact exist, as opposed to not being returned because of a
> > > retryable device error.
> > > 
> > > Aren't requests for things like VPD distinguished from regular 
> > > data-block accesses by a flag in the request structure already?  The 
> > > block layer should take this flag into account when deciding whether to 
> > > continue trying to transfer the "missing" data.  Maybe that needs to be 
> > > fixed first.
> > 
> > I'm sure Jens will look at patches.
> 
> Actually it looks as though the faulty logic is in scsi_end_request()  
> and/or scsi_io_completion().  The interface between those two routines
> is quite confusing in regard to how the "requeue" argument is used.
> 
> At the site of the first call to scsi_end_request(), the code says:
> 
> 	/* A number of bytes were successfully read.  If there
> 	 * are leftovers and there is some kind of error
> 	 * (result != 0), retry the rest.
> 	 */
> 	if (scsi_end_request(cmd, 0, good_bytes, result == 0) == NULL)
> 		return;
> 
> 	/* good_bytes = 0, or (inclusive) there were leftovers and
> 	 * result = 0, so scsi_end_request couldn't retry.
> 	 */
> 
> These comments do a wonderful job of misdirecting the reader and 
> encouraging him to conflate "requeue" with "retry".
> 
> If there are leftovers but error is 0, it seems reasonable to requeue
> the remainder only when blk_fs_request(req) is true.  Anything else 
> could simply be a short, variable-length response.
> 
> Do you think adding something like this is the right way to go?  I'm 
> not sure it will work correctly.  What does one pass to 
> blk_end_request() to indicate that the remaining data can't be 
> transferred because it doesn't exist?
> 
> Or do you think that when blk_fs_request(req) isn't true, then 
> scsi_io_completion() should get to decide whether or not to retry 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
> @@ -667,10 +667,15 @@ static struct scsi_cmnd *scsi_end_reques
>  		if (blk_pc_request(req))
>  			leftover = req->data_len;
>  
> -		/* kill remainder if no retrys */
> -		if (error && blk_noretry_request(req))
> +		/* Kill remainder if no retrys.  For request types
> +		 * other than REQ_TYPE_FS, !error indicates a normal
> +		 * variable-length response so the remainder should
> +		 * not be requeued.
> +		 */
> +		if ((error && blk_noretry_request(req)) ||
> +		    (!error && !blk_fs_request(req))) {
>  			blk_end_request(req, error, leftover);
> -		else {
> +		} else {
>  			if (requeue) {
>  				/*
>  				 * Bleah.  Leftovers again.  Stick the
> @@ -888,15 +893,16 @@ void scsi_io_completion(struct scsi_cmnd
>  	if (clear_errors)
>  		req->errors = 0;
>  
> -	/* A number of bytes were successfully read.  If there
> -	 * are leftovers and there is some kind of error
> -	 * (result != 0), retry the rest.
> +	/* A number of bytes were successfully read.  If there are
> +	 * leftovers and no error (result == 0), simply requeue them.
> +	 * But if this isn't possible, we must figure out whether to
> +	 * retry the command.
>  	 */
>  	if (scsi_end_request(cmd, 0, good_bytes, result == 0) == NULL)
>  		return;
>  
>  	/* good_bytes = 0, or (inclusive) there were leftovers and
> -	 * result = 0, so scsi_end_request couldn't retry.
> +	 * result != 0, so scsi_end_request couldn't requeue.
>  	 */
>  	if (sense_valid && !sense_deferred) {
>  		switch (sshdr.sense_key) {

This is really pretty complex, and it's adjusting an already complex
path we've had quite a bit of difficulty with previously.

What I was actually looking for was something simpler.  How about this?
I'll still have to run it through the test wringer since it might still
end up causing problems if drivers are careless about their residue
calculations.

James

---

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index e5c6f6a..96dbc63 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -759,9 +759,16 @@ void scsi_finish_command(struct scsi_cmnd *cmd)
 
 	good_bytes = scsi_bufflen(cmd) + cmd->request->extra_len;
         if (cmd->request->cmd_type != REQ_TYPE_BLOCK_PC) {
+		int old_good_bytes = good_bytes;
 		drv = scsi_cmd_to_driver(cmd);
 		if (drv->done)
 			good_bytes = drv->done(cmd);
+		/*
+		 * USB may not give sense identifying bad sector and
+		 * simply return a residue instead.
+		 */
+		if (good_bytes == old_good_bytes)
+			good_bytes -= scsi_get_resid(cmd);
 	}
 	scsi_io_completion(cmd, good_bytes);
 }

Re: [PATCH] SCSI: make use of the residue value

[Alan Stern]

On Sat, 8 Mar 2008, James Bottomley wrote:

> This is really pretty complex, and it's adjusting an already complex
> path we've had quite a bit of difficulty with previously.

The new part of the patch isn't complex.  Apart from modifying a bunch
of comments, all it does is add one more clause to an "if" test.  But I
have to agree that the test is in a rather complex path.  And it looks
as though that path, complex as it is, isn't set up to handle
situations where variable-length data ends up being shorter than
requested.  Am I missing something pretty basic?

(I still think that modifying the comments is a good idea.  When
dealing with complex code, misleading comments are the last thing you
need.)

> What I was actually looking for was something simpler.  How about this?
> I'll still have to run it through the test wringer since it might still
> end up causing problems if drivers are careless about their residue
> calculations.
> 
> James
> 
> ---
> 
> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> index e5c6f6a..96dbc63 100644
> --- a/drivers/scsi/scsi.c
> +++ b/drivers/scsi/scsi.c
> @@ -759,9 +759,16 @@ void scsi_finish_command(struct scsi_cmnd *cmd)
>  
>  	good_bytes = scsi_bufflen(cmd) + cmd->request->extra_len;
>          if (cmd->request->cmd_type != REQ_TYPE_BLOCK_PC) {
> +		int old_good_bytes = good_bytes;
>  		drv = scsi_cmd_to_driver(cmd);
>  		if (drv->done)
>  			good_bytes = drv->done(cmd);
> +		/*
> +		 * USB may not give sense identifying bad sector and
> +		 * simply return a residue instead.
> +		 */

This comment is inaccurate.  It would be better to say "USB may provide
a residue to indicate the data isn't entirely valid, even if there is
no sense".  That is, a residue may be provided even when:

	the requested amount of data has been transferred; or

	the data is variable length, not a group of sectors.

Furthermore this is true for any type of request, including BLOCK_PC.  
In fact, I would expect this to be true for any transport, not just
USB.  How else would you know that the amount of VPD data returned is
less than the amount asked for?

> +		if (good_bytes == old_good_bytes)
> +			good_bytes -= scsi_get_resid(cmd);

I don't like this calculation.  The residue should be checked, not
blindly accepted.  It is reasonable only it doesn't cause good_bytes to
wrap below 0.  Maybe the lower-level driver can be relied on for this 
checking, maybe not.

>  	}
>  	scsi_io_completion(cmd, good_bytes);
>  }

Alan Stern

Re: [PATCH] SCSI: make use of the residue value

[James Bottomley]

On Sat, 2008-03-08 at 22:05 -0500, Alan Stern wrote:
> On Sat, 8 Mar 2008, James Bottomley wrote:
> 
> > This is really pretty complex, and it's adjusting an already complex
> > path we've had quite a bit of difficulty with previously.
> 
> The new part of the patch isn't complex.  Apart from modifying a bunch
> of comments, all it does is add one more clause to an "if" test.  But I
> have to agree that the test is in a rather complex path.  And it looks
> as though that path, complex as it is, isn't set up to handle
> situations where variable-length data ends up being shorter than
> requested.  Am I missing something pretty basic?
> 
> (I still think that modifying the comments is a good idea.  When
> dealing with complex code, misleading comments are the last thing you
> need.)
> 
> > What I was actually looking for was something simpler.  How about this?
> > I'll still have to run it through the test wringer since it might still
> > end up causing problems if drivers are careless about their residue
> > calculations.
> > 
> > James
> > 
> > ---
> > 
> > diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> > index e5c6f6a..96dbc63 100644
> > --- a/drivers/scsi/scsi.c
> > +++ b/drivers/scsi/scsi.c
> > @@ -759,9 +759,16 @@ void scsi_finish_command(struct scsi_cmnd *cmd)
> >  
> >  	good_bytes = scsi_bufflen(cmd) + cmd->request->extra_len;
> >          if (cmd->request->cmd_type != REQ_TYPE_BLOCK_PC) {
> > +		int old_good_bytes = good_bytes;
> >  		drv = scsi_cmd_to_driver(cmd);
> >  		if (drv->done)
> >  			good_bytes = drv->done(cmd);
> > +		/*
> > +		 * USB may not give sense identifying bad sector and
> > +		 * simply return a residue instead.
> > +		 */
> 
> This comment is inaccurate.  It would be better to say "USB may provide
> a residue to indicate the data isn't entirely valid, even if there is
> no sense".  That is, a residue may be provided even when:
> 
> 	the requested amount of data has been transferred; or
> 
> 	the data is variable length, not a group of sectors.

That's true, but irrelevant.  Placing the condition inside the check for
BLOCK_PC eliminates the possibility of variable length commands, which
is why I did it this way.

> Furthermore this is true for any type of request, including BLOCK_PC.  
> In fact, I would expect this to be true for any transport, not just
> USB.  How else would you know that the amount of VPD data returned is
> less than the amount asked for?
> 
> > +		if (good_bytes == old_good_bytes)
> > +			good_bytes -= scsi_get_resid(cmd);
> 
> I don't like this calculation.  The residue should be checked, not
> blindly accepted.  It is reasonable only it doesn't cause good_bytes to
> wrap below 0.  Maybe the lower-level driver can be relied on for this 
> checking, maybe not.

Um, it's actually a narrowing of what you did in your original patch.
There you looked at good bytes and the original minus the residue and
took the smaller.

Here, I'm only subtracting the residue if good_bytes wasn't altered.
(If we get any error identification at all, we believe it over the
residue).

James

Re: [PATCH] SCSI: make use of the residue value

[Alan Stern]

On Sun, 9 Mar 2008, James Bottomley wrote:

> > > --- a/drivers/scsi/scsi.c
> > > +++ b/drivers/scsi/scsi.c
> > > @@ -759,9 +759,16 @@ void scsi_finish_command(struct scsi_cmnd *cmd)
> > >  
> > >  	good_bytes = scsi_bufflen(cmd) + cmd->request->extra_len;
> > >          if (cmd->request->cmd_type != REQ_TYPE_BLOCK_PC) {
> > > +		int old_good_bytes = good_bytes;
> > >  		drv = scsi_cmd_to_driver(cmd);
> > >  		if (drv->done)
> > >  			good_bytes = drv->done(cmd);
> > > +		/*
> > > +		 * USB may not give sense identifying bad sector and
> > > +		 * simply return a residue instead.
> > > +		 */
> > 
> > This comment is inaccurate.  It would be better to say "USB may provide
> > a residue to indicate the data isn't entirely valid, even if there is
> > no sense".  That is, a residue may be provided even when:
> > 
> > 	the requested amount of data has been transferred; or
> > 
> > 	the data is variable length, not a group of sectors.
> 
> That's true, but irrelevant.  Placing the condition inside the check for
> BLOCK_PC eliminates the possibility of variable length commands, which
> is why I did it this way.

But the comment talks about identifying a bad sector.  That makes no 
sense if the command isn't sector-oriented to begin with.

Why do you think that only BLOCK_PC commands can have variable length?

And why do you want to ignore the residue for variable-length commands?

> > > +		if (good_bytes == old_good_bytes)
> > > +			good_bytes -= scsi_get_resid(cmd);
> > 
> > I don't like this calculation.  The residue should be checked, not
> > blindly accepted.  It is reasonable only it doesn't cause good_bytes to
> > wrap below 0.  Maybe the lower-level driver can be relied on for this 
> > checking, maybe not.
> 
> Um, it's actually a narrowing of what you did in your original patch.

Not so.

> There you looked at good bytes and the original minus the residue and
> took the smaller.

Yes.

> Here, I'm only subtracting the residue if good_bytes wasn't altered.

Yes.  And if residue > good_bytes then you end up taking the larger
instead of the smaller: Since good_bytes is unsigned, subtracting a
larger quantity from it yields a very large positive result.

> (If we get any error identification at all, we believe it over the
> residue).

I don't mind ignoring the residue when good_bytes was changed; 
believing the error identification makes sense.  What I do mind about 
your patch is:

	It doesn't check for underflow when subtracting the residue;

	It doesn't use the residue at all for BLOCK_PC requests.

BTW, you never answered my question: How does the SCSI midlayer tell
callers that a command returned less data than requested because the
device claims the remainder doesn't exist?

Alan Stern

Re: [PATCH] SCSI: make use of the residue value

[James Bottomley]

On Sun, 2008-03-09 at 15:13 -0400, Alan Stern wrote:
> On Sun, 9 Mar 2008, James Bottomley wrote:
> 
> > > > --- a/drivers/scsi/scsi.c
> > > > +++ b/drivers/scsi/scsi.c
> > > > @@ -759,9 +759,16 @@ void scsi_finish_command(struct scsi_cmnd *cmd)
> > > >  
> > > >  	good_bytes = scsi_bufflen(cmd) + cmd->request->extra_len;
> > > >          if (cmd->request->cmd_type != REQ_TYPE_BLOCK_PC) {
> > > > +		int old_good_bytes = good_bytes;
> > > >  		drv = scsi_cmd_to_driver(cmd);
> > > >  		if (drv->done)
> > > >  			good_bytes = drv->done(cmd);
> > > > +		/*
> > > > +		 * USB may not give sense identifying bad sector and
> > > > +		 * simply return a residue instead.
> > > > +		 */
> > > 
> > > This comment is inaccurate.  It would be better to say "USB may provide
> > > a residue to indicate the data isn't entirely valid, even if there is
> > > no sense".  That is, a residue may be provided even when:
> > > 
> > > 	the requested amount of data has been transferred; or
> > > 
> > > 	the data is variable length, not a group of sectors.
> > 
> > That's true, but irrelevant.  Placing the condition inside the check for
> > BLOCK_PC eliminates the possibility of variable length commands, which
> > is why I did it this way.
> 
> But the comment talks about identifying a bad sector.  That makes no 
> sense if the command isn't sector-oriented to begin with.

At the moment we have two (well, a lot more if you include the token
based PM commands, but SCSI doesn't really do those) command types:
BLOCK_PC and FS.  The latter go through this clause and *are* sector
based.

> Why do you think that only BLOCK_PC commands can have variable length?
> 
> And why do you want to ignore the residue for variable-length commands?
> 
> > > > +		if (good_bytes == old_good_bytes)
> > > > +			good_bytes -= scsi_get_resid(cmd);
> > > 
> > > I don't like this calculation.  The residue should be checked, not
> > > blindly accepted.  It is reasonable only it doesn't cause good_bytes to
> > > wrap below 0.  Maybe the lower-level driver can be relied on for this 
> > > checking, maybe not.
> > 
> > Um, it's actually a narrowing of what you did in your original patch.
> 
> Not so.
> 
> > There you looked at good bytes and the original minus the residue and
> > took the smaller.
> 
> Yes.
> 
> > Here, I'm only subtracting the residue if good_bytes wasn't altered.
> 
> Yes.  And if residue > good_bytes then you end up taking the larger
> instead of the smaller: Since good_bytes is unsigned, subtracting a
> larger quantity from it yields a very large positive result.

For that to happen the driver would have to have returned a larger
residue than it was given bytes to write or read, which should be an
impossible condition given that they count down from the bytes to write.
If any driver is doing this, it needs catching and shooting.

> > (If we get any error identification at all, we believe it over the
> > residue).
> 
> I don't mind ignoring the residue when good_bytes was changed; 
> believing the error identification makes sense.  What I do mind about 
> your patch is:
> 
> 	It doesn't check for underflow when subtracting the residue;

If you really want it, I can but a BUG_ON for this, but it should be an
impossible condition.  We certainly don't want real code assuming it's
possible.

> 	It doesn't use the residue at all for BLOCK_PC requests.

The residue for these is returned in a different way.

> BTW, you never answered my question: How does the SCSI midlayer tell
> callers that a command returned less data than requested because the
> device claims the remainder doesn't exist?

For BLOCK_PC that's returned in req->data_len, which is placed in resid
(sgv3) or din/dout_resid (sgv4) for the user.

James

Re: [PATCH] SCSI: make use of the residue value

[Alan Stern]

On Sun, 9 Mar 2008, James Bottomley wrote:

> > But the comment talks about identifying a bad sector.  That makes no 
> > sense if the command isn't sector-oriented to begin with.
> 
> At the moment we have two (well, a lot more if you include the token
> based PM commands, but SCSI doesn't really do those) command types:
> BLOCK_PC and FS.  The latter go through this clause and *are* sector
> based.

And we really don't have to worry about the other types?  Okay...

> > Yes.  And if residue > good_bytes then you end up taking the larger
> > instead of the smaller: Since good_bytes is unsigned, subtracting a
> > larger quantity from it yields a very large positive result.
> 
> For that to happen the driver would have to have returned a larger
> residue than it was given bytes to write or read, which should be an
> impossible condition given that they count down from the bytes to write.
> If any driver is doing this, it needs catching and shooting.

In short, you're trusting the low-level drivers to catch this
impossible condition.  (usb-storage does check for it, incidentally.)  
That's fine with me, as long as it's explicit.

> > 	It doesn't use the residue at all for BLOCK_PC requests.
> 
> The residue for these is returned in a different way.
> 
> > BTW, you never answered my question: How does the SCSI midlayer tell
> > callers that a command returned less data than requested because the
> > device claims the remainder doesn't exist?
> 
> For BLOCK_PC that's returned in req->data_len, which is placed in resid
> (sgv3) or din/dout_resid (sgv4) for the user.

I see.  Although oddly enough, the scsi_execute() routine throws this 
information away.  Don't its callers need to know?

All right, your version of the patch seems okay.  But there still is an
unanswered question:

What's the reason for this baroque arrangement?  Why tell the block 
layer that the data was transferred successfully and then use a 
back-door arrangement like req->data_len to let the caller know that 
no, the data wan't really all transferred?

Alan Stern

P.S.: In case you're interested...

It's not common, even among USB devices, to return a residue without
identifying the bad sector.  The one example where I saw it happen was
explained by the fact that max_sectors was too high.  The device
returned as much data as it could, with the residue set to indicate
that some was missing.  But since it never actually encountered a
sector-read error, it didn't return any sense information.

Yes, the device _should_ have returned Illegal Request, Invalid 
Field in CDB, or something of the sort.  That would have alerted us to 
the problem.  But it didn't.  And as a result, the midlayer thought the 
missing data was present and correct.  The user reported it as data 
corruption: Files written to the device and then read back did not 
compare equal to the original files.

Re: [PATCH] SCSI: make use of the residue value

[Boaz Harrosh]

On Sun, Mar 09 2008 at 23:50 +0200, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Sun, 9 Mar 2008, James Bottomley wrote:
> 
>>> But the comment talks about identifying a bad sector.  That makes no 
>>> sense if the command isn't sector-oriented to begin with.
>> At the moment we have two (well, a lot more if you include the token
>> based PM commands, but SCSI doesn't really do those) command types:
>> BLOCK_PC and FS.  The latter go through this clause and *are* sector
>> based.
> 
> And we really don't have to worry about the other types?  Okay...
> 
>>> Yes.  And if residue > good_bytes then you end up taking the larger
>>> instead of the smaller: Since good_bytes is unsigned, subtracting a
>>> larger quantity from it yields a very large positive result.
>> For that to happen the driver would have to have returned a larger
>> residue than it was given bytes to write or read, which should be an
>> impossible condition given that they count down from the bytes to write.
>> If any driver is doing this, it needs catching and shooting.
> 
> In short, you're trusting the low-level drivers to catch this
> impossible condition.  (usb-storage does check for it, incidentally.)  
> That's fine with me, as long as it's explicit.
> 
>>> 	It doesn't use the residue at all for BLOCK_PC requests.
>> The residue for these is returned in a different way.
>>
>>> BTW, you never answered my question: How does the SCSI midlayer tell
>>> callers that a command returned less data than requested because the
>>> device claims the remainder doesn't exist?
>> For BLOCK_PC that's returned in req->data_len, which is placed in resid
>> (sgv3) or din/dout_resid (sgv4) for the user.
> 
> I see.  Although oddly enough, the scsi_execute() routine throws this 
> information away.  Don't its callers need to know?
> 
> All right, your version of the patch seems okay.  But there still is an
> unanswered question:
> 
> What's the reason for this baroque arrangement?  Why tell the block 
> layer that the data was transferred successfully and then use a 
> back-door arrangement like req->data_len to let the caller know that 
> no, the data wan't really all transferred?

Yes this is very ugly and bug-full. We tripped over that a few times
in the passed. There was somewhat an agreement at LSF that struct request
should have a residue field, just as scsi_cmnd do. and that the scsi_cmnd's
field can go away. (Easily done with scsi accessors in place). Current system
was an HACK to reuse req->data_len for residue after the request was completed.
for sg only at the time. It is on my TODO list, together with some other changes
that we agree upon.

> 
> Alan Stern
> 
> P.S.: In case you're interested...
> 
> It's not common, even among USB devices, to return a residue without
> identifying the bad sector.  The one example where I saw it happen was
> explained by the fact that max_sectors was too high.  The device
> returned as much data as it could, with the residue set to indicate
> that some was missing.  But since it never actually encountered a
> sector-read error, it didn't return any sense information.

There was a fix submitted to sd.c and separately to sr.c not long ago, I 
would say 2.6.25, (haven't checked though). That fixes such a problem. It was 
agreed that other ULDs do not really use proper FS commands. What are the 
kernels you have reports for? Do you need that I dig up the commits / mailinglist
posts?

> 
> Yes, the device _should_ have returned Illegal Request, Invalid 
> Field in CDB, or something of the sort.  That would have alerted us to 
> the problem.  But it didn't.  And as a result, the midlayer thought the 
> missing data was present and correct.  The user reported it as data 
> corruption: Files written to the device and then read back did not 
> compare equal to the original files.
> 

The way I see it, what you are saying is impossible, since there is a check
for max sector in place, and VFS/ULD will not write passed that. If so it's a bug
that should be fixed. (Wherever the bug is).
If this was done as a BLOCK_PC via sg then surly the residue was reported back
to user mode and the application should be fixed.

Boaz

Re: [PATCH] SCSI: make use of the residue value

[Alan Stern]

On Mon, 10 Mar 2008, Boaz Harrosh wrote:

> > What's the reason for this baroque arrangement?  Why tell the block 
> > layer that the data was transferred successfully and then use a 
> > back-door arrangement like req->data_len to let the caller know that 
> > no, the data wan't really all transferred?
> 
> Yes this is very ugly and bug-full. We tripped over that a few times
> in the passed. There was somewhat an agreement at LSF that struct request
> should have a residue field, just as scsi_cmnd do. and that the scsi_cmnd's
> field can go away. (Easily done with scsi accessors in place). Current system
> was an HACK to reuse req->data_len for residue after the request was completed.
> for sg only at the time. It is on my TODO list, together with some other changes
> that we agree upon.

Sounds like a good thing to do.

> > P.S.: In case you're interested...
> > 
> > It's not common, even among USB devices, to return a residue without
> > identifying the bad sector.  The one example where I saw it happen was
> > explained by the fact that max_sectors was too high.  The device
> > returned as much data as it could, with the residue set to indicate
> > that some was missing.  But since it never actually encountered a
> > sector-read error, it didn't return any sense information.
> 
> There was a fix submitted to sd.c and separately to sr.c not long ago, I 
> would say 2.6.25, (haven't checked though). That fixes such a problem. It was 
> agreed that other ULDs do not really use proper FS commands. What are the 
> kernels you have reports for? Do you need that I dig up the commits / mailinglist
> posts?

No need, the problem you're thinking of in sd has been fixed.  The
problem was that sd did not check the value of error_sector to make
sure it was within the range of sectors accessed by the command.  I'm
not sure if an equivalent fix has been applied to sr, however.

> > Yes, the device _should_ have returned Illegal Request, Invalid 
> > Field in CDB, or something of the sort.  That would have alerted us to 
> > the problem.  But it didn't.  And as a result, the midlayer thought the 
> > missing data was present and correct.  The user reported it as data 
> > corruption: Files written to the device and then read back did not 
> > compare equal to the original files.
> > 
> 
> The way I see it, what you are saying is impossible, since there is a check
> for max sector in place, and VFS/ULD will not write passed that.

As I said, max_sectors was too big.  It was set to 120 KB but it 
should have been 56 KB, or something like that.  Hence the VFS/ULD 
tried to transfer more data than the device was capable of handling.

>  If so it's a bug
> that should be fixed. (Wherever the bug is).

It has already been fixed; there is now a blacklist entry for that
particular device specifying a lower value for max_sectors.

> If this was done as a BLOCK_PC via sg then surly the residue was reported back
> to user mode and the application should be fixed.

No, it was done by a normal filesystem I/O.

Alan Stern

Re: [PATCH] SCSI: make use of the residue value

[Boaz Harrosh]

On Mon, Mar 10 2008 at 16:11 +0200, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Mon, 10 Mar 2008, Boaz Harrosh wrote:
> 
>>> What's the reason for this baroque arrangement?  Why tell the block 
>>> layer that the data was transferred successfully and then use a 
>>> back-door arrangement like req->data_len to let the caller know that 
>>> no, the data wan't really all transferred?
>> Yes this is very ugly and bug-full. We tripped over that a few times
>> in the passed. There was somewhat an agreement at LSF that struct request
>> should have a residue field, just as scsi_cmnd do. and that the scsi_cmnd's
>> field can go away. (Easily done with scsi accessors in place). Current system
>> was an HACK to reuse req->data_len for residue after the request was completed.
>> for sg only at the time. It is on my TODO list, together with some other changes
>> that we agree upon.
> 
> Sounds like a good thing to do.
> 
>>> P.S.: In case you're interested...
>>>
>>> It's not common, even among USB devices, to return a residue without
>>> identifying the bad sector.  The one example where I saw it happen was
>>> explained by the fact that max_sectors was too high.  The device
>>> returned as much data as it could, with the residue set to indicate
>>> that some was missing.  But since it never actually encountered a
>>> sector-read error, it didn't return any sense information.
>> There was a fix submitted to sd.c and separately to sr.c not long ago, I 
>> would say 2.6.25, (haven't checked though). That fixes such a problem. It was 
>> agreed that other ULDs do not really use proper FS commands. What are the 
>> kernels you have reports for? Do you need that I dig up the commits / mailinglist
>> posts?
> 
> No need, the problem you're thinking of in sd has been fixed.  The
> problem was that sd did not check the value of error_sector to make
> sure it was within the range of sectors accessed by the command.  I'm
> not sure if an equivalent fix has been applied to sr, however.

I certainly remember a very nice patch sent following the one to sd.
That revamps the all sr error handling path, including above problem. 
I'm not sure it was submitted upstream though.

> 
>>> Yes, the device _should_ have returned Illegal Request, Invalid 
>>> Field in CDB, or something of the sort.  That would have alerted us to 
>>> the problem.  But it didn't.  And as a result, the midlayer thought the 
>>> missing data was present and correct.  The user reported it as data 
>>> corruption: Files written to the device and then read back did not 
>>> compare equal to the original files.
>>>
>> The way I see it, what you are saying is impossible, since there is a check
>> for max sector in place, and VFS/ULD will not write passed that.
> 
> As I said, max_sectors was too big.  It was set to 120 KB but it 
> should have been 56 KB, or something like that.  Hence the VFS/ULD 
> tried to transfer more data than the device was capable of handling.
> 
>>  If so it's a bug
>> that should be fixed. (Wherever the bug is).
> 
> It has already been fixed; there is now a blacklist entry for that
> particular device specifying a lower value for max_sectors.
> 

OK, I see what you are saying: In case of bad behaving device, that we do
not know about. The residue can be an indication of an actual error that should
not be ignored. Sound's reasonable. In my opinion only a ULD like sd/sr could
really know such things for sure. (Given the knowledge of command submitted)
So a fix should go there. Both your patch and Jame's, touch code that does not
have enough information to be sure.

>> If this was done as a BLOCK_PC via sg then surly the residue was reported back
>> to user mode and the application should be fixed.
> 
> No, it was done by a normal filesystem I/O.
> 
> Alan Stern
> 

Boaz

Re: [PATCH] SCSI: make use of the residue value

[Alan Stern]

On Mon, 10 Mar 2008, Boaz Harrosh wrote:

> > No need, the problem you're thinking of in sd has been fixed.  The
> > problem was that sd did not check the value of error_sector to make
> > sure it was within the range of sectors accessed by the command.  I'm
> > not sure if an equivalent fix has been applied to sr, however.
> 
> I certainly remember a very nice patch sent following the one to sd.
> That revamps the all sr error handling path, including above problem. 
> I'm not sure it was submitted upstream though.

I don't think it was.  The MEDIUM_ERROR case in sr_done() is still a 
mess.  That function doesn't even use the new sense-buffer accessor 
routines.  Something else to add to your TODO list?


> OK, I see what you are saying: In case of bad behaving device, that we do
> not know about. The residue can be an indication of an actual error that should
> not be ignored. Sound's reasonable. In my opinion only a ULD like sd/sr could
> really know such things for sure. (Given the knowledge of command submitted)
> So a fix should go there. Both your patch and Jame's, touch code that does not
> have enough information to be sure.

At the moment sd and sr are the only ULDs which support block access
FS commands, so it doesn't make much difference.

However I disagree that only a ULD could know for sure whether a 
positive residue in a FS command indicates an error.  IMO, a residue in 
any FS command should always be taken seriously.

Alan Stern

Re: [PATCH] SCSI: make use of the residue value

[Alan Stern]

Was there every any definite reason for not accepting this, or 
something like it?  Did it turn out not to be needed?

Alan Stern


On Sat, 8 Mar 2008, James Bottomley wrote:

> What I was actually looking for was something simpler.  How about this?
> I'll still have to run it through the test wringer since it might still
> end up causing problems if drivers are careless about their residue
> calculations.
> 
> James
> 
> ---
> 
> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> index e5c6f6a..96dbc63 100644
> --- a/drivers/scsi/scsi.c
> +++ b/drivers/scsi/scsi.c
> @@ -759,9 +759,16 @@ void scsi_finish_command(struct scsi_cmnd *cmd)
>  
>  	good_bytes = scsi_bufflen(cmd) + cmd->request->extra_len;
>          if (cmd->request->cmd_type != REQ_TYPE_BLOCK_PC) {
> +		int old_good_bytes = good_bytes;
>  		drv = scsi_cmd_to_driver(cmd);
>  		if (drv->done)
>  			good_bytes = drv->done(cmd);
> +		/*
> +		 * USB may not give sense identifying bad sector and
> +		 * simply return a residue instead.
> +		 */
> +		if (good_bytes == old_good_bytes)
> +			good_bytes -= scsi_get_resid(cmd);
>  	}
>  	scsi_io_completion(cmd, good_bytes);
>  }