Re: [PATCH v2 8/9] fuse: refactor out shared logic in fuse_writepages_fill() and fuse_writepage_locked()

[Miklos Szeredi <miklos@szeredi.hu>]

On Thu, 22 Aug 2024 at 01:25, Joanne Koong <joannelkoong@gmail.com> wrote:
>
> This change refactors the shared logic in fuse_writepages_fill() and
> fuse_writepages_locked() into two separate helper functions,
> fuse_writepage_args_page_fill() and fuse_writepage_args_setup().
>
> No functional changes added.
>
> Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> ---
>  fs/fuse/file.c | 102 +++++++++++++++++++++++++++----------------------
>  1 file changed, 57 insertions(+), 45 deletions(-)
>
> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> index 812b3d043b26..fe8ae19587fb 100644
> --- a/fs/fuse/file.c
> +++ b/fs/fuse/file.c
> @@ -1936,7 +1936,6 @@ static void fuse_writepage_end(struct fuse_mount *fm, struct fuse_args *args,
>
>                 wpa->next = next->next;
>                 next->next = NULL;
> -               next->ia.ff = fuse_file_get(wpa->ia.ff);
>                 tree_insert(&fi->writepages, next);
>
>                 /*
> @@ -2049,50 +2048,78 @@ static void fuse_writepage_add_to_bucket(struct fuse_conn *fc,
>         rcu_read_unlock();
>  }
>
> +static void fuse_writepage_args_page_fill(struct fuse_writepage_args *wpa, struct folio *folio,
> +                                         struct folio *tmp_folio, uint32_t page_index)
> +{
> +       struct inode *inode = folio->mapping->host;
> +       struct fuse_args_pages *ap = &wpa->ia.ap;
> +
> +       folio_copy(tmp_folio, folio);
> +
> +       ap->pages[page_index] = &tmp_folio->page;
> +       ap->descs[page_index].offset = 0;
> +       ap->descs[page_index].length = PAGE_SIZE;
> +
> +       inc_wb_stat(&inode_to_bdi(inode)->wb, WB_WRITEBACK);
> +       inc_node_page_state(&tmp_folio->page, NR_WRITEBACK_TEMP);
> +}
> +
> +static struct fuse_writepage_args *fuse_writepage_args_setup(struct folio *folio,
> +                                                            struct fuse_file *ff)
> +{
> +       struct inode *inode = folio->mapping->host;
> +       struct fuse_conn *fc = get_fuse_conn(inode);
> +       struct fuse_writepage_args *wpa;
> +       struct fuse_args_pages *ap;
> +
> +       wpa = fuse_writepage_args_alloc();
> +       if (!wpa)
> +              return NULL;
> +
> +       fuse_writepage_add_to_bucket(fc, wpa);
> +       fuse_write_args_fill(&wpa->ia, ff, folio_pos(folio), 0);
> +       wpa->ia.write.in.write_flags |= FUSE_WRITE_CACHE;
> +       wpa->inode = inode;
> +       wpa->ia.ff = ff;
> +
> +       ap = &wpa->ia.ap;
> +       ap->args.in_pages = true;
> +       ap->args.end = fuse_writepage_end;
> +
> +       return wpa;
> +}
> +
>  static int fuse_writepage_locked(struct folio *folio)
>  {
>         struct address_space *mapping = folio->mapping;
>         struct inode *inode = mapping->host;
> -       struct fuse_conn *fc = get_fuse_conn(inode);
>         struct fuse_inode *fi = get_fuse_inode(inode);
>         struct fuse_writepage_args *wpa;
>         struct fuse_args_pages *ap;
>         struct folio *tmp_folio;
> +       struct fuse_file *ff;
>         int error = -ENOMEM;
>
> -       wpa = fuse_writepage_args_alloc();
> -       if (!wpa)
> -               goto err;
> -
>         tmp_folio = folio_alloc(GFP_NOFS | __GFP_HIGHMEM, 0);
>         if (!tmp_folio)
> -               goto err_free;
> +               goto err;
>
>         error = -EIO;
> -       wpa->ia.ff = fuse_write_file_get(fi);
> -       if (!wpa->ia.ff)
> +       ff = fuse_write_file_get(fi);
> +       if (!ff)
>                 goto err_nofile;
>
> -       fuse_writepage_add_to_bucket(fc, wpa);
> -       fuse_write_args_fill(&wpa->ia, wpa->ia.ff, folio_pos(folio), 0);
> -
> -       wpa->ia.write.in.write_flags |= FUSE_WRITE_CACHE;
> -       wpa->next = NULL;
> -       wpa->inode = inode;
> +       wpa = fuse_writepage_args_setup(folio, ff);
> +       if (!wpa) {
> +               error = -ENOMEM;
> +               goto err_writepage_args;
> +       }

Please use the following pattern, unless there's a good reason not to:

+       error = -ENOMEM;
+       if (!wpa)
+               goto err_writepage_args;