[RFC/PATCH 0/7] enable honoring write cache setting of IDE drive

[RFC/PATCH 0/7] enable honoring write cache setting of IDE drive

[Doug Maxey]

Howdy!

Some IDE drives destined for use in server class or datacenter
machines will come with "write cache" disabled.  With the current
code, the setting of the drive is effectively ignored, and the cache
is always enabled if the drive has cache.

It is hard to define or enable certain behaviors that will keep
everyone happy, but hey, this is my (and a certain vendor's) take on
this.  If you are happy with the status quo, no behavior change.  If
the user desires to have write cache disabled by default (per the
drive), she can enable a more server friendly behavior.

These patches are against BK5.  No attempt has been made to integrate
with Jens' latest "scsi/sata write barrier" patches, but will look
into that when the dust has settled for both.

With BLK_DEV_HDWC at the default of 0, the current driver behavior is
maintained, i.e., the drive will always use the write cache per the
current code.  With the setting enabled, the driver will use the
default value of the drive write cache.

One area this does not touch on is locking.  There does appear to be
some issues in changing the setting on the fly, but this is ignored in
the existing code.  Bart has been making some effort.

This set of patches changes the way the write cache setting of IDE
drives is handled to:

1) move the cache_write code to ide-io, where it will be callable from
   kernels built without ide-disk.

2) allow the drive to define the default setting of the cache when
   BLK_DEV_HDWC is set.

3) correctly handle the change of write cache setting from hdparm.
   This means that the call also changes the blk_queue_ordered()
   visible variable.


1/7 - add ide_write_cache() to ide-io.c.  This is in preparation for
      the following patches that remove the current write_cache() from
      ide-disk.c.  Moving to ide-io is allow ide_write_cache() to
      handle calls (no-op) on systems without ide disks (IDE may not
      have been built for disks).

2/7 - add hook in ide-taskfile:ide_cmd_ioctl() to call the new
      function ide_write_cache().

3/7 - Add documentation for the use of the Kbuild variable
      BLK_DEV_HDWC.

4/7 - use new config variable BLK_DEV_HDWC in idedisk_setup code.

5/7 - remove (now obsolete) write_cache() from ide-disk.c.  replace calls in
      ide-disk.c with calls to ide_write_cache().

6/7 - change the way that ide_write_cache controls the use of flushing
      via blk_queue_ordered() to match the setting of the drive's
      write cache setting.  This patch leaves the idedisk_issue_flush
      in ide-disk.c, but the func could be moved to ide-io to complete
      the break of the dependency between ide-io and ide-disk.

7/7 - move idedisk_issue_flush to ide-io.c supporting
      ide_write_cache()

Re: [RFC/PATCH 0/7] enable honoring write cache setting of IDE drive

[Bartlomiej Zolnierkiewicz]

On Fri, 28 Jan 2005 12:16:01 -0600, Doug Maxey <dwm@austin.ibm.com> wrote:
> Howdy!

Hi,
 
> Some IDE drives destined for use in server class or datacenter
> machines will come with "write cache" disabled.  With the current
> code, the setting of the drive is effectively ignored, and the cache
> is always enabled if the drive has cache.
> 
> It is hard to define or enable certain behaviors that will keep
> everyone happy, but hey, this is my (and a certain vendor's) take on
> this.  If you are happy with the status quo, no behavior change.  If
> the user desires to have write cache disabled by default (per the
> drive), she can enable a more server friendly behavior.
>
> These patches are against BK5.  No attempt has been made to integrate
> with Jens' latest "scsi/sata write barrier" patches, but will look
> into that when the dust has settled for both.
> 
> With BLK_DEV_HDWC at the default of 0, the current driver behavior is
> maintained, i.e., the drive will always use the write cache per the
> current code.  With the setting enabled, the driver will use the
> default value of the drive write cache.

We have too many config options already.

Behavior should be simple:
* no cache flushes - wcache off by default
* cache flushes - wcache on by default
* inform user about the wcache status 
* allow changing of wcache by user

> One area this does not touch on is locking.  There does appear to be
> some issues in changing the setting on the fly, but this is ignored in
> the existing code.  Bart has been making some effort.
>
> This set of patches changes the way the write cache setting of IDE
> drives is handled to:
> 
> 1) move the cache_write code to ide-io, where it will be callable from
>    kernels built without ide-disk.

