one-line-patch against SCSI-Read-Error-BUG()

one-line-patch against SCSI-Read-Error-BUG()

[Ralf Oehler]

Hi, List

I think, I found a very simple solution for this annoying BUG().

Since at least kernel 2.4.16 there is a BUG() in pci.h,
that crashes the kernel on any attempt to read a SCSI-Sector
from an erased MO-Medium and on any attempt to read
a sector from a SCSI-disk, which returns "Read-Error".

There seems to be a thinko in the corresponding code, which 
does not take into account the case where a SCSI-READ
does not return any data because of a "sense code: read error"
or a "sense code: blank sector".

I simply commented out this BUG() statement (see below)
and everything worked well from there on. The BUG()
seems to be inadequate.

Please could you check if I'm right and apply this change to the
current kernel? Again I want to stress that in my
oppinion it is dangerous for the stability of a production
machine if it crashes on the next SCSI-Read-Error it sees.
Most SCSI-Hardware today shows very few read errors, but 
please let's not rely on that. That'd be playing vabanque.


If there are tests to do, I can offer my time and hardware
(SCSI-MO drives and media with various sector sizes) to
test and in case to provide stack traces.

Regards,
        Ralf




include/asm/pci.h:
static inline int pci_map_sg(struct pci_dev *hwdev, struct scatterlist *sg,
                             int nents, int direction)
{
        int i;

        if (direction == PCI_DMA_NONE)
                BUG();
 
        /*
         * temporary 2.4 hack
         */
        for (i = 0; i < nents; i++ ) {
                if (sg[i].address && sg[i].page)
                        BUG();
    -------->   else if (!sg[i].address && !sg[i].page)
    -------->         BUG();
 
                if (sg[i].address)
                        sg[i].dma_address = virt_to_bus(sg[i].address);
                else
                        sg[i].dma_address = page_to_bus(sg[i].page) + sg[i].offset;
        }
 
        flush_write_buffers();
        return nents;
}




 --------------------------------------------------------------------------
|  Ralf Oehler                          
|                                       
|  GDA - Gesellschaft fuer Digitale                              _/
|        Archivierungstechnik mbH & CoKG                        _/
|  Ein Unternehmen der Bechtle AG               #/_/_/_/ _/_/_/_/ _/_/_/_/
|                                              _/    _/ _/    _/       _/
|  E-Mail:      R.Oehler@GDAmbH.com           _/    _/ _/    _/ _/    _/
|  Tel.:        +49 6182-9271-23             _/_/_/_/ _/_/_/#/ _/_/_/#/
|  Fax.:        +49 6182-25035                    _/
|  Mail:        GDA, Bensbruchstraße 11,   _/_/_/_/
|               D-63533 Mainhausen      
|  HTTP:        www.GDAmbH.com         
 --------------------------------------------------------------------------

time is a funny concept

Re: one-line-patch against SCSI-Read-Error-BUG()

[Jens Axboe]

On Tue, Feb 05 2002, Ralf Oehler wrote:
> Hi, List
> 
> I think, I found a very simple solution for this annoying BUG().

You fail to understand that the BUG triggering indicates that their is a
BUG _somewhere_ -- the triggered BUG is not the bug itself, of course,
that would be stupid :-)

> Since at least kernel 2.4.16 there is a BUG() in pci.h,
> that crashes the kernel on any attempt to read a SCSI-Sector
> from an erased MO-Medium and on any attempt to read
> a sector from a SCSI-disk, which returns "Read-Error".
> 
> There seems to be a thinko in the corresponding code, which 
> does not take into account the case where a SCSI-READ
> does not return any data because of a "sense code: read error"
> or a "sense code: blank sector".
> 
> I simply commented out this BUG() statement (see below)
> and everything worked well from there on. The BUG()
> seems to be inadequate.

The BUG is dangerous, because it means mapping DMA to a 0 address (plus
offset). Naturally there is no way in hell your "fix" will be applied,
since it a pretty bad bandaid.

A safer solution for you right now would be to just terminate the
mapping when you encounter this. So something ala

	if (!sg[i].page && !sg[i].address)
		return i;

That's not a bug fix either, but at least it's safer than your version.
Stock kernel won't be patched until the real problem is found, though.

-- 
Jens Axboe

Re: one-line-patch against SCSI-Read-Error-BUG()

[Momchil Velikov]

