Bad reactions to QUEUE FULL

Bad reactions to QUEUE FULL

[Tim Pepper]

It seems like there are some nasty reactions to a QUEUE FULL return in the
scsi stack.  A colleague found at least the bit patched below, although
I'm not sure it's 100% right.  Prior to that patch we'd seen crashes with:
	Kernel panic:scsi_free:Bad offset
With it I'm still seeing flakey behaviour...out of six machines with a variety
of kernels each time we drive our disk subsystem to the point of returning
queue full the 6 hosts are randomly but evenly spread across:
- total system lock up
- hung IO threads
- everything fine
I've got lots of syslogs captured that sort of generally show bad things
happening, but don't point me at any particular code in the scsi stack
to start looking at.

The machines are all 2.4 kernels x86 with qlogic and emulex hbas and the
latest drivers from those vendors running SLES7, SLES8, RH7.x, RHAS2.1
(latest errata kernels of each as of a week or two ago anyway).  I could
probably get a recreate on a recent kernel.org 2.4 kernel, but I don't
think the scsi stack is that different between them all (could be
wrong :).

I've trolled up some info that makes it sound like some s390 kernel has
some sort of a work(s?) around...

Anybody interested in having a look at this or is it a known issue?

Tim
-- 
*********************************************************
*  tpepper@vato dot org             * Venimus, Vidimus, *
*  http://www.vato.org/~tpepper     * Dolavimus         *
*********************************************************


--- linux-2.4.20/drivers/scsi/scsi_queue.c.orig	Tue Apr 29 21:29:59 2003
+++ linux-2.4.20/drivers/scsi/scsi_queue.c	Tue Apr 29 21:31:02 2003
@@ -143,6 +143,11 @@
 	spin_unlock_irqrestore(&io_request_lock, flags);
 
 	/*
+	 * Restore the direction
+	 */
+	cmd->sc_data_direction = cmd->sc_old_data_direction;
+
+	/*
 	 * Insert this command at the head of the queue for it's device.
 	 * It will go before all other commands that are already in the queue.
 	 */

Re: Bad reactions to QUEUE FULL

[James Bottomley]

On Wed, 2003-04-30 at 00:02, Tim Pepper wrote:
> Anybody interested in having a look at this or is it a known issue?

Actually, I'd be most interested in knowing what the problem is that the
patch solves.

The "old" fields (and all inconsistently named) are designed to keep a
copy of the original command for when the actual command gets re-used. 
This re-use happens in the error handler (to get sense, send TURs etc)
and sometimes in the device drivers if they simulate ACA).  Everything
that replaces the command is supposed to copy the old one back after
it's finished using it.

However, QUEUE FULL is a status return, not a sense code, so any command
that gets QUEUE FULL should be an original command, and thus not need
the fields replacing.  Even more curious, if the command were replaced,
then more fields than just the data direction would need putting back.

A viable theory seems to be that the drivers (qlogic and emulex) change
the sc_data_direction field for their own purposes and forget to restore
it again.  I don't have the source, so I can't check this.

Does anyone have any other theories?

James

Re: Bad reactions to QUEUE FULL

[Mike Anderson]

James Bottomley [James.Bottomley@steeleye.com] wrote:
> On Wed, 2003-04-30 at 00:02, Tim Pepper wrote:
> > Anybody interested in having a look at this or is it a known issue?
> 
> Actually, I'd be most interested in knowing what the problem is that the
> patch solves.
> 
> The "old" fields (and all inconsistently named) are designed to keep a
> copy of the original command for when the actual command gets re-used. 
> This re-use happens in the error handler (to get sense, send TURs etc)
> and sometimes in the device drivers if they simulate ACA).  Everything
> that replaces the command is supposed to copy the old one back after
> it's finished using it.
> 
> However, QUEUE FULL is a status return, not a sense code, so any command
> that gets QUEUE FULL should be an original command, and thus not need
> the fields replacing.  Even more curious, if the command were replaced,
> then more fields than just the data direction would need putting back.
> 
> A viable theory seems to be that the drivers (qlogic and emulex) change
> the sc_data_direction field for their own purposes and forget to restore
> it again.  I don't have the source, so I can't check this.
> 
> Does anyone have any other theories?
> 

Previously I have seen a failure similar to this. The signature of the
failure differs depending on which LLDD you are using, the types of
errors and possibly which kernel you are using (vendor, mainline, etc). 

The failure happens due to a command being re-enqueued for retry and
getting the SPECIAL flag set.  Then sometime later having this flag set
cause the code to make some assumptions which are not correct. The order
is important. 

One scenario is:

1.) During some error condition the LLDD returns a non-zero value from
queuecommand. This cause scsi_mlqueue_insert to be called.
scsi_mlqueue_insert calls scsi_insert_special_cmd which calls
__scsi_insert_special. __scsi_insert_special sets request cmd field to
SPECIAL. 

2.) Sometime later the LLDD queuecommand accepts the command, but
completes the IO with a non-good status.

3.) scsi_io_completion is eventually called on the IO. At the top of
scsi_io_completion we free the sg list. Now when this IO finally makes
it back to scsi_request_fn the SPECIAL flag will cause the IO to not get
scsi_init_io_fn called to allocate the sg list again.

The patch below is a hack to work around this one case, but there could
be other issues you are hitting.

-andmike
--
Michael Anderson
andmike@us.ibm.com

 scsi_lib.c |   11 +++++++++++
 1 files changed, 11 insertions(+)