I've already pointed this out - this is not needed, you should add
check similar to this for 'xfer_set'.  Another reason not to do this
is that write_cache() uses REQ_DRIVE_TASKFILE internally.

> 2) allow the drive to define the default setting of the cache when
>    BLK_DEV_HDWC is set.
> 
> 3) correctly handle the change of write cache setting from hdparm.
>    This means that the call also changes the blk_queue_ordered()
>    visible variable.

I've asked you many times to separate this two changes.
No luck so far.

> 1/7 - add ide_write_cache() to ide-io.c.  This is in preparation for
>       the following patches that remove the current write_cache() from
>       ide-disk.c.  Moving to ide-io is allow ide_write_cache() to
>       handle calls (no-op) on systems without ide disks (IDE may not
>       have been built for disks).
> 
> 2/7 - add hook in ide-taskfile:ide_cmd_ioctl() to call the new
>       function ide_write_cache().
> 
> 3/7 - Add documentation for the use of the Kbuild variable
>       BLK_DEV_HDWC.
> 
> 4/7 - use new config variable BLK_DEV_HDWC in idedisk_setup code.
> 
> 5/7 - remove (now obsolete) write_cache() from ide-disk.c.  replace calls in
>       ide-disk.c with calls to ide_write_cache().
> 
> 6/7 - change the way that ide_write_cache controls the use of flushing
>       via blk_queue_ordered() to match the setting of the drive's
>       write cache setting.  This patch leaves the idedisk_issue_flush
>       in ide-disk.c, but the func could be moved to ide-io to complete
>       the break of the dependency between ide-io and ide-disk.
> 
> 7/7 - move idedisk_issue_flush to ide-io.c supporting
>       ide_write_cache()

Re: [RFC/PATCH 0/7] enable honoring write cache setting of IDE drive

[Doug Maxey]

On Fri, 28 Jan 2005 20:11:37 +0100, Bartlomiej Zolnierkiewicz wrote:
>On Fri, 28 Jan 2005 12:16:01 -0600, Doug Maxey <dwm@austin.ibm.com> wrote:
>
>We have too many config options already.
>
>Behavior should be simple:
>* no cache flushes - wcache off by default
>* cache flushes - wcache on by default
>* inform user about the wcache status
>* allow changing of wcache by user

My interpretation of what you are saying here is the selection of
"cache flush" drives the setting of wcache.  Are you saying that
"barrier=off" on the boot line the currently (only and will remain)
supported method controlling flushes?

AFAICS, "barrier=off" just tells the FS (only ext2 and reiser) to not
flush writes.  I may be misunderstanding how that controls the
behavior of the drive at the ll_blk layer.  Is that what you want to
see a patch to, pick this out of the FS?

>>
>> 1) move the cache_write code to ide-io, where it will be callable from
>>    kernels built without ide-disk.
>
>I've already pointed this out - this is not needed, you should add
>check similar to this for 'xfer_set'.  Another reason not to do this
>is that write_cache() uses REQ_DRIVE_TASKFILE internally.

Is taskfile going away?

>
>> 2) allow the drive to define the default setting of the cache when
>>    BLK_DEV_HDWC is set.
>>
>> 3) correctly handle the change of write cache setting from hdparm.
>>    This means that the call also changes the blk_queue_ordered()
>>    visible variable.
>
>I've asked you many times to separate this two changes.
>No luck so far.
>

Split patch does not count? :)

Re: [RFC/PATCH 0/7] enable honoring write cache setting of IDE drive

[Bartlomiej Zolnierkiewicz]

