[PATCH] New QStor SATA/RAID Driver for 2.6.9-rc2

[PATCH] New QStor SATA/RAID Driver for 2.6.9-rc2

[Mark Lord]

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

My first attempt at posting this seems to have gone AWOL,
so here it comes again.  Also being posted to linux-scsi.

Here is the first public release of the 2.6.xx driver
source code for the Pacific Digital Corporation QStor SATA/RAID chip.

This 4-channel chip has hardware-assisted RAID0/RAID1/RAID10,
host-queuing, per-request TCQ/NCQ support, support for hot insertion
and removal of drives, etc..  The 64-bit/66Mhz chip shows throughput
in excess of 200MByte/sec on my ancient P3-1GHz test system,
and can do much better when installed in a PCI-X slot.

The driver (attached) supports most of the chip features,
including host, native and legacy tagged queuing,
but does not yet include boot-from-raid support (coming soon).

Both hdparm and smartmontools are fully supported by this driver.

This patch is against linux-2.6.9-rc2.

Please email me any errors or corrections you may deem necessary
for kernel inclusion.

Signed-off-by: Mark Lord <mlord@pobox.com>
-- 
Mark Lord
(hdparm keeper & the original "Linux IDE Guy")

[-- Attachment #2: qstor.patch-2.6.9-rc2.gz --]
[-- Type: application/x-gzip, Size: 35858 bytes --]

Re: [PATCH] New QStor SATA/RAID Driver for 2.6.9-rc2

[Jeff Garzik]

Mark Lord wrote:
> My first attempt at posting this seems to have gone AWOL,
> so here it comes again.  Also being posted to linux-scsi.
> 
> Here is the first public release of the 2.6.xx driver
> source code for the Pacific Digital Corporation QStor SATA/RAID chip.
> 
> This 4-channel chip has hardware-assisted RAID0/RAID1/RAID10,
> host-queuing, per-request TCQ/NCQ support, support for hot insertion
> and removal of drives, etc..  The 64-bit/66Mhz chip shows throughput
> in excess of 200MByte/sec on my ancient P3-1GHz test system,
> and can do much better when installed in a PCI-X slot.

How much of the RAID is actually hardware-assisted?


> The driver (attached) supports most of the chip features,
> including host, native and legacy tagged queuing,
> but does not yet include boot-from-raid support (coming soon).
> 
> Both hdparm and smartmontools are fully supported by this driver.

Linus vetoed future SCSI->ATA translators.  He only allowed libata 
because I promised to remove the translation and make it a native block 
driver in the future, which I have been working towards.

	Jeff

Re: [PATCH] New QStor SATA/RAID Driver for 2.6.9-rc2

[Jeff Garzik]

Jeff Garzik wrote:
> Linus vetoed future SCSI->ATA translators.  He only allowed libata 
> because I promised to remove the translation and make it a native block 
> driver in the future, which I have been working towards.


This is why the Promise SX8 driver is a block driver, for example...

	Jeff

Re: [PATCH] New QStor SATA/RAID Driver for 2.6.9-rc2

[Nathan Bryant]

Jeff Garzik wrote:

> Linus vetoed future SCSI->ATA translators.  He only allowed libata 
> because I promised to remove the translation and make it a native block 
> driver in the future, which I have been working towards.

Oh, this is incredibly good to hear. It definitely clarifies the path 
towards implementing new features such as power management...

Is this written up in a FAQ somewhere?

Nathan

Re: [PATCH] New QStor SATA/RAID Driver for 2.6.9-rc2

[Jeff Garzik]

Nathan Bryant wrote:
> Jeff Garzik wrote:
> 
>> Linus vetoed future SCSI->ATA translators.  He only allowed libata 
>> because I promised to remove the translation and make it a native 
>> block driver in the future, which I have been working towards.
> 
> 
> Oh, this is incredibly good to hear. It definitely clarifies the path 
> towards implementing new features such as power management...
> 
> Is this written up in a FAQ somewhere?

Not AFAIK.

The basic rule is "don't translate the native protocol into something 
else." and it has been in place for quite a while.

I was allowed a one-time exception for libata, which translates the 
native ATA into SCSI.  The SCSI portions are separated into 
libata-scsi.c so that they may be easily excised or made optional (likely).

	Jeff

Re: [PATCH] New QStor SATA/RAID Driver for 2.6.9-rc2

[Douglas Gilbert]

Jeff Garzik wrote:
> Mark Lord wrote:
> 
>> My first attempt at posting this seems to have gone AWOL,
>> so here it comes again.  Also being posted to linux-scsi.
>>
>> Here is the first public release of the 2.6.xx driver
>> source code for the Pacific Digital Corporation QStor SATA/RAID chip.
>>
>> This 4-channel chip has hardware-assisted RAID0/RAID1/RAID10,
>> host-queuing, per-request TCQ/NCQ support, support for hot insertion
>> and removal of drives, etc..  The 64-bit/66Mhz chip shows throughput
>> in excess of 200MByte/sec on my ancient P3-1GHz test system,
>> and can do much better when installed in a PCI-X slot.
> 
> 
> How much of the RAID is actually hardware-assisted?
> 
> 
>> The driver (attached) supports most of the chip features,
>> including host, native and legacy tagged queuing,
>> but does not yet include boot-from-raid support (coming soon).
>>
>> Both hdparm and smartmontools are fully supported by this driver.

Mark,
I'm curious how smartmontools is supported. Does the driver
support SCSI LOG SENSE commands on the physical units or does
it take the 3ware/Marvell route?
There is no sign of QStor specific code in smartmontools's CVS.

> Linus vetoed future SCSI->ATA translators.  He only allowed libata 
> because I promised to remove the translation and make it a native block 
> driver in the future, which I have been working towards.

As Jeff is aware, SCSI->ATA translation (SAT) is in
the process of being standardized at t10.org . The
model being used outlines a SAT layer going into any
of three places:
   - in the host OS above a SATA/ATA HBA driver (i.e.
     what libata does and Linus frowns upon)
   - in the host OS above a SAS HBA driver which,
     amongst other protocols, has the SAS Tunnelling Protocol
     (STP) which conveys ATA/ATAPI7 commands through SAS
     infrastructure
   - somewhere in the service delivery subsystem, specifically
     in SAS expanders which have phys connected to SATA disks
     (Linus vetoes have no influence here)

Consider this pathological situation:
Start with one SATA II disk and connect it to a port selector
(a SATA II device) which effectively makes the SATA disk dual
ported. Connect one of those ports to a SATA HBA that lives
in the Linux ATA subsystem (where no SAT is allowed). Connect the
other port to a SAS expander which includes a SAT layer and then
connect that expander to a SAS HBA in the Linux SCSI subsytem.
That should confuse the !@#$ out of any application client
trying to get a handle on what is happening.

I assume that OS-based ATA->SCSI translation is also out of
the question.

Doug Gilbert

Re: [PATCH] New QStor SATA/RAID Driver for 2.6.9-rc2

[Jeff Garzik]

On Wed, Sep 15, 2004 at 02:22:36PM +1000, Douglas Gilbert wrote:
> I assume that OS-based ATA->SCSI translation is also out of
> the question.


In order to keep compatibility with existing libata users after libata
becomes a block driver, the ATA->SCSI translation will live on as a
completely separate, independent, optional kernel module.

If we are really lucky we can use it as a library for some of the other
drivers that do some amount of ATA->SCSI translation, or ide-scsi...

	Jeff

Re: [PATCH] New QStor SATA/RAID Driver for 2.6.9-rc2

[Mark Lord]

Douglas Gilbert wrote:
>>>
>>> Both hdparm and smartmontools are fully supported by this driver.
> 
> I'm curious how smartmontools is supported. Does the driver
> support SCSI LOG SENSE commands on the physical units or does
> it take the 3ware/Marvell route?
> There is no sign of QStor specific code in smartmontools's CVS.

This driver simply implements the HDIO_DRIVE_CMD/HDIO_DRIVE_TASK
ioctl call (from Linux IDE).  By doing so, it automatically gains
compatibility with smartmontools (-d ata), and with hdparm.

Currently, the implementation of those is native to the controller.
But eventually, I'd like to see it implement Curtis's ATA Passthrough
spec (to which I've been contributing).

Port selectors aside --> a nightmare for any sysadmin or driver writer
--> Linux has a huge unresolved issue for block devices.

It is currently next-to-impossible for a vendor to support a
new drive controller card under Linux unless it uses either SCSI
or libata.  Doing one as a new block driver is not an option,
because of the installation issues for the end-user during the
first couple of years of deployment --> until the distros begin
to include the required /dev/ entries everywhere.

To solve this requires a nice generic /dev/ interface for disks,
common to all drivers which talk to such devices.

Currently on Linux, that interface is called "SCSI".
I think it might not be unreasonable to gradually evolve
the SCSI host interface to include, say, a non-translating
queuecommand() method, and associated pals.

This would get rid of much (not all) of the double-translation objection,
and provide a smooth path for supporting Firewire, USB, parallel-port,
SATA, ATA, and of course SCSI, disks all under one naming subsystem.

We practically have that already today.
The SCSI mid-layer is a nice generic block device glue system.
We just need perhaps to make it less SCSI-specific.

Cheers
-- 
Mark Lord
(hdparm keeper & the original "Linux IDE Guy")

Re: [PATCH] New QStor SATA/RAID Driver for 2.6.9-rc2

[Jens Axboe]

On Wed, Sep 15 2004, Mark Lord wrote:
> It is currently next-to-impossible for a vendor to support a
> new drive controller card under Linux unless it uses either SCSI
> or libata.  Doing one as a new block driver is not an option,
> because of the installation issues for the end-user during the
> first couple of years of deployment --> until the distros begin
> to include the required /dev/ entries everywhere.
> 
> To solve this requires a nice generic /dev/ interface for disks,
> common to all drivers which talk to such devices.
> 
> Currently on Linux, that interface is called "SCSI".
> I think it might not be unreasonable to gradually evolve
> the SCSI host interface to include, say, a non-translating
> queuecommand() method, and associated pals.
> 
> This would get rid of much (not all) of the double-translation objection,
> and provide a smooth path for supporting Firewire, USB, parallel-port,
> SATA, ATA, and of course SCSI, disks all under one naming subsystem.
> 
> We practically have that already today.
> The SCSI mid-layer is a nice generic block device glue system.
> We just need perhaps to make it less SCSI-specific.

This is what we have been doing for quite some time. If you look, a lot
of the helper functions have actually been moved _out_ of SCSI and into
the block layer. Most of it is already there. The SCSI mid layer is
getting smaller, not bigger.

The remaining piece (well the biggest one at least) is a nice /dev/disk
abstraction. That's currently the biggest obstacle to nice generic block
drivers, not the infrastructure.

-- 
Jens Axboe

Re: [PATCH] New QStor SATA/RAID Driver for 2.6.9-rc2

[Matthew Wilcox]

On Wed, Sep 15, 2004 at 02:55:46PM +0200, Jens Axboe wrote:
> The remaining piece (well the biggest one at least) is a nice /dev/disk
> abstraction. That's currently the biggest obstacle to nice generic block
> drivers, not the infrastructure.

I'll just take this opportunity to plug /dev/drive over /dev/disk.
Not only does it avoid the disc/disk dialect problem, it is also more
accurate in the case of removable media.

-- 
"Next the statesmen will invent cheap lies, putting the blame upon 
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince 
himself that the war is just, and will thank God for the better sleep 
he enjoys after this process of grotesque self-deception." -- Mark Twain

Re: [PATCH] New QStor SATA/RAID Driver for 2.6.9-rc2

[Jeff Garzik]

Matthew Wilcox wrote:
> On Wed, Sep 15, 2004 at 02:55:46PM +0200, Jens Axboe wrote:
> 
>>The remaining piece (well the biggest one at least) is a nice /dev/disk
>>abstraction. That's currently the biggest obstacle to nice generic block
>>drivers, not the infrastructure.
> 
> 
> I'll just take this opportunity to plug /dev/drive over /dev/disk.


I'll take this opportunity to recommend that posters avoid following 
this sub-thread, for fear of distracting from the real technical issues....

	Jeff

Re: [PATCH] New QStor SATA/RAID Driver for 2.6.9-rc2

[Christoph Hellwig]

Please fix all the style issues in here, aka

 - linebreaks after 80 characters
 - don't typedef structs unless they're used opaqueuely
 - please avoid C++-style comments even if C99 allows them
 - don't use "scsi.h", only headers from <scsi/*.h>, that also
   means getting rid of the Scsi_Foo typedefs
 - please don't include headers in other headers unless nessecary

I also don't want another SCSI->ATA translator in drivers/scsi, please find
some way to share codce with an existing one.

I'll do a real review once the driver is readable.

Re: [PATCH] New QStor SATA/RAID Driver for 2.6.9-rc2

[Jeff Garzik]

Mark Lord wrote:
> My first attempt at posting this seems to have gone AWOL,
> so here it comes again.  Also being posted to linux-scsi.

There is that CC feature in your mailer, you know... :)

Repeating what I posted to linux-scsi:


> Here is the first public release of the 2.6.xx driver
> source code for the Pacific Digital Corporation QStor SATA/RAID chip.
> 
> This 4-channel chip has hardware-assisted RAID0/RAID1/RAID10,
> host-queuing, per-request TCQ/NCQ support, support for hot insertion
> and removal of drives, etc..  The 64-bit/66Mhz chip shows throughput
> in excess of 200MByte/sec on my ancient P3-1GHz test system,
> and can do much better when installed in a PCI-X slot.

How much of the RAID is actually hardware-assisted?

This looks pretty much like an ATA driver to me.

Linus vetoed future SCSI->ATA translators.  He only allowed libata 
because I promised to remove the translation and make it a native block 
driver in the future, which I have been working towards (see struct 
ata_queued_cmd, etc.)


> The driver (attached) supports most of the chip features,
> including host, native and legacy tagged queuing,
> but does not yet include boot-from-raid support (coming soon).
> 
> Both hdparm and smartmontools are fully supported by this driver.

Actual comments on the code:
0) so far, this driver looks like fake RAID to me...  if so that's a big 
veto.  if it's real RAID, only the following grumbles apply :)

