[PATCH] SCSI: handle HARDWARE_ERROR sense correctly

[PATCH] SCSI: handle HARDWARE_ERROR sense correctly

[Alan Stern]

This patch (as1183) fixes a bug in scsi_check_sense().  The routine is
documented as returning one of SUCCESS, FAILED, or NEEDS_RETRY.  But
in the HARDWARE_ERROR case it can return ADD_TO_MLQUEUE.  And since it
does this without bothering to increment the retry count, it can lead
to an infinite retry loop.

The fix is to return NEEDS_RETRY instead.  Then the caller,
scsi_decide_disposition(), will do the right thing.

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

---

Index: usb-2.6/drivers/scsi/scsi_error.c
===================================================================
--- usb-2.6.orig/drivers/scsi/scsi_error.c
+++ usb-2.6/drivers/scsi/scsi_error.c
@@ -330,7 +330,7 @@ static int scsi_check_sense(struct scsi_
 
 	case HARDWARE_ERROR:
 		if (scmd->device->retry_hwerror)
-			return ADD_TO_MLQUEUE;
+			return NEEDS_RETRY;
 		else
 			return SUCCESS;
 

Re: [PATCH] SCSI: handle HARDWARE_ERROR sense correctly

[James Bottomley]

On Thu, 2008-12-04 at 15:49 -0500, Alan Stern wrote:
> This patch (as1183) fixes a bug in scsi_check_sense().  The routine is
> documented as returning one of SUCCESS, FAILED, or NEEDS_RETRY.  But
> in the HARDWARE_ERROR case it can return ADD_TO_MLQUEUE.  And since it
> does this without bothering to increment the retry count, it can lead
> to an infinite retry loop.
> 
> The fix is to return NEEDS_RETRY instead.  Then the caller,
> scsi_decide_disposition(), will do the right thing.

OK, but why?

The current behaviour is to retry the error until the command timeout
expires, which, I think is what was needed by the annoying arrays that
have retryable hardware errors.

What bug would this patch fix?  Because I can see it causing problems
with the arrays that originally reported this problem.

James

Re: [PATCH] SCSI: handle HARDWARE_ERROR sense correctly

[Alan Stern]

On Thu, 4 Dec 2008, James Bottomley wrote:

> On Thu, 2008-12-04 at 15:49 -0500, Alan Stern wrote:
> > This patch (as1183) fixes a bug in scsi_check_sense().  The routine is
> > documented as returning one of SUCCESS, FAILED, or NEEDS_RETRY.  But
> > in the HARDWARE_ERROR case it can return ADD_TO_MLQUEUE.  And since it
> > does this without bothering to increment the retry count, it can lead
> > to an infinite retry loop.
> > 
> > The fix is to return NEEDS_RETRY instead.  Then the caller,
> > scsi_decide_disposition(), will do the right thing.
> 
> OK, but why?
> 
> The current behaviour is to retry the error until the command timeout
> expires, which, I think is what was needed by the annoying arrays that
> have retryable hardware errors.
> 
> What bug would this patch fix?  Because I can see it causing problems
> with the arrays that originally reported this problem.

You're right and I was wrong.  It only _appeared_ to be an infinite
retry loop because there were 6 iterations allowed and each iteration
had a 60-second timeout.  So I retract the patch submission.

Waiting six minutes for a command to complete is a bit ridiculous,
though -- especially when the user program generating that command
decides to reissue it a few times.  Can we do anything to expedite the
failure?  For example, does it really make sense for scsi_softirq_done
to multiply cmd->allowed by rq->timeout?  After all, if a command
aborts with a timeout instead of failing outright, what point is there
in retrying it?  The proper approach would have been to use a longer 
timeout initially.

By the way, is there any reason to keep scmd->retries now?  If commands
are going to be retried until they timeout, why bother to count the
retries?

Alan Stern

Re: [PATCH] SCSI: handle HARDWARE_ERROR sense correctly

[Mike Anderson]

Alan Stern <stern@rowland.harvard.edu> wrote:
> On Thu, 4 Dec 2008, James Bottomley wrote:
> 
> > On Thu, 2008-12-04 at 15:49 -0500, Alan Stern wrote:
> > > This patch (as1183) fixes a bug in scsi_check_sense().  The routine is
> > > documented as returning one of SUCCESS, FAILED, or NEEDS_RETRY.  But
> > > in the HARDWARE_ERROR case it can return ADD_TO_MLQUEUE.  And since it
> > > does this without bothering to increment the retry count, it can lead
> > > to an infinite retry loop.
> > > 
> > > The fix is to return NEEDS_RETRY instead.  Then the caller,
> > > scsi_decide_disposition(), will do the right thing.
> > 
> > OK, but why?
> > 
> > The current behaviour is to retry the error until the command timeout
> > expires, which, I think is what was needed by the annoying arrays that
> > have retryable hardware errors.
> > 

Yes there are some arrays that need this behavior. The two users: 
usb disks and the devinfo entries with BLIST_RETRY_HWERROR appear to have
two different expected behaviors.

> > What bug would this patch fix?  Because I can see it causing problems
> > with the arrays that originally reported this problem.
> 
> You're right and I was wrong.  It only _appeared_ to be an infinite
> retry loop because there were 6 iterations allowed and each iteration
> had a 60-second timeout.  So I retract the patch submission.
> 
> Waiting six minutes for a command to complete is a bit ridiculous,
> though -- especially when the user program generating that command
> decides to reissue it a few times.  Can we do anything to expedite the
> failure?

A short term hack until retry policy is aligned and updated would be to
use the sdev_bflags to differentiate the usb case from use of the
BLIST_RETRY_HWERROR bflag.

> For example, does it really make sense for scsi_softirq_done
> to multiply cmd->allowed by rq->timeout?  After all, if a command
> aborts with a timeout instead of failing outright, what point is there
> in retrying it?  The proper approach would have been to use a longer 
> timeout initially.
> 

The wait_for is used for more than retries of timeouts.

I had thought it might be a good idea to expose the wait_for value and
then users could control the wait_for behavior if needed by using a udev
rule to set it near the IO timeout value if so required.

> By the way, is there any reason to keep scmd->retries now?  If commands
> are going to be retried until they timeout, why bother to count the
> retries? 

Previously I had submitted some patches on scsi mid retry with a short text
on current retry policy (this cover mid retry policy vs
scsi_io_completion, which should be unified).

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

I will try to refresh my patches with a updated policy document and also
align that with the changes to scsi_io_completion posted prior to
re-submit.

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

Re: [PATCH] SCSI: handle HARDWARE_ERROR sense correctly

[Alan Stern]

On Thu, 4 Dec 2008, Mike Anderson wrote:

> > > The current behaviour is to retry the error until the command timeout
> > > expires, which, I think is what was needed by the annoying arrays that
> > > have retryable hardware errors.
> > > 
> 
> Yes there are some arrays that need this behavior. The two users: 
> usb disks and the devinfo entries with BLIST_RETRY_HWERROR appear to have
> two different expected behaviors.

This leads me to question why hardware errors aren't always retried as 
a matter of course?

Of course, in most cases it makes sense to retry only a few times.  
(In other words, don't do five retries per second for 60 seconds!)  
Tape arrays needing indefinite retries appear to be out of the
ordinary.


> > For example, does it really make sense for scsi_softirq_done
> > to multiply cmd->allowed by rq->timeout?  After all, if a command
> > aborts with a timeout instead of failing outright, what point is there
> > in retrying it?  The proper approach would have been to use a longer 
> > timeout initially.
> > 
> 
> The wait_for is used for more than retries of timeouts.

What else is it used for?  In my copy of scsi_softirq_done() it appears
in just one place, and that is a test to see whether the command should
fail with a timeout error.

> I had thought it might be a good idea to expose the wait_for value and
> then users could control the wait_for behavior if needed by using a udev
> rule to set it near the IO timeout value if so required.

Why should the wait_for value be any different from the regular I/O 
timeout?  Isn't it in fact a timeout value?

Alan Stern

Re: [PATCH] SCSI: handle HARDWARE_ERROR sense correctly

[Alan Stern]

On Thu, 4 Dec 2008, Mike Anderson wrote:

> Previously I had submitted some patches on scsi mid retry with a short text
> on current retry policy (this cover mid retry policy vs
> scsi_io_completion, which should be unified).
> 
> http://marc.info/?l=linux-scsi&m=122210133628085&w=2
> 
> I will try to refresh my patches with a updated policy document and also
> align that with the changes to scsi_io_completion posted prior to
> re-submit.

Your policy discussion needs to be expanded.  And it needs to apply to 
scsi_io_completion() as well as scsi_decide_disposition().

As I see it, the set of possible retry actions is as follows:

     1: Don't retry at all.  This is appropriate for certain kinds
	of errors (such as LBA out of range); you know that they will
	never succeed no matter how many times you try them.

     2: Keep on retrying until the request times out.  This is
	appropriate in only a few circumstances (like the tape arrays
	James mentioned earlier).

     3: Retry a few times, generally with a short delay between 
	attempts, and then give up.  I favor a total of 3 attempts
	but the current code tends to use 6 -- okay, fine.

What's needed is a clear classification of errors into these three 
cases; that's what your policy document tries to do.  However the 
implementation of case (3) in particular needs to be fixed, since the 
code does not limit the number of retries correctly.


By the way, there seems to be some confusion over how to handle
HARDWARE ERROR (SK = 4).  The spec says "nonrecoverable".  This does
not mean non-retryable!

"Nonrecoverable" means that the hardware was unable to recover from the 
error.  But it still might be a transient error, and it might go away 
if the command was tried again.  In fact, the spec specifically 
mentions "parity error" as a possible cause; certainly a parity error 
might go away the next time the command is issued.

So the whole idea of the retry_hwerr flag is bogus; hardware errors
should always be retried.  Or perhaps only the name is bogus, since the
flag really indicates that the command should be tried over and over
again without pause until it succeeds or the request times out (whereas
normally hardware errors should be retried only a few times).

Alan Stern

Re: [PATCH] SCSI: handle HARDWARE_ERROR sense correctly

[James Bottomley]

On Tue, 2008-12-16 at 10:27 -0500, Alan Stern wrote:
> Your policy discussion needs to be expanded.  And it needs to apply
> to 
> scsi_io_completion() as well as scsi_decide_disposition().
> 
> As I see it, the set of possible retry actions is as follows:
> 
>      1: Don't retry at all.  This is appropriate for certain kinds
>         of errors (such as LBA out of range); you know that they will
>         never succeed no matter how many times you try them.
> 
>      2: Keep on retrying until the request times out.  This is
>         appropriate in only a few circumstances (like the tape arrays
>         James mentioned earlier).
> 
>      3: Retry a few times, generally with a short delay between 
>         attempts, and then give up.  I favor a total of 3 attempts
>         but the current code tends to use 6 -- okay, fine.
> 
> What's needed is a clear classification of errors into these three 
> cases; that's what your policy document tries to do.  However the 
> implementation of case (3) in particular needs to be fixed, since the 
> code does not limit the number of retries correctly.

It would be nice ... unfortunately, errors tend to migrate around the
categories because of empirical results, so such a document could never
be definitive.

As a rule of thumb:  errors which produced an action in the device which
errors but is retryable count down the retries (things like parity/crc
errors on the bus).  Things which would produce no benefit retrying
(like Medium errors) get failed immediately and things which indicate
transient resource issues either in the device (QUEUE_FULL) or the host
(DID_REQUEUE) get retried upto the timeout limit with a suitable backoff
(mostly we refuse to issue more commands until one returns).

> By the way, there seems to be some confusion over how to handle
> HARDWARE ERROR (SK = 4).  The spec says "nonrecoverable".  This does
> not mean non-retryable!

> "Nonrecoverable" means that the hardware was unable to recover from
> the 
> error.  But it still might be a transient error, and it might go away 
> if the command was tried again.  In fact, the spec specifically 
> mentions "parity error" as a possible cause; certainly a parity error 
> might go away the next time the command is issued.

Really, no ... most array and disk vendors have specifically requested
that we not retry either medium or hardware errors (with the exception
of those we have the flag for).  The reason is that by the time the
sense is returned, the device has been retrying on its own for quite a
while.  If we also retry, we trigger a delay in reporting the error to
the user.

> So the whole idea of the retry_hwerr flag is bogus; hardware errors
> should always be retried.  Or perhaps only the name is bogus, since
> the
> flag really indicates that the command should be tried over and over
> again without pause until it succeeds or the request times out
> (whereas
> normally hardware errors should be retried only a few times).

It doesn't actually; the MLQUEUE return blocks the device from further
issue until a command returns (or, if empty issue queue, until I/O
pressure causes a block unplug 3 times).

James

Re: [PATCH] SCSI: handle HARDWARE_ERROR sense correctly

[Alan Stern]

On Tue, 16 Dec 2008, James Bottomley wrote:

> It would be nice ... unfortunately, errors tend to migrate around the
> categories because of empirical results, so such a document could never
> be definitive.
> 
> As a rule of thumb:  errors which produced an action in the device which
> errors but is retryable count down the retries (things like parity/crc
> errors on the bus).  Things which would produce no benefit retrying
> (like Medium errors) get failed immediately and things which indicate
> transient resource issues either in the device (QUEUE_FULL) or the host
> (DID_REQUEUE) get retried upto the timeout limit with a suitable backoff
> (mostly we refuse to issue more commands until one returns).

It would be wonderful if Mike or someone else would implement this
scheme.  The necessary changes shouldn't be very extensive.

(And I still think the wait_for logic in scsi_softirq_done() is wrong; 
rq->timeout shouldn't be multiplied by cmd->allowed.)


> > So the whole idea of the retry_hwerr flag is bogus; hardware errors
> > should always be retried.  Or perhaps only the name is bogus, since
> > the
> > flag really indicates that the command should be tried over and over
> > again without pause until it succeeds or the request times out
> > (whereas
> > normally hardware errors should be retried only a few times).
> 
> It doesn't actually; the MLQUEUE return blocks the device from further
> issue until a command returns (or, if empty issue queue, until I/O
> pressure causes a block unplug 3 times).

Okay, I misunderstood how that works.  Still, the code bypasses the 
normal retry pathways, leaving it vulnerable to these sorts of 
problems.  So why put the retry_hwerr test in check_sense()?  Why not 
put it in scsi_io_completion() instead, so that retries can be limited 
appropriately?

BTW, what happens if the issue queue is empty and there is no I/O
pressure?  Then the command wouldn't be retried at all, it would just
time out.  That doesn't seem like what you want.

Alan Stern

Re: [PATCH] SCSI: handle HARDWARE_ERROR sense correctly

[James Bottomley]

On Tue, 2008-12-16 at 14:56 -0500, Alan Stern wrote:
> On Tue, 16 Dec 2008, James Bottomley wrote:
> 
> > It would be nice ... unfortunately, errors tend to migrate around the
> > categories because of empirical results, so such a document could never
> > be definitive.
> > 
> > As a rule of thumb:  errors which produced an action in the device which
> > errors but is retryable count down the retries (things like parity/crc
> > errors on the bus).  Things which would produce no benefit retrying
> > (like Medium errors) get failed immediately and things which indicate
> > transient resource issues either in the device (QUEUE_FULL) or the host
> > (DID_REQUEUE) get retried upto the timeout limit with a suitable backoff
> > (mostly we refuse to issue more commands until one returns).
> 
> It would be wonderful if Mike or someone else would implement this
> scheme.  The necessary changes shouldn't be very extensive.
> 
> (And I still think the wait_for logic in scsi_softirq_done() is wrong; 
> rq->timeout shouldn't be multiplied by cmd->allowed.)

It's the logical retry timeout ... if a command fails and times out it
gets retried, if it continues to time out, retries*timeout is the max
before it fails.

> > > So the whole idea of the retry_hwerr flag is bogus; hardware errors
> > > should always be retried.  Or perhaps only the name is bogus, since
> > > the
> > > flag really indicates that the command should be tried over and over
> > > again without pause until it succeeds or the request times out
> > > (whereas
> > > normally hardware errors should be retried only a few times).
> > 
> > It doesn't actually; the MLQUEUE return blocks the device from further
> > issue until a command returns (or, if empty issue queue, until I/O
> > pressure causes a block unplug 3 times).
> 
> Okay, I misunderstood how that works.  Still, the code bypasses the 
> normal retry pathways, leaving it vulnerable to these sorts of 
> problems. 

It does?  How?  decide_disposition() goes straigh into the expiry check.

>  So why put the retry_hwerr test in check_sense()?  Why not 
> put it in scsi_io_completion() instead, so that retries can be limited 
> appropriately?

because check_sense goes into decide disposition which gets the timeout
test applied on MLQUEUE return.

> BTW, what happens if the issue queue is empty and there is no I/O
> pressure?  Then the command wouldn't be retried at all, it would just
> time out.  That doesn't seem like what you want.

I/O pressure is proportional to the size of the request queue.  By
definition, a requeue means at least 1 outstnading request and thus some
pressure.

James

Re: [PATCH] SCSI: handle HARDWARE_ERROR sense correctly

[Alan Stern]

On Tue, 16 Dec 2008, James Bottomley wrote:

> > It would be wonderful if Mike or someone else would implement this
> > scheme.  The necessary changes shouldn't be very extensive.
> > 
> > (And I still think the wait_for logic in scsi_softirq_done() is wrong; 
> > rq->timeout shouldn't be multiplied by cmd->allowed.)
> 
> It's the logical retry timeout ... if a command fails and times out it
> gets retried, if it continues to time out, retries*timeout is the max
> before it fails.

Yes, certainly -- that's what the current code does.  And it's wrong.

rq->timeout should be the overall timeout for the request.  No matter
how many commands get prepped and issued, no matter how many times the 
request gets pushed back on the queue, if the request isn't finished
by then it should expire.

If I submit a request with a timeout of 60 seconds, then I want it to
be aborted if it hasn't finished within 60 seconds after the time it
starts.  I don't want to wait 360 seconds just because the SCSI
midlayer decides that it has to issue 6 separate commands to carry out
my request.


> > > It doesn't actually; the MLQUEUE return blocks the device from further
> > > issue until a command returns (or, if empty issue queue, until I/O
> > > pressure causes a block unplug 3 times).
> > 
> > Okay, I misunderstood how that works.  Still, the code bypasses the 
> > normal retry pathways, leaving it vulnerable to these sorts of 
> > problems. 
> 
> It does?  How?  decide_disposition() goes straigh into the expiry check.

Consider the various pathways there are for retries:

	scsi_softirq_done -> scsi_decide_disposition which returns
	NEEDS_RETRY -> scsi_queue_insert

	scsi_softirq_done -> scsi_decide_disposition which returns
	ADD_TO_MLQUEUE -> scsi_queue_insert

	scsi_io_completion -> scsi_end_request -> scsi_requeue_command
	-> blk_requeue_request

	scsi_io_completion decides on ACTION_REPREP ->
	scsi_requeue_command -> blk_requeue_request

	scsi_io_completion decides on ACTION_RETRY -> scsi_queue_insert

	scsi_io_completion decides on ACTION_DELAYED_RETRY ->
	scsi_queue_insert

6 different pathways for retries!  And exactly how consistent are they 
about deciding when there have been too many retries or the request 
should expire?  Not very.

> >  So why put the retry_hwerr test in check_sense()?  Why not 
> > put it in scsi_io_completion() instead, so that retries can be limited 
> > appropriately?
> 
> because check_sense goes into decide disposition which gets the timeout
> test applied on MLQUEUE return.

This merely reinforces my point that there are too many different ways 
to retry commands, with inconsistent timeout checks.


> > BTW, what happens if the issue queue is empty and there is no I/O
> > pressure?  Then the command wouldn't be retried at all, it would just
> > time out.  That doesn't seem like what you want.
> 
> I/O pressure is proportional to the size of the request queue.  By
> definition, a requeue means at least 1 outstnading request and thus some
> pressure.

But you mentioned above that it takes 3 unplug events before the
command is retried.  If that command is the single outstanding request,
will it ever be retried?  How many block unplug events do you get from
a single request?

Alan Stern

Re: [PATCH] SCSI: handle HARDWARE_ERROR sense correctly

[Kai Makisara]

On Thu, 4 Dec 2008, James Bottomley wrote:

> On Thu, 2008-12-04 at 15:49 -0500, Alan Stern wrote:
> > This patch (as1183) fixes a bug in scsi_check_sense().  The routine is
> > documented as returning one of SUCCESS, FAILED, or NEEDS_RETRY.  But
> > in the HARDWARE_ERROR case it can return ADD_TO_MLQUEUE.  And since it
> > does this without bothering to increment the retry count, it can lead
> > to an infinite retry loop.
> > 
> > The fix is to return NEEDS_RETRY instead.  Then the caller,
> > scsi_decide_disposition(), will do the right thing.
> 
> OK, but why?
> 
> The current behaviour is to retry the error until the command timeout
> expires, which, I think is what was needed by the annoying arrays that
> have retryable hardware errors.
> 
So, a tape command returning (non-recoverable) HARDWARE_ERROR is retried 
until the timeout (default 3.8 hours if the command happens to use the 
long timout)? And is the result returned to the upper level timeout 
instead of sense data? Does not sound good.

And another thing is that retrying an error that is not clearly retryable 
"outside" retry counting does not sound good.

> What bug would this patch fix?  Because I can see it causing problems
> with the arrays that originally reported this problem.
> 
Is a quirk needed?

Kai

Re: [PATCH] SCSI: handle HARDWARE_ERROR sense correctly

[James Bottomley]

On Fri, 2008-12-05 at 16:41 +0200, Kai Makisara wrote:
> On Thu, 4 Dec 2008, James Bottomley wrote:
> 
> > On Thu, 2008-12-04 at 15:49 -0500, Alan Stern wrote:
> > > This patch (as1183) fixes a bug in scsi_check_sense().  The routine is
> > > documented as returning one of SUCCESS, FAILED, or NEEDS_RETRY.  But
> > > in the HARDWARE_ERROR case it can return ADD_TO_MLQUEUE.  And since it
> > > does this without bothering to increment the retry count, it can lead
> > > to an infinite retry loop.
> > > 
> > > The fix is to return NEEDS_RETRY instead.  Then the caller,
> > > scsi_decide_disposition(), will do the right thing.
> > 
> > OK, but why?
> > 
> > The current behaviour is to retry the error until the command timeout
> > expires, which, I think is what was needed by the annoying arrays that
> > have retryable hardware errors.
> > 
> So, a tape command returning (non-recoverable) HARDWARE_ERROR is retried 
> until the timeout (default 3.8 hours if the command happens to use the 
> long timout)? And is the result returned to the upper level timeout 
> instead of sense data? Does not sound good.

No.  This is abnormal behaviour and it's conditioned on a flag in device
info.  The standards say that HARDWARE_ERROR is an immediate failure ...
we just have some stupid arrays (won't name names) that violate the
standard and the option was either to give the user spurious I/O errors
or allow retry.

> And another thing is that retrying an error that is not clearly retryable 
> "outside" retry counting does not sound good.

It's not by standard HARDWARE_ERROR is never retryable, so we don't in
the usual case.

> > What bug would this patch fix?  Because I can see it causing problems
> > with the arrays that originally reported this problem.
> > 
> Is a quirk needed?

BLIST_RETRY_HWERROR

James