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

["Yin, Fengwei" <fengwei.yin@intel.com>]

On 1/30/2023 9:35 PM, Matthew Wilcox wrote:
> 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.
OK.

> 
>> +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.
OK. I will correct all the indent problems in this series in next version.
> 
> 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?
I will keep all the comments in the right place in next version.

Regards
Yin, Fengwei

> 
>> -		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!