Re: [PATCH 01/13] fuse: support folios in struct fuse_args_pages and fuse_copy_pages()

[Josef Bacik <josef@toxicpanda.com>]

On Wed, Oct 02, 2024 at 09:52:41AM -0700, Joanne Koong wrote:
> This adds support in struct fuse_args_pages and fuse_copy_pages() for
> using folios instead of pages for transferring data. Both folios and
> pages must be supported right now in struct fuse_args_pages and
> fuse_copy_pages() until all request types have been converted to use
> folios. Once all have been converted, then
> struct fuse_args_pages and fuse_copy_pages() will only support folios.
> 
> Right now in fuse, all folios are one page (large folios are not yet
> supported). As such, copying folio->page is sufficient for copying
> the entire folio in fuse_copy_pages().
> 
> No functional changes.
> 
> Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> ---
>  fs/fuse/dev.c    | 36 ++++++++++++++++++++++++++++--------
>  fs/fuse/fuse_i.h | 22 +++++++++++++++++++---
>  2 files changed, 47 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
> index 7e4c5be45aec..cd9c5e0eefca 100644
> --- a/fs/fuse/dev.c
> +++ b/fs/fuse/dev.c
> @@ -1028,17 +1028,37 @@ static int fuse_copy_pages(struct fuse_copy_state *cs, unsigned nbytes,
>  	struct fuse_req *req = cs->req;
>  	struct fuse_args_pages *ap = container_of(req->args, typeof(*ap), args);
>  
> +	if (ap->uses_folios) {
> +		for (i = 0; i < ap->num_folios && (nbytes || zeroing); i++) {
> +			int err;
> +			unsigned int offset = ap->folio_descs[i].offset;
> +			unsigned int count = min(nbytes, ap->folio_descs[i].length);
> +			struct page *orig, *pagep;
>  
> -	for (i = 0; i < ap->num_pages && (nbytes || zeroing); i++) {
> -		int err;
> -		unsigned int offset = ap->descs[i].offset;
> -		unsigned int count = min(nbytes, ap->descs[i].length);
> +			orig = pagep = &ap->folios[i]->page;
>  
> -		err = fuse_copy_page(cs, &ap->pages[i], offset, count, zeroing);
> -		if (err)
> -			return err;
> +			err = fuse_copy_page(cs, &pagep, offset, count, zeroing);
> +			if (err)
> +				return err;
> +
> +			nbytes -= count;
> +
> +			/* Check if the folio was replaced in the page cache */

This comment confused me, I think it would be better to say something like

/*
 * fuse_copy_page may have moved a page from a pipe instead of copying into our
 * given page, so update the folios if it was replaced.
 */

Or something like that.  Thanks,

Josef