Re: [patch for 2.6.33? 1/1] ata: call flush_dcache_page() around PIO data transfers in libata-aff.c

[James Bottomley <James.Bottomley@suse.de>]

On Wed, 2010-02-03 at 12:29 -0500, Jeff Garzik wrote:
> On 02/03/2010 12:20 PM, Andrew Morton wrote:
> > On Wed, 03 Feb 2010 12:15:46 -0500 Jeff Garzik<jgarzik@pobox.com>  wrote:
> >
> >> On 02/03/2010 12:06 PM, Andrew Morton wrote:
> >>> On Wed, 03 Feb 2010 12:00:58 -0500 Jeff Garzik<jgarzik@pobox.com>   wrote:
> >>>
> >>>> On 02/03/2010 11:40 AM, James Bottomley wrote:
> >>>>> The fix to libata looks to be just that it should kmap all the time
> >>>>> rather than trying to fiddle with the page to see if its higmem.  For
> >>>>> kmap on a normal page, we should just return the offset map address and
> >>>>> do all the flushing.
> >>>>
> >>>> libata tests PageHighMem() because it was measurably faster to do things
> >>>> the current way (which includes local_irq_save/restore, only for
> >>>> highmem) on boxes where it actually matters.
> >>>>
> >>>> It seems more efficient to add a flush where necessary, than
> >>>> unconditionally punish everyone...
> >>>
> >>> kmap_atomic() tests PageHighMem() too - it's pretty lightweight for
> >>> lowmem pages.
> >>
> >> Note the lack of local_irq_save/restore in our code, though...  These
> >> PIO xfers are __slow__, from the perspective of a CPU manufactured in
> >> the past decade; you are definitely disabling local interrupts for a
> >> long time.  I suppose we could do
> >>
> >> 	if (high mem)
> >> 		local irq save
> >> 		kmap
> >> 		xfer
> >> 		kunmap
> >> 		local irq restore
> >> 	else
> >> 		kmap
> >> 		xfer
> >> 		kunmap
> >>
> >> does that solve the problem for ARM, for 2.6.33?
> >>
> >
> > It's unclear (to me) why that code is using KM_IRQ0 at all.  Can't it
> > use a non-irq kmap slot?
> 
> libata may have to transfer data in response to an interrupt.  That is 
> normal interrupt-driven PIO -- although it should be noted that the code 
> supports polled PIO as well.
> 
> 
> >>> Anyway, I'd draw your attention to this claim in the changelog: "This
> >>> patch allows the ARM boards to use a rootfs on CompactFlash with the
> >>> PATA platform driver." Immediate-term, we should be looking for a small
> >>> fix for this issue which is acceptable for 2.6.33 and 2.6.32 and earlier.
> >>
> >> Sure...  see above.  hopefully one that does not punish -everybody-
> >> though.  It would be sad to unconditionally slow down millions of volume
> >> platform (read: x86) users for some embedded boards.
> >
> > Well.
> > ata-call-flush_dcache_page-around-pio-data-transfers-in-libata-affc.patch
> > is a no-op on x86.  It only has an effect on architectures which
> > implement flush_dcache_page().  And I expect flush_dcache_page() is
> > pretty light on those architectures, when compared with a PIO-mode
> > transfer.
> 
> I don't object to the patch...  as long as the arch people are happy. 
> Arch people seem to be the ones complaining, though

Apart from the contents, which is looking like sprinkle mainline with
random flushes, I'm unhappy that something which could be better
implemented by thinking about the problem is now being rammed through as
a must have bug fix.

We got this piece of crap in the same way:

commit 2d4dc890b5c8fabd818a8586607e6843c4375e62
Author: Ilya Loginov <isloginov@gmail.com>
Date:   Thu Nov 26 09:16:19 2009 +0100

    block: add helpers to run flush_dcache_page() against a bio and a
request's pages

Which is another race to flush everywhere until my coherence problem
goes away.

This scattershot approach to coherency is really damaging in the long
term because all these random flushes are going to mask real problems we
may or may not have in the arch APIs ... and worse, they'll mask
mostly ... there'll likely be times when the masking is incomplete and
we're left with incredibly hard to debug data loss or binary crashes.

James