regression: disk error loop (panic?) ide_do_rw_disk-bad:

regression: disk error loop (panic?) ide_do_rw_disk-bad:

[Giacomo Catenazzi]

Hello,

last git changes to git give me the following error (repead very quickly):

sector 14657019, nr/cmr 0/0
bio f7b59280, biotail f7b59280, buffer 0000000, date 0000000, len 36
ide_do_rw_disk-bad command: dev hda: type 2, flags 104c8

[note: manually copied]


I tried bisect:


cate@catee:~/kernel/6,v2.6/linux-2.6$ git bisect log
# bad: [a5fcaa210626a79465321e344c91a6a7dc3881fa] Merge branch
'drm-patches' of
ssh://master.kernel.org/pub/scm/linux/kernel/git/airlied/drm-2.6
# good: [8f41958bdd577731f7411c9605cfaa9db6766809] Merge
git://git.infradead.org/battery-2.6
git-bisect start 'a5fcaa21' '8f41958b'
# good: [02b2318e07f98a7cdf7089a4457a8d62424aa824] Merge branch 'master'
of master.kernel.org:/pub/scm/linux/kernel/git/davem/sparc-2.6
git-bisect good 02b2318e07f98a7cdf7089a4457a8d62424aa824
# good: [f716a425c15ebadf60286cd4fb60d1d6f46e3cf9] [POWERPC] VIOTAPE:
Use designated initializers for fops member structures.
git-bisect good f716a425c15ebadf60286cd4fb60d1d6f46e3cf9
# good: [14dc5249728ff699b1ca4dac01ad416a350a147a] Merge branch
'for-linus' of git://git.kernel.dk/data/git/linux-2.6-block
git-bisect good 14dc5249728ff699b1ca4dac01ad416a350a147a
# good: [c2dc1ad582196208a2f990eb0230eb922046c684] Merge branch
'drm-patches' of
ssh://master.kernel.org/pub/scm/linux/kernel/git/airlied/drm-2.6
git-bisect good c2dc1ad582196208a2f990eb0230eb922046c684
# good: [c5e3ae8823693b260ce1f217adca8add1bc0b3de] forcedeth bug fix:
realtek phy
git-bisect good c5e3ae8823693b260ce1f217adca8add1bc0b3de
# good: [70584578ab3e940ac9d7820f268f9adc9884e407] [POWERPC] Check for
NULL ppc_md.init_IRQ() before calling
git-bisect good 70584578ab3e940ac9d7820f268f9adc9884e407
# good: [bf22f6fe2d72b4d7e9035be8ceb340414cf490e3] Merge branch
'for-2.6.23' into merge
git-bisect good bf22f6fe2d72b4d7e9035be8ceb340414cf490e3
# bad: [f798634d806615bee27d1b83479034087a02aa0f] [SERIAL] SUNHV: Fix
jerky console on LDOM guests.
git-bisect bad f798634d806615bee27d1b83479034087a02aa0f
# bad: [29417b899a77aaba1c060f5e123db4f50006f58a] Make BLK_DEV_BSG
depend strictly on SCSI=y
git-bisect bad 29417b899a77aaba1c060f5e123db4f50006f58a

but last changeset don't compile (and it doesn't seem relevant).

ciao
	cate

Re: regression: disk error loop (panic?) ide_do_rw_disk-bad:

[Michal Piotrowski]

Hi,

On 17/07/07, Giacomo Catenazzi <cate@cateee.net> wrote:
> Hello,
>
> last git changes to git give me the following error (repead very quickly):
>
> sector 14657019, nr/cmr 0/0
> bio f7b59280, biotail f7b59280, buffer 0000000, date 0000000, len 36
> ide_do_rw_disk-bad command: dev hda: type 2, flags 104c8
>
> [note: manually copied]
>
>
> I tried bisect:
>
>
> cate@catee:~/kernel/6,v2.6/linux-2.6$ git bisect log
> # bad: [a5fcaa210626a79465321e344c91a6a7dc3881fa] Merge branch
> 'drm-patches' of
> ssh://master.kernel.org/pub/scm/linux/kernel/git/airlied/drm-2.6
> # good: [8f41958bdd577731f7411c9605cfaa9db6766809] Merge
> git://git.infradead.org/battery-2.6
> git-bisect start 'a5fcaa21' '8f41958b'
> # good: [02b2318e07f98a7cdf7089a4457a8d62424aa824] Merge branch 'master'
> of master.kernel.org:/pub/scm/linux/kernel/git/davem/sparc-2.6
> git-bisect good 02b2318e07f98a7cdf7089a4457a8d62424aa824
> # good: [f716a425c15ebadf60286cd4fb60d1d6f46e3cf9] [POWERPC] VIOTAPE:
> Use designated initializers for fops member structures.
> git-bisect good f716a425c15ebadf60286cd4fb60d1d6f46e3cf9
> # good: [14dc5249728ff699b1ca4dac01ad416a350a147a] Merge branch
> 'for-linus' of git://git.kernel.dk/data/git/linux-2.6-block
> git-bisect good 14dc5249728ff699b1ca4dac01ad416a350a147a
> # good: [c2dc1ad582196208a2f990eb0230eb922046c684] Merge branch
> 'drm-patches' of
> ssh://master.kernel.org/pub/scm/linux/kernel/git/airlied/drm-2.6
> git-bisect good c2dc1ad582196208a2f990eb0230eb922046c684
> # good: [c5e3ae8823693b260ce1f217adca8add1bc0b3de] forcedeth bug fix:
> realtek phy
> git-bisect good c5e3ae8823693b260ce1f217adca8add1bc0b3de
> # good: [70584578ab3e940ac9d7820f268f9adc9884e407] [POWERPC] Check for
> NULL ppc_md.init_IRQ() before calling
> git-bisect good 70584578ab3e940ac9d7820f268f9adc9884e407
> # good: [bf22f6fe2d72b4d7e9035be8ceb340414cf490e3] Merge branch
> 'for-2.6.23' into merge
> git-bisect good bf22f6fe2d72b4d7e9035be8ceb340414cf490e3
> # bad: [f798634d806615bee27d1b83479034087a02aa0f] [SERIAL] SUNHV: Fix
> jerky console on LDOM guests.
> git-bisect bad f798634d806615bee27d1b83479034087a02aa0f
> # bad: [29417b899a77aaba1c060f5e123db4f50006f58a] Make BLK_DEV_BSG
> depend strictly on SCSI=y
> git-bisect bad 29417b899a77aaba1c060f5e123db4f50006f58a
>
> but last changeset don't compile (and it doesn't seem relevant).