On Fri, 28 Jan 2005 15:06:22 -0600, Doug Maxey <dwm@maxeymade.com> wrote:
> 
> On Fri, 28 Jan 2005 20:11:37 +0100, Bartlomiej Zolnierkiewicz wrote:
> >On Fri, 28 Jan 2005 12:16:01 -0600, Doug Maxey <dwm@austin.ibm.com> wrote:
> >
> >We have too many config options already.
> >
> >Behavior should be simple:
> >* no cache flushes - wcache off by default
> >* cache flushes - wcache on by default
> >* inform user about the wcache status
> >* allow changing of wcache by user
> 
> My interpretation of what you are saying here is the selection of
> "cache flush" drives the setting of wcache.  Are you saying that
> "barrier=off" on the boot line the currently (only and will remain)
> supported method controlling flushes?

No, I am saying that cache flush is a property of disk not fs.
On the contrary barrier is a property of filesystem.

> AFAICS, "barrier=off" just tells the FS (only ext2 and reiser) to not
> flush writes.  I may be misunderstanding how that controls the
> behavior of the drive at the ll_blk layer.  Is that what you want to
> see a patch to, pick this out of the FS?

No, why?

> >>
> >> 1) move the cache_write code to ide-io, where it will be callable from
> >>    kernels built without ide-disk.
> >
> >I've already pointed this out - this is not needed, you should add
> >check similar to this for 'xfer_set'.  Another reason not to do this
> >is that write_cache() uses REQ_DRIVE_TASKFILE internally.
> 
> Is taskfile going away?

Quite the opposite but if you want to use taskfile, map whole
ide_cmd_ioctl() to it not CACHE_FLUSH_{EXT} command only
and don't intermix bugfixes with potentially risky changes.

Re: [RFC/PATCH 0/7] enable honoring write cache setting of IDE drive

[Doug Maxey]

On Fri, 28 Jan 2005 22:35:17 +0100, Bartlomiej Zolnierkiewicz wrote:
>On Fri, 28 Jan 2005 15:06:22 -0600, Doug Maxey <dwm@maxeymade.com> wrote:
>> 
>> On Fri, 28 Jan 2005 20:11:37 +0100, Bartlomiej Zolnierkiewicz wrote:
>> >On Fri, 28 Jan 2005 12:16:01 -0600, Doug Maxey <dwm@austin.ibm.com> wrote:
>> >
>> >We have too many config options already.
>> >
>> >Behavior should be simple:
>> >* no cache flushes - wcache off by default
>> >* cache flushes - wcache on by default
>> >* inform user about the wcache status
>> >* allow changing of wcache by user
>> 
>> My interpretation of what you are saying here is the selection of
>> "cache flush" drives the setting of wcache.  Are you saying that
>> "barrier=off" on the boot line the currently (only and will remain)
>> supported method controlling flushes?
>
>No, I am saying that cache flush is a property of disk not fs.
>On the contrary barrier is a property of filesystem.

Ok.  We are talking apples and oranges here.  The disk DOES have FLUSH CACHE, 
which is the main reason these drives work in the later kernels.  You can 
switch wcache back and forth, provided that the correct flushing is done.

Write cache, on the otherhand, which is disabled on the drive by default,
is not honored.

>
>> AFAICS, "barrier=off" just tells the FS (only ext2 and reiser) to not
>> flush writes.  I may be misunderstanding how that controls the
>> behavior of the drive at the ll_blk layer.  Is that what you want to
>> see a patch to, pick this out of the FS?
>
>No, why?
>
>> >>
>> >> 1) move the cache_write code to ide-io, where it will be callable from
>> >>    kernels built without ide-disk.
>> >
>> >I've already pointed this out - this is not needed, you should add
>> >check similar to this for 'xfer_set'.  Another reason not to do this
>> >is that write_cache() uses REQ_DRIVE_TASKFILE internally.
>> 
>> Is taskfile going away?
>
>Quite the opposite but if you want to use taskfile, map whole
>ide_cmd_ioctl() to it not CACHE_FLUSH_{EXT} command only
>and don't intermix bugfixes with potentially risky changes.
>

