[PATCH 1/2] libata: cosmetic changes to constants

[PATCH 1/2] libata: cosmetic changes to constants

[Tejun Heo]

Cosmetic changes to ATA_DFLAG_* constants for soon-to-follow NCQ-off
patch.
---
 include/linux/libata.h |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/libata.h b/include/linux/libata.h
index df44b09..e54a5fd 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -142,8 +142,8 @@ enum {
 	ATA_DFLAG_NCQ		= (1 << 3), /* device supports NCQ */
 	ATA_DFLAG_CFG_MASK	= (1 << 8) - 1,
 
-	ATA_DFLAG_PIO		= (1 << 8), /* device currently in PIO mode */
-	ATA_DFLAG_SUSPENDED	= (1 << 9), /* device suspended */
+	ATA_DFLAG_PIO		= (1 << 8), /* device limited to PIO mode */
+	ATA_DFLAG_SUSPENDED	= (1 << 10), /* device suspended */
 	ATA_DFLAG_INIT_MASK	= (1 << 16) - 1,
 
 	ATA_DFLAG_DETACH	= (1 << 16),
-- 
1.4.2.1

[PATCH 2/2] libata: turn off NCQ if queue depth is adjusted to 1

[Tejun Heo]

Turn off NCQ if queue depth is adjusted to 1.
---
 drivers/ata/libata-scsi.c |   12 +++++++++++-
 include/linux/libata.h    |    1 +
 2 files changed, 12 insertions(+), 1 deletions(-)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 3986ec8..bea9f7a 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -889,6 +889,7 @@ int ata_scsi_change_queue_depth(struct s
 {
 	struct ata_port *ap = ata_shost_to_port(sdev->host);
 	struct ata_device *dev;
+	unsigned long flags;
 	int max_depth;
 
 	if (queue_depth < 1)
@@ -904,6 +905,14 @@ int ata_scsi_change_queue_depth(struct s
 		queue_depth = max_depth;
 
 	scsi_adjust_queue_depth(sdev, MSG_SIMPLE_TAG, queue_depth);
+
+	spin_lock_irqsave(ap->lock, flags);
+	if (queue_depth > 1)
+		dev->flags &= ~ATA_DFLAG_NCQ_OFF;
+	else
+		dev->flags |= ATA_DFLAG_NCQ_OFF;
+	spin_unlock_irqrestore(ap->lock, flags);
+
 	return queue_depth;
 }
 
@@ -1293,7 +1302,8 @@ static unsigned int ata_scsi_rw_xlat(str
 		 */
 		goto nothing_to_do;
 
-	if ((dev->flags & (ATA_DFLAG_PIO | ATA_DFLAG_NCQ)) == ATA_DFLAG_NCQ) {
+	if ((dev->flags & (ATA_DFLAG_PIO | ATA_DFLAG_NCQ_OFF |
+			   ATA_DFLAG_NCQ)) == ATA_DFLAG_NCQ) {
 		/* yay, NCQ */
 		if (!lba_48_ok(block, n_block))
 			goto out_of_range;
diff --git a/include/linux/libata.h b/include/linux/libata.h
index e54a5fd..d1af1db 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -143,6 +143,7 @@ enum {
 	ATA_DFLAG_CFG_MASK	= (1 << 8) - 1,
 
 	ATA_DFLAG_PIO		= (1 << 8), /* device limited to PIO mode */
+	ATA_DFLAG_NCQ_OFF	= (1 << 9), /* devied limited to non-NCQ mode */
 	ATA_DFLAG_SUSPENDED	= (1 << 10), /* device suspended */
 	ATA_DFLAG_INIT_MASK	= (1 << 16) - 1,
 
-- 
1.4.2.1

Re: [PATCH 2/2] libata: turn off NCQ if queue depth is adjusted to 1

[Jens Axboe]

On Sat, Sep 30 2006, Tejun Heo wrote:
> Turn off NCQ if queue depth is adjusted to 1.

I had thought of that too but discarded it - it would be nicer to have
an independent way of turning off NCQ. Say you are debugging a weird FIS
issue, NCQ depth 1 is still a different beast to non-NCQ. I see Jeff
already applied the patches, but just thought I'd voice my opinion.

-- 
Jens Axboe

Re: [PATCH 2/2] libata: turn off NCQ if queue depth is adjusted to 1

[Alan Cox]

Ar Sad, 2006-09-30 am 20:04 +0200, ysgrifennodd Jens Axboe:
> On Sat, Sep 30 2006, Tejun Heo wrote:
> > Turn off NCQ if queue depth is adjusted to 1.
> 
> I had thought of that too but discarded it - it would be nicer to have
> an independent way of turning off NCQ. Say you are debugging a weird FIS
> issue, NCQ depth 1 is still a different beast to non-NCQ. I see Jeff
> already applied the patches, but just thought I'd voice my opinion.

I've got a blacklist for NCQ in my work tree but really we need the
vendors to help fill it. So far it has some raptors in it but I am sure
there are more, and I am sure there are cases we should be advising
newer firmware or just tweaking our queue sizes etc.

Alan

Re: [PATCH 2/2] libata: turn off NCQ if queue depth is adjusted to 1

[Jens Axboe]

On Sat, Sep 30 2006, Alan Cox wrote:
> Ar Sad, 2006-09-30 am 20:04 +0200, ysgrifennodd Jens Axboe:
> > On Sat, Sep 30 2006, Tejun Heo wrote:
> > > Turn off NCQ if queue depth is adjusted to 1.
> > 
> > I had thought of that too but discarded it - it would be nicer to have
> > an independent way of turning off NCQ. Say you are debugging a weird FIS
> > issue, NCQ depth 1 is still a different beast to non-NCQ. I see Jeff
> > already applied the patches, but just thought I'd voice my opinion.
> 
> I've got a blacklist for NCQ in my work tree but really we need the
> vendors to help fill it. So far it has some raptors in it but I am sure
> there are more, and I am sure there are cases we should be advising
> newer firmware or just tweaking our queue sizes etc.

Lots of the older Maxtors are pretty crappy for NCQ, so those too. Queue
size tweaks fixed them for me (as low as 4, and I can't say for sure if
it fixes all crashes).

-- 
Jens Axboe

Re: [PATCH 2/2] libata: turn off NCQ if queue depth is adjusted to 1

[Ric Wheeler]

Jens Axboe wrote:

>On Sat, Sep 30 2006, Alan Cox wrote:
>  
>
>>Ar Sad, 2006-09-30 am 20:04 +0200, ysgrifennodd Jens Axboe:
>>    
>>
>>>On Sat, Sep 30 2006, Tejun Heo wrote:
>>>      
>>>
>>>>Turn off NCQ if queue depth is adjusted to 1.
>>>>        
>>>>
>>>I had thought of that too but discarded it - it would be nicer to have
>>>an independent way of turning off NCQ. Say you are debugging a weird FIS
>>>issue, NCQ depth 1 is still a different beast to non-NCQ. I see Jeff
>>>already applied the patches, but just thought I'd voice my opinion.
>>>      
>>>
>>I've got a blacklist for NCQ in my work tree but really we need the
>>vendors to help fill it. So far it has some raptors in it but I am sure
>>there are more, and I am sure there are cases we should be advising
>>newer firmware or just tweaking our queue sizes etc.
>>    
>>
>
>Lots of the older Maxtors are pretty crappy for NCQ, so those too. Queue
>size tweaks fixed them for me (as low as 4, and I can't say for sure if
>it fixes all crashes).
>
>  
>
We have to be careful with the blacklisting - specifically, drive model 
is often less critical than the version of the firmware (which gets 
updated as people work drives through qualification, etc). 

Updating drive firmware for end users is pretty rare (and almost all 
tools are still DOS based ;-)).

Certain drives should default to non-NCQ (based on model), but we should 
be able to enable it if the firmware supports it.

Most of the newest drives are fine, but we will still need something 
like Tejun's fix to be able to turn it off for the odd cases that show 
up in certain odd applications, etc.

Re: [PATCH 2/2] libata: turn off NCQ if queue depth is adjusted to 1

[Tejun Heo]

Ric Wheeler wrote:
> Jens Axboe wrote:
>> On Sat, Sep 30 2006, Alan Cox wrote:
>>> Ar Sad, 2006-09-30 am 20:04 +0200, ysgrifennodd Jens Axboe:
>>>> On Sat, Sep 30 2006, Tejun Heo wrote:
>>>>     
>>>>> Turn off NCQ if queue depth is adjusted to 1.
>>>>>       
>>>> I had thought of that too but discarded it - it would be nicer to have
>>>> an independent way of turning off NCQ. Say you are debugging a weird 
>>>> FIS
>>>> issue, NCQ depth 1 is still a different beast to non-NCQ. I see Jeff
>>>> already applied the patches, but just thought I'd voice my opinion.

Yeah, I was putting off this patch for the same reason.  0 depth would 
have been nice but SCSI midlayer filters such input and libata still 
doesn't have its own sysfs tree.  So, this was the middle ground.

>>> I've got a blacklist for NCQ in my work tree but really we need the
>>> vendors to help fill it. So far it has some raptors in it but I am sure
>>> there are more, and I am sure there are cases we should be advising
>>> newer firmware or just tweaking our queue sizes etc.
>>
>> Lots of the older Maxtors are pretty crappy for NCQ, so those too. Queue
>> size tweaks fixed them for me (as low as 4, and I can't say for sure if
>> it fixes all crashes).

I'm gonna update EH such that NCQ is turned off after certain number of 
errors.  That should at least eventually make the machine usable in such 
cases, but we definitely need NCQ blacklist.

> We have to be careful with the blacklisting - specifically, drive model 
> is often less critical than the version of the firmware (which gets 
> updated as people work drives through qualification, etc).
> Updating drive firmware for end users is pretty rare (and almost all 
> tools are still DOS based ;-)).
> 
> Certain drives should default to non-NCQ (based on model), but we should 
> be able to enable it if the firmware supports it.
> 
> Most of the newest drives are fine, but we will still need something 
> like Tejun's fix to be able to turn it off for the odd cases that show 
> up in certain odd applications, etc.

Ric, can you press harddisk vendors hard enough such that those model 
and revision numbers squeeze out of them?

Thanks.

-- 
tejun

Re: [PATCH 2/2] libata: turn off NCQ if queue depth is adjusted to 1

[Ric Wheeler]

Tejun Heo wrote:

> Ric Wheeler wrote:
>
>
>> We have to be careful with the blacklisting - specifically, drive 
>> model is often less critical than the version of the firmware (which 
>> gets updated as people work drives through qualification, etc).
>> Updating drive firmware for end users is pretty rare (and almost all 
>> tools are still DOS based ;-)).
>>
>> Certain drives should default to non-NCQ (based on model), but we 
>> should be able to enable it if the firmware supports it.
>>
>> Most of the newest drives are fine, but we will still need something 
>> like Tejun's fix to be able to turn it off for the odd cases that 
>> show up in certain odd applications, etc.
>
>
> Ric, can you press harddisk vendors hard enough such that those model 
> and revision numbers squeeze out of them?
>
> Thanks.

I will certainly push to get drive vendors to work with us on this. It 
is in everyone's interest to have the drives avoid stressing features 
that are not stable ;-)

Even better, we can usually try to work the bugs out with vendors before 
the drives become public which should help minimize the issues.

Re: [PATCH 2/2] libata: turn off NCQ if queue depth is adjusted to 1

[Ric Wheeler]

Ric Wheeler wrote:

>
> Tejun Heo wrote:
>
>>
>> Ric, can you press harddisk vendors hard enough such that those model 
>> and revision numbers squeeze out of them?
>>
>> Thanks.
>
>
> I will certainly push to get drive vendors to work with us on this. It 
> is in everyone's interest to have the drives avoid stressing features 
> that are not stable ;-)
>
> Even better, we can usually try to work the bugs out with vendors 
> before the drives become public which should help minimize the issues.
>
Just thinking out loud, but it would be really helpful to get drive 
vendor's a basic set of tests for Linux systems -  error handling, 
performance, SMART features, etc - that would run natively on linux. 

We would need to get something really easy to deploy, like a live CD 
image with the test suite that could be booted on a pc, to get into an 
environment that is used to booting DOS based floppies...

Re: [PATCH 2/2] libata: turn off NCQ if queue depth is adjusted to 1

[Jeff Garzik]

Ric Wheeler wrote:
> Just thinking out loud, but it would be really helpful to get drive 
> vendor's a basic set of tests for Linux systems -  error handling, 
> performance, SMART features, etc - that would run natively on linux.
> We would need to get something really easy to deploy, like a live CD 
> image with the test suite that could be booted on a pc, to get into an 
> environment that is used to booting DOS based floppies...

Strongly agreed.

I know some people use DOS-based environments; I would prefer the 
following test environment:

Equip systems with NICs that can do wake-on-lan and PXE.  To initiate 
testing of a system, perform a PXE boot, which downloads a 
custom-compiled kernel and initrd over the net.  The kernel boots, sets 
up a test environment in either ramfs or nfs (or a combination thereof), 
and runs a "do everything" script which starts the tests specified by 
the network admin.

The tests performed should be in three classes:  (1) data and non-data 
tests performed over a "direct submit" interface like SG_IO, (2) data 
tests performed by directly accessing the block device, and (3) data 
tests performed by accessing data through a common filesystem [ext3 or 
whatever is popular].

It is already trivial to write tests for #2 and #3.  Tests in class #1 
may require some thought and complexity, such as using multiple threads, 
to achieve maximal use of command queueing features.  I'm not aware of 
any userspace interface that allows fine-grained control of TCQ (Jens 
correct me here), or even an interface that does not require multiple 
threads to submit multiple tasks simultaneously.

	Jeff

Re: [PATCH 2/2] libata: turn off NCQ if queue depth is adjusted to 1

[Jens Axboe]

On Sat, Dec 16 2006, Jeff Garzik wrote:
> Ric Wheeler wrote:
> >Just thinking out loud, but it would be really helpful to get drive 
> >vendor's a basic set of tests for Linux systems -  error handling, 
> >performance, SMART features, etc - that would run natively on linux.
> >We would need to get something really easy to deploy, like a live CD 
> >image with the test suite that could be booted on a pc, to get into an 
> >environment that is used to booting DOS based floppies...
> 
> Strongly agreed.
> 
> I know some people use DOS-based environments; I would prefer the 
> following test environment:
> 
> Equip systems with NICs that can do wake-on-lan and PXE.  To initiate 
> testing of a system, perform a PXE boot, which downloads a 
> custom-compiled kernel and initrd over the net.  The kernel boots, sets 
> up a test environment in either ramfs or nfs (or a combination thereof), 
> and runs a "do everything" script which starts the tests specified by 
> the network admin.
> 
> The tests performed should be in three classes:  (1) data and non-data 
> tests performed over a "direct submit" interface like SG_IO, (2) data 
> tests performed by directly accessing the block device, and (3) data 
> tests performed by accessing data through a common filesystem [ext3 or 
> whatever is popular].
> 
> It is already trivial to write tests for #2 and #3.  Tests in class #1 
> may require some thought and complexity, such as using multiple threads, 
> to achieve maximal use of command queueing features.  I'm not aware of 
> any userspace interface that allows fine-grained control of TCQ (Jens 
> correct me here), or even an interface that does not require multiple 
> threads to submit multiple tasks simultaneously.

fio can do all of that, it supports a variety of io engines that allow
you to control the queue depth. So for SG_IO, you can obviously only do
depth of 1 as it's a sync interface, but the fio sg io engine also
supports the async /dev/sg (or bsg) interface that allows you to do
queueing.

Using fio it's quite simple to stress test the various interfaces. From
the io scheduler POV, it's also quite interesting to mix eg normal block
device access with SG_IO, to test that as well.

-- 
Jens Axboe

Re: [PATCH 2/2] libata: turn off NCQ if queue depth is adjusted to 1

[Jens Axboe]

On Sun, Oct 01 2006, Tejun Heo wrote:
> Ric Wheeler wrote:
> >Jens Axboe wrote:
> >>On Sat, Sep 30 2006, Alan Cox wrote:
> >>>Ar Sad, 2006-09-30 am 20:04 +0200, ysgrifennodd Jens Axboe:
> >>>>On Sat, Sep 30 2006, Tejun Heo wrote:
> >>>>    
> >>>>>Turn off NCQ if queue depth is adjusted to 1.
> >>>>>      
> >>>>I had thought of that too but discarded it - it would be nicer to have
> >>>>an independent way of turning off NCQ. Say you are debugging a weird 
> >>>>FIS
> >>>>issue, NCQ depth 1 is still a different beast to non-NCQ. I see Jeff
> >>>>already applied the patches, but just thought I'd voice my opinion.
> 
> Yeah, I was putting off this patch for the same reason.  0 depth would 
> have been nice but SCSI midlayer filters such input and libata still 
> doesn't have its own sysfs tree.  So, this was the middle ground.

Don't let that stop you, with good reason it could be changed (or just
allowed for some llds).

> >>>I've got a blacklist for NCQ in my work tree but really we need the
> >>>vendors to help fill it. So far it has some raptors in it but I am sure
> >>>there are more, and I am sure there are cases we should be advising
> >>>newer firmware or just tweaking our queue sizes etc.
> >>
> >>Lots of the older Maxtors are pretty crappy for NCQ, so those too. Queue
> >>size tweaks fixed them for me (as low as 4, and I can't say for sure if
> >>it fixes all crashes).
> 
> I'm gonna update EH such that NCQ is turned off after certain number of 
> errors.  That should at least eventually make the machine usable in such 
> cases, but we definitely need NCQ blacklist.

Depends on what the error is. Some drives completely lock up and need a
hard power cycle (I've seen this), there's no way the EH can recover
that.

> >We have to be careful with the blacklisting - specifically, drive model 
> >is often less critical than the version of the firmware (which gets 
> >updated as people work drives through qualification, etc).
> >Updating drive firmware for end users is pretty rare (and almost all 
> >tools are still DOS based ;-)).
> >
> >Certain drives should default to non-NCQ (based on model), but we should 
> >be able to enable it if the firmware supports it.
> >
> >Most of the newest drives are fine, but we will still need something 
> >like Tejun's fix to be able to turn it off for the odd cases that show 
> >up in certain odd applications, etc.
> 
> Ric, can you press harddisk vendors hard enough such that those model 
> and revision numbers squeeze out of them?

I'm guessing that'll be hard. We can scour the .inf files on the drivers
that enable NCQ on Windows, that's probably a good set of defaults.

-- 
Jens Axboe

Re: [PATCH 2/2] libata: turn off NCQ if queue depth is adjusted to 1

[saeed bishara]

the NCQ commands contain the FUA (force unit access) field
(mandatory), so disabling the NCQ will waste this feature.
I don't know how much ans when this feature is usuable, but someday it
can be usefull.

-saeed

Re: [PATCH 2/2] libata: turn off NCQ if queue depth is adjusted to 1

[Jens Axboe]

On Wed, Oct 04 2006, saeed bishara wrote:
> the NCQ commands contain the FUA (force unit access) field
> (mandatory), so disabling the NCQ will waste this feature.
> I don't know how much ans when this feature is usuable, but someday it
> can be usefull.

It's already useful, for io barriers. Yet another reason why the
distinction between depth == 1 and NCQ needs to available.

-- 
Jens Axboe

Re: [PATCH 2/2] libata: turn off NCQ if queue depth is adjusted to 1

[Jens Axboe]

On Wed, Oct 04 2006, Jens Axboe wrote:
> On Wed, Oct 04 2006, saeed bishara wrote:
> > the NCQ commands contain the FUA (force unit access) field
> > (mandatory), so disabling the NCQ will waste this feature.
> > I don't know how much ans when this feature is usuable, but someday it
> > can be usefull.
> 
> It's already useful, for io barriers. Yet another reason why the
> distinction between depth == 1 and NCQ needs to available.

Ehm, I completely missed the bigger picture - non-ncq has FUA available
too. So NCQ or not has no bearing on FUA functionality.

-- 
Jens Axboe

Re: [PATCH 1/2] libata: cosmetic changes to constants

[Jeff Garzik]

applied 1-2