On Thu, Aug 22, 2024 at 2:58 AM Miklos Szeredi <miklos@xxxxxxxxxx> wrote:
>
> On Thu, 22 Aug 2024 at 01:25, Joanne Koong <joannelkoong@xxxxxxxxx> 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@xxxxxxxxx>
> > ---
> > 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;
Gotcha. I'll use this pattern and drop the last patch in this series.
Thanks,
Joanne
