On Tuesday 31 August 2004 18:50, Alan Cox wrote:
> This fixes the logic so we always check for the cache. It also defaults
> to safer behaviour for the non cache flush case now we have the right bits
> in the right places. I've also played a bit with timings - the worst case
> timings I can get for the flush are about 7 seconds (which I'd expect
> as the engineering worst cases will include retries)
>
> Probably what should happen is that the barrier logic is enabled providing
> the wcache is disabled. I've not meddled with this as I don't know what
> the intended semantics and rules are for disabling barrier on a live disk
> (eg when a user uses hdparm to turn on the write cache). In the current
> code as with Jens original that cannot occur.
I think that logic is reversed here, I guess it should be: enable barrier
if user enables wcache and disable it if user disables wcache.
> I've also fixed the new printk's as per a private request from Matt Domsch.
Patch looks fine except:
> + /* Now we have barrier awareness we can be properly conservative
> + by default with other drives. We turn off write caching when
> + barrier is not available. Users can adjust this at runtime if
This is not true because there is a check for flush cache in write_cache().
I agree that disabling write cache by default is a good thing but user
should be informed about this fact (ideally there also should be easily
available FAQ somewhere) otherwise we will get a lot of bogus bugreports
about decreased performance...
> + they need unsafe but fast filesystems. This will reduce the
> + performance of non cache flush supporting disks but it means
> + you get the data order guarantees the journalling fs's require */
> +
> + write_cache(drive, barrier);
I'll drop this chunk and resend to Linus.
Thanks!
PS: please also cc: linux-ide on all ATA related stuff
On Gwe, 2004-09-03 at 14:54, Bartlomiej Zolnierkiewicz wrote:
> I think that logic is reversed here, I guess it should be: enable barrier
> if user enables wcache and disable it if user disables wcache.
Thinking about it from the drive side. If the drive wcache is off then
you have barrier semantics anyway.
So the logic is something like
has-flush cache-on barrier semantics
no-flush cache-on random semantics
no-flush cache-off barrier semantics
(the barrier itself is a nop)
> > + /* Now we have barrier awareness we can be properly conservative
> > + by default with other drives. We turn off write caching when
> > + barrier is not available. Users can adjust this at runtime if
>
> This is not true because there is a check for flush cache in write_cache().
>
> I agree that disabling write cache by default is a good thing but user
> should be informed about this fact (ideally there also should be easily
> available FAQ somewhere) otherwise we will get a lot of bogus bugreports
> about decreased performance...
Agreed. Unfortunately a lot of users are getting burned by journalling
fs's and IDE write caching. As the caches get bigger the risk gets
bigger. SCSI turns it off (and prints a message) so I'd rather see the
write_cache() changed to something like "write_cache_verbose()" and the
printk done than the issue ignored for longer.
Alan
On Fri, Sep 03 2004, Bartlomiej Zolnierkiewicz wrote:
>
> On Tuesday 31 August 2004 18:50, Alan Cox wrote:
> > This fixes the logic so we always check for the cache. It also defaults
> > to safer behaviour for the non cache flush case now we have the right bits
> > in the right places. I've also played a bit with timings - the worst case
> > timings I can get for the flush are about 7 seconds (which I'd expect
> > as the engineering worst cases will include retries)
> >
> > Probably what should happen is that the barrier logic is enabled providing
> > the wcache is disabled. I've not meddled with this as I don't know what
> > the intended semantics and rules are for disabling barrier on a live disk
> > (eg when a user uses hdparm to turn on the write cache). In the current
> > code as with Jens original that cannot occur.
>
> I think that logic is reversed here, I guess it should be: enable barrier
> if user enables wcache and disable it if user disables wcache.
There's no need for changes, ide_queue_flush_cmd() handles this fine
right now.
--
Jens Axboe