Re: [RFC PATCH 2/5] filemap: do file page mapping with folio granularity

[Matthew Wilcox <willy@infradead.org>]

On Mon, Jan 30, 2023 at 08:55:01PM +0800, Yin Fengwei wrote:
> Add function to do file page mapping based on folio and update
> filemap_map_pages() to use new function. So the filemap page
> mapping will deal with folio granularity instead of page
> granularity. This allow batched folio refcount update.
> 
> Signed-off-by: Yin Fengwei <fengwei.yin@intel.com>
> ---
>  mm/filemap.c | 82 ++++++++++++++++++++++++++++++----------------------
>  1 file changed, 48 insertions(+), 34 deletions(-)
> 
> diff --git a/mm/filemap.c b/mm/filemap.c
> index c915ded191f0..fe0c226c8b1e 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -3351,6 +3351,43 @@ static inline struct folio *next_map_page(struct address_space *mapping,
>  				  mapping, xas, end_pgoff);
>  }
>  
> +

I'd remove this blank line, we typically only have one blank line
between functions.

> +static vm_fault_t filemap_map_folio_range(struct vm_fault *vmf,
> +	struct folio *folio, struct page *page, unsigned long addr,
> +	int len)

I see this under-indentation in other parts of the mm and it drives me
crazy.  Two tabs to indent the arguments please, otherwise they look
like part of the function.

Also, 'len' is ambiguous.  I'd call this 'nr' or 'nr_pages'.  Also
it should be an unsigned int.

> +{
> +	vm_fault_t ret = 0;
> +	struct vm_area_struct *vma = vmf->vma;
> +	struct file *file = vma->vm_file;
> +	unsigned int mmap_miss = READ_ONCE(file->f_ra.mmap_miss);
> +	int ref_count = 0, count = 0;

Also make these unsigned.

> -		/*
> -		 * NOTE: If there're PTE markers, we'll leave them to be
> -		 * handled in the specific fault path, and it'll prohibit the
> -		 * fault-around logic.
> -		 */

I'd rather not lose this comment; can you move it into
filemap_map_folio_range() please?

> -		if (!pte_none(*vmf->pte))
> -			goto unlock;
> -
> -		/* We're about to handle the fault */
> -		if (vmf->address == addr)
> +		if (VM_FAULT_NOPAGE ==
> +			filemap_map_folio_range(vmf, folio, page, addr, len))
>  			ret = VM_FAULT_NOPAGE;

That indentation is also confusing.  Try this:

		if (filemap_map_folio_range(vmf, folio, page, addr, len) ==
				VM_FAULT_NOPAGE)
			ret = VM_FAULT_NOPAGE;

Except there's an easier way to write it:

		ret |= filemap_map_folio_range(vmf, folio, page, addr, len);


Thanks for doing this!  Looks so much better and performs better!