2012-05-08 03:17:18

by Zheng Liu

[permalink] [raw]
Subject: [RFC][PATCH] libata: enable SATA disk fua detection on default

From: Zheng Liu <[email protected]>

Currently, SATA disk fua detection is disabled on default because most of
devices don't support this feature at that time. With the development of
technology, more and more SATA disks support this feature. So now we can enable
this detection on default.

Although fua detection is defined as a kernel module parameter, it is too hard
to set its value because it must be loaded and set before system starts up.
That needs to modify initrd file. So it is inconvenient for administrator who
needs to manage a huge number of servers.

CC: Jeff Garzik <[email protected]>
Signed-off-by: Zheng Liu <[email protected]>
---
drivers/ata/libata-core.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 23763a1..3627251 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -133,9 +133,9 @@ int atapi_passthru16 = 1;
module_param(atapi_passthru16, int, 0444);
MODULE_PARM_DESC(atapi_passthru16, "Enable ATA_16 passthru for ATAPI devices (0=off, 1=on [default])");

-int libata_fua = 0;
+int libata_fua = 1;
module_param_named(fua, libata_fua, int, 0444);
-MODULE_PARM_DESC(fua, "FUA support (0=off [default], 1=on)");
+MODULE_PARM_DESC(fua, "FUA support (0=off, 1=on [default])");

static int ata_ignore_hpa;
module_param_named(ignore_hpa, ata_ignore_hpa, int, 0644);
--
1.7.4.1


2012-05-09 05:38:39

by Robert Hancock

[permalink] [raw]
Subject: Re: [RFC][PATCH] libata: enable SATA disk fua detection on default

On 05/07/2012 09:24 PM, Zheng Liu wrote:
> From: Zheng Liu<[email protected]>
>
> Currently, SATA disk fua detection is disabled on default because most of
> devices don't support this feature at that time. With the development of
> technology, more and more SATA disks support this feature. So now we can enable
> this detection on default.
>
> Although fua detection is defined as a kernel module parameter, it is too hard
> to set its value because it must be loaded and set before system starts up.
> That needs to modify initrd file. So it is inconvenient for administrator who
> needs to manage a huge number of servers.
>
> CC: Jeff Garzik<[email protected]>
> Signed-off-by: Zheng Liu<[email protected]>
> ---
> drivers/ata/libata-core.c | 4 ++--
> 1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 23763a1..3627251 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -133,9 +133,9 @@ int atapi_passthru16 = 1;
> module_param(atapi_passthru16, int, 0444);
> MODULE_PARM_DESC(atapi_passthru16, "Enable ATA_16 passthru for ATAPI devices (0=off, 1=on [default])");
>
> -int libata_fua = 0;
> +int libata_fua = 1;
> module_param_named(fua, libata_fua, int, 0444);
> -MODULE_PARM_DESC(fua, "FUA support (0=off [default], 1=on)");
> +MODULE_PARM_DESC(fua, "FUA support (0=off, 1=on [default])");
>
> static int ata_ignore_hpa;
> module_param_named(ignore_hpa, ata_ignore_hpa, int, 0644);

I don't see that this is very likely to cause issues, but the FUA
implementation also seems a bit incomplete. FUA will only get enabled if
the drive claims support for the FUA-specific commands like
WRITE_DMA_FUA_EXT. But theoretically any drive that supports NCQ should
be able to do FUA since there's a FUA bit provided in those commands. Of
course this makes things a bit more complicated (for example, if NCQ
gets disabled by user request or due to too many errors, and the drive
doesn't support the separate FUA commands, then FUA needs to be disabled
too.)

2012-05-09 06:12:59

by Zheng Liu

[permalink] [raw]
Subject: Re: [RFC][PATCH] libata: enable SATA disk fua detection on default