Well, if you turn off write cache, then you must not, at least on these 
drives, use FLUSH.  All write commands are errored.

Re: [RFC/PATCH 0/7] enable honoring write cache setting of IDE drive

[Bartlomiej Zolnierkiewicz]

On Fri, 28 Jan 2005 15:43:52 -0600, Doug Maxey <dwm@austin.ibm.com> wrote:
> 
> On Fri, 28 Jan 2005 22:35:17 +0100, Bartlomiej Zolnierkiewicz wrote:
> >On Fri, 28 Jan 2005 15:06:22 -0600, Doug Maxey <dwm@maxeymade.com> wrote:
> >>
> >> On Fri, 28 Jan 2005 20:11:37 +0100, Bartlomiej Zolnierkiewicz wrote:
> >> >On Fri, 28 Jan 2005 12:16:01 -0600, Doug Maxey <dwm@austin.ibm.com> wrote:
> >> >
> >> >We have too many config options already.
> >> >
> >> >Behavior should be simple:
> >> >* no cache flushes - wcache off by default
> >> >* cache flushes - wcache on by default
> >> >* inform user about the wcache status
> >> >* allow changing of wcache by user
> >>
> >> My interpretation of what you are saying here is the selection of
> >> "cache flush" drives the setting of wcache.  Are you saying that
> >> "barrier=off" on the boot line the currently (only and will remain)
> >> supported method controlling flushes?
> >
> >No, I am saying that cache flush is a property of disk not fs.
> >On the contrary barrier is a property of filesystem.
> 
> Ok.  We are talking apples and oranges here.  The disk DOES have FLUSH CACHE,
> which is the main reason these drives work in the later kernels.  You can
> switch wcache back and forth, provided that the correct flushing is done.
> 
> Write cache, on the otherhand, which is disabled on the drive by default,
> is not honored.

What is the practical reason to honor it?

> >
> >> AFAICS, "barrier=off" just tells the FS (only ext2 and reiser) to not
> >> flush writes.  I may be misunderstanding how that controls the
> >> behavior of the drive at the ll_blk layer.  Is that what you want to
> >> see a patch to, pick this out of the FS?
> >
> >No, why?
> >
> >> >>
> >> >> 1) move the cache_write code to ide-io, where it will be callable from
> >> >>    kernels built without ide-disk.
> >> >
> >> >I've already pointed this out - this is not needed, you should add
> >> >check similar to this for 'xfer_set'.  Another reason not to do this
> >> >is that write_cache() uses REQ_DRIVE_TASKFILE internally.
> >>
> >> Is taskfile going away?
> >
> >Quite the opposite but if you want to use taskfile, map whole
> >ide_cmd_ioctl() to it not CACHE_FLUSH_{EXT} command only
> >and don't intermix bugfixes with potentially risky changes.
> >
> 
> Well, if you turn off write cache, then you must not, at least on these
> drives, use FLUSH.  All write commands are errored.

With the 'drive->wcache' bugfix this shouldn't be a problem
as FLUSH commands wouldn't be sent to a drive.

Re: [RFC/PATCH 0/7] enable honoring write cache setting of IDE drive

[Doug Maxey]

