Re: [PATCH v12 04/20] mm: Allow page fault handlers to perform the COW

From: Matthew Wilcox
Date: Tue Jan 13 2015 - 13:58:36 EST


On Mon, Jan 12, 2015 at 03:09:35PM -0800, Andrew Morton wrote:
> On Fri, 24 Oct 2014 17:20:36 -0400 Matthew Wilcox <matthew.r.wilcox@xxxxxxxxx> wrote:
> > Currently COW of an XIP file is done by first bringing in a read-only
> > mapping, then retrying the fault and copying the page. It is much more
> > efficient to tell the fault handler that a COW is being attempted (by
> > passing in the pre-allocated page in the vm_fault structure), and allow
> > the handler to perform the COW operation itself.
> >
> > The handler cannot insert the page itself if there is already a read-only
> > mapping at that address, so allow the handler to return VM_FAULT_LOCKED
> > and set the fault_page to be NULL. This indicates to the MM code that
> > the i_mmap_mutex is held instead of the page lock.
>
> Again, the locking gets a bit subtle. How can we make this clearer to
> readers of the core code. I had a shot but it's a bit lame - DAX uses
> i_mmap_lock for what???

It's not just DAX ... any fault handler that wants to optimise its COW
can use the same technique. I could turn this around and ask the mm
people why it is the struct page has to be returned locked; what is it
protecting against?

I'm pretty sure the answer is only truncate, and so (as with the previous
patch), the read lock is perfectly appropriate.

> If I know that, I'd know whether to have used i_mmap_lock_read() or
> i_mmap_lock_write() :(
>
>
> From: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> Subject: mm-allow-page-fault-handlers-to-perform-the-cow-fix
>
> Cc: Kirill A. Shutemov <kirill.shutemov@xxxxxxxxxxxxxxx>
> Cc: Matthew Wilcox <matthew.r.wilcox@xxxxxxxxx>
> Signed-off-by: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> ---
>
> mm/memory.c | 12 ++++++++++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff -puN include/linux/mm.h~mm-allow-page-fault-handlers-to-perform-the-cow-fix include/linux/mm.h
> diff -puN mm/memory.c~mm-allow-page-fault-handlers-to-perform-the-cow-fix mm/memory.c
> --- a/mm/memory.c~mm-allow-page-fault-handlers-to-perform-the-cow-fix
> +++ a/mm/memory.c
> @@ -2961,7 +2961,11 @@ static int do_cow_fault(struct mm_struct
> unlock_page(fault_page);
> page_cache_release(fault_page);
> } else {
> - mutex_unlock(&vma->vm_file->f_mapping->i_mmap_mutex);
> + /*
> + * DAX doesn't have a page to lock, so it uses
> + * i_mmap_lock()
> + */
> + i_mmap_unlock_read(&vma->vm_file->f_mapping);

How about:
/*
* The fault handler has no page to lock, so it
* holds i_mmap_lock for read to protect against
* truncate.
*/

> }
> goto uncharge_out;
> }
> @@ -2973,7 +2977,11 @@ static int do_cow_fault(struct mm_struct
> unlock_page(fault_page);
> page_cache_release(fault_page);
> } else {
> - mutex_unlock(&vma->vm_file->f_mapping->i_mmap_mutex);
> + /*
> + * DAX doesn't have a page to lock, so it uses
> + * i_mmap_lock()
> + */
> + i_mmap_unlock_read(&vma->vm_file->f_mapping);

(as Jan already pointed out, the indentation needs to be fixed here anyway)

> }
> return ret;
> uncharge_out:
> _
>
--
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/