Re: [PATCH v5 10/11] fuse: optimize direct io large folios processing

[David Hildenbrand <david@redhat.com>]

On 08.07.25 01:27, Joanne Koong wrote:
> On Fri, Jul 4, 2025 at 3:24 AM David Hildenbrand <david@redhat.com> wrote:
>>
>> On 26.04.25 02:08, Joanne Koong wrote:
>>> Optimize processing folios larger than one page size for the direct io
>>> case. If contiguous pages are part of the same folio, collate the
>>> processing instead of processing each page in the folio separately.
>>>
>>> Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
>>> Reviewed-by: Jeff Layton <jlayton@kernel.org>
>>> ---
>>>    fs/fuse/file.c | 55 +++++++++++++++++++++++++++++++++++++-------------
>>>    1 file changed, 41 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
>>> index 9a31f2a516b9..61eaec1c993b 100644
>>> --- a/fs/fuse/file.c
>>> +++ b/fs/fuse/file.c
>>> @@ -1490,7 +1490,8 @@ static int fuse_get_user_pages(struct fuse_args_pages *ap, struct iov_iter *ii,
>>>        }
>>>
>>>        while (nbytes < *nbytesp && nr_pages < max_pages) {
>>> -             unsigned nfolios, i;
>>> +             struct folio *prev_folio = NULL;
>>> +             unsigned npages, i;
>>>                size_t start;
>>>
>>>                ret = iov_iter_extract_pages(ii, &pages,
>>> @@ -1502,23 +1503,49 @@ static int fuse_get_user_pages(struct fuse_args_pages *ap, struct iov_iter *ii,
>>>
>>>                nbytes += ret;
>>>
>>> -             nfolios = DIV_ROUND_UP(ret + start, PAGE_SIZE);
>>> +             npages = DIV_ROUND_UP(ret + start, PAGE_SIZE);
>>>
>>> -             for (i = 0; i < nfolios; i++) {
>>> -                     struct folio *folio = page_folio(pages[i]);
>>> -                     unsigned int offset = start +
>>> -                             (folio_page_idx(folio, pages[i]) << PAGE_SHIFT);
>>> -                     unsigned int len = min_t(unsigned int, ret, PAGE_SIZE - start);
>>> +             /*
>>> +              * We must check each extracted page. We can't assume every page
>>> +              * in a large folio is used. For example, userspace may mmap() a
>>> +              * file PROT_WRITE, MAP_PRIVATE, and then store to the middle of
>>> +              * a large folio, in which case the extracted pages could be
>>> +              *
>>> +              * folio A page 0
>>> +              * folio A page 1
>>> +              * folio B page 0
>>> +              * folio A page 3
>>> +              *
>>> +              * where folio A belongs to the file and folio B is an anonymous
>>> +              * COW page.
>>> +              */
>>> +             for (i = 0; i < npages && ret; i++) {
>>> +                     struct folio *folio;
>>> +                     unsigned int offset;
>>> +                     unsigned int len;
>>> +
>>> +                     WARN_ON(!pages[i]);
>>> +                     folio = page_folio(pages[i]);
>>> +
>>> +                     len = min_t(unsigned int, ret, PAGE_SIZE - start);
>>> +
>>> +                     if (folio == prev_folio && pages[i] != pages[i - 1]) {
>>
>> I don't really understand the "pages[i] != pages[i - 1]" part.
>>
>> Why would you have to equal page pointers in there?
>>
> 
> The pages extracted are user pages from a userspace iovec. AFAICT,
> there's the possibility, eg if userspace mmaps() the file with
> copy-on-write, that the same physical page could back multiple
> contiguous virtual addresses.

Yes, I but I was rather curious why that would be a condition we are 
checking. It's quite the ... corner case :)

> 
>>
>> Something that might be simpler to understand and implement would be using
>>
>>          num_pages_contiguous()
>>
>> from
>>
>>          https://lore.kernel.org/all/20250704062602.33500-2-lizhe.67@bytedance.com/T/#u
>>
>> and then just making sure that we don't exceed the current folio, if we
>> ever get contiguous pages that cross a folio.
> 
> Thanks for the link. I think here it's common that the pages array
> would hold pages from multiple different folios, so maybe a new helper
> num_pages_contiguous_folio() would be useful to return back the number
> of contiguous pages that are within the scope of the same folio.

Yes, something like that can be useful.

-- 
Cheers,

David / dhildenb