1) not endian safe at all

2) I am dubious about including Yet Another set of event logging functions

3) in qstor_read_events you unlock the spinlock without first locking 
it, in one path (wait_event_interruptible rc==0)

4) qstor_extract_id_string appears to be generic across IDE/libata/qstor

5) new procfs stuff discouraged

6) style:  ditch braces on single statements, e.g.,

+               if (drive != NULL) {
+                       qstor_destroy_device(drive);
+               }


7) double spin_unlock_irq possible in qstor_create_device

8) use msleep() rather than schedule_timeout()

9) use of do_sleep paradigm is dubious:  you should instead try to keep 
your locked code regions as small as possible.  in general, this code 
has far too many unlock-doit-lock sections.

Experience has shown that too much unlock-doit-lock leads to bugs and 
increases the pain when analyzing your locking.

In particular, releasing the lock and sleeping would be very wrong in 
the ->queuecommand and error handling paths (alas...  I would love to 
sleep in the fine-grained eh hooks)

10) in qstor_scsi_done, when is cmd->scsi_done ever NULL?

11) do you properly keep track of the 'done' function passed to you in 
->queuecommand?  or do you mistakenly assume that cmd->scsi_done is the 
same as the ->queuecommand argument?

12) fix the sd.c code, don't add silly driver-specific workarounds:

+       buf[0] = TYPE_DISK;     // Cannot use TYPE_RAID -- sd.c rejects it

13) doh!  check for pci_map_xxx failure

14) use sg_dma_len() macro, not sg->length

15) Bart and I are slowly moving over to using linux/ata.h for 
ATA-generic constants and enums.  Please use ATA_CMD_xxx (and add 
constants to that header as required).

16) There are WAY too many magic numbers in this driver :(

17) Are you 100% certain of your queued error handling?  The reason why 
libata doesn't do NCQ is purely because error handling is so 
complicated.  Potential problems I don't see you handling (but I could 
be missing something!):

	a) on a bus error (not device error), one has _no idea_ which
	commands complete etc.
	b) if SERVICE interrupt is enabled, you may not get back the
	correct D2H Register FIS showing the errored device in question
	c) even if you receive the correct D2H Register FIS, you may
	need to manually abort the queue with a NOP

Queueing is easy.  Picking up the pieces when it fails isn't.

libata's error handling is dumb, but also easy to review and verify.

18) return -EFOO values from your PCI probe function

19) propagate return value from pci_enable_device

20) check (and return) pci_set_dma_mask retval

21) use the proper ULL suffix for pci_set_dma_mask argument

22) use pci_set_consistent_dma_mask also

23) use pci_request_regions to reserve resources