Please read Linux Kernel Tester's Guide - 4.5 Binary searching with
the help of git-bisect
"git-reset --hard" hint.

http://www.stardust.webpages.pl/files/handbook/handbook-en-0.3-rc1.pdf

>
> ciao
>         cate

Regards,
Michal

-- 
LOG
http://www.stardust.webpages.pl/log/

Re: regression: disk error loop (panic?) ide_do_rw_disk-bad:

[Bartlomiej Zolnierkiewicz]

Hi,

On Tuesday 17 July 2007, Giacomo Catenazzi wrote:
> Hello,
> 
> last git changes to git give me the following error (repead very quickly):
> 
> sector 14657019, nr/cmr 0/0
> bio f7b59280, biotail f7b59280, buffer 0000000, date 0000000, len 36
> ide_do_rw_disk-bad command: dev hda: type 2, flags 104c8

Heh, I've just finished describing the source of the issue:

http://lkml.org/lkml/2007/7/17/487

ide-disk driver and type 2 (REQ_TYPE_BLOCK_PC) requests don't mix well

Probably some dumb application is sending packet commands without
checking the device type...

> [note: manually copied]
> 
> 
> I tried bisect:
> 
> 
> cate@catee:~/kernel/6,v2.6/linux-2.6$ git bisect log
> # bad: [a5fcaa210626a79465321e344c91a6a7dc3881fa] Merge branch
> 'drm-patches' of
> ssh://master.kernel.org/pub/scm/linux/kernel/git/airlied/drm-2.6
> # good: [8f41958bdd577731f7411c9605cfaa9db6766809] Merge
> git://git.infradead.org/battery-2.6
> git-bisect start 'a5fcaa21' '8f41958b'
> # good: [02b2318e07f98a7cdf7089a4457a8d62424aa824] Merge branch 'master'
> of master.kernel.org:/pub/scm/linux/kernel/git/davem/sparc-2.6
> git-bisect good 02b2318e07f98a7cdf7089a4457a8d62424aa824
> # good: [f716a425c15ebadf60286cd4fb60d1d6f46e3cf9] [POWERPC] VIOTAPE:
> Use designated initializers for fops member structures.
> git-bisect good f716a425c15ebadf60286cd4fb60d1d6f46e3cf9
> # good: [14dc5249728ff699b1ca4dac01ad416a350a147a] Merge branch
> 'for-linus' of git://git.kernel.dk/data/git/linux-2.6-block
> git-bisect good 14dc5249728ff699b1ca4dac01ad416a350a147a
> # good: [c2dc1ad582196208a2f990eb0230eb922046c684] Merge branch
> 'drm-patches' of
> ssh://master.kernel.org/pub/scm/linux/kernel/git/airlied/drm-2.6
> git-bisect good c2dc1ad582196208a2f990eb0230eb922046c684
> # good: [c5e3ae8823693b260ce1f217adca8add1bc0b3de] forcedeth bug fix:
> realtek phy
> git-bisect good c5e3ae8823693b260ce1f217adca8add1bc0b3de
> # good: [70584578ab3e940ac9d7820f268f9adc9884e407] [POWERPC] Check for
> NULL ppc_md.init_IRQ() before calling
> git-bisect good 70584578ab3e940ac9d7820f268f9adc9884e407
> # good: [bf22f6fe2d72b4d7e9035be8ceb340414cf490e3] Merge branch
> 'for-2.6.23' into merge
> git-bisect good bf22f6fe2d72b4d7e9035be8ceb340414cf490e3
> # bad: [f798634d806615bee27d1b83479034087a02aa0f] [SERIAL] SUNHV: Fix
> jerky console on LDOM guests.
> git-bisect bad f798634d806615bee27d1b83479034087a02aa0f
> # bad: [29417b899a77aaba1c060f5e123db4f50006f58a] Make BLK_DEV_BSG
> depend strictly on SCSI=y
> git-bisect bad 29417b899a77aaba1c060f5e123db4f50006f58a
> 
> but last changeset don't compile (and it doesn't seem relevant).

commit 3d6392cfbd7dc11f23058e3493683afab4ac13a3
Author: Jens Axboe <jens.axboe@oracle.com>
Date:   Mon Jul 9 12:38:05 2007 +0200

    bsg: support for full generic block layer SG v3

    Signed-off-by: Jens Axboe <jens.axboe@oracle.com>

is the guilty one

Thanks,
Bart

Re: regression: disk error loop (panic?) ide_do_rw_disk-bad:

[Linus Torvalds]

On Tue, 17 Jul 2007, Bartlomiej Zolnierkiewicz wrote:
> 
> ide-disk driver and type 2 (REQ_TYPE_BLOCK_PC) requests don't mix well
> 
> Probably some dumb application is sending packet commands without
> checking the device type...

Ok, we should definitely try to just translate the things, and instead of 
having user apps that have to know about the (generally not very 
interesting) differences between IDE and SCSI command set, and when the 
IDE driver gets a SCSI request (whether from the new generic SG layer or 
obviously the older SCSI-ioctl layer) it should "just work".

So I object to that "dumb application" statement. It's the kernel that has 
traditionally been dumb in not smoothing over the differences between 
devices well enough.

We shouldn't _need_ to have applications care. They should be able to just 
use regular SCSI commands, and if the device cannot handle a 10-byte read 
command, the kernel should have translated that into a 6-byte one (for 
example) rather than the application having to know about idiotic small 
differences like that.

That said, I dunno how to fix this particular one, and the IDE driver is 
singularly unhelpful in actually talking about *what* the command tried to 
be.

			Linus

Re: regression: disk error loop (panic?) ide_do_rw_disk-bad:

[Linus Torvalds]

