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

[Matthew Wilcox]

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/