24) when qstor_probe fails, don't just return!  undo the stuff that 
occurred before the error (such as calling pci_disable_device or 
pci_release_regions or iounmap)

25) propagate return value from request_irq

26) check scsi_add_host return value

27) the following is highly silly.  you are locking a function, just so 
you can unlock a function so you can sleep.

+       spin_lock_irqsave(uhba->lock, flags);
+       qstor_reset(uhba);
+       spin_unlock_irqrestore(uhba->lock, flags);

28) "SECTOR_BYTES" -- how many more definitions of the 512-byte sector 
do we need?  :)

29) You use QSTOR_PACKED_STRUCTURE when not needed, which causes gcc to 
generate horribly sub-optimal code

30) style:  use u32/u64 as kernel standard.

+       unsigned pLEN           :32;    // Byte count
+       unsigned spare32        :32;    // 0

31) none of your bitfield structures are endian-safe

Re: [PATCH] New QStor SATA/RAID Driver for 2.6.9-rc2

[Mark Lord]

Thanks Jeff,

I'll look into most of your points and give responses and changes
where required.  But first, a few overall notes on the approach.

This is a hardware RAID device, but it requires driver knowledge
of the RAID features.  It does not map to libata at all, unfortunately,
because all of the queuing features are completely non-SATA standard,
and the RAID stuff is (as normal) peculiar to the chip.