On Wed, 18 Jul 2007, Bartlomiej Zolnierkiewicz wrote:
> 
> The new generic SG layer is CONFIG_SCSI=y "generic" in the current tree.

Yeah. I know, I already talked to Jens about it - in order to be generic, 
the BSG stuff really does end up having to be able to stand on its own, 
since not everybody wants/needs the whole SCSI layer.

> > obviously the older SCSI-ioctl layer) it should "just work".
> 
> Agreed but IDE driver has never claimed to have full SAT layer and
> full SCSI-ioctl layer support has been provided only for ide-cd.

I agree, and understand why it happened, but hope that maybe it can be 
improved in the future, or at least the error returned earlier so that the 
request will never even hit the ATA device at all if the IDE layer cannot 
then handle it.

> For now it should be sufficient to revert ide.c chunks of
> 
> commit 3d6392cfbd7dc11f23058e3493683afab4ac13a3

Jens?

		Linus

Re: regression: disk error loop (panic?) ide_do_rw_disk-bad:

[Bartlomiej Zolnierkiewicz]

On Tuesday 17 July 2007, Linus Torvalds wrote:
> 
> On Tue, 17 Jul 2007, Bartlomiej Zolnierkiewicz wrote:
> > 
> > ide-disk driver and type 2 (REQ_TYPE_BLOCK_PC) requests don't mix well
> > 
> > Probably some dumb application is sending packet commands without
> > checking the device type...

My original mail contained link to mail explaining the source of the issue
and also commit number introducing the problem...  Both have been stripped
from the reply thus valuable context is lost.

> Ok, we should definitely try to just translate the things, and instead of 
> having user apps that have to know about the (generally not very 
> interesting) differences between IDE and SCSI command set, and when the 
> IDE driver gets a SCSI request (whether from the new generic SG layer or 

The new generic SG layer is CONFIG_SCSI=y "generic" in the current tree.

James has a patch to fix it but IDE subsystem (and probably not only it)
still requires addition of struct class devices to be able to use bsg.

> obviously the older SCSI-ioctl layer) it should "just work".

Agreed but IDE driver has never claimed to have full SAT layer and
full SCSI-ioctl layer support has been provided only for ide-cd.

Full SAT could be done though, possibly by reusing libata-scsi.c.

> So I object to that "dumb application" statement. It's the kernel that has 
> traditionally been dumb in not smoothing over the differences between 
> devices well enough.
> 
> We shouldn't _need_ to have applications care. They should be able to just 
> use regular SCSI commands, and if the device cannot handle a 10-byte read 
> command, the kernel should have translated that into a 6-byte one (for 
> example) rather than the application having to know about idiotic small 
> differences like that.
> 
> That said, I dunno how to fix this particular one, and the IDE driver is 

For now it should be sufficient to revert ide.c chunks of

commit 3d6392cfbd7dc11f23058e3493683afab4ac13a3
Author: Jens Axboe <jens.axboe@oracle.com>
Date:   Mon Jul 9 12:38:05 2007 +0200

    bsg: support for full generic block layer SG v3

    Signed-off-by: Jens Axboe <jens.axboe@oracle.com>

which are completely unrelated to bsg and which never got posted for review.

> singularly unhelpful in actually talking about *what* the command tried to 
> be.

Agreed, debugging info needs some love, being worked on.

Thanks,
Bart

Re: regression: disk error loop (panic?) ide_do_rw_disk-bad:

[Linus Torvalds]

[ Got back to this after thinking about it some more .. ]

On Wed, 18 Jul 2007, Bartlomiej Zolnierkiewicz wrote:
> 
> For now it should be sufficient to revert ide.c chunks of
> 
> commit 3d6392cfbd7dc11f23058e3493683afab4ac13a3

Come to think about this, doesn't the old code trigger this problem too, 
if you use CDROMEJECT or CDROMCLOSETRAY on a IDE disk (as opposed to an 
IDE CD-ROM)?

But I'll revert that part of it for now, and at least this issue won't be 
anything new. In the meantime, BSG has also gotten some other fixes, most 
notably about them the fact that it's not marked default any more, and 
properly warning about its EXPERIMENTAL status..

		Linus

Re: regression: disk error loop (panic?) ide_do_rw_disk-bad:

[Bartlomiej Zolnierkiewicz]

On Wednesday 18 July 2007, Linus Torvalds wrote:
> 
> On Wed, 18 Jul 2007, Bartlomiej Zolnierkiewicz wrote:
> > 
> > The new generic SG layer is CONFIG_SCSI=y "generic" in the current tree.
> 
> Yeah. I know, I already talked to Jens about it - in order to be generic, 
> the BSG stuff really does end up having to be able to stand on its own, 
> since not everybody wants/needs the whole SCSI layer.

bsg looks really promising and it would be great if we can use it also
for IDE driver.

> > > obviously the older SCSI-ioctl layer) it should "just work".
> > 
> > Agreed but IDE driver has never claimed to have full SAT layer and
> > full SCSI-ioctl layer support has been provided only for ide-cd.
> 
> I agree, and understand why it happened, but hope that maybe it can be 
> improved in the future, or at least the error returned earlier so that the 
> request will never even hit the ATA device at all if the IDE layer cannot 
> then handle it.

This is _exactly_ how these requests were handled before commit
3d6392cfbd7dc11f23058e3493683afab4ac13a3.

Reverting ide.c chunks is a safe solution for now (no SG_IO and friends
for ide-floppy but this is how it was before this commit).

Thanks,
Bart

Re: regression: disk error loop (panic?) ide_do_rw_disk-bad:

[Jeff Garzik]

Bartlomiej Zolnierkiewicz wrote:
> On Wednesday 18 July 2007, Linus Torvalds wrote:
>> On Wed, 18 Jul 2007, Bartlomiej Zolnierkiewicz wrote:
>>> The new generic SG layer is CONFIG_SCSI=y "generic" in the current tree.
>> Yeah. I know, I already talked to Jens about it - in order to be generic, 
>> the BSG stuff really does end up having to be able to stand on its own, 
>> since not everybody wants/needs the whole SCSI layer.
> 
> bsg looks really promising and it would be great if we can use it also
> for IDE driver.

