sym53c8xx_2 data corruption

sym53c8xx_2 data corruption

[Mikulas Patocka]

Hi

I sent this about twice to linux-scsi and got no reseponse, neither from 
conference nor from Matthew. So I'm sending it here, James, you are the 
maintainer of SCSI, could you please look at the patch and incorporate it 
to the kernel in this cycle?

The problem is that if the disk returns QUEUE FULL, the requests are 
aborted with DID_SOFT_ERROR (rather than DID_REQUEUE), which results in 
too few retries and premature errors. The errors happen mostly on writes, 
resulting in data corruption.

Mikulas

---

sym53c8xx_2: Set DID_REQUEUE return code when aborting squeue.

When the controller encounters an error (including QUEUE FULL or BUSY status),
it aborts all not yet submitted requests in the function
sym_dequeue_from_squeue.

This function aborts them with DID_SOFT_ERROR.

If the disk has a full tag queue, the request that caused the overflow is
aborted with QUEUE FULL status (and the scsi midlayer properly retries it
until it is accepted by the disk), but other requests are aborted with
DID_SOFT_ERROR --- for them, the midlayer does just a few retries and then
signals the error up to sd.

The result is that disk returning QUEUE FULL causes request failures.

The error was reproduced on 53c895 with COMPAQ BD03685A24 disk (rebranded
ST336607LC) with command queue 48 or 64 tags. The disk has 64 tags, but
under some access patterns it return QUEUE FULL when there are less than
64 pending tags. The SCSI specification allows returning QUEUE FULL
anytime and it is up to the host to retry.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>

---
 drivers/scsi/sym53c8xx_2/sym_hipd.c |    4 ++++
 1 file changed, 4 insertions(+)