On Tue, May 08, 2012 at 11:38:34PM -0600, Robert Hancock wrote:
> On 05/07/2012 09:24 PM, Zheng Liu wrote:
> >From: Zheng Liu<[email protected]>
> >
> >Currently, SATA disk fua detection is disabled on default because most of
> >devices don't support this feature at that time. With the development of
> >technology, more and more SATA disks support this feature. So now we can enable
> >this detection on default.
> >
> >Although fua detection is defined as a kernel module parameter, it is too hard
> >to set its value because it must be loaded and set before system starts up.
> >That needs to modify initrd file. So it is inconvenient for administrator who
> >needs to manage a huge number of servers.
> >
> >CC: Jeff Garzik<[email protected]>
> >Signed-off-by: Zheng Liu<[email protected]>
> >---
> > drivers/ata/libata-core.c | 4 ++--
> > 1 files changed, 2 insertions(+), 2 deletions(-)
> >
> >diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> >index 23763a1..3627251 100644
> >--- a/drivers/ata/libata-core.c
> >+++ b/drivers/ata/libata-core.c
> >@@ -133,9 +133,9 @@ int atapi_passthru16 = 1;
> > module_param(atapi_passthru16, int, 0444);
> > MODULE_PARM_DESC(atapi_passthru16, "Enable ATA_16 passthru for ATAPI devices (0=off, 1=on [default])");
> >
> >-int libata_fua = 0;
> >+int libata_fua = 1;
> > module_param_named(fua, libata_fua, int, 0444);
> >-MODULE_PARM_DESC(fua, "FUA support (0=off [default], 1=on)");
> >+MODULE_PARM_DESC(fua, "FUA support (0=off, 1=on [default])");
> >
> > static int ata_ignore_hpa;
> > module_param_named(ignore_hpa, ata_ignore_hpa, int, 0644);
>
> I don't see that this is very likely to cause issues, but the FUA
> implementation also seems a bit incomplete. FUA will only get
> enabled if the drive claims support for the FUA-specific commands
> like WRITE_DMA_FUA_EXT. But theoretically any drive that supports
> NCQ should be able to do FUA since there's a FUA bit provided in
> those commands. Of course this makes things a bit more complicated
> (for example, if NCQ gets disabled by user request or due to too
> many errors, and the drive doesn't support the separate FUA
> commands, then FUA needs to be disabled too.)

Hi Reboert,

Thanks for your reply. Actually, we met a problem in our product
system. We have thousands of database servers that run on Linux.
These servers have a lot of SATA disks that support FUA feature.
However, it displays that the disk doesn't support FUA in dmesg. I
digged this problem and found that libata_fua variable is set to 0 to
disable FUA detection for SATA disk. If FUA feature can be used, we
could disable io_barrier to improve the performance. Now one solution
is that we set libata_fua to 1 when this kernel module is loaded. But
it needs to modify initrd file, and it is too complicated for
administrator who needs to modify every server manually. So that
enabling FUA detection is another solution. Have you any other idea
to solve this problem? Thank you.

Regards,
Zheng

2012-05-09 08:25:31

by Christoph Hellwig

[permalink] [raw]
Subject: Re: [RFC][PATCH] libata: enable SATA disk fua detection on default

On Wed, May 09, 2012 at 02:19:56PM +0800, Zheng Liu wrote:
> disable FUA detection for SATA disk. If FUA feature can be used, we
> could disable io_barrier to improve the performance. Now one solution

You can't run with -o nobarrier just because the driver/hardware support
FUA. However the implementation of -o barrier (aka REQ_FUA) is a lot
more efficient if it is implemented. If your workload has a high
percentage of REQ_FUA requests it might be better to switch the disks to
write through mode entirely.

2012-05-09 09:23:16

by Zheng Liu

[permalink] [raw]
Subject: Re: [RFC][PATCH] libata: enable SATA disk fua detection on default