Agreed.  IMO:  bsg should be able to be used for sending ATA commands 
(taskfiles) as well as SCSI commands.

	Jeff

Re: regression: disk error loop (panic?) ide_do_rw_disk-bad:

[Giacomo Catenazzi]

Linus Torvalds wrote:
> 
> On Tue, 17 Jul 2007, Bartlomiej Zolnierkiewicz wrote:
>> ide-disk driver and type 2 (REQ_TYPE_BLOCK_PC) requests don't mix well
>>
>> Probably some dumb application is sending packet commands without
>> checking the device type...
> 
> Ok, we should definitely try to just translate the things, and instead of 
> having user apps that have to know about the (generally not very 
> interesting) differences between IDE and SCSI command set, and when the 
> IDE driver gets a SCSI request (whether from the new generic SG layer or 
> obviously the older SCSI-ioctl layer) it should "just work".
> 
> So I object to that "dumb application" statement. It's the kernel that has 
> traditionally been dumb in not smoothing over the differences between 
> devices well enough.

BTW I found the application: it is hddtemp

Debian description:
Utility to monitor the temperature of your hard drive
.
hddtemp will give you the temperature of your PATA, SATA or SCSI hard
drive by reading Self-Monitoring Analysis and Reporting Technology
(S.M.A.R.T.) information (on drives that support this feature). Only
modern hard drives have a temperature sensor.

See http://www.guzu.net/linux/hddtemp.php , CCed to author.

ciao
	cate


PS: more info about /dev/hdc:

# hdparm -i /dev/hdc

/dev/hdc:

 Model=HDS722525VLAT80, FwRev=V36OA6EA, SerialNo=VN69TECFDDLE3A
 Config={ HardSect NotMFM HdSw>15uSec Fixed DTR>10Mbs }
 RawCHS=16383/16/63, TrkSize=0, SectSize=0, ECCbytes=52
 BuffType=DualPortCache, BuffSize=7938kB, MaxMultSect=16, MultSect=off
 CurCHS=16383/16/63, CurSects=16514064, LBA=yes, LBAsects=268435455
 IORDY=on/off, tPIO={min:240,w/IORDY:120}, tDMA={min:120,rec:120}
 PIO modes:  pio0 pio1 pio2 pio3 pio4
 DMA modes:  mdma0 mdma1 mdma2
 UDMA modes: udma0 udma1 udma2 udma3 udma4 *udma5
 AdvancedPM=yes: disabled (255) WriteCache=enabled
 Drive conforms to: ATA/ATAPI-6 T13 1410D revision 3a:  ATA/ATAPI-2,3,4,5,6

 * signifies the current active mode


# hdparm -I /dev/hdc

/dev/hdc:

ATA device, with non-removable media
powers-up in standby; SET FEATURES subcmd spins-up.
        Model Number:       HDS722525VLAT80
        Serial Number:      VN69TECFDDLE3A
        Firmware Revision:  V36OA6EA
Standards:
        Used: ATA/ATAPI-6 T13 1410D revision 3a
        Supported: 6 5 4
Configuration:
        Logical         max     current
        cylinders       16383   65535
        heads           16      1
        sectors/track   63      63
        --
        CHS current addressable sectors:    4128705
        LBA    user addressable sectors:  268435455
        LBA48  user addressable sectors:  488397168
        device size with M = 1024*1024:      238475 MBytes
        device size with M = 1000*1000:      250059 MBytes (250 GB)
Capabilities:
        LBA, IORDY(can be disabled)
        Queue depth: 32
        Standby timer values: spec'd by Standard, no device specific minimum
        R/W multiple sector transfer: Max = 16  Current = 0
        Advanced power management level: unknown setting (0x0000)
        Recommended acoustic management value: 128, current value: 254
        DMA: mdma0 mdma1 mdma2 udma0 udma1 udma2 udma3 udma4 *udma5
             Cycle time: min=120ns recommended=120ns
        PIO: pio0 pio1 pio2 pio3 pio4
             Cycle time: no flow control=240ns  IORDY flow control=120ns
Commands/features:
        Enabled Supported:
           *    SMART feature set
                Security Mode feature set
           *    Power Management feature set
           *    Write cache
           *    Look-ahead
           *    Release interrupt
           *    Host Protected Area feature set
           *    WRITE_BUFFER command
           *    READ_BUFFER command
           *    NOP cmd
           *    READ/WRITE_DMA_QUEUED
                Advanced Power Management feature set
                Power-Up In Standby feature set
                SET_FEATURES required to spinup after power up
                Address Offset Reserved Area Boot
                SET_MAX security extension
                Automatic Acoustic Management feature set
           *    48-bit Address feature set
           *    Device Configuration Overlay feature set
           *    Mandatory FLUSH_CACHE
           *    FLUSH_CACHE_EXT
           *    SMART error logging
           *    SMART self-test
Security:
        Master password revision code = 65534
                supported
        not     enabled
        not     locked
                frozen
        not     expired: security count
        not     supported: enhanced erase
        120min for SECURITY ERASE UNIT.
HW reset results:
        CBLID- above Vih
        Device num = 0 determined by the jumper
Checksum: correct
       *    General Purpose Logging feature set

Re: regression: disk error loop (panic?) ide_do_rw_disk-bad:

[Jens Axboe]

On Tue, Jul 17 2007, Linus Torvalds wrote:
> 
> 
> On Wed, 18 Jul 2007, Bartlomiej Zolnierkiewicz wrote:
> > 
> > The new generic SG layer is CONFIG_SCSI=y "generic" in the current tree.
> 
> Yeah. I know, I already talked to Jens about it - in order to be generic, 
> the BSG stuff really does end up having to be able to stand on its own, 
> since not everybody wants/needs the whole SCSI layer.

Fully agree!

> > > obviously the older SCSI-ioctl layer) it should "just work".
> > 
> > Agreed but IDE driver has never claimed to have full SAT layer and
> > full SCSI-ioctl layer support has been provided only for ide-cd.
> 
> I agree, and understand why it happened, but hope that maybe it can be 
> improved in the future, or at least the error returned earlier so that the 
> request will never even hit the ATA device at all if the IDE layer cannot 
> then handle it.
> 
> > For now it should be sufficient to revert ide.c chunks of
> > 
> > commit 3d6392cfbd7dc11f23058e3493683afab4ac13a3
> 
> Jens?

