[PATCH] fix sata_sil quirk

[PATCH] fix sata_sil quirk

[Jeff Garzik]

[-- Attachment #1: Type: text/plain, Size: 727 bytes --]

Here's my suggested fix...  good catch Ricky.

Yes, unfortunately performance will be dog slow.

Silicon Image 311x is fully SATA compliant -- but it's the only 
controller that sends odd-sized packets to the SATA device.  That causes 
no end of problems, including the thing that SIL_QUIRK_MOD15WRITE 
attempts to work around.

I've got contacts at Silicon Image, and have been meaning to bug them 
for a "real fix" for a while.  It is rumored that there is a much better 
fix, which allows full performance while at the same time not killing 
your SATA drive due to odd-sized SATA frames on the wire.

Unfortunately, at this point in time, we must err on the side of caution 
and cripple performance, for stability.

	Jeff



[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 1365 bytes --]

===== drivers/scsi/libata-scsi.c 1.39 vs edited =====
--- 1.39/drivers/scsi/libata-scsi.c	2004-05-12 11:46:21 -04:00
+++ edited/drivers/scsi/libata-scsi.c	2004-06-22 16:18:57 -04:00
@@ -182,7 +182,8 @@
 		 * 65534 when Jens Axboe's patch for dynamically
 		 * determining max_sectors is merged.
 		 */
-		if (dev->flags & ATA_DFLAG_LBA48) {
+		if ((dev->flags & ATA_DFLAG_LBA48) &&
+		    ((dev->flags & ATA_DFLAG_LOCK_SECTORS) == 0)) {
 			sdev->host->max_sectors = 2048;
 			blk_queue_max_sectors(sdev->request_queue, 2048);
 		}
===== drivers/scsi/sata_sil.c 1.26 vs edited =====
--- 1.26/drivers/scsi/sata_sil.c	2004-06-15 00:29:32 -04:00
+++ edited/drivers/scsi/sata_sil.c	2004-06-22 16:18:21 -04:00
@@ -302,6 +302,7 @@
 		       ap->id, dev->devno);
 		ap->host->max_sectors = 15;
 		ap->host->hostt->max_sectors = 15;
+		dev->flags |= ATA_DFLAG_LOCK_SECTORS;
 		return;
 	}
 
===== include/linux/libata.h 1.38 vs edited =====
--- 1.38/include/linux/libata.h	2004-06-22 00:54:44 -04:00
+++ edited/include/linux/libata.h	2004-06-22 16:17:44 -04:00
@@ -91,6 +91,7 @@
 	ATA_DFLAG_MASTER	= (1 << 2), /* is device 0? */
 	ATA_DFLAG_WCACHE	= (1 << 3), /* has write cache we can
 					     * (hopefully) flush? */
+	ATA_DFLAG_LOCK_SECTORS	= (1 << 4), /* don't adjust max_sectors */
 
 	ATA_DEV_UNKNOWN		= 0,	/* unknown device */
 	ATA_DEV_ATA		= 1,	/* ATA device */

Re: [PATCH] fix sata_sil quirk

[Ricky Beam]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1533 bytes --]

On Tue, 22 Jun 2004, Jeff Garzik wrote:
>Here's my suggested fix...  good catch Ricky.

And I don't even know why I looked at max_sectors :-) (I need more Dew.)

>Yes, unfortunately performance will be dog slow.

Well, at least puppy slow...
Device:            tps    kB_read/s    kB_wrtn/s    kB_read    kB_wrtn
sda            1811.65         0.00      9629.85          0     577887
sdb            1807.15         0.00      9629.60          0     577872
sdc            1807.25         0.00      9629.86          0     577888
sdd            1807.05         0.00      9629.86          0     577888
md_d0         14444.64         0.00     48148.84          0    2889412
md_d0p2        9629.78         0.00     38519.11          0    2311532
(over 60sec,  8M O_DIRECT accesses, 128 stripes * 16k RAID0)

Without the MOD15 hack, the numbers are 2x higher, but they stop after
a few minutes :-)