>>>>> "Jens" == Jens Axboe <axboe@suse.de> writes:

Jens> On Tue, Feb 05 2002, Ralf Oehler wrote:
>> Hi, List
>> 
>> I think, I found a very simple solution for this annoying BUG().

Jens> You fail to understand that the BUG triggering indicates that their is a
Jens> BUG _somewhere_ -- the triggered BUG is not the bug itself, of course,
Jens> that would be stupid :-)

Erm, having a BUG() somewhere can be a bug by itself ;)

I think that's what he meant (regardless if he was right or not). 

Re: one-line-patch against SCSI-Read-Error-BUG()

[Jens Axboe]

On Tue, Feb 05 2002, Momchil Velikov wrote:
> >>>>> "Jens" == Jens Axboe <axboe@suse.de> writes:
> 
> Jens> On Tue, Feb 05 2002, Ralf Oehler wrote:
> >> Hi, List
> >> 
> >> I think, I found a very simple solution for this annoying BUG().
> 
> Jens> You fail to understand that the BUG triggering indicates that their is a
> Jens> BUG _somewhere_ -- the triggered BUG is not the bug itself, of course,
> Jens> that would be stupid :-)
> 
> Erm, having a BUG() somewhere can be a bug by itself ;)
> 
> I think that's what he meant (regardless if he was right or not). 

Of course it can, any statement can be a bug. That's hardly the point
:-)

-- 
Jens Axboe

Re: one-line-patch against SCSI-Read-Error-BUG()

[Douglas Gilbert]

Jens Axboe wrote:
> 
> On Tue, Feb 05 2002, Ralf Oehler wrote:
> > Hi, List
> >
> > I think, I found a very simple solution for this annoying BUG().
> 
> You fail to understand that the BUG triggering indicates that their is a
> BUG _somewhere_ -- the triggered BUG is not the bug itself, of course,
> that would be stupid :-)
> 
> > Since at least kernel 2.4.16 there is a BUG() in pci.h,
> > that crashes the kernel on any attempt to read a SCSI-Sector
> > from an erased MO-Medium and on any attempt to read
> > a sector from a SCSI-disk, which returns "Read-Error".
> >
> > There seems to be a thinko in the corresponding code, which
> > does not take into account the case where a SCSI-READ
> > does not return any data because of a "sense code: read error"
> > or a "sense code: blank sector".
> >
> > I simply commented out this BUG() statement (see below)
> > and everything worked well from there on. The BUG()
> > seems to be inadequate.
> 
> The BUG is dangerous, because it means mapping DMA to a 0 address (plus
> offset). Naturally there is no way in hell your "fix" will be applied,
> since it a pretty bad bandaid.
> 
> A safer solution for you right now would be to just terminate the
> mapping when you encounter this. So something ala
> 
>         if (!sg[i].page && !sg[i].address)
>                 return i;
> 
> That's not a bug fix either, but at least it's safer than your version.
> Stock kernel won't be patched until the real problem is found, though.

Jens,
The sg driver was tripping that BUG() and I have submitted
a patch for that. However Ralf probably wasn't using sg
to read his MO disk.

There are several uses of "sgpnt[i].address" in scsi_lib.c that
you might like to look at. Can't see any other uses in sd, sr
or the mid level. Another possibility is a missing memset or
explicit NULL initialisation on a scatterlist array.

Doug Gilbert

Re: one-line-patch against SCSI-Read-Error-BUG()

[Alan Cox]

> Since at least kernel 2.4.16 there is a BUG() in pci.h,
> that crashes the kernel on any attempt to read a SCSI-Sector
> from an erased MO-Medium and on any attempt to read
> a sector from a SCSI-disk, which returns "Read-Error".

Adaptec aic7xxx card ?

Re: one-line-patch against SCSI-Read-Error-BUG()

[Ralf Oehler]

On 05-Feb-2002 Alan Cox wrote:
|   > Since at least kernel 2.4.16 there is a BUG() in pci.h,
|   > that crashes the kernel on any attempt to read a SCSI-Sector
|   > from an erased MO-Medium and on any attempt to read
|   > a sector from a SCSI-disk, which returns "Read-Error".
|    
|    Adaptec aic7xxx card ?

Yes. And I only have Adaptecs...    

 --------------------------------------------------------------------------