On Wed, May 09, 2012 at 04:25:27AM -0400, Christoph Hellwig wrote:
> On Wed, May 09, 2012 at 02:19:56PM +0800, Zheng Liu wrote:
> > disable FUA detection for SATA disk. If FUA feature can be used, we
> > could disable io_barrier to improve the performance. Now one solution
>
> You can't run with -o nobarrier just because the driver/hardware support
> FUA. However the implementation of -o barrier (aka REQ_FUA) is a lot
> more efficient if it is implemented. If your workload has a high
> percentage of REQ_FUA requests it might be better to switch the disks to
> write through mode entirely.

Thanks for your advice. Indeed, it is more efficient when data is
flushed with REQ_FUA, and this is the reason why I submit this patch.
IMHO, FUA provides a solution that we can get better performance in write
cache mode when we do many flush operations. If I set the disk to write
through mode, I won't get this benefit. Am I missing something?

Currently, the key issue is that we disable FUA detection for SATA disk.
We almost have no chance to change it because it is too complicated to
set libata_fua variable when this module is loaded. So why not give
SATA disk an opportunity to enable this feature? After all, there is a
lot of SATA disks that support this feature.

Regards,
Zheng

2012-05-09 11:12:49

by Christoph Hellwig

[permalink] [raw]
Subject: Re: [RFC][PATCH] libata: enable SATA disk fua detection on default

On Wed, May 09, 2012 at 05:30:16PM +0800, Zheng Liu wrote:
> IMHO, FUA provides a solution that we can get better performance in write
> cache mode when we do many flush operations. If I set the disk to write
> through mode, I won't get this benefit. Am I missing something?

If you set the disk to write through mode you never have to flush the
cache. So as soonas your number of flushes gets close to the number of
writes it tends to be a clear win - for ATA the tradeoff is even more in
favour of write through because the flush command can't be queued yetin
commonly available standards versions. So if you have a workload that
basically needs to flush out every write you win - if you have workloads
where you have a lot more writes than cache flushes write back mode
wins.

> Currently, the key issue is that we disable FUA detection for SATA disk.
> We almost have no chance to change it because it is too complicated to
> set libata_fua variable when this module is loaded. So why not give
> SATA disk an opportunity to enable this feature? After all, there is a
> lot of SATA disks that support this feature.

I'm all in favour of your patch, I just wanted to point out that the
argument in the description wasn't quite correct.

2012-05-09 12:41:28

by Zheng Liu

[permalink] [raw]
Subject: Re: [RFC][PATCH] libata: enable SATA disk fua detection on default

On Wed, May 09, 2012 at 07:12:43AM -0400, Christoph Hellwig wrote:
> On Wed, May 09, 2012 at 05:30:16PM +0800, Zheng Liu wrote:
> > IMHO, FUA provides a solution that we can get better performance in write
> > cache mode when we do many flush operations. If I set the disk to write
> > through mode, I won't get this benefit. Am I missing something?
>
> If you set the disk to write through mode you never have to flush the
> cache. So as soonas your number of flushes gets close to the number of
> writes it tends to be a clear win - for ATA the tradeoff is even more in
> favour of write through because the flush command can't be queued yetin
> commonly available standards versions. So if you have a workload that
> basically needs to flush out every write you win - if you have workloads
> where you have a lot more writes than cache flushes write back mode
> wins.

Thanks for your explanation. It seems that there still has a problem.
If I set the disk to write through mode, I need to modify my application
to remove all of flush/sync operations. It is unacceptable for us.

> > Currently, the key issue is that we disable FUA detection for SATA disk.
> > We almost have no chance to change it because it is too complicated to
> > set libata_fua variable when this module is loaded. So why not give
> > SATA disk an opportunity to enable this feature? After all, there is a
> > lot of SATA disks that support this feature.
>
> I'm all in favour of your patch, I just wanted to point out that the
> argument in the description wasn't quite correct.

Thank you. I will fix it. :-)

Regards,
Zheng

2012-05-09 13:21:09

by Bernd Schubert

[permalink] [raw]
Subject: Re: [RFC][PATCH] libata: enable SATA disk fua detection on default