>I've got contacts at Silicon Image, and have been meaning to bug them
>for a "real fix" for a while.  It is rumored that there is a much better
>fix, which allows full performance while at the same time not killing
>your SATA drive due to odd-sized SATA frames on the wire.

Ask them what they do in their driver? (the linux one and the windows one)
Looking at the linux driver, the mod15 quirk is there, but there doesn't
appear to be any associated device list. (I've already post the single
Maxtor device listed.)  FreeBSD detects the stall, resets the chip and
hopes that clears the problem. (People are not happy about that.)

--Ricky


[-- Attachment #2: Type: TEXT/PLAIN, Size: 1365 bytes --]

===== drivers/scsi/libata-scsi.c 1.39 vs edited =====
--- 1.39/drivers/scsi/libata-scsi.c	2004-05-12 11:46:21 -04:00
+++ edited/drivers/scsi/libata-scsi.c	2004-06-22 16:18:57 -04:00
@@ -182,7 +182,8 @@
 		 * 65534 when Jens Axboe's patch for dynamically
 		 * determining max_sectors is merged.
 		 */
-		if (dev->flags & ATA_DFLAG_LBA48) {
+		if ((dev->flags & ATA_DFLAG_LBA48) &&
+		    ((dev->flags & ATA_DFLAG_LOCK_SECTORS) == 0)) {
 			sdev->host->max_sectors = 2048;
 			blk_queue_max_sectors(sdev->request_queue, 2048);
 		}
===== drivers/scsi/sata_sil.c 1.26 vs edited =====
--- 1.26/drivers/scsi/sata_sil.c	2004-06-15 00:29:32 -04:00
+++ edited/drivers/scsi/sata_sil.c	2004-06-22 16:18:21 -04:00
@@ -302,6 +302,7 @@
 		       ap->id, dev->devno);
 		ap->host->max_sectors = 15;
 		ap->host->hostt->max_sectors = 15;
+		dev->flags |= ATA_DFLAG_LOCK_SECTORS;
 		return;
 	}
 
===== include/linux/libata.h 1.38 vs edited =====
--- 1.38/include/linux/libata.h	2004-06-22 00:54:44 -04:00
+++ edited/include/linux/libata.h	2004-06-22 16:17:44 -04:00
@@ -91,6 +91,7 @@
 	ATA_DFLAG_MASTER	= (1 << 2), /* is device 0? */
 	ATA_DFLAG_WCACHE	= (1 << 3), /* has write cache we can
 					     * (hopefully) flush? */
+	ATA_DFLAG_LOCK_SECTORS	= (1 << 4), /* don't adjust max_sectors */
 
 	ATA_DEV_UNKNOWN		= 0,	/* unknown device */
 	ATA_DEV_ATA		= 1,	/* ATA device */

Re: [PATCH] fix sata_sil quirk

[Jeff Garzik]

On Tue, Jun 22, 2004 at 04:29:08PM -0400, Ricky Beam wrote:
> On Tue, 22 Jun 2004, Jeff Garzik wrote:
> >Here's my suggested fix...  good catch Ricky.
> 
> And I don't even know why I looked at max_sectors :-) (I need more Dew.)
> 
> >Yes, unfortunately performance will be dog slow.
> 
> Well, at least puppy slow...
> Device:            tps    kB_read/s    kB_wrtn/s    kB_read    kB_wrtn
> sda            1811.65         0.00      9629.85          0     577887
> sdb            1807.15         0.00      9629.60          0     577872
> sdc            1807.25         0.00      9629.86          0     577888
> sdd            1807.05         0.00      9629.86          0     577888
> md_d0         14444.64         0.00     48148.84          0    2889412
> md_d0p2        9629.78         0.00     38519.11          0    2311532
> (over 60sec,  8M O_DIRECT accesses, 128 stripes * 16k RAID0)
> 
> Without the MOD15 hack, the numbers are 2x higher, but they stop after
> a few minutes :-)

Stability first!