Yep, those were probably a little over eager. I can see that this revert
has already been executed, so perfect.

Yep, those were probably a little over eager. I can see that this revert
has already been executed, so perfect.

-- 
Jens Axboe

Re: regression: disk error loop (panic?) ide_do_rw_disk-bad:

[Linus Torvalds]

On Tue, 17 Jul 2007, Bartlomiej Zolnierkiewicz wrote:
> 
> On Tuesday 17 July 2007, Giacomo Catenazzi wrote:
> > 
> > last git changes to git give me the following error (repead very quickly):
> > 
> > sector 14657019, nr/cmr 0/0
> > bio f7b59280, biotail f7b59280, buffer 0000000, date 0000000, len 36
> > ide_do_rw_disk-bad command: dev hda: type 2, flags 104c8
> 
> ide-disk driver and type 2 (REQ_TYPE_BLOCK_PC) requests don't mix well

Hmm. I started wondering about this. 

Sure, ide-disk doesn't like non-fs requests, but I'm wondering why the 
error apparently keeps repeating? We should error out once, and that 
should be it.

So I'm wondering whether the

	ide_end_request(drive, 0, 0);
	return ide_stopped;

is the real bug.

The thing is, non-fs requests won't have nr_sectors set, so the 0 will 
stay as a zero (the IDE layer has some "turn zero into current sector 
number", but that doesn't work for commands that aren't sector-based!), 
and as a result, we'll "complete" zero bytes, and the bio will stay 
active!

So the code should probably at the _least_ say that it's finished one 
whole sector, and it would probably be much better to make it be based on 
"rq->data_len", which should be reliable.

Of course, since the "ide_end_request()" takes the data in sectors, but 
the request layer does it in bytes, we have to round the thing up and the 
ide_end_request() thing will turn it into too many bytes, but that's ok - 
"end_that_request_first()" doesn't care if we complete "too many" bytes.

So a diff something like this is, I think, the right thing to do, and 
should make the IDE disk driver handle non-fs requests gracefully (it will 
*fail* them, of course, but that's another issue).

Bartlomiej? Am I missing soemthing?

		Linus

---
 drivers/ide/ide-disk.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/ide/ide-disk.c b/drivers/ide/ide-disk.c
index b1304a7..fde2f79 100644
--- a/drivers/ide/ide-disk.c
+++ b/drivers/ide/ide-disk.c
@@ -311,8 +311,9 @@ static ide_startstop_t ide_do_rw_disk (ide_drive_t *drive, struct request *rq, s
 	BUG_ON(drive->blocked);
 
 	if (!blk_fs_request(rq)) {
+		int nsectors = (rq->data_len + 511) >> 9;
 		blk_dump_rq_flags(rq, "ide_do_rw_disk - bad command");
-		ide_end_request(drive, 0, 0);
+		ide_end_request(drive, 0, nsectors);
 		return ide_stopped;
 	}
 

Re: regression: disk error loop (panic?) ide_do_rw_disk-bad:

[Jens Axboe]

On Wed, Jul 18 2007, Linus Torvalds wrote:
> 
> 
> On Tue, 17 Jul 2007, Bartlomiej Zolnierkiewicz wrote:
> > 
> > On Tuesday 17 July 2007, Giacomo Catenazzi wrote:
> > > 
> > > last git changes to git give me the following error (repead very quickly):
> > > 
> > > sector 14657019, nr/cmr 0/0
> > > bio f7b59280, biotail f7b59280, buffer 0000000, date 0000000, len 36
> > > ide_do_rw_disk-bad command: dev hda: type 2, flags 104c8
> > 
> > ide-disk driver and type 2 (REQ_TYPE_BLOCK_PC) requests don't mix well
> 
> Hmm. I started wondering about this. 
> 
> Sure, ide-disk doesn't like non-fs requests, but I'm wondering why the 
> error apparently keeps repeating? We should error out once, and that 
> should be it.
> 
> So I'm wondering whether the
> 
> 	ide_end_request(drive, 0, 0);
> 	return ide_stopped;
> 
> is the real bug.
> 
> The thing is, non-fs requests won't have nr_sectors set, so the 0 will 
> stay as a zero (the IDE layer has some "turn zero into current sector 
> number", but that doesn't work for commands that aren't sector-based!), 
> and as a result, we'll "complete" zero bytes, and the bio will stay 
> active!
> 
> So the code should probably at the _least_ say that it's finished one 
> whole sector, and it would probably be much better to make it be based on 
> "rq->data_len", which should be reliable.
> 
> Of course, since the "ide_end_request()" takes the data in sectors, but 
> the request layer does it in bytes, we have to round the thing up and the 
> ide_end_request() thing will turn it into too many bytes, but that's ok - 
> "end_that_request_first()" doesn't care if we complete "too many" bytes.
> 
> So a diff something like this is, I think, the right thing to do, and 
> should make the IDE disk driver handle non-fs requests gracefully (it will 
> *fail* them, of course, but that's another issue).
> 
> Bartlomiej? Am I missing soemthing?

I think your analysis is pretty good, however you'd probably want to
incorporate that direct in ide_end_request(). Passing in 0 means end the
first chunk of the request, for pc type requests that should just be the
full thing as we cannot just move forward like in a read/write request.

Better still would be to make __ide_end_request() take a byte count
instead and use end_that_request_chunk(). Then you can get rid of the
rounding as well.

diff --git a/drivers/ide/ide-io.c b/drivers/ide/ide-io.c
index c5b5011..71b317a 100644
--- a/drivers/ide/ide-io.c
+++ b/drivers/ide/ide-io.c
@@ -114,8 +114,12 @@ int ide_end_request (ide_drive_t *drive, int uptodate, int nr_sectors)
 	spin_lock_irqsave(&ide_lock, flags);
 	rq = HWGROUP(drive)->rq;
 
-	if (!nr_sectors)
-		nr_sectors = rq->hard_cur_sectors;
+	if (!nr_sectors) {
+		if (!blk_fs_request(rq))
+			nr_sectors = (rq->data_len + 511) >> 9;
+		else
+			nr_sectors = rq->hard_cur_sectors;
+	}
 
 	ret = __ide_end_request(drive, rq, uptodate, nr_sectors);
 

-- 
Jens Axboe

Re: regression: disk error loop (panic?) ide_do_rw_disk-bad:

[Jens Axboe]

On Wed, Jul 18 2007, Jens Axboe wrote:
> Better still would be to make __ide_end_request() take a byte count
> instead and use end_that_request_chunk(). Then you can get rid of the
> rounding as well.

Ala:

diff --git a/drivers/ide/ide-io.c b/drivers/ide/ide-io.c
index c5b5011..f9de798 100644
--- a/drivers/ide/ide-io.c
+++ b/drivers/ide/ide-io.c
@@ -55,7 +55,7 @@
 #include <asm/bitops.h>
 
 static int __ide_end_request(ide_drive_t *drive, struct request *rq,
-			     int uptodate, int nr_sectors)
+			     int uptodate, unsigned int nr_bytes)
 {
 	int ret = 1;
 
@@ -64,7 +64,7 @@ static int __ide_end_request(ide_drive_t *drive, struct request *rq,
 	 * complete the whole request right now
 	 */
 	if (blk_noretry_request(rq) && end_io_error(uptodate))
-		nr_sectors = rq->hard_nr_sectors;
+		nr_bytes = rq->hard_nr_sectors << 9;
 
 	if (!blk_fs_request(rq) && end_io_error(uptodate) && !rq->errors)
 		rq->errors = -EIO;
@@ -78,7 +78,7 @@ static int __ide_end_request(ide_drive_t *drive, struct request *rq,
 		HWGROUP(drive)->hwif->ide_dma_on(drive);
 	}
 
-	if (!end_that_request_first(rq, uptodate, nr_sectors)) {
+	if (!end_that_request_chunk(rq, uptodate, nr_bytes)) {
 		add_disk_randomness(rq->rq_disk);
 		if (!list_empty(&rq->queuelist))
 			blkdev_dequeue_request(rq);
@@ -103,6 +103,7 @@ static int __ide_end_request(ide_drive_t *drive, struct request *rq,
 
 int ide_end_request (ide_drive_t *drive, int uptodate, int nr_sectors)
 {
+	unsigned int nr_bytes = nr_sectors << 9;
 	struct request *rq;
 	unsigned long flags;
 	int ret = 1;
@@ -114,10 +115,14 @@ int ide_end_request (ide_drive_t *drive, int uptodate, int nr_sectors)
 	spin_lock_irqsave(&ide_lock, flags);
 	rq = HWGROUP(drive)->rq;
 
-	if (!nr_sectors)
-		nr_sectors = rq->hard_cur_sectors;
+	if (!nr_bytes) {
+		if (blk_pc_request(rq))
+			nr_bytes = rq->data_len;
+		else
+			nr_bytes = rq->hard_cur_sectors << 9;
+	}
 
-	ret = __ide_end_request(drive, rq, uptodate, nr_sectors);
+	ret = __ide_end_request(drive, rq, uptodate, nr_bytes);
 
 	spin_unlock_irqrestore(&ide_lock, flags);
 	return ret;

-- 
Jens Axboe

Re: regression: disk error loop (panic?) ide_do_rw_disk-bad:

[Linus Torvalds]

On Wed, 18 Jul 2007, Jens Axboe wrote:
> 
> I think your analysis is pretty good, however you'd probably want to
> incorporate that direct in ide_end_request().

Ok, that makes sense too.

And yes, the further cleanup would be:

> Better still would be to make __ide_end_request() take a byte count
> instead and use end_that_request_chunk(). Then you can get rid of the
> rounding as well.

and that sounds fine, but is an independent issue and not strictly 
necessary.

Bartlomiej?

		Linus

Re: regression: disk error loop (panic?) ide_do_rw_disk-bad:

[Jens Axboe]

On Wed, Jul 18 2007, Linus Torvalds wrote:
> 
> 
> On Wed, 18 Jul 2007, Jens Axboe wrote:
> > 
> > I think your analysis is pretty good, however you'd probably want to
> > incorporate that direct in ide_end_request().
> 
> Ok, that makes sense too.
> 
> And yes, the further cleanup would be:
> 
> > Better still would be to make __ide_end_request() take a byte count
> > instead and use end_that_request_chunk(). Then you can get rid of the
> > rounding as well.
> 
> and that sounds fine, but is an independent issue and not strictly 
> necessary.

Oh yeah, it's just a further improvement, not strictly necessary for
fixing this bug. So it could be done at a later time, all up to Bart...

-- 
Jens Axboe

Re: regression: disk error loop (panic?) ide_do_rw_disk-bad:

[Bartlomiej Zolnierkiewicz]

On Wednesday 18 July 2007, Jens Axboe wrote:
> On Wed, Jul 18 2007, Linus Torvalds wrote:
> > 
> > 
> > On Wed, 18 Jul 2007, Jens Axboe wrote:
> > > 
> > > I think your analysis is pretty good, however you'd probably want to
> > > incorporate that direct in ide_end_request().
> > 
> > Ok, that makes sense too.
> > 
> > And yes, the further cleanup would be:
> > 
> > > Better still would be to make __ide_end_request() take a byte count
> > > instead and use end_that_request_chunk(). Then you can get rid of the
> > > rounding as well.
> > 
> > and that sounds fine, but is an independent issue and not strictly 
> > necessary.
> 
> Oh yeah, it's just a further improvement, not strictly necessary for
> fixing this bug. So it could be done at a later time, all up to Bart...

Thanks for finding and fixing this.

The latest patch (with additional cleanups) also looks good and should be
safe enough (unchanged behavior for all non-pc requests) to merge it now.

Acked-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>

Thanks,
Bart

Re: regression: disk error loop (panic?) ide_do_rw_disk-bad:

[Linus Torvalds]

On Thu, 19 Jul 2007, Bartlomiej Zolnierkiewicz wrote:
> 
> Thanks for finding and fixing this.
> 
> The latest patch (with additional cleanups) also looks good and should be
> safe enough (unchanged behavior for all non-pc requests) to merge it now.
> 
> Acked-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>

Ok, Jens - mind signing off on the patch you sent out, and writing an 
explanatory message? Feel free to just crib from my explanation of my 
original patch, or whatever.

And it would be beautiful if people who saw the bad behaviour before 
reverting the ide.c changes were to go back to that broken state, and try 
the patch, and just verify that it acts like it should (ie you should see 
just a few error messages, and it shouldn't cause the IDE layer to go 
ballistic any more).

		Linus

Re: regression: disk error loop (panic?) ide_do_rw_disk-bad:

[Jens Axboe]

On Wed, Jul 18 2007, Linus Torvalds wrote:
> 
> 
> On Thu, 19 Jul 2007, Bartlomiej Zolnierkiewicz wrote:
> > 
> > Thanks for finding and fixing this.
> > 
> > The latest patch (with additional cleanups) also looks good and should be
> > safe enough (unchanged behavior for all non-pc requests) to merge it now.
> > 
> > Acked-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> 
> Ok, Jens - mind signing off on the patch you sent out, and writing an 
> explanatory message? Feel free to just crib from my explanation of my 
> original patch, or whatever.

Sure thing, it's below.

> And it would be beautiful if people who saw the bad behaviour before
> reverting the ide.c changes were to go back to that broken state, and
> try the patch, and just verify that it acts like it should (ie you
> should see just a few error messages, and it shouldn't cause the IDE
> layer to go ballistic any more).

---

[PATCH] IDE: fix termination of non-fs requests

ide-disk calls

        ide_end_request(drive, 0, 0);

to finish an unknown request, but this doesn't work so well for non-fs
requests, since ide_end_request() internally looks at ->hard_cur_sectors
to see how much data to end. Only file system requests store a transfer
value in there, pc requests fill out ->data_len as a byte based transfer
value instead.

Since we ask to end 0 bytes of that request, it will never be terminated
and ide-disk gets stuck in a loop "handling" that same request over and
over.

Switch __ide_end_request() to take a byte based transfer count, and
adjust ide_end_request() to look at the right field to determine how
much IO to end when it's being passed in 0.

Acked-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Signed-off-by: Jens Axboe <jens.axboe@oracle.com>

diff --git a/drivers/ide/ide-io.c b/drivers/ide/ide-io.c
index c5b5011..f9de798 100644
--- a/drivers/ide/ide-io.c
+++ b/drivers/ide/ide-io.c
@@ -55,7 +55,7 @@
 #include <asm/bitops.h>
 
 static int __ide_end_request(ide_drive_t *drive, struct request *rq,
-			     int uptodate, int nr_sectors)
+			     int uptodate, unsigned int nr_bytes)
 {
 	int ret = 1;
 
@@ -64,7 +64,7 @@ static int __ide_end_request(ide_drive_t *drive, struct request *rq,
 	 * complete the whole request right now
 	 */
 	if (blk_noretry_request(rq) && end_io_error(uptodate))
-		nr_sectors = rq->hard_nr_sectors;
+		nr_bytes = rq->hard_nr_sectors << 9;
 
 	if (!blk_fs_request(rq) && end_io_error(uptodate) && !rq->errors)
 		rq->errors = -EIO;
@@ -78,7 +78,7 @@ static int __ide_end_request(ide_drive_t *drive, struct request *rq,
 		HWGROUP(drive)->hwif->ide_dma_on(drive);
 	}
 
-	if (!end_that_request_first(rq, uptodate, nr_sectors)) {
+	if (!end_that_request_chunk(rq, uptodate, nr_bytes)) {
 		add_disk_randomness(rq->rq_disk);
 		if (!list_empty(&rq->queuelist))
 			blkdev_dequeue_request(rq);
@@ -103,6 +103,7 @@ static int __ide_end_request(ide_drive_t *drive, struct request *rq,
 
 int ide_end_request (ide_drive_t *drive, int uptodate, int nr_sectors)
 {
+	unsigned int nr_bytes = nr_sectors << 9;
 	struct request *rq;
 	unsigned long flags;
 	int ret = 1;
@@ -114,10 +115,14 @@ int ide_end_request (ide_drive_t *drive, int uptodate, int nr_sectors)
 	spin_lock_irqsave(&ide_lock, flags);
 	rq = HWGROUP(drive)->rq;
 
-	if (!nr_sectors)
-		nr_sectors = rq->hard_cur_sectors;
+	if (!nr_bytes) {
+		if (blk_pc_request(rq))
+			nr_bytes = rq->data_len;
+		else
+			nr_bytes = rq->hard_cur_sectors << 9;
+	}
 
-	ret = __ide_end_request(drive, rq, uptodate, nr_sectors);
+	ret = __ide_end_request(drive, rq, uptodate, nr_bytes);
 
 	spin_unlock_irqrestore(&ide_lock, flags);
 	return ret;

-- 
Jens Axboe

Re: regression: disk error loop (panic?) ide_do_rw_disk-bad:

[Giacomo Catenazzi]

Linus Torvalds wrote:
> 
> On Thu, 19 Jul 2007, Bartlomiej Zolnierkiewicz wrote:
>> Thanks for finding and fixing this.
>>
>> The latest patch (with additional cleanups) also looks good and should be
>> safe enough (unchanged behavior for all non-pc requests) to merge it now.
>>
>> Acked-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> 
> Ok, Jens - mind signing off on the patch you sent out, and writing an 
> explanatory message? Feel free to just crib from my explanation of my 
> original patch, or whatever.
> 
> And it would be beautiful if people who saw the bad behaviour before 
> reverting the ide.c changes were to go back to that broken state, and try 
> the patch, and just verify that it acts like it should (ie you should see 
> just a few error messages, and it shouldn't cause the IDE layer to go 
> ballistic any more).

Ok, I tested a5fcaa210626a79465321e344c91a6a7dc3881fa , with
the Jeans' patch with clean-up (Message-ID:
<20070718201152.GL11657@kernel.dk>).

I don't see the error loop. but only 4 errors (2 for each hd, at hddtemp
start)

Jul 19 08:22:19 catee kernel: hda: selected mode 0x45
Jul 19 08:22:23 catee kernel: ide_do_rw_disk - bad command: dev hda:
type=2, flags=104c8
Jul 19 08:22:23 catee kernel:
Jul 19 08:22:23 catee kernel: sector 14657019, nr/cnr 0/0
Jul 19 08:22:23 catee kernel: bio c21a4780, biotail c21a4780, buffer
00000000, data 00000000, len 36
Jul 19 08:22:23 catee kernel: cdb: 12 00 00 00 24 00 00 00 00 00 00 00
00 00 00 00
Jul 19 08:22:23 catee kernel: ide_do_rw_disk - bad command: dev hdc:
type=2, flags=104c8
Jul 19 08:22:23 catee kernel:
Jul 19 08:22:23 catee kernel: sector 34711027, nr/cnr 0/0
Jul 19 08:22:23 catee kernel: bio c21a4740, biotail c21a4740, buffer
00000000, data 00000000, len 36
Jul 19 08:22:23 catee kernel: cdb: 12 00 00 00 24 00 00 00 00 00 00 00
00 00 00 00
Jul 19 08:22:23 catee kernel: ide_do_rw_disk - bad command: dev hdc:
type=2, flags=104c8
Jul 19 08:22:23 catee kernel:
Jul 19 08:22:23 catee kernel: sector 7387152, nr/cnr 0/0
Jul 19 08:22:23 catee kernel: bio c21a4900, biotail c21a4900, buffer
00000000, data 00000000, len 36
Jul 19 08:22:23 catee kernel: cdb: 12 00 00 00 24 00 00 00 00 00 00 00
00 00 00 00
Jul 19 08:22:23 catee kernel: ide_do_rw_disk - bad command: dev hda:
type=2, flags=104c8
Jul 19 08:22:23 catee kernel:
Jul 19 08:22:23 catee kernel: sector 7387152, nr/cnr 0/0
Jul 19 08:22:23 catee kernel: bio c21a4900, biotail c21a4900, buffer
00000000, data 00000000, len 36
Jul 19 08:22:23 catee kernel: cdb: 12 00 00 00 24 00 00 00 00 00 00 00
00 00 00 00
Jul 19 08:22:27 catee kernel: ttyS1: LSR safety check engaged!

The last git tree give me no errors.

patch in Message-ID: <20070718201152.GL11657@kernel.dk>

Tested-By: Giacomo Catenazzi <cate@debian.org>

ciao
	cate

Re: regression: disk error loop (panic?) ide_do_rw_disk-bad:

[Jens Axboe]

On Thu, Jul 19 2007, Giacomo Catenazzi wrote:
> Linus Torvalds wrote:
> > 
> > On Thu, 19 Jul 2007, Bartlomiej Zolnierkiewicz wrote:
> >> Thanks for finding and fixing this.
> >>
> >> The latest patch (with additional cleanups) also looks good and should be
> >> safe enough (unchanged behavior for all non-pc requests) to merge it now.
> >>
> >> Acked-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> > 
> > Ok, Jens - mind signing off on the patch you sent out, and writing an 
> > explanatory message? Feel free to just crib from my explanation of my 
> > original patch, or whatever.
> > 
> > And it would be beautiful if people who saw the bad behaviour before 
> > reverting the ide.c changes were to go back to that broken state, and try 
> > the patch, and just verify that it acts like it should (ie you should see 
> > just a few error messages, and it shouldn't cause the IDE layer to go 
> > ballistic any more).
> 
> Ok, I tested a5fcaa210626a79465321e344c91a6a7dc3881fa , with
> the Jeans' patch with clean-up (Message-ID:
> <20070718201152.GL11657@kernel.dk>).
> 
> I don't see the error loop. but only 4 errors (2 for each hd, at hddtemp
> start)
> 
> Jul 19 08:22:19 catee kernel: hda: selected mode 0x45
> Jul 19 08:22:23 catee kernel: ide_do_rw_disk - bad command: dev hda:
> type=2, flags=104c8
> Jul 19 08:22:23 catee kernel:
> Jul 19 08:22:23 catee kernel: sector 14657019, nr/cnr 0/0
> Jul 19 08:22:23 catee kernel: bio c21a4780, biotail c21a4780, buffer
> 00000000, data 00000000, len 36
> Jul 19 08:22:23 catee kernel: cdb: 12 00 00 00 24 00 00 00 00 00 00 00
> 00 00 00 00
> Jul 19 08:22:23 catee kernel: ide_do_rw_disk - bad command: dev hdc:
> type=2, flags=104c8
> Jul 19 08:22:23 catee kernel:
> Jul 19 08:22:23 catee kernel: sector 34711027, nr/cnr 0/0
> Jul 19 08:22:23 catee kernel: bio c21a4740, biotail c21a4740, buffer
> 00000000, data 00000000, len 36
> Jul 19 08:22:23 catee kernel: cdb: 12 00 00 00 24 00 00 00 00 00 00 00
> 00 00 00 00
> Jul 19 08:22:23 catee kernel: ide_do_rw_disk - bad command: dev hdc:
> type=2, flags=104c8
> Jul 19 08:22:23 catee kernel:
> Jul 19 08:22:23 catee kernel: sector 7387152, nr/cnr 0/0
> Jul 19 08:22:23 catee kernel: bio c21a4900, biotail c21a4900, buffer
> 00000000, data 00000000, len 36
> Jul 19 08:22:23 catee kernel: cdb: 12 00 00 00 24 00 00 00 00 00 00 00
> 00 00 00 00
> Jul 19 08:22:23 catee kernel: ide_do_rw_disk - bad command: dev hda:
> type=2, flags=104c8
> Jul 19 08:22:23 catee kernel:
> Jul 19 08:22:23 catee kernel: sector 7387152, nr/cnr 0/0
> Jul 19 08:22:23 catee kernel: bio c21a4900, biotail c21a4900, buffer
> 00000000, data 00000000, len 36
> Jul 19 08:22:23 catee kernel: cdb: 12 00 00 00 24 00 00 00 00 00 00 00
> 00 00 00 00

Perfect, thanks a lot for testing!

Tested-By: Giacomo Catenazzi <cate@debian.org>

Linus, if you merge the patch I sent, can you just add this Tested-by?

-- 
Jens Axboe