|  Ralf Oehler                          
|                                       
|  GDA - Gesellschaft fuer Digitale                              _/
|        Archivierungstechnik mbH & CoKG                        _/
|  Ein Unternehmen der Bechtle AG               #/_/_/_/ _/_/_/_/ _/_/_/_/
|                                              _/    _/ _/    _/       _/
|  E-Mail:      R.Oehler@GDAmbH.com           _/    _/ _/    _/ _/    _/
|  Tel.:        +49 6182-9271-23             _/_/_/_/ _/_/_/#/ _/_/_/#/
|  Fax.:        +49 6182-25035                    _/
|  Mail:        GDA, Bensbruchstraße 11,   _/_/_/_/
|               D-63533 Mainhausen      
|  HTTP:        www.GDAmbH.com         
 --------------------------------------------------------------------------

time is a funny concept

Re: one-line-patch against SCSI-Read-Error-BUG()

[Helge Hafting]

Alan Cox wrote:
> 
> > Since at least kernel 2.4.16 there is a BUG() in pci.h,
> > that crashes the kernel on any attempt to read a SCSI-Sector
> > from an erased MO-Medium and on any attempt to read
> > a sector from a SCSI-disk, which returns "Read-Error".
> 
> Adaptec aic7xxx card ?

You don't need an adaptec to BUG on scsi read errors.

I have a tekram adapter using the new SYM53C8XX version 2 driver.
One of my quantum atlas IV have a few bad sectors.  Reading
the file (ext2 fs on top of raid0) tends to merely cause error messages.
badblocks also runs fine.  But fsck -c triggers the BUG.

Helge Hafting

Re: one-line-patch against SCSI-Read-Error-BUG()

[James Stevenson]

> Hi, List
>
> I think, I found a very simple solution for this annoying BUG().
>
> Since at least kernel 2.4.16 there is a BUG() in pci.h,
> that crashes the kernel on any attempt to read a SCSI-Sector
> from an erased MO-Medium and on any attempt to read
> a sector from a SCSI-disk, which returns "Read-Error".
>
> There seems to be a thinko in the corresponding code, which
> does not take into account the case where a SCSI-READ
> does not return any data because of a "sense code: read error"
> or a "sense code: blank sector".
>

would this also happen with the ide-scsi driver then
and would this explain why i see panic's on when reading
cd's ?


    James

Re: one-line-patch against SCSI-Read-Error-BUG()

[Ralf Oehler]

On 05-Feb-2002 James Stevenson wrote:
|   > Hi, List
|   >
|   > I think, I found a very simple solution for this annoying BUG().
|   >
|   > Since at least kernel 2.4.16 there is a BUG() in pci.h,
|   > that crashes the kernel on any attempt to read a SCSI-Sector
|   > from an erased MO-Medium and on any attempt to read
|   > a sector from a SCSI-disk, which returns "Read-Error".
|   >
|   > There seems to be a thinko in the corresponding code, which
|   > does not take into account the case where a SCSI-READ
|   > does not return any data because of a "sense code: read error"
|   > or a "sense code: blank sector".
|   >
|    
|    would this also happen with the ide-scsi driver then
|    and would this explain why i see panic's on when reading
|    cd's ?
|    
 I don't know (yet), but it`s well possible.



 --------------------------------------------------------------------------
|  Ralf Oehler                          
|                                       
|  GDA - Gesellschaft fuer Digitale                              _/
|        Archivierungstechnik mbH & CoKG                        _/
|  Ein Unternehmen der Bechtle AG               #/_/_/_/ _/_/_/_/ _/_/_/_/
|                                              _/    _/ _/    _/       _/
|  E-Mail:      R.Oehler@GDAmbH.com           _/    _/ _/    _/ _/    _/
|  Tel.:        +49 6182-9271-23             _/_/_/_/ _/_/_/#/ _/_/_/#/
|  Fax.:        +49 6182-25035                    _/
|  Mail:        GDA, Bensbruchstraße 11,   _/_/_/_/
|               D-63533 Mainhausen      
|  HTTP:        www.GDAmbH.com         
 --------------------------------------------------------------------------

time is a funny concept

Re: one-line-patch against SCSI-Read-Error-BUG()

[Patrick Mansfield]