> >I've got contacts at Silicon Image, and have been meaning to bug them
> >for a "real fix" for a while.  It is rumored that there is a much better
> >fix, which allows full performance while at the same time not killing
> >your SATA drive due to odd-sized SATA frames on the wire.
> 
> Ask them what they do in their driver? (the linux one and the windows one)
> Looking at the linux driver, the mod15 quirk is there, but there doesn't
> appear to be any associated device list. (I've already post the single
> Maxtor device listed.)  FreeBSD detects the stall, resets the chip and
> hopes that clears the problem. (People are not happy about that.)

I just poked them.  I'm not satisfied with what any Linux or FreeBSD
driver does, I want to Get It Right(tm)  :)  I'm willing to bet that
their Linux driver does mod15 only because they didn't know kernel
internals that well.

	Jeff

Re: [PATCH] fix sata_sil quirk

[Jeff Garzik]

Ricky Beam wrote:
> On Tue, 22 Jun 2004, Jeff Garzik wrote:
> 
>> Here's my suggested fix...  good catch Ricky.
> 
> 
> And I don't even know why I looked at max_sectors :-) (I need more Dew.)
> 
>> Yes, unfortunately performance will be dog slow.
> 
> 
> Well, at least puppy slow...
> Device:            tps    kB_read/s    kB_wrtn/s    kB_read    kB_wrtn
> sda            1811.65         0.00      9629.85          0     577887
> sdb            1807.15         0.00      9629.60          0     577872
> sdc            1807.25         0.00      9629.86          0     577888
> sdd            1807.05         0.00      9629.86          0     577888
> md_d0         14444.64         0.00     48148.84          0    2889412
> md_d0p2        9629.78         0.00     38519.11          0    2311532
> (over 60sec,  8M O_DIRECT accesses, 128 stripes * 16k RAID0)
> 
> Without the MOD15 hack, the numbers are 2x higher, but they stop after
> a few minutes :-)

Is this with my patch?

If so, I'll go ahead and forward it upstream, since I would certainly 
like a stabilization fix applied ASAP.


>> I've got contacts at Silicon Image, and have been meaning to bug them
>> for a "real fix" for a while.  It is rumored that there is a much better
>> fix, which allows full performance while at the same time not killing
>> your SATA drive due to odd-sized SATA frames on the wire.
> 
> 
> Ask them what they do in their driver? (the linux one and the windows one)
> Looking at the linux driver, the mod15 quirk is there, but there doesn't
> appear to be any associated device list. (I've already post the single
> Maxtor device listed.)  FreeBSD detects the stall, resets the chip and
> hopes that clears the problem. (People are not happy about that.)

The full-speed fix requires splitting affected DMA writes into two 
separate commands, when the sector count matches "sectors % 15 == 1".

	Jeff

Re: [PATCH] fix sata_sil quirk

[Ricky Beam]

On Tue, 22 Jun 2004, Jeff Garzik wrote:
>Is this with my patch?

It is now.  Either way, max_sectors is staying 15.  I'll post the results
after it's had time to zero 40G a few dozen times.

>The full-speed fix requires splitting affected DMA writes into two
>separate commands, when the sector count matches "sectors % 15 == 1".

Why 15 I wonder?

--Ricky

Re: [PATCH] fix sata_sil quirk

[Paul Jakma]

On Tue, 22 Jun 2004, Jeff Garzik wrote:

> Here's my suggested fix...  good catch Ricky.
>
> Yes, unfortunately performance will be dog slow.
>
> Silicon Image 311x is fully SATA compliant -- but it's the only controller 
> that sends odd-sized packets to the SATA device.  That causes no end of 
> problems, including the thing that SIL_QUIRK_MOD15WRITE attempts to work 
> around.

As an extra data point, i have:

# cat /proc/scsi/scsi
Attached devices:
Host: scsi0 Channel: 00 Id: 00 Lun: 00
   Vendor: ATA      Model: WDC WD1600JD-00G Rev: 02.0
   Type:   Direct-Access                    ANSI SCSI revision: 05