Here's a question for you:  like all of the other RAID drivers,
this one needs an interface to a userland RAID management GUI.

The usual method for this is to create a fake character device driver,
and use that as the interface to userland.  This is commonly done,
but is it the best way to handle such?  A /proc/ or /sys/ interface
could achieve similar goals, but without the need of a fake device.

We can go either way with this one, so lets hear some opinions on it.

For the rest, this driver has been around (vendor driver) since before
libata became usable, and certainly before libata existed in 2.4.xx.
The driver will eventuall need to compile and run in 2.4.20,
for customers using old Redhat kernels.   It's not there yet,
but if it were to lean more heavily on 2.6.xx stuff,
then that will be more difficult to achieve.

Cheers
-- 
Mark Lord
(hdparm keeper & the original "Linux IDE Guy")

Re: [PATCH] New QStor SATA/RAID Driver for 2.6.9-rc2

[Jeff Garzik]

On Tue, Sep 14, 2004 at 01:27:54PM -0400, Mark Lord wrote:
> Here's a question for you:  like all of the other RAID drivers,
> this one needs an interface to a userland RAID management GUI.
> 
> The usual method for this is to create a fake character device driver,
> and use that as the interface to userland.  This is commonly done,
> but is it the best way to handle such?  A /proc/ or /sys/ interface
> could achieve similar goals, but without the need of a fake device.
> 
> We can go either way with this one, so lets hear some opinions on it.

Well,

* if the userland interface is 100% sending cdbs or taskfiles, then I
would prefer that Jens Axboe's "bsg" be used.  Its a chardev interface
for sending/receiving commands to a request queue.

* otherwise, I would pick either chrdev or sysfs.  if you gotta support
2.4, I guess that means chrdev.


> For the rest, this driver has been around (vendor driver) since before
> libata became usable, and certainly before libata existed in 2.4.xx.
> The driver will eventuall need to compile and run in 2.4.20,
> for customers using old Redhat kernels.   It's not there yet,
> but if it were to lean more heavily on 2.6.xx stuff,
> then that will be more difficult to achieve.

libata and all its drivers work on RHEL2.1 (2.4.9), and someone is
even crazy enough to be porting libata to 2.2.x ;-)

	Jeff

Re: [PATCH] New QStor SATA/RAID Driver for 2.6.9-rc2

[Mark Lord]

 >In particular, releasing the lock and sleeping would be very wrong
 >in the ->queuecommand and error handling paths
 >(alas...  I would love to sleep in the fine-grained eh hooks)

Mmm.. definitely no sleeps in queuecommand(), but sleeping seems
necessary in host_reset_handler() -- the alternative is to just
busywait inline.. which would really not be good.

Isn't the protocol for the eh host_reset_handler() basically
just "do the reset, and return whether it worked or not?".
If so, the driver really has to hang around until the reset
completes so that correct status can be returned.  This generally
takes a couple of milliseconds in practice (measured it).

