On Wed, 2011-01-05 at 17:26 +0000, Russell King - ARM Linux wrote: 

On Wed, Jan 05, 2011 at 12:17:27PM -0500, Trond Myklebust wrote:

quoted

We should already be flushing the kernel direct mapping after writing by
means of the calls to flush_dcache_page() in xdr_partial_copy_from_skb()
and all the helpers in net/sunrpc/xdr.c.

Hmm, we're getting into the realms of what flush_dcache_page() is supposed
to do and what it's not supposed to do.

Is this page an associated with a mapping (iow, page_mapping(page) is non-
NULL)?  If not, flush_dcache_page() won't do anything, and from my
understanding, its flush_anon_page() which you want to be using there
instead.

Actually, none of these pages are ever mapped into userspace, nor are
they mapped into the page cache.

They are allocated directly using alloc_page() by the thread that called
the readdir() syscall, so afaics there should be no incoherent mappings
other than the kernel direct mapping and the one created by
vm_map_ram().

So, yes, you are right that we don't need the flush_dcache_page() here.

quoted

The only new thing is the read access through the virtual address
mapping. That mapping is created outside the loop in
nfs_readdir_xdr_to_array(), which is why I'm thinking we do need the
invalidate_kernel_vmap_range(): we're essentially doing a series of
writes through the kernel direct mapping (i.e. readdir RPC calls), then
reading the results through the virtual mapping.

i.e. we're doing

ptr = vm_map_ram(lowmem_pages);
while (need_more_data) {

for (i = 0; i < npages; i++) {
addr = kmap_atomic(lowmem_page[i]);
*addr = rpc_stuff;
flush_dcache_page(lowmem_page[i]);
kunmap_atomic(lowmem_page[i]);
}

invalidate_kernel_vmap_range(ptr); // Needed here?

Yes, you're going to need some cache maintainence in there to make it work,
because accessing 'ptr' will load that data into the cache, and that won't
be updated by the writes via kmap_atomic().

Provided you don't write to ptr, then using invalidate_kernel_vmap_range()
will be safe.

Thanks! That is what Marc's testing appears to confirm.

-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust at netapp.com
www.netapp.com