On Tue, Jul 8, 2025 at 9:05 AM David Hildenbrand <david@xxxxxxxxxx> wrote:
>
> On 08.07.25 01:27, Joanne Koong wrote:
> > On Fri, Jul 4, 2025 at 3:24 AM David Hildenbrand <david@xxxxxxxxxx> 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@xxxxxxxxx>
> >>> Reviewed-by: Jeff Layton <jlayton@xxxxxxxxxx>
> >>> ---
> >>> 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 :)
>
Agreed, definitely the corner case :)
In the fuse code, later on when the buffer gets copied to/from the
server, it'll use ap->descs[index].length as the number of bytes to
copy. If we don't check for this duplicate page corner case, then
it'll copy the wrong offsets in the folio, which may even lead to a
page fault if the folio is only one page. This buffer copying logic if
you're curious happens in fuse_copy_args() -> fuse_copy_folios() ->
fuse_copy_folio().
> >
> >>
> >> 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@xxxxxxxxxxxxx/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
>
>