--- linux-2.4/drivers/scsi/scsi_lib.c		Wed Apr 30 09:04:16 2003
+++ linux-2.4-p/drivers/scsi/scsi_lib.c		Wed Apr 30 09:04:17 2003
@@ -256,6 +256,17 @@
 	if (SCpnt != NULL) {
 
 		/*
+		 * This is a work around for a case where this Scsi_Cmnd
+		 * may have been through the busy retry paths already. We
+		 * clear the special flag and try to restore the
+		 * read/write request cmd value.
+		 */
+		if (SCpnt->request.cmd == SPECIAL)
+			SCpnt->request.cmd = 
+				(SCpnt->sc_data_direction ==
+				 SCSI_DATA_WRITE) ? WRITE : READ;
+
+		/*
 		 * For some reason, we are not done with this request.
 		 * This happens for I/O errors in the middle of the request,
 		 * in which case we need to request the blocks that come after

Re: Bad reactions to QUEUE FULL

[Tim Pepper]

Thanks Mike!  I'll run with this and see what happens.  Should I do it instead of
the one restoring the old direction?

Since it sounds LLDD related I'll bounce detailed trace info from each of
those drivers to the vendors for comment.

Thanks!

Tim

> 
> --- linux-2.4/drivers/scsi/scsi_lib.c		Wed Apr 30 09:04:16 2003
> +++ linux-2.4-p/drivers/scsi/scsi_lib.c		Wed Apr 30 09:04:17 2003
> @@ -256,6 +256,17 @@
>  	if (SCpnt != NULL) {
>  
>  		/*
> +		 * This is a work around for a case where this Scsi_Cmnd
> +		 * may have been through the busy retry paths already. We
> +		 * clear the special flag and try to restore the
> +		 * read/write request cmd value.
> +		 */
> +		if (SCpnt->request.cmd == SPECIAL)
> +			SCpnt->request.cmd = 
> +				(SCpnt->sc_data_direction ==
> +				 SCSI_DATA_WRITE) ? WRITE : READ;
> +
> +		/*
>  		 * For some reason, we are not done with this request.
>  		 * This happens for I/O errors in the middle of the request,
>  		 * in which case we need to request the blocks that come after

-- 
*********************************************************
*  tpepper@vato dot org             * Venimus, Vidimus, *
*  http://www.vato.org/~tpepper     * Dolavimus         *
*********************************************************

Re: Bad reactions to QUEUE FULL

[Mike Anderson]

You should not need the previous patch you posted.

As James stated I would be suspicious about only restoring this one field
in the command.

The side effect of the work around patch is that sd_init_command (if you
where doing IO to sd) will be called again on the command and
sc_data_direction will get set.

Tim Pepper [tpepper@vato.org] wrote:
> Thanks Mike!  I'll run with this and see what happens.  Should I do it instead of
> the one restoring the old direction?
> 
> Since it sounds LLDD related I'll bounce detailed trace info from each of
> those drivers to the vendors for comment.
> 
> Thanks!
> 
> Tim
> 
> > 
> > --- linux-2.4/drivers/scsi/scsi_lib.c		Wed Apr 30 09:04:16 2003
> > +++ linux-2.4-p/drivers/scsi/scsi_lib.c		Wed Apr 30 09:04:17 2003
> > @@ -256,6 +256,17 @@
> >  	if (SCpnt != NULL) {
> >  
> >  		/*
> > +		 * This is a work around for a case where this Scsi_Cmnd
> > +		 * may have been through the busy retry paths already. We
> > +		 * clear the special flag and try to restore the
> > +		 * read/write request cmd value.
> > +		 */
> > +		if (SCpnt->request.cmd == SPECIAL)
> > +			SCpnt->request.cmd = 
> > +				(SCpnt->sc_data_direction ==
> > +				 SCSI_DATA_WRITE) ? WRITE : READ;
> > +
> > +		/*
> >  		 * For some reason, we are not done with this request.
> >  		 * This happens for I/O errors in the middle of the request,
> >  		 * in which case we need to request the blocks that come after
> 
> -- 
> *********************************************************
> *  tpepper@vato dot org             * Venimus, Vidimus, *
> *  http://www.vato.org/~tpepper     * Dolavimus         *
> *********************************************************
> -
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
-andmike
--
Michael Anderson
andmike@us.ibm.com

Re: Bad reactions to QUEUE FULL

[Tim Pepper]

Bad news...this doesn't appear to have made much difference.  With it 3 of the
six machines were pretty much dead the morning after and the other three all have
hung IO.

On the other front I'm still waiting for word back from Emulex and Qlogic
on the detailed tracing I sent them.

> 
> --- linux-2.4/drivers/scsi/scsi_lib.c		Wed Apr 30 09:04:16 2003
> +++ linux-2.4-p/drivers/scsi/scsi_lib.c		Wed Apr 30 09:04:17 2003
> @@ -256,6 +256,17 @@
>  	if (SCpnt != NULL) {
>  
>  		/*
> +		 * This is a work around for a case where this Scsi_Cmnd
> +		 * may have been through the busy retry paths already. We
> +		 * clear the special flag and try to restore the
> +		 * read/write request cmd value.
> +		 */
> +		if (SCpnt->request.cmd == SPECIAL)
> +			SCpnt->request.cmd = 
> +				(SCpnt->sc_data_direction ==
> +				 SCSI_DATA_WRITE) ? WRITE : READ;
> +
> +		/*
>  		 * For some reason, we are not done with this request.
>  		 * This happens for I/O errors in the middle of the request,
>  		 * in which case we need to request the blocks that come after

-- 
*********************************************************
*  tpepper@vato dot org             * Venimus, Vidimus, *
*  http://www.vato.org/~tpepper     * Dolavimus         *
*********************************************************