2004-06-22 20:28:01

by Jeff Garzik

[permalink] [raw]
Subject: [PATCH] fix sata_sil quirk

===== 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 */


Attachments:
patch (1.33 kB)

2004-06-22 20:50:05

by Jeff Garzik

[permalink] [raw]
Subject: Re: [PATCH] fix sata_sil quirk

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



2004-06-22 20:58:38

by Ricky Beam

[permalink] [raw]
Subject: Re: [PATCH] fix sata_sil quirk

===== 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 */

2004-06-22 22:18:33

by Jeff Garzik

[permalink] [raw]
Subject: Re: [PATCH] fix sata_sil quirk

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


2004-06-23 00:40:42

by Ricky Beam

[permalink] [raw]
Subject: Re: [PATCH] fix sata_sil quirk

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


2004-06-23 03:25:13

by Paul Jakma

[permalink] [raw]
Subject: Re: [PATCH] fix sata_sil quirk

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 [email protected] [email protected] Key ID: 64A2FF6A
warning: do not ever send email to [email protected]
Fortune:
A bird in the hand makes it awfully hard to blow your nose.

2004-06-23 03:41:06

by Jeff Garzik

[permalink] [raw]
Subject: Re: [PATCH] fix sata_sil quirk

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


2004-06-23 03:47:03

by Paul Jakma

[permalink] [raw]
Subject: Re: [PATCH] fix sata_sil quirk

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 [email protected] [email protected] Key ID: 64A2FF6A
warning: do not ever send email to [email protected]
Fortune:
It is so soon that I am done for, I wonder what I was begun for.
-- Epitaph, Cheltenham Churchyard

2004-06-23 03:52:14

by Jeff Garzik

[permalink] [raw]
Subject: Re: [PATCH] fix sata_sil quirk

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;
}

2004-06-23 06:40:46

by Ricky Beam

[permalink] [raw]
Subject: Re: [PATCH] fix sata_sil quirk

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.


2004-06-28 01:54:54

by George Georgalis

[permalink] [raw]
Subject: Re: [PATCH] fix sata_sil quirk

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?

// George


--
George Georgalis, Architect and administrator, Linux services. IXOYE
http://galis.org/george/ cell:646-331-2027 mailto:[email protected]
Key fingerprint = 5415 2738 61CF 6AE1 E9A7 9EF0 0186 503B 9831 1631

2004-09-27 03:40:18

by George Georgalis

[permalink] [raw]
Subject: Re: [PATCH] fix sata_sil quirk

On Sun, 27 Jun 2004 21:54:31 -0400, George Georgalis <[email protected]> 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:[email protected]