On Tue, Feb 05, 2002 at 03:32:10PM +0100, Ralf Oehler wrote:
> Hi, List
> 
> I think, I found a very simple solution for this annoying BUG().
> 
> Since at least kernel 2.4.16 there is a BUG() in pci.h,
> that crashes the kernel on any attempt to read a SCSI-Sector
> from an erased MO-Medium and on any attempt to read
> a sector from a SCSI-disk, which returns "Read-Error".
> 
> There seems to be a thinko in the corresponding code, which 
> does not take into account the case where a SCSI-READ
> does not return any data because of a "sense code: read error"
> or a "sense code: blank sector".

> Regards,
>         Ralf

Ralf -

A retried IO for a host busy (queuecommand returns 1) or SCSI queue full
case calls scsi_mlqueue_insert; it eventually calls __scsi_insert_special,
where we then set:

    rq->cmd = SPECIAL

The IO is then resent, and if it completes, it looks like everything will
complete OK.

But, if we then get a check condition for the same command, sd.c will call
scsi_io_completion - it nulls out some buffers, including request_buffer,
and then it requeues the command without modifying rq->cmd.

scsi_request_fn() sees that we now have cmd == SPECIAL, and a valid
SCpnt, and does not re-init the IO - this means request_buffer is still
NULL.

I can't tell from your earlier oops stack where exactly you are in the aic
code, but it could be a dereference of request_buffer (stored in cur_seg),
but I would think you would have seen a NULL pointer dereference rather
than hitting the BUG().

I don't know what conditions on your system might lead to a host busy or
queue full, but, it seems possible that we could have a queue full or host
busy immediately followed by a check condition.

In 2.4, I don't know how this can be fixed without adding another field
in Scsi_Cmnd, we could just try setting cmd back to not be SPECIAL, but
the original value is not saved - you could try such a hack and see if
it still oopses. You could also add a check to pci_map_sg to print
out the value of sg when the BUG() is hit.

In 2.5, we might be able to clear REQ_CMD, then set REQ_SPECIAL (in
__scsi_insert_special), and then clear REQ_SPECIAL and set REQ_CMD
in scsi_queue_next_request (when SCpnt != NULL).

You can see if you are hitting the busy cases by turning on SCSI mlcomplete
(to see queue full retries) and mlqueue logging (to see the host busy retries),
and looking for log messages. (Turning on scsi logging with syslogd and/or
klogd? running can flood your system with log messages.)

It also looks the race condition checked for in scsi_mlqueue_insert
is faulty (device_busy or host_busy == 0, but we are just about to
decrement them, so they can't ever be 0, it should compare them to 1),
but that would just hang your IO's (on queue full no matter what the
comparison it could hang IO for clustered systems sharing SCSI devices).

-- Patrick Mansfield

Re: one-line-patch against SCSI-Read-Error-BUG()

[Douglas Gilbert]

Patrick Mansfield wrote:
> 
> On Tue, Feb 05, 2002 at 03:32:10PM +0100, Ralf Oehler wrote:
> > Hi, List
> >
> > I think, I found a very simple solution for this annoying BUG().
> >
> > Since at least kernel 2.4.16 there is a BUG() in pci.h,
> > that crashes the kernel on any attempt to read a SCSI-Sector
> > from an erased MO-Medium and on any attempt to read
> > a sector from a SCSI-disk, which returns "Read-Error".
> >
> > There seems to be a thinko in the corresponding code, which
> > does not take into account the case where a SCSI-READ
> > does not return any data because of a "sense code: read error"
> > or a "sense code: blank sector".
> 
> > Regards,
> >         Ralf
> 
> Ralf -
> 
> A retried IO for a host busy (queuecommand returns 1) or SCSI queue full
> case calls scsi_mlqueue_insert; it eventually calls __scsi_insert_special,
> where we then set:
> 
>     rq->cmd = SPECIAL
> 
> The IO is then resent, and if it completes, it looks like everything will
> complete OK.
> 
> But, if we then get a check condition for the same command, sd.c will call
> scsi_io_completion - it nulls out some buffers, including request_buffer,
> and then it requeues the command without modifying rq->cmd.
> 
> scsi_request_fn() sees that we now have cmd == SPECIAL, and a valid
> SCpnt, and does not re-init the IO - this means request_buffer is still
> NULL.
> 
> I can't tell from your earlier oops stack where exactly you are in the aic
> code, but it could be a dereference of request_buffer (stored in cur_seg),
> but I would think you would have seen a NULL pointer dereference rather
> than hitting the BUG().
> 
> I don't know what conditions on your system might lead to a host busy or
> queue full, but, it seems possible that we could have a queue full or host
> busy immediately followed by a check condition.
> 
> In 2.4, I don't know how this can be fixed without adding another field
> in Scsi_Cmnd, we could just try setting cmd back to not be SPECIAL, but
> the original value is not saved - you could try such a hack and see if
> it still oopses. You could also add a check to pci_map_sg to print
> out the value of sg when the BUG() is hit.
> 
> In 2.5, we might be able to clear REQ_CMD, then set REQ_SPECIAL (in
> __scsi_insert_special), and then clear REQ_SPECIAL and set REQ_CMD
> in scsi_queue_next_request (when SCpnt != NULL).
> 
> You can see if you are hitting the busy cases by turning on SCSI mlcomplete
> (to see queue full retries) and mlqueue logging (to see the host busy retries),
> and looking for log messages. (Turning on scsi logging with syslogd and/or
> klogd? running can flood your system with log messages.)
> 
> It also looks the race condition checked for in scsi_mlqueue_insert
> is faulty (device_busy or host_busy == 0, but we are just about to
> decrement them, so they can't ever be 0, it should compare them to 1),
> but that would just hang your IO's (on queue full no matter what the
> comparison it could hang IO for clustered systems sharing SCSI devices).

