Re: cache alias in mmap + write

[anfei zhou]

On Thu, Jan 21, 2010 at 9:10 AM, KOSAKI Motohiro
<kosaki.motohiro@xxxxxxxxxxxxxx> wrote:
>> On Wed, Jan 20, 2010 at 06:10:11PM +0900, KOSAKI Motohiro wrote:
>> > Hello,
>> >
>> > > diff --git a/mm/filemap.c b/mm/filemap.c
>> > > index 96ac6b0..07056fb 100644
>> > > --- a/mm/filemap.c
>> > > +++ b/mm/filemap.c
>> > > @@ -2196,6 +2196,9 @@ again:
>> > >           if (unlikely(status))
>> > >                   break;
>> > >
>> > > +         if (mapping_writably_mapped(mapping))
>> > > +                 flush_dcache_page(page);
>> > > +
>> > >           pagefault_disable();
>> > >           copied = iov_iter_copy_from_user_atomic(page, i, offset, bytes);
>> > >           pagefault_enable();
>> >
>> > I'm not sure ARM cache coherency model. but I guess correct patch is here.
>> >
>> > +           if (mapping_writably_mapped(mapping))
>> > +                   flush_dcache_page(page);
>> > +
>> >             pagefault_disable();
>> >             copied = iov_iter_copy_from_user_atomic(page, i, offset, bytes);
>> >             pagefault_enable();
>> > -           flush_dcache_page(page);
>> >
>> > Why do we need to call flush_dcache_page() twice?
>> >
>> The latter flush_dcache_page is used to flush the kernel changes
>> (iov_iter_copy_from_user_atomic), which makes the userspace to see the
>> write,  and the one I added is used to flush the userspace changes.
>> And I think it's better to split this function into two:
>>       flush_dcache_user_page(page);
>>       kmap_atomic(page);
>>       write to  page;
>>       kunmap_atomic(page);
>>       flush_dcache_kern_page(page);
>> But currently there is no such API.
>
> Why can't we create new api? this your pseudo code looks very fine to me.
>
Thanks for your suggestion, I will try to add the new APIs.  But
firstly, as Jamie
pointed out, can we confirm is this a real bug? Or it depends on the arch.

>
> note: if you don't like to create new api. I can agree your current patch.
> but I have three requests.
>  1. Move flush_dcache_page() into iov_iter_copy_from_user_atomic().
>    Your above explanation indicate it is real intention. plus, change
>    iov_iter_copy_from_user_atomic() fixes fuse too.

OK.

>  2. Add some commnet. almost developer only have x86 machine. so, arm
>    specific trick need additional explicit explanation. otherwise anybody
>    might break this code in the future.
>  3. Resend the patch. original mail isn't good patch format. please consider
>    to reduce akpm suffer.
>
OK.

Thanks,
Anfei.
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/