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