Host: scsi1 Channel: 00 Id: 00 Lun: 00
   Vendor: ATA      Model: WDC WD1600JD-00G Rev: 02.0
   Type:   Direct-Access                    ANSI SCSI revision: 05
Host: scsi2 Channel: 00 Id: 00 Lun: 00
   Vendor: ATA      Model: WDC WD1600JD-00G Rev: 02.0
   Type:   Direct-Access                    ANSI SCSI revision: 05

# lspci  | grep Sil
00:09.0 RAID bus controller: Silicon Image, Inc. (formerly CMD 
Technology Inc) Silicon Image Serial ATARaid Controller [ CMD/Sil
3112/3112A ] (rev 02)
00:0a.0 RAID bus controller: Silicon Image, Inc. (formerly CMD 
Technology Inc) Silicon Image Serial ATARaid Controller
[ CMD/Sil 3112/3112A ] (rev 02)

# modinfo sata_sil
author:         Jeff Garzik
description:    low-level driver for Silicon Image SATA controller
license:        GPL
vermagic:       2.6.6-1.397.root K6 REGPARM gcc-3.3

And am not having (touch wood) any stability problems using these 
disks with linux md RAID1 and RAID5. Though, they're in a K6-II 350, 
so performance is slow anyway. (i get about 25MB/s absolute max 
reading from a RAID-5 array).

  > 	 Jeff

regards,
-- 
Paul Jakma	paul@clubi.ie	paul@jakma.org	Key ID: 64A2FF6A
 	warning: do not ever send email to spam@dishone.st
Fortune:
A bird in the hand makes it awfully hard to blow your nose.

Re: [PATCH] fix sata_sil quirk

[Jeff Garzik]

Paul Jakma wrote:
> # cat /proc/scsi/scsi
> Attached devices:
> Host: scsi0 Channel: 00 Id: 00 Lun: 00
>   Vendor: ATA      Model: WDC WD1600JD-00G Rev: 02.0
>   Type:   Direct-Access                    ANSI SCSI revision: 05
> Host: scsi1 Channel: 00 Id: 00 Lun: 00
>   Vendor: ATA      Model: WDC WD1600JD-00G Rev: 02.0
>   Type:   Direct-Access                    ANSI SCSI revision: 05
> Host: scsi2 Channel: 00 Id: 00 Lun: 00
>   Vendor: ATA      Model: WDC WD1600JD-00G Rev: 02.0
>   Type:   Direct-Access                    ANSI SCSI revision: 05

> And am not having (touch wood) any stability problems using these disks 
> with linux md RAID1 and RAID5. Though, they're in a K6-II 350, so 
> performance is slow anyway. (i get about 25MB/s absolute max reading 
> from a RAID-5 array).


Cool.  Yeah, non-Seagate should be full speed and unaffected...

	Jeff

Re: [PATCH] fix sata_sil quirk

[Paul Jakma]

On Tue, 22 Jun 2004, Jeff Garzik wrote:

> Cool.  Yeah, non-Seagate should be full speed and unaffected...

But I got the impression your patch enables mod15-quirk for all LBA48 
drives, is that correct? If so, if I have to update kernels here, I 
think I'll reverse that one if it affects all LBA48, as I'd rather 
not suffer the performance hit (its slow enough already because of 
slow CPU/chipset/contended PCI bus thank you very much ;) ) - until 
such time as a better fix is known.

> 	Jeff

regards,
-- 
Paul Jakma	paul@clubi.ie	paul@jakma.org	Key ID: 64A2FF6A
 	warning: do not ever send email to spam@dishone.st
Fortune:
It is so soon that I am done for, I wonder what I was begun for.
 		-- Epitaph, Cheltenham Churchyard

Re: [PATCH] fix sata_sil quirk

[Jeff Garzik]