My investigations show an infinite loop commences on the
adapter driver's queue_command() when a MEDIUM_ERROR is
reported. I have added an option to scsi_debug to simulate
a MEDIUM ERROR at a fixed address (0x1234). If I use sg
to read the scsi_debug ram disk it is well behaved,
stopping in an orderly fashion in sg_dd at the bad block.

However if that bad block is read via sd then the scsi_debug
driver's queue_command() entry point is called indefinitely
trying to reread the range that contains the bad block.


A new scsi_debug driver, version 1.58 has been placed
on http://www.torque.net/sg/sdebug.html
Load the scsi_debug module with "scsi_debug_opts=3" to
get debug output and the medium_error at block 0x1234 .

So the problem has been reported on these adapter
drivers: aic7xxx, sym53c8xx and scsi_debug . It is
interesting that sg doesn't have problems, probably
because it sets "retries" to 1 (i.e. don't retry).
Looks like the error handling logic is broken.

Doug Gilbert

Re: one-line-patch against SCSI-Read-Error-BUG()

[Jens Axboe]

On Wed, Feb 06 2002, Patrick Mansfield wrote:
> On Tue, Feb 05, 2002 at 03:32:10PM +0100, Ralf Oehler wrote:
> > Hi, List
> > 
> > I think, I found a very simple solution for this annoying BUG().
> > 
> > Since at least kernel 2.4.16 there is a BUG() in pci.h,
> > that crashes the kernel on any attempt to read a SCSI-Sector
> > from an erased MO-Medium and on any attempt to read
> > a sector from a SCSI-disk, which returns "Read-Error".
> > 
> > There seems to be a thinko in the corresponding code, which 
> > does not take into account the case where a SCSI-READ
> > does not return any data because of a "sense code: read error"
> > or a "sense code: blank sector".
> 
> > Regards,
> >         Ralf

Sorry guys, I should have written a followup yesterday. I did solved
Ralf's problem for him. It was due to some paths not agreeing on when to
use clustering for a request and when not to -- for some historical
reason, MO drives have clustering disabled. The easy (and correct
solution, IMO) is to just allow clustering if the host permits it. So
the patch to solve the problem is as follows, also sent to Marcelo
yesterday.

--- /opt/kernel/linux-2.4.18-pre8/drivers/scsi/scsi_merge.c	Thu Oct 25 23:05:31 2001
+++ drivers/scsi/scsi_merge.c	Wed Feb  6 10:49:50 2002
@@ -150,14 +150,7 @@
 	panic("DMA pool exhausted");
 }
 
-/*
- * FIXME(eric) - the original disk code disabled clustering for MOD
- * devices.  I have no idea why we thought this was a good idea - my
- * guess is that it was an attempt to limit the size of requests to MOD
- * devices.
- */
-#define CLUSTERABLE_DEVICE(SH,SD) (SH->use_clustering && \
-				   SD->type != TYPE_MOD)
+#define CLUSTERABLE_DEVICE(SH,SD) (SH->use_clustering)
 
 /*
  * This entire source file deals with the new queueing code.

-- 
Jens Axboe