Index: linux-2.6.36-rc5-fast/drivers/scsi/sym53c8xx_2/sym_hipd.c
===================================================================
--- linux-2.6.36-rc5-fast.orig/drivers/scsi/sym53c8xx_2/sym_hipd.c	2010-09-27 10:25:59.000000000 +0200
+++ linux-2.6.36-rc5-fast/drivers/scsi/sym53c8xx_2/sym_hipd.c	2010-09-27 10:26:27.000000000 +0200
@@ -3000,7 +3000,11 @@ sym_dequeue_from_squeue(struct sym_hcb *
 		if ((target == -1 || cp->target == target) &&
 		    (lun    == -1 || cp->lun    == lun)    &&
 		    (task   == -1 || cp->tag    == task)) {
+#ifdef SYM_OPT_HANDLE_DEVICE_QUEUEING
 			sym_set_cam_status(cp->cmd, DID_SOFT_ERROR);
+#else
+			sym_set_cam_status(cp->cmd, DID_REQUEUE);
+#endif
 			sym_remque(&cp->link_ccbq);
 			sym_insque_tail(&cp->link_ccbq, &np->comp_ccbq);
 		}

Re: sym53c8xx_2 data corruption

[James Bottomley]

On Wed, 2010-10-27 at 11:04 +0200, Mikulas Patocka wrote:
> Hi
> 
> I sent this about twice to linux-scsi and got no reseponse, neither from 
> conference nor from Matthew. So I'm sending it here, James, you are the 
> maintainer of SCSI, could you please look at the patch and incorporate it 
> to the kernel in this cycle?
> 
> The problem is that if the disk returns QUEUE FULL, the requests are 
> aborted with DID_SOFT_ERROR (rather than DID_REQUEUE), which results in 
> too few retries and premature errors. The errors happen mostly on writes, 
> resulting in data corruption.
> 
> Mikulas
> 
> ---
> 
> sym53c8xx_2: Set DID_REQUEUE return code when aborting squeue.
> 
> When the controller encounters an error (including QUEUE FULL or BUSY status),
> it aborts all not yet submitted requests in the function
> sym_dequeue_from_squeue.
> 
> This function aborts them with DID_SOFT_ERROR.
> 
> If the disk has a full tag queue, the request that caused the overflow is
> aborted with QUEUE FULL status (and the scsi midlayer properly retries it
> until it is accepted by the disk), but other requests are aborted with
> DID_SOFT_ERROR --- for them, the midlayer does just a few retries and then
> signals the error up to sd.
> 
> The result is that disk returning QUEUE FULL causes request failures.
> 
> The error was reproduced on 53c895 with COMPAQ BD03685A24 disk (rebranded
> ST336607LC) with command queue 48 or 64 tags. The disk has 64 tags, but
> under some access patterns it return QUEUE FULL when there are less than
> 64 pending tags. The SCSI specification allows returning QUEUE FULL
> anytime and it is up to the host to retry.

So the description isn't really complete.  the function is
dequeue_from_squeue which is used to requeue all unissued scbs when the
sequencer is restarted.  This doesn't just affect QUEUE_FULL, it affects
everything.  As long as the pushback is done before the status is
returned (which it looks like it is), I think the patch after fixing
looks fine.

The problem isn't the actual command which returns queue full ... it's
that the sequencer accepts and queues a pile of commands and then
returns all of them on the first queue full ... that means that deeply
queued commands in the sequencer issue queue can get returned >5 times
on multiple QUEUE_FULL conditions which would cause a failure.

> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> 
> ---
>  drivers/scsi/sym53c8xx_2/sym_hipd.c |    4 ++++
>  1 file changed, 4 insertions(+)
> 
> Index: linux-2.6.36-rc5-fast/drivers/scsi/sym53c8xx_2/sym_hipd.c
> ===================================================================
> --- linux-2.6.36-rc5-fast.orig/drivers/scsi/sym53c8xx_2/sym_hipd.c	2010-09-27 10:25:59.000000000 +0200
> +++ linux-2.6.36-rc5-fast/drivers/scsi/sym53c8xx_2/sym_hipd.c	2010-09-27 10:26:27.000000000 +0200
> @@ -3000,7 +3000,11 @@ sym_dequeue_from_squeue(struct sym_hcb *
>  		if ((target == -1 || cp->target == target) &&
>  		    (lun    == -1 || cp->lun    == lun)    &&
>  		    (task   == -1 || cp->tag    == task)) {
> +#ifdef SYM_OPT_HANDLE_DEVICE_QUEUEING
>  			sym_set_cam_status(cp->cmd, DID_SOFT_ERROR);
> +#else
> +			sym_set_cam_status(cp->cmd, DID_REQUEUE);
> +#endif

So the ifdef is definitely wrong.  SYM_OPT_HANDLE_DEVICE_QUEUEING is a
leftover from when the driver did explicit internal queueing. Just make
this do DID_REQUEUE and I *think* everything will be OK.

There's a danger in that DID_REQUEUE will requeue forever, so this
working depends on the original failing command being returned with the
correct code (which I think it is, but more eyes looking at this would
be helpful).

James

Re: sym53c8xx_2 data corruption

[Mikulas Patocka]

On Wed, 27 Oct 2010, James Bottomley wrote:

> On Wed, 2010-10-27 at 11:04 +0200, Mikulas Patocka wrote:
> > Hi
> > 
> > I sent this about twice to linux-scsi and got no reseponse, neither from 
> > conference nor from Matthew. So I'm sending it here, James, you are the 
> > maintainer of SCSI, could you please look at the patch and incorporate it 
> > to the kernel in this cycle?
> > 
> > The problem is that if the disk returns QUEUE FULL, the requests are 
> > aborted with DID_SOFT_ERROR (rather than DID_REQUEUE), which results in 
> > too few retries and premature errors. The errors happen mostly on writes, 
> > resulting in data corruption.
> > 
> > Mikulas
> > 
> > ---
> > 
> > sym53c8xx_2: Set DID_REQUEUE return code when aborting squeue.
> > 
> > When the controller encounters an error (including QUEUE FULL or BUSY status),
> > it aborts all not yet submitted requests in the function
> > sym_dequeue_from_squeue.
> > 
> > This function aborts them with DID_SOFT_ERROR.
> > 
> > If the disk has a full tag queue, the request that caused the overflow is
> > aborted with QUEUE FULL status (and the scsi midlayer properly retries it
> > until it is accepted by the disk), but other requests are aborted with
> > DID_SOFT_ERROR --- for them, the midlayer does just a few retries and then
> > signals the error up to sd.
> > 
> > The result is that disk returning QUEUE FULL causes request failures.
> > 
> > The error was reproduced on 53c895 with COMPAQ BD03685A24 disk (rebranded
> > ST336607LC) with command queue 48 or 64 tags. The disk has 64 tags, but
> > under some access patterns it return QUEUE FULL when there are less than
> > 64 pending tags. The SCSI specification allows returning QUEUE FULL
> > anytime and it is up to the host to retry.
> 
> So the description isn't really complete.  the function is
> dequeue_from_squeue which is used to requeue all unissued scbs when the
> sequencer is restarted.  This doesn't just affect QUEUE_FULL, it affects
> everything.  As long as the pushback is done before the status is
> returned (which it looks like it is), I think the patch after fixing
> looks fine.
>
> The problem isn't the actual command which returns queue full ... it's
> that the sequencer accepts and queues a pile of commands and then
> returns all of them on the first queue full ... that means that deeply
> queued commands in the sequencer issue queue can get returned >5 times
> on multiple QUEUE_FULL conditions which would cause a failure.

Sure, that's how I understood it from the code and debug prints. You can 
add this to the description.

That QUEUE_FULL command is actually retired fine, the following commands 
are problematic.

> > Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> > 
> > ---
> >  drivers/scsi/sym53c8xx_2/sym_hipd.c |    4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > Index: linux-2.6.36-rc5-fast/drivers/scsi/sym53c8xx_2/sym_hipd.c
> > ===================================================================
> > --- linux-2.6.36-rc5-fast.orig/drivers/scsi/sym53c8xx_2/sym_hipd.c	2010-09-27 10:25:59.000000000 +0200
> > +++ linux-2.6.36-rc5-fast/drivers/scsi/sym53c8xx_2/sym_hipd.c	2010-09-27 10:26:27.000000000 +0200
> > @@ -3000,7 +3000,11 @@ sym_dequeue_from_squeue(struct sym_hcb *
> >  		if ((target == -1 || cp->target == target) &&
> >  		    (lun    == -1 || cp->lun    == lun)    &&
> >  		    (task   == -1 || cp->tag    == task)) {
> > +#ifdef SYM_OPT_HANDLE_DEVICE_QUEUEING
> >  			sym_set_cam_status(cp->cmd, DID_SOFT_ERROR);
> > +#else
> > +			sym_set_cam_status(cp->cmd, DID_REQUEUE);
> > +#endif
> 
> So the ifdef is definitely wrong.  SYM_OPT_HANDLE_DEVICE_QUEUEING is a
> leftover from when the driver did explicit internal queueing. Just make
> this do DID_REQUEUE and I *think* everything will be OK.

When I tried to enable SYM_OPT_HANDLE_DEVICE_QUEUEING, it didn't work, it 
crashed on something --- it is leftover from some other operating system 
that didn't handle requeuing in the midlayer.

When looking at the other parts of code that handles this driver-internal 
requeueing, it expects DID_SOFT_ERROR there. But it doesn't matter, that 
code is useless for Linux and broken anyway.

> There's a danger in that DID_REQUEUE will requeue forever, so this
> working depends on the original failing command being returned with the
> correct code (which I think it is, but more eyes looking at this would
> be helpful).

Requeuing forever is dangerous anyway, a device returning QUEUE_FULL 
constantly could deadlock the system. Question: is it better to risk a 
deadlock with a broken device or to risk a false timeout under high load? 
--- I don't know --- maybe there are valid cases where the device is 
returning QUEUE_FULL for long time (some raid reconfiguration?) ... do you 
know about them?

Anyway, if sym_dequeue_from_squeue was called from some other error that 
causes limited retry or command abort, I think it is still valid to use 
DID_REQUEUE for the following commands --- it can't deadlock with 
DID_REQUEUE, because on that error, the first command is aborted or has 
its retry count decremented --- so the first command must be eventually 
completed, and the second command (which was being retried with 
DID_REQUEUE) becomes the first --- and once it's first, it cannot loop 
forever. So with induction you can prove that every command completes in 
finite time.

Mikulas

> James
> 
> 

Re: sym53c8xx_2 data corruption

[James Bottomley]

On Wed, 2010-10-27 at 18:19 +0200, Mikulas Patocka wrote:
> 
> On Wed, 27 Oct 2010, James Bottomley wrote:
> 
> > On Wed, 2010-10-27 at 11:04 +0200, Mikulas Patocka wrote:
> > > Hi
> > > 
> > > I sent this about twice to linux-scsi and got no reseponse, neither from 
> > > conference nor from Matthew. So I'm sending it here, James, you are the 
> > > maintainer of SCSI, could you please look at the patch and incorporate it 
> > > to the kernel in this cycle?
> > > 
> > > The problem is that if the disk returns QUEUE FULL, the requests are 
> > > aborted with DID_SOFT_ERROR (rather than DID_REQUEUE), which results in 
> > > too few retries and premature errors. The errors happen mostly on writes, 
> > > resulting in data corruption.
> > > 
> > > Mikulas
> > > 
> > > ---
> > > 
> > > sym53c8xx_2: Set DID_REQUEUE return code when aborting squeue.
> > > 
> > > When the controller encounters an error (including QUEUE FULL or BUSY status),
> > > it aborts all not yet submitted requests in the function
> > > sym_dequeue_from_squeue.
> > > 
> > > This function aborts them with DID_SOFT_ERROR.
> > > 
> > > If the disk has a full tag queue, the request that caused the overflow is
> > > aborted with QUEUE FULL status (and the scsi midlayer properly retries it
> > > until it is accepted by the disk), but other requests are aborted with
> > > DID_SOFT_ERROR --- for them, the midlayer does just a few retries and then
> > > signals the error up to sd.
> > > 
> > > The result is that disk returning QUEUE FULL causes request failures.
> > > 
> > > The error was reproduced on 53c895 with COMPAQ BD03685A24 disk (rebranded
> > > ST336607LC) with command queue 48 or 64 tags. The disk has 64 tags, but
> > > under some access patterns it return QUEUE FULL when there are less than
> > > 64 pending tags. The SCSI specification allows returning QUEUE FULL
> > > anytime and it is up to the host to retry.
> > 
> > So the description isn't really complete.  the function is
> > dequeue_from_squeue which is used to requeue all unissued scbs when the
> > sequencer is restarted.  This doesn't just affect QUEUE_FULL, it affects
> > everything.  As long as the pushback is done before the status is
> > returned (which it looks like it is), I think the patch after fixing
> > looks fine.
> >
> > The problem isn't the actual command which returns queue full ... it's
> > that the sequencer accepts and queues a pile of commands and then
> > returns all of them on the first queue full ... that means that deeply
> > queued commands in the sequencer issue queue can get returned >5 times
> > on multiple QUEUE_FULL conditions which would cause a failure.
> 
> Sure, that's how I understood it from the code and debug prints. You can 
> add this to the description.
> 
> That QUEUE_FULL command is actually retired fine, the following commands 
> are problematic.
> 
> > > Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> > > 
> > > ---
> > >  drivers/scsi/sym53c8xx_2/sym_hipd.c |    4 ++++
> > >  1 file changed, 4 insertions(+)
> > > 
> > > Index: linux-2.6.36-rc5-fast/drivers/scsi/sym53c8xx_2/sym_hipd.c
> > > ===================================================================
> > > --- linux-2.6.36-rc5-fast.orig/drivers/scsi/sym53c8xx_2/sym_hipd.c	2010-09-27 10:25:59.000000000 +0200
> > > +++ linux-2.6.36-rc5-fast/drivers/scsi/sym53c8xx_2/sym_hipd.c	2010-09-27 10:26:27.000000000 +0200
> > > @@ -3000,7 +3000,11 @@ sym_dequeue_from_squeue(struct sym_hcb *
> > >  		if ((target == -1 || cp->target == target) &&
> > >  		    (lun    == -1 || cp->lun    == lun)    &&
> > >  		    (task   == -1 || cp->tag    == task)) {
> > > +#ifdef SYM_OPT_HANDLE_DEVICE_QUEUEING
> > >  			sym_set_cam_status(cp->cmd, DID_SOFT_ERROR);
> > > +#else
> > > +			sym_set_cam_status(cp->cmd, DID_REQUEUE);
> > > +#endif
> > 
> > So the ifdef is definitely wrong.  SYM_OPT_HANDLE_DEVICE_QUEUEING is a
> > leftover from when the driver did explicit internal queueing. Just make
> > this do DID_REQUEUE and I *think* everything will be OK.
> 
> When I tried to enable SYM_OPT_HANDLE_DEVICE_QUEUEING, it didn't work, it 
> crashed on something --- it is leftover from some other operating system 
> that didn't handle requeuing in the midlayer.
> 
> When looking at the other parts of code that handles this driver-internal 
> requeueing, it expects DID_SOFT_ERROR there. But it doesn't matter, that 
> code is useless for Linux and broken anyway.
> 
> > There's a danger in that DID_REQUEUE will requeue forever, so this
> > working depends on the original failing command being returned with the
> > correct code (which I think it is, but more eyes looking at this would
> > be helpful).
> 
> Requeuing forever is dangerous anyway, a device returning QUEUE_FULL 
> constantly could deadlock the system. Question: is it better to risk a 
> deadlock with a broken device or to risk a false timeout under high load? 
> --- I don't know --- maybe there are valid cases where the device is 
> returning QUEUE_FULL for long time (some raid reconfiguration?) ... do you 
> know about them?
> 
> Anyway, if sym_dequeue_from_squeue was called from some other error that 
> causes limited retry or command abort, I think it is still valid to use 
> DID_REQUEUE for the following commands --- it can't deadlock with 
> DID_REQUEUE, because on that error, the first command is aborted or has 
> its retry count decremented --- so the first command must be eventually 
> completed, and the second command (which was being retried with 
> DID_REQUEUE) becomes the first --- and once it's first, it cannot loop 
> forever. So with induction you can prove that every command completes in 
> finite time.

As long as we see the QUEUE_FULL return, there's no danger.  The mid
layer has a timeout beyond which it won't allow a QUEUE_FULL return to
be retried.

James

Re: sym53c8xx_2 data corruption

[Grant Grundler]

On Wed, Oct 27, 2010 at 06:19:32PM +0200, Mikulas Patocka wrote:
...
> Requeuing forever is dangerous anyway, a device returning QUEUE_FULL 
> constantly could deadlock the system. Question: is it better to risk a 
> deadlock with a broken device or to risk a false timeout under high load? 
> --- I don't know --- maybe there are valid cases where the device is 
> returning QUEUE_FULL for long time (some raid reconfiguration?) ... do you 
> know about them?

This was a problem in multi-initiator SCSI systems and I'm guessing also
an issue for FC SAN. Multiple hosts "compete" for filling the device's
available command slots. If all hosts used available queue_depth
(say 32 commands) and device only supported 64 commands at a time,
then the 65th command from host #3 might get QUEUEFULL status back.

I'm not sure what the difference is to BUSY status. Wikipedia
suggests "QUEUE_FULL" is a hint that the device is already
processing commands from the same initiator.

hth,
grant