On Fri, 28 Jan 2005 22:49:08 +0100, Bartlomiej Zolnierkiewicz wrote:
>On Fri, 28 Jan 2005 15:43:52 -0600, Doug Maxey <dwm@austin.ibm.com> wrote:
>> 
>> On Fri, 28 Jan 2005 22:35:17 +0100, Bartlomiej Zolnierkiewicz wrote:
>> >On Fri, 28 Jan 2005 15:06:22 -0600, Doug Maxey <dwm@maxeymade.com> wrote:
>> >>
>> >> On Fri, 28 Jan 2005 20:11:37 +0100, Bartlomiej Zolnierkiewicz wrote:
>> >> >On Fri, 28 Jan 2005 12:16:01 -0600, Doug Maxey <dwm@austin.ibm.com> wrote:
>> >> >
>> >> >We have too many config options already.
>> >> >
>> >> >Behavior should be simple:
>> >> >* no cache flushes - wcache off by default
>> >> >* cache flushes - wcache on by default
>> >> >* inform user about the wcache status
>> >> >* allow changing of wcache by user
>> >>
>> >> My interpretation of what you are saying here is the selection of
>> >> "cache flush" drives the setting of wcache.  Are you saying that
>> >> "barrier=off" on the boot line the currently (only and will remain)
>> >> supported method controlling flushes?
>> >
>> >No, I am saying that cache flush is a property of disk not fs.
>> >On the contrary barrier is a property of filesystem.
>> 
>> Ok.  We are talking apples and oranges here.  The disk DOES have FLUSH CACHE,
>> which is the main reason these drives work in the later kernels.  You can
>> switch wcache back and forth, provided that the correct flushing is done.
>> 
>> Write cache, on the otherhand, which is disabled on the drive by default,
>> is not honored.
>
>What is the practical reason to honor it?

The datacenters/server folks that would be using these drives would
expect them to remain as set.  I have to check, but unless something
has changed very recently in the kernel, setting with hdparm does not
"stick" in the sense that the command succeeds to the disk, but no change
is made to the barrier.

here is the current scenario with a drive that can work with wcache off.
- idedisk_setup() forces enabled wcache.
- enabled wcache uses FLUSH.
- hdparm -W0 disables cache without matching change to barriers.

- Setting "barrier=off" has the undesired side effect of not flushing
  with wcache enabled on the drive.  Some people with certain servers are
  _ssooo_ picky. :)

I can forward an IDE bus trace if you would like.  It may be a while (like 
monday) before that can happen though, I threw the drive off the table.  It 
was not intentional, I swear.

>
>> >
>> >> AFAICS, "barrier=off" just tells the FS (only ext2 and reiser) to not
>> >> flush writes.  I may be misunderstanding how that controls the
>> >> behavior of the drive at the ll_blk layer.  Is that what you want to
>> >> see a patch to, pick this out of the FS?
>> >
>> >No, why?
>> >
>> >> >>
>> >> >> 1) move the cache_write code to ide-io, where it will be callable from
>> >> >>    kernels built without ide-disk.
>> >> >
>> >> >I've already pointed this out - this is not needed, you should add
>> >> >check similar to this for 'xfer_set'.  Another reason not to do this
>> >> >is that write_cache() uses REQ_DRIVE_TASKFILE internally.
>> >>
>> >> Is taskfile going away?
>> >
>> >Quite the opposite but if you want to use taskfile, map whole
>> >ide_cmd_ioctl() to it not CACHE_FLUSH_{EXT} command only
>> >and don't intermix bugfixes with potentially risky changes.
>> >
>> 
>> Well, if you turn off write cache, then you must not, at least on these
>> drives, use FLUSH.  All write commands are errored.
>
>With the 'drive->wcache' bugfix this shouldn't be a problem
>as FLUSH commands wouldn't be sent to a drive.
>

Re: [RFC/PATCH 0/7] enable honoring write cache setting of IDE drive

[Bartlomiej Zolnierkiewicz]