Is there a better way to do that?

I really would prefer never to have to reset the drives,
but when they have a queuing error, many of them simply
won't talk to us again without a reset.  The driver avoids
the reset as much as it can for other situations, though.

Cheers
-- 
Mark Lord
(hdparm keeper & the original "Linux IDE Guy")

Re: [PATCH] New QStor SATA/RAID Driver for 2.6.9-rc2

[Jeff Garzik]

Mark Lord wrote:
>  >In particular, releasing the lock and sleeping would be very wrong
>  >in the ->queuecommand and error handling paths
>  >(alas...  I would love to sleep in the fine-grained eh hooks)
> 
> Mmm.. definitely no sleeps in queuecommand(), but sleeping seems
> necessary in host_reset_handler() -- the alternative is to just
> busywait inline.. which would really not be good.
> 
> Isn't the protocol for the eh host_reset_handler() basically
> just "do the reset, and return whether it worked or not?".
> If so, the driver really has to hang around until the reset
> completes so that correct status can be returned.  This generally
> takes a couple of milliseconds in practice (measured it).
> 
> Is there a better way to do that?
> 
> I really would prefer never to have to reset the drives,
> but when they have a queuing error, many of them simply
> won't talk to us again without a reset.  The driver avoids
> the reset as much as it can for other situations, though.


James and I occasionally talk about this.  This is a _big_ reason why I 
use the ->eh_strategy_handler() rather than the more fine-grained ->eh 
hooks:  you get unlocked, unfettered sleep priveleges inside the scsi EH 
thread.

The SCSI LLD API really needs to -not- spinlock on the EH hooks, and 
instead simply guarantee that ->queuecommand and other hooks will not be 
called while the driver is in EH.

ISTR James didn't disagree, so maybe a patch can be worked out...

Of course, you could always just use ->eh_strategy_handler and do 100% 
of the error handling yourself.  That's the route libata chose.

	Jeff

Re: [PATCH] New QStor SATA/RAID Driver for 2.6.9-rc2

[Mark Lord]

 >The SCSI LLD API really needs to -not- spinlock on the EH hooks,
 >and instead simply guarantee that ->queuecommand and other hooks
 >will not be called while the driver is in EH.
 >
 >ISTR James didn't disagree, so maybe a patch can be worked out...

It looks to me as if the eh code prevents further queuecommand()
calls while the LLD *_reset_handler() code is running.
I wonder if it also does so for the eh_strategy_handler() ?

Have to look at it, I guess.

 >Of course, you could always just use ->eh_strategy_handler
 >and do 100% of the error handling yourself.

Mmmm.. yes, that may be better, perhaps.

Whatever this driver does, it has to be reasonably portable
back to early 2.4.xx kernels, so it cannot depend too much
upon newly (or to-be) implemented semantics in 2.6.xx.

Cheers
-- 
Mark Lord
(hdparm keeper & the original "Linux IDE Guy")

Re: [PATCH] New QStor SATA/RAID Driver for 2.6.9-rc2

[Jeff Garzik]

On Tue, Sep 14, 2004 at 02:03:28PM -0400, Mark Lord wrote:
> Whatever this driver does, it has to be reasonably portable
> back to early 2.4.xx kernels, so it cannot depend too much
> upon newly (or to-be) implemented semantics in 2.6.xx.

Feel free to examine libata's use of ->eh_strategy_handler
in 2.4.x ;-)

	Jeff

Re: [PATCH] New QStor SATA/RAID Driver for 2.6.9-rc2

[Jeff Garzik]

On Tue, Sep 14, 2004 at 02:03:28PM -0400, Mark Lord wrote:
> It looks to me as if the eh code prevents further queuecommand()
> calls while the LLD *_reset_handler() code is running.
> I wonder if it also does so for the eh_strategy_handler() ?

Yes, it definitely does, by definition:

SCSI's fine-grained eh hooks are called inside scsi_unjam_host(),
which is the default ->eh_strategy_handler.

	Jeff

Re: [PATCH] New QStor SATA/RAID Driver for 2.6.9-rc2

[James Bottomley]