Paul Jakma wrote:
> On Tue, 22 Jun 2004, Jeff Garzik wrote:
> 
>> Cool.  Yeah, non-Seagate should be full speed and unaffected...
> 
> 
> But I got the impression your patch enables mod15-quirk for all LBA48 
> drives, is that correct? If so, if I have to update kernels here, I 
> think I'll reverse that one if it affects all LBA48, as I'd rather not 
> suffer the performance hit (its slow enough already because of slow 
> CPU/chipset/contended PCI bus thank you very much ;) ) - until such time 
> as a better fix is known.


Nope, my patch only enables ATA_DFLAG_LOCK_SECTORS for devices flagged 
with SIL_QUIRK_MOD15WRITE:

         { "ST320012AS",         SIL_QUIRK_MOD15WRITE },
         { "ST330013AS",         SIL_QUIRK_MOD15WRITE },
         { "ST340017AS",         SIL_QUIRK_MOD15WRITE },
         { "ST360015AS",         SIL_QUIRK_MOD15WRITE },
         { "ST380023AS",         SIL_QUIRK_MOD15WRITE },
         { "ST3120023AS",        SIL_QUIRK_MOD15WRITE },
         { "ST340014ASL",        SIL_QUIRK_MOD15WRITE },
         { "ST360014ASL",        SIL_QUIRK_MOD15WRITE },
         { "ST380011ASL",        SIL_QUIRK_MOD15WRITE },
         { "ST3120022ASL",       SIL_QUIRK_MOD15WRITE },
         { "ST3160021ASL",       SIL_QUIRK_MOD15WRITE },
         { "Maxtor 4D060H3",     SIL_QUIRK_UDMA5MAX },
[...]
         if (quirks & SIL_QUIRK_MOD15WRITE) {
                 printk(KERN_INFO "ata%u(%u): applying errata fix\n",
                        ap->id, dev->devno);
                 ap->host->max_sectors = 15;
                 ap->host->hostt->max_sectors = 15;
                 dev->flags |= ATA_DFLAG_LOCK_SECTORS;
                 return;
         }

Re: [PATCH] fix sata_sil quirk

[Ricky Beam]

On Tue, 22 Jun 2004, Jeff Garzik wrote:
>Nope, my patch only enables ATA_DFLAG_LOCK_SECTORS for devices flagged
>with SIL_QUIRK_MOD15WRITE:
...

That list needs a:
         { "ST3160023AS",        SIL_QUIRK_MOD15WRITE },
as well.

--Ricky

PS: I've completed 23 cycles of zeroing a 40G section of RAID0 space without
    error.

Re: [PATCH] fix sata_sil quirk

[George Georgalis]

On Sun, 27 Jun 2004 21:54:31 -0400, George Georgalis <george@galis.org> wrote:
> On Wed, Jun 23, 2004 at 02:34:35AM -0400, Ricky Beam wrote:
> >
> >That list needs a:
> >         { "ST3160023AS",        SIL_QUIRK_MOD15WRITE },
> >as well.
> 
> happens to be my drive, is there any way to tell a drive needs
> be in the quirk 15 list, other than it's Seagate and big writes
> block the dev?


Actually my problem with big writes was the bk kernel I downloaded was
a rev too early (2.6.7-bk7 ?).

I tested the sata_sil.c version from June 25 (via bk checkout)
extensively and had no problem... posted my results then, hdparm
reported ~42 - 51 MB/sec, and never a write block.

Today I try putting 2.6.8.1 on the box and I'm getting ~14 MB/sec, the
drive has been added to the sil_blacklist. Why? What did I miss?

I've taken my drive out of the black list, abused it with 5 continuous
writes and "top id 0" no problem after 58Gb. I'm doing it again this
time simultaneously with a massive rm -rf of backup directories. I
really don't anticipate a problem.

So what's up with 

         /*{ "ST3160023AS",      SIL_QUIRK_MOD15WRITE },*/

before it got in there, it was said the black list "would not grow"
(for unexplained reasons).

// George

-- 
George Georgalis, systems architect, administrator Linux BSD IXOYE
http://galis.org/george/ cell:646-331-2027 mailto:george@galis.org