On 05/09/2012 02:48 PM, Zheng Liu wrote:
> On Wed, May 09, 2012 at 07:12:43AM -0400, Christoph Hellwig wrote:
>> On Wed, May 09, 2012 at 05:30:16PM +0800, Zheng Liu wrote:
>>> IMHO, FUA provides a solution that we can get better performance in write
>>> cache mode when we do many flush operations. If I set the disk to write
>>> through mode, I won't get this benefit. Am I missing something?
>>
>> If you set the disk to write through mode you never have to flush the
>> cache. So as soonas your number of flushes gets close to the number of
>> writes it tends to be a clear win - for ATA the tradeoff is even more in
>> favour of write through because the flush command can't be queued yetin
>> commonly available standards versions. So if you have a workload that
>> basically needs to flush out every write you win - if you have workloads
>> where you have a lot more writes than cache flushes write back mode
>> wins.
>
> Thanks for your explanation. It seems that there still has a problem.
> If I set the disk to write through mode, I need to modify my application
> to remove all of flush/sync operations. It is unacceptable for us.

Why? You still need to flush the linux page cache. Just the disk cache
does not need to be flushed anymore. So changing the application would
be the wrong thing to do.


Cheers,
Bernd

2012-05-09 13:23:22

by Christoph Hellwig

[permalink] [raw]
Subject: Re: [RFC][PATCH] libata: enable SATA disk fua detection on default

On Wed, May 09, 2012 at 08:48:04PM +0800, Zheng Liu wrote:
> Thanks for your explanation. It seems that there still has a problem.
> If I set the disk to write through mode, I need to modify my application
> to remove all of flush/sync operations. It is unacceptable for us.

You don't have to - the kernel will not send a SYNCHRONIZE CACHE command
unless the disk advertises a write back cache.

2012-08-17 18:06:53

by Jeff Garzik

[permalink] [raw]
Subject: Enabling FUA for SATA drives (was Re: [RFC][PATCH] libata: enable SATA disk fua detection on default)

On 05/07/2012 11:24 PM, Zheng Liu wrote:
> From: Zheng Liu <[email protected]>
>
> Currently, SATA disk fua detection is disabled on default because most of
> devices don't support this feature at that time. With the development of
> technology, more and more SATA disks support this feature. So now we can enable
> this detection on default.
>
> Although fua detection is defined as a kernel module parameter, it is too hard
> to set its value because it must be loaded and set before system starts up.
> That needs to modify initrd file. So it is inconvenient for administrator who
> needs to manage a huge number of servers.
>
> CC: Jeff Garzik <[email protected]>
> Signed-off-by: Zheng Liu <[email protected]>
> ---
> drivers/ata/libata-core.c | 4 ++--
> 1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 23763a1..3627251 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -133,9 +133,9 @@ int atapi_passthru16 = 1;
> module_param(atapi_passthru16, int, 0444);
> MODULE_PARM_DESC(atapi_passthru16, "Enable ATA_16 passthru for ATAPI devices (0=off, 1=on [default])");
>
> -int libata_fua = 0;
> +int libata_fua = 1;
> module_param_named(fua, libata_fua, int, 0444);
> -MODULE_PARM_DESC(fua, "FUA support (0=off [default], 1=on)");
> +MODULE_PARM_DESC(fua, "FUA support (0=off, 1=on [default])");

Applied. Let's see how far down the rabbit hole we go ;-)

The FUA decision, as previously indicated, was based on early SATA
drives, and perhaps better ones are available now. Only testing will
tell, at this point.

The larger questions, raised by Christoph and others remain unaddressed
(though perhaps we can start addressing them now, with this patch):

* what is smart flushing policy for ATA devices with FUA?

* ATA NCQ's flush is not queued

* ATA NCQ always had the FUA bit...

* ...but mixing ATA NCQ FUA and !FUA in a queue is not fully supported
by the existing code

and probably a few other details I forgot :)

Jeff