diff mbox series

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

Message ID 20240821232241.3573997-9-joannelkoong@gmail.com (mailing list archive)
State New
Headers show
Series fuse: writeback clean up / refactoring | expand

Commit Message

Joanne Koong Aug. 21, 2024, 11:22 p.m. UTC 
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(-)

Comments

Miklos Szeredi Aug. 22, 2024, 9:58 a.m. UTC | #1 
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;
Joanne Koong Aug. 22, 2024, 5:08 p.m. UTC | #2 
On Thu, Aug 22, 2024 at 2:58 AM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> 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;

Gotcha. I'll use this pattern and drop the last patch in this series.

Thanks,
Joanne
diff mbox series

Patch

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;
+	}
 
 	ap = &wpa->ia.ap;
-	ap->args.in_pages = true;
 	ap->num_pages = 1;
-	ap->args.end = fuse_writepage_end;
 
 	folio_start_writeback(folio);
-	folio_copy(tmp_folio, folio);
-	ap->pages[0] = &tmp_folio->page;
-	ap->descs[0].offset = 0;
-	ap->descs[0].length = PAGE_SIZE;
-
-	inc_wb_stat(&inode_to_bdi(inode)->wb, WB_WRITEBACK);
-	node_stat_add_folio(tmp_folio, NR_WRITEBACK_TEMP);
+	fuse_writepage_args_page_fill(wpa, folio, tmp_folio, 0);
 
 	spin_lock(&fi->lock);
 	tree_insert(&fi->writepages, wpa);
@@ -2104,10 +2131,10 @@  static int fuse_writepage_locked(struct folio *folio)
 
 	return 0;
 
+err_writepage_args:
+	fuse_file_put(ff, false);
 err_nofile:
 	folio_put(tmp_folio);
-err_free:
-	kfree(wpa);
 err:
 	mapping_set_error(folio->mapping, error);
 	return error;
@@ -2155,7 +2182,6 @@  static void fuse_writepages_send(struct fuse_fill_wb_data *data)
 	int num_pages = wpa->ia.ap.num_pages;
 	int i;
 
-	wpa->ia.ff = fuse_file_get(data->ff);
 	spin_lock(&fi->lock);
 	list_add_tail(&wpa->queue_entry, &fi->queued_writes);
 	fuse_flush_writepages(inode);
@@ -2288,34 +2314,20 @@  static int fuse_writepages_fill(struct folio *folio,
 	 */
 	if (data->wpa == NULL) {
 		err = -ENOMEM;
-		wpa = fuse_writepage_args_alloc();
+		wpa = fuse_writepage_args_setup(folio, data->ff);
 		if (!wpa) {
 			folio_put(tmp_folio);
 			goto out_unlock;
 		}
-		fuse_writepage_add_to_bucket(fc, wpa);
-
+		fuse_file_get(wpa->ia.ff);
 		data->max_pages = 1;
-
 		ap = &wpa->ia.ap;
-		fuse_write_args_fill(&wpa->ia, data->ff, folio_pos(folio), 0);
-		wpa->ia.write.in.write_flags |= FUSE_WRITE_CACHE;
-		wpa->next = NULL;
-		ap->args.in_pages = true;
-		ap->args.end = fuse_writepage_end;
-		ap->num_pages = 0;
-		wpa->inode = inode;
 	}
 	folio_start_writeback(folio);
 
-	folio_copy(tmp_folio, folio);
-	ap->pages[ap->num_pages] = &tmp_folio->page;
-	ap->descs[ap->num_pages].offset = 0;
-	ap->descs[ap->num_pages].length = PAGE_SIZE;
-	data->orig_pages[ap->num_pages] = &folio->page;
+	fuse_writepage_args_page_fill(wpa, folio, tmp_folio, ap->num_pages);
 
-	inc_wb_stat(&inode_to_bdi(inode)->wb, WB_WRITEBACK);
-	inc_node_page_state(&tmp_folio->page, NR_WRITEBACK_TEMP);
+	data->orig_pages[ap->num_pages] = &folio->page;
 
 	err = 0;
 	if (data->wpa) {