On Fri, 28 Jan 2005 16:15:40 -0600, Doug Maxey <dwm@austin.ibm.com> wrote:
> 
> On Fri, 28 Jan 2005 22:49:08 +0100, Bartlomiej Zolnierkiewicz wrote:
> >On Fri, 28 Jan 2005 15:43:52 -0600, Doug Maxey <dwm@austin.ibm.com> wrote:
> >>
> >> On Fri, 28 Jan 2005 22:35:17 +0100, Bartlomiej Zolnierkiewicz wrote:
> >> >On Fri, 28 Jan 2005 15:06:22 -0600, Doug Maxey <dwm@maxeymade.com> wrote:
> >> >>
> >> >> On Fri, 28 Jan 2005 20:11:37 +0100, Bartlomiej Zolnierkiewicz wrote:
> >> >> >On Fri, 28 Jan 2005 12:16:01 -0600, Doug Maxey <dwm@austin.ibm.com> wrote:
> >> >> >
> >> >> >We have too many config options already.
> >> >> >
> >> >> >Behavior should be simple:
> >> >> >* no cache flushes - wcache off by default
> >> >> >* cache flushes - wcache on by default
> >> >> >* inform user about the wcache status
> >> >> >* allow changing of wcache by user
> >> >>
> >> >> My interpretation of what you are saying here is the selection of
> >> >> "cache flush" drives the setting of wcache.  Are you saying that
> >> >> "barrier=off" on the boot line the currently (only and will remain)
> >> >> supported method controlling flushes?
> >> >
> >> >No, I am saying that cache flush is a property of disk not fs.
> >> >On the contrary barrier is a property of filesystem.
> >>
> >> Ok.  We are talking apples and oranges here.  The disk DOES have FLUSH CACHE,
> >> which is the main reason these drives work in the later kernels.  You can
> >> switch wcache back and forth, provided that the correct flushing is done.
> >>
> >> Write cache, on the otherhand, which is disabled on the drive by default,
> >> is not honored.
> >
> >What is the practical reason to honor it?
> 
> The datacenters/server folks that would be using these drives would
> expect them to remain as set.  I have to check, but unless something
> has changed very recently in the kernel, setting with hdparm does not
> "stick" in the sense that the command succeeds to the disk, but no change
> is made to the barrier.

It is not a problem for IDE driver (flushes become no-ops)
and this way you can later enable wcache and still use barries.

> here is the current scenario with a drive that can work with wcache off.
> - idedisk_setup() forces enabled wcache.
> - enabled wcache uses FLUSH.
> - hdparm -W0 disables cache without matching change to barriers.
> 
> - Setting "barrier=off" has the undesired side effect of not flushing
>   with wcache enabled on the drive.  Some people with certain servers are
>   _ssooo_ picky. :)

This is fs layer problem.

Re: [RFC/PATCH 0/7] enable honoring write cache setting of IDE drive

[Jens Axboe]

On Fri, Jan 28 2005, Bartlomiej Zolnierkiewicz wrote:
> On Fri, 28 Jan 2005 12:16:01 -0600, Doug Maxey <dwm@austin.ibm.com> wrote:
> > Howdy!
> 
> Hi,
>  
> > Some IDE drives destined for use in server class or datacenter
> > machines will come with "write cache" disabled.  With the current
> > code, the setting of the drive is effectively ignored, and the cache
> > is always enabled if the drive has cache.
> > 
> > It is hard to define or enable certain behaviors that will keep
> > everyone happy, but hey, this is my (and a certain vendor's) take on
> > this.  If you are happy with the status quo, no behavior change.  If
> > the user desires to have write cache disabled by default (per the
> > drive), she can enable a more server friendly behavior.
> >
> > These patches are against BK5.  No attempt has been made to integrate
> > with Jens' latest "scsi/sata write barrier" patches, but will look
> > into that when the dust has settled for both.
> > 
> > With BLK_DEV_HDWC at the default of 0, the current driver behavior is
> > maintained, i.e., the drive will always use the write cache per the
> > current code.  With the setting enabled, the driver will use the
> > default value of the drive write cache.
> 
> We have too many config options already.
> 
> Behavior should be simple:
> * no cache flushes - wcache off by default
> * cache flushes - wcache on by default
> * inform user about the wcache status 
> * allow changing of wcache by user

Fully agree, there's no point in making this a config option. Then you
could add options for tens of drive options. The default should be to
leave the drive setting alone.

If ->wcache is always correct, then I don't see an issue. User space can
change the write cache setting as they please, the barrier handling
should work accordingly. Always enable barriers on the journalled fs, if
write cache is off the pre and post flushes are not sent.

-- 
Jens Axboe