On Tue, 2004-09-14 at 13:00, Jeff Garzik wrote:
> 9) use of do_sleep paradigm is dubious:  you should instead try to keep 
> your locked code regions as small as possible.  in general, this code 
> has far too many unlock-doit-lock sections.
> 
> Experience has shown that too much unlock-doit-lock leads to bugs and 
> increases the pain when analyzing your locking.
> 
> In particular, releasing the lock and sleeping would be very wrong in 
> the ->queuecommand and error handling paths (alas...  I would love to 
> sleep in the fine-grained eh hooks)

Actually, its only wrong in queuecommand because that can be called in
softirq context.

Sleeping in the eh paths is fine (as long as you drop the locks that the
EH thread has uselessly taken for you).  Indeed it's often required
since the return is supposed to tell the eh thread whether the action
was successful or not.

James

Re: [PATCH] New QStor SATA/RAID Driver for 2.6.9-rc2

[Jeff Garzik]

On Tue, Sep 14, 2004 at 02:25:35PM -0400, James Bottomley wrote:
> Sleeping in the eh paths is fine (as long as you drop the locks that the
> EH thread has uselessly taken for you).  Indeed it's often required
> since the return is supposed to tell the eh thread whether the action
> was successful or not.

I'm not sure this true for all arches?

The lock is taken in the SCSI layer with spin_lock_irqsave(), but the
low-level driver cannot perform the exact opposite,
spin_unlock_irqrestore().  The best they can do is spin_lock_irq(),
which isnt 100% the same.

	Jeff

Re: [PATCH] New QStor SATA/RAID Driver for 2.6.9-rc2

[James Bottomley]

On Tue, 2004-09-14 at 14:35, Jeff Garzik wrote:
> The lock is taken in the SCSI layer with spin_lock_irqsave(), but the
> low-level driver cannot perform the exact opposite,
> spin_unlock_irqrestore().  The best they can do is spin_lock_irq(),
> which isnt 100% the same.

That's what they do if you look.

The eh_ stubs are only called from the eh_ thread, so it's safe to
enable interrupts as well.

The business of the mid-layer taking the locks is an annoying holdover
from the "drivers don't need to do locking" mentality.  Unfortunately
most drivers now simply drop the locks immediately they begin an eh_
entry point and reacquire them just prior to returning ... which makes
all the eh code look messy.

James

Re: [PATCH] New QStor SATA/RAID Driver for 2.6.9-rc2

[Mark Lord]

 >Actually, its only wrong in queuecommand because that can be called in
 >softirq context.
 >
 >Sleeping in the eh paths is fine (as long as you drop the locks that the
 >EH thread has uselessly taken for you).

Good, that's how I understood it as well.

But the locking is certainly a mess as-is in the QStor driver.
Sure, it is actually all technically correct, but hard to follow.

I believe I can remove nearly all of it and really tidy things up
as a result.

Thanks guys, this has been really helpful so far.
-- 
Mark Lord
(hdparm keeper & the original "Linux IDE Guy")

Re: [PATCH] New QStor SATA/RAID Driver for 2.6.9-rc2

[Jeff Garzik]

Groovy.  FWIW (if it wasn't obvious from context) my objection in
general to the driver is withdrawn, since you explained it is RAID and
not an ATA driver.

I would really like to work on consolidating the ATA code in libata,
though.  As the name implies, it's a library -- don't feel that your
driver must conform to the libata driver API in order to make use of all
its functions.  And feel free to add to it.

	Jeff

Re: [PATCH] New QStor SATA/RAID Driver for 2.6.9-rc2

[Mark Lord]

I would really like to work on consolidating the ATA code in libata,
though.  As the name implies, it's a library -- don't feel that your
driver must conform to the libata driver API in order to make use of all
its functions.  And feel free to add to it.

Yes, there are definite code sharing possibilities there to be explored.
Right now, my first priority is to get support for this hardware
into the kernel.  This same driver source will also be backported
to mid-2.4.xx series, both Redhat and generic.

After that, we can modify some interfaces to reduce the small overlaps
that may present.

Next revision is due out later today.  It may still have a few warts
to work out, but I think it is looking much better than before.

Better to have a decent hardware driver within the tree,
than an unknown vendor-only binary driver outside the tree.

Cheers
-- 
Mark Lord
(hdparm keeper & the original "Linux IDE Guy")