[PATCH v3 0/9] fuse: writeback clean up / refactoring

[PATCH v3 0/9] fuse: writeback clean up / refactoring

[Joanne Koong]

This patchset contains some minor clean up / refactoring for the fuse
writeback code.

As a sanity check, I ran fio to check against crashes -
./libfuse/build/example/passthrough_ll -o cache=always -o writeback -o source=~/fstests ~/tmp_mount
fio --name=test --ioengine=psync --iodepth=1 --rw=randwrite --bs=1M --direct=0 --size=2G --numjobs=2 --directory=/home/user/tmp_mount

and (suggested by Miklos) fsx test -
sudo HOST_OPTIONS=fuse.config ./check -fuse generic/616
generic/616 (soak buffered fsx test) without the -U (io_uring) flag
(verified this uses the fuse_writepages_fill path)


v2: https://lore.kernel.org/linux-fsdevel/20240821232241.3573997-1-joannelkoong@gmail.com/
Changes from v2 -> v3:
* Drop v2 9/9 (Miklos)
* Split v2 8/9 into 2 patches (v3 8/9 and 9/9) to review easier
* Change error pattern usage (Miklos)

v1: https://lore.kernel.org/linux-fsdevel/20240819182417.504672-1-joannelkoong@gmail.com/
Changes from v1 -> v2:
* Added patches 2 and 4-9
* Add commit message to patch 1 (Jingbo)

Joanne Koong (9):
  fuse: drop unused fuse_mount arg in fuse_writepage_finish()
  fuse: refactor finished writeback stats updates into helper function
  fuse: update stats for pages in dropped aux writeback list
  fuse: clean up error handling in fuse_writepages()
  fuse: move initialization of fuse_file to fuse_writepages() instead of
    in callback
  fuse: convert fuse_writepages_fill() to use a folio for its tmp page
  fuse: move folio_start_writeback to after the allocations in
    fuse_writepage_locked()
  fuse: move fuse file initialization to wpa allocation time
  fuse: refactor out shared logic in fuse_writepages_fill() and
    fuse_writepage_locked()

 fs/fuse/file.c | 169 +++++++++++++++++++++++++------------------------
 1 file changed, 87 insertions(+), 82 deletions(-)

-- 
2.43.5

[PATCH v3 1/9] fuse: drop unused fuse_mount arg in fuse_writepage_finish()

[Joanne Koong]

Drop the unused "struct fuse_mount *fm" arg in
fuse_writepage_finish().

No functional changes added.

Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
Reviewed-by: Jingbo Xu <jefflexu@linux.alibaba.com>
---
 fs/fuse/file.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index f39456c65ed7..63fd5fc6872e 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -1769,8 +1769,7 @@ static void fuse_writepage_free(struct fuse_writepage_args *wpa)
 	kfree(wpa);
 }
 
-static void fuse_writepage_finish(struct fuse_mount *fm,
-				  struct fuse_writepage_args *wpa)
+static void fuse_writepage_finish(struct fuse_writepage_args *wpa)
 {
 	struct fuse_args_pages *ap = &wpa->ia.ap;
 	struct inode *inode = wpa->inode;
@@ -1829,7 +1828,7 @@ __acquires(fi->lock)
  out_free:
 	fi->writectr--;
 	rb_erase(&wpa->writepages_entry, &fi->writepages);
-	fuse_writepage_finish(fm, wpa);
+	fuse_writepage_finish(wpa);
 	spin_unlock(&fi->lock);
 
 	/* After fuse_writepage_finish() aux request list is private */
@@ -1959,7 +1958,7 @@ static void fuse_writepage_end(struct fuse_mount *fm, struct fuse_args *args,
 		fuse_send_writepage(fm, next, inarg->offset + inarg->size);
 	}
 	fi->writectr--;
-	fuse_writepage_finish(fm, wpa);
+	fuse_writepage_finish(wpa);
 	spin_unlock(&fi->lock);
 	fuse_writepage_free(wpa);
 }
-- 
2.43.5

[PATCH v3 2/9] fuse: refactor finished writeback stats updates into helper function

[Joanne Koong]

Move the logic for updating the bdi and page stats for a finished
writeback into a separate helper function, where it can be called from
both fuse_writepage_finish() and fuse_writepage_add() (in the case
where there is already an auxiliary write request for the page).

No functional changes added.

Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
Suggested by: Jingbo Xu <jefflexu@linux.alibaba.com>
---
 fs/fuse/file.c | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index 63fd5fc6872e..320fa26b23e8 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -1769,19 +1769,25 @@ static void fuse_writepage_free(struct fuse_writepage_args *wpa)
 	kfree(wpa);
 }
 
+static void fuse_writepage_finish_stat(struct inode *inode, struct page *page)
+{
+	struct backing_dev_info *bdi = inode_to_bdi(inode);
+
+	dec_wb_stat(&bdi->wb, WB_WRITEBACK);
+	dec_node_page_state(page, NR_WRITEBACK_TEMP);
+	wb_writeout_inc(&bdi->wb);
+}
+
 static void fuse_writepage_finish(struct fuse_writepage_args *wpa)
 {
 	struct fuse_args_pages *ap = &wpa->ia.ap;
 	struct inode *inode = wpa->inode;
 	struct fuse_inode *fi = get_fuse_inode(inode);
-	struct backing_dev_info *bdi = inode_to_bdi(inode);
 	int i;
 
-	for (i = 0; i < ap->num_pages; i++) {
-		dec_wb_stat(&bdi->wb, WB_WRITEBACK);
-		dec_node_page_state(ap->pages[i], NR_WRITEBACK_TEMP);
-		wb_writeout_inc(&bdi->wb);
-	}
+	for (i = 0; i < ap->num_pages; i++)
+		fuse_writepage_finish_stat(inode, ap->pages[i]);
+
 	wake_up(&fi->page_waitq);
 }
 
@@ -2203,11 +2209,7 @@ static bool fuse_writepage_add(struct fuse_writepage_args *new_wpa,
 	spin_unlock(&fi->lock);
 
 	if (tmp) {
-		struct backing_dev_info *bdi = inode_to_bdi(new_wpa->inode);
-
-		dec_wb_stat(&bdi->wb, WB_WRITEBACK);
-		dec_node_page_state(new_ap->pages[0], NR_WRITEBACK_TEMP);
-		wb_writeout_inc(&bdi->wb);
+		fuse_writepage_finish_stat(new_wpa->inode, new_ap->pages[0]);
 		fuse_writepage_free(new_wpa);
 	}
 
-- 
2.43.5

[PATCH v3 3/9] fuse: update stats for pages in dropped aux writeback list

[Joanne Koong]

In the case where the aux writeback list is dropped (eg the pages
have been truncated or the connection is broken), the stats for
its pages and backing device info need to be updated as well.

Fixes: e2653bd53a98 ("fuse: fix leaked aux requests")
Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
---
 fs/fuse/file.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index 320fa26b23e8..1ae58f93884e 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -1837,10 +1837,11 @@ __acquires(fi->lock)
 	fuse_writepage_finish(wpa);
 	spin_unlock(&fi->lock);
 
-	/* After fuse_writepage_finish() aux request list is private */
+	/* After rb_erase() aux request list is private */
 	for (aux = wpa->next; aux; aux = next) {
 		next = aux->next;
 		aux->next = NULL;
+		fuse_writepage_finish_stat(aux->inode, aux->ia.ap.pages[0]);
 		fuse_writepage_free(aux);
 	}
 
-- 
2.43.5

[PATCH v3 4/9] fuse: clean up error handling in fuse_writepages()

[Joanne Koong]

Clean up the error handling paths in fuse_writepages().
No functional changes added.

Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
---
 fs/fuse/file.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index 1ae58f93884e..8a9b6e8dbd1b 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -2352,9 +2352,8 @@ static int fuse_writepages(struct address_space *mapping,
 	struct fuse_fill_wb_data data;
 	int err;
 
-	err = -EIO;
 	if (fuse_is_bad(inode))
-		goto out;
+		return -EIO;
 
 	if (wbc->sync_mode == WB_SYNC_NONE &&
 	    fc->num_background >= fc->congestion_threshold)
@@ -2364,12 +2363,11 @@ static int fuse_writepages(struct address_space *mapping,
 	data.wpa = NULL;
 	data.ff = NULL;
 
-	err = -ENOMEM;
 	data.orig_pages = kcalloc(fc->max_pages,
 				  sizeof(struct page *),
 				  GFP_NOFS);
 	if (!data.orig_pages)
-		goto out;
+		return -ENOMEM;
 
 	err = write_cache_pages(mapping, wbc, fuse_writepages_fill, &data);
 	if (data.wpa) {
@@ -2380,7 +2378,6 @@ static int fuse_writepages(struct address_space *mapping,
 		fuse_file_put(data.ff, false);
 
 	kfree(data.orig_pages);
-out:
 	return err;
 }
 
-- 
2.43.5

[PATCH v3 5/9] fuse: move initialization of fuse_file to fuse_writepages() instead of in callback

[Joanne Koong]

Prior to this change, data->ff is checked and if not initialized then
initialized in the fuse_writepages_fill() callback, which gets called
for every dirty page in the address space mapping.

This logic is better placed in the main fuse_writepages() caller where
data.ff is initialized before walking the dirty pages.

No functional changes added.

Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
---
 fs/fuse/file.c | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index 8a9b6e8dbd1b..a51b0b085616 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -2263,13 +2263,6 @@ static int fuse_writepages_fill(struct folio *folio,
 	struct page *tmp_page;
 	int err;
 
-	if (!data->ff) {
-		err = -EIO;
-		data->ff = fuse_write_file_get(fi);
-		if (!data->ff)
-			goto out_unlock;
-	}
-
 	if (wpa && fuse_writepage_need_send(fc, &folio->page, ap, data)) {
 		fuse_writepages_send(data);
 		data->wpa = NULL;
@@ -2348,6 +2341,7 @@ static int fuse_writepages(struct address_space *mapping,
 			   struct writeback_control *wbc)
 {
 	struct inode *inode = mapping->host;
+	struct fuse_inode *fi = get_fuse_inode(inode);
 	struct fuse_conn *fc = get_fuse_conn(inode);
 	struct fuse_fill_wb_data data;
 	int err;
@@ -2361,23 +2355,26 @@ static int fuse_writepages(struct address_space *mapping,
 
 	data.inode = inode;
 	data.wpa = NULL;
-	data.ff = NULL;
+	data.ff = fuse_write_file_get(fi);
+	if (!data.ff)
+		return -EIO;
 
 	data.orig_pages = kcalloc(fc->max_pages,
 				  sizeof(struct page *),
 				  GFP_NOFS);
+	err = -ENOMEM;
 	if (!data.orig_pages)
-		return -ENOMEM;
+		goto out;
 
 	err = write_cache_pages(mapping, wbc, fuse_writepages_fill, &data);
 	if (data.wpa) {
 		WARN_ON(!data.wpa->ia.ap.num_pages);
 		fuse_writepages_send(&data);
 	}
-	if (data.ff)
-		fuse_file_put(data.ff, false);
 
 	kfree(data.orig_pages);
+out:
+	fuse_file_put(data.ff, false);
 	return err;
 }
 
-- 
2.43.5

Re: [PATCH v3 5/9] fuse: move initialization of fuse_file to fuse_writepages() instead of in callback

[Josef Bacik]

On Fri, Aug 23, 2024 at 09:27:26AM -0700, Joanne Koong wrote:
> Prior to this change, data->ff is checked and if not initialized then
> initialized in the fuse_writepages_fill() callback, which gets called
> for every dirty page in the address space mapping.
> 
> This logic is better placed in the main fuse_writepages() caller where
> data.ff is initialized before walking the dirty pages.
> 
> No functional changes added.
> 
> Signed-off-by: Joanne Koong <joannelkoong@gmail.com>

You remove the out label in the previous patch, and then add it back here, you
can probably merge the previous patch and this patch into one patch and it would
look fine, and reduce the churn a bit.  Thanks,

Josef

Re: [PATCH v3 5/9] fuse: move initialization of fuse_file to fuse_writepages() instead of in callback

[Joanne Koong]

On Fri, Aug 23, 2024 at 11:59 AM Josef Bacik <josef@toxicpanda.com> wrote:
>
> On Fri, Aug 23, 2024 at 09:27:26AM -0700, Joanne Koong wrote:
> > Prior to this change, data->ff is checked and if not initialized then
> > initialized in the fuse_writepages_fill() callback, which gets called
> > for every dirty page in the address space mapping.
> >
> > This logic is better placed in the main fuse_writepages() caller where
> > data.ff is initialized before walking the dirty pages.
> >
> > No functional changes added.
> >
> > Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
>
> You remove the out label in the previous patch, and then add it back here, you
> can probably merge the previous patch and this patch into one patch and it would
> look fine, and reduce the churn a bit.  Thanks,

Gotcha. I'll merge the previous patch (4/9) with this one together in
the next version.

Thanks,
Joanne

>
> Josef

[PATCH v3 6/9] fuse: convert fuse_writepages_fill() to use a folio for its tmp page

[Joanne Koong]

To pave the way for refactoring out the shared logic in
fuse_writepages_fill() and fuse_writepage_locked(), this change converts
the temporary page in fuse_writepages_fill() to use the folio API.

This is similar to the change in e0887e095a80 ("fuse: Convert
fuse_writepage_locked to take a folio"), which converted the tmp page in
fuse_writepage_locked() to use the folio API.

inc_node_page_state() is intentionally preserved here instead of
converting to node_stat_add_folio() since it is updating the stat of the
underlying page and to better maintain API symmetry with
dec_node_page_stat() in fuse_writepage_finish_stat().

No functional changes added.

Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
---
 fs/fuse/file.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index a51b0b085616..905b202a7acd 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -2260,7 +2260,7 @@ static int fuse_writepages_fill(struct folio *folio,
 	struct inode *inode = data->inode;
 	struct fuse_inode *fi = get_fuse_inode(inode);
 	struct fuse_conn *fc = get_fuse_conn(inode);
-	struct page *tmp_page;
+	struct folio *tmp_folio;
 	int err;
 
 	if (wpa && fuse_writepage_need_send(fc, &folio->page, ap, data)) {
@@ -2269,8 +2269,8 @@ static int fuse_writepages_fill(struct folio *folio,
 	}
 
 	err = -ENOMEM;
-	tmp_page = alloc_page(GFP_NOFS | __GFP_HIGHMEM);
-	if (!tmp_page)
+	tmp_folio = folio_alloc(GFP_NOFS | __GFP_HIGHMEM, 0);
+	if (!tmp_folio)
 		goto out_unlock;
 
 	/*
@@ -2290,7 +2290,7 @@ static int fuse_writepages_fill(struct folio *folio,
 		err = -ENOMEM;
 		wpa = fuse_writepage_args_alloc();
 		if (!wpa) {
-			__free_page(tmp_page);
+			folio_put(tmp_folio);
 			goto out_unlock;
 		}
 		fuse_writepage_add_to_bucket(fc, wpa);
@@ -2308,14 +2308,14 @@ static int fuse_writepages_fill(struct folio *folio,
 	}
 	folio_start_writeback(folio);
 
-	copy_highpage(tmp_page, &folio->page);
-	ap->pages[ap->num_pages] = tmp_page;
+	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;
 
 	inc_wb_stat(&inode_to_bdi(inode)->wb, WB_WRITEBACK);
-	inc_node_page_state(tmp_page, NR_WRITEBACK_TEMP);
+	inc_node_page_state(&tmp_folio->page, NR_WRITEBACK_TEMP);
 
 	err = 0;
 	if (data->wpa) {
-- 
2.43.5

Re: [PATCH v3 6/9] fuse: convert fuse_writepages_fill() to use a folio for its tmp page

[Josef Bacik]

On Fri, Aug 23, 2024 at 09:27:27AM -0700, Joanne Koong wrote:
> To pave the way for refactoring out the shared logic in
> fuse_writepages_fill() and fuse_writepage_locked(), this change converts
> the temporary page in fuse_writepages_fill() to use the folio API.
> 
> This is similar to the change in e0887e095a80 ("fuse: Convert
> fuse_writepage_locked to take a folio"), which converted the tmp page in
> fuse_writepage_locked() to use the folio API.
> 
> inc_node_page_state() is intentionally preserved here instead of
> converting to node_stat_add_folio() since it is updating the stat of the
> underlying page and to better maintain API symmetry with
> dec_node_page_stat() in fuse_writepage_finish_stat().
> 
> No functional changes added.
> 
> Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> ---
>  fs/fuse/file.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> index a51b0b085616..905b202a7acd 100644
> --- a/fs/fuse/file.c
> +++ b/fs/fuse/file.c
> @@ -2260,7 +2260,7 @@ static int fuse_writepages_fill(struct folio *folio,
>  	struct inode *inode = data->inode;
>  	struct fuse_inode *fi = get_fuse_inode(inode);
>  	struct fuse_conn *fc = get_fuse_conn(inode);
> -	struct page *tmp_page;
> +	struct folio *tmp_folio;
>  	int err;
>  
>  	if (wpa && fuse_writepage_need_send(fc, &folio->page, ap, data)) {
> @@ -2269,8 +2269,8 @@ static int fuse_writepages_fill(struct folio *folio,
>  	}
>  
>  	err = -ENOMEM;
> -	tmp_page = alloc_page(GFP_NOFS | __GFP_HIGHMEM);
> -	if (!tmp_page)
> +	tmp_folio = folio_alloc(GFP_NOFS | __GFP_HIGHMEM, 0);
> +	if (!tmp_folio)
>  		goto out_unlock;
>  
>  	/*
> @@ -2290,7 +2290,7 @@ static int fuse_writepages_fill(struct folio *folio,
>  		err = -ENOMEM;
>  		wpa = fuse_writepage_args_alloc();
>  		if (!wpa) {
> -			__free_page(tmp_page);
> +			folio_put(tmp_folio);
>  			goto out_unlock;
>  		}
>  		fuse_writepage_add_to_bucket(fc, wpa);
> @@ -2308,14 +2308,14 @@ static int fuse_writepages_fill(struct folio *folio,
>  	}
>  	folio_start_writeback(folio);
>  
> -	copy_highpage(tmp_page, &folio->page);
> -	ap->pages[ap->num_pages] = tmp_page;
> +	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;
>  
>  	inc_wb_stat(&inode_to_bdi(inode)->wb, WB_WRITEBACK);
> -	inc_node_page_state(tmp_page, NR_WRITEBACK_TEMP);
> +	inc_node_page_state(&tmp_folio->page, NR_WRITEBACK_TEMP);

I *think* you can use

node_stat_add_folio(tmp_folio, NR_WRITEBACK_TEMP);

here instead of inc_node_page_state().  Thanks,

Josef

Re: [PATCH v3 6/9] fuse: convert fuse_writepages_fill() to use a folio for its tmp page

[Joanne Koong]

On Fri, Aug 23, 2024 at 12:03 PM Josef Bacik <josef@toxicpanda.com> wrote:
>
> On Fri, Aug 23, 2024 at 09:27:27AM -0700, Joanne Koong wrote:
> > To pave the way for refactoring out the shared logic in
> > fuse_writepages_fill() and fuse_writepage_locked(), this change converts
> > the temporary page in fuse_writepages_fill() to use the folio API.
> >
> > This is similar to the change in e0887e095a80 ("fuse: Convert
> > fuse_writepage_locked to take a folio"), which converted the tmp page in
> > fuse_writepage_locked() to use the folio API.
> >
> > inc_node_page_state() is intentionally preserved here instead of
> > converting to node_stat_add_folio() since it is updating the stat of the
> > underlying page and to better maintain API symmetry with
> > dec_node_page_stat() in fuse_writepage_finish_stat().
> >
> > No functional changes added.
> >
> > Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> > ---
> >  fs/fuse/file.c | 14 +++++++-------
> >  1 file changed, 7 insertions(+), 7 deletions(-)
> >
> > diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> > index a51b0b085616..905b202a7acd 100644
> > --- a/fs/fuse/file.c
> > +++ b/fs/fuse/file.c
> > @@ -2260,7 +2260,7 @@ static int fuse_writepages_fill(struct folio *folio,
> >       struct inode *inode = data->inode;
> >       struct fuse_inode *fi = get_fuse_inode(inode);
> >       struct fuse_conn *fc = get_fuse_conn(inode);
> > -     struct page *tmp_page;
> > +     struct folio *tmp_folio;
> >       int err;
> >
> >       if (wpa && fuse_writepage_need_send(fc, &folio->page, ap, data)) {
> > @@ -2269,8 +2269,8 @@ static int fuse_writepages_fill(struct folio *folio,
> >       }
> >
> >       err = -ENOMEM;
> > -     tmp_page = alloc_page(GFP_NOFS | __GFP_HIGHMEM);
> > -     if (!tmp_page)
> > +     tmp_folio = folio_alloc(GFP_NOFS | __GFP_HIGHMEM, 0);
> > +     if (!tmp_folio)
> >               goto out_unlock;
> >
> >       /*
> > @@ -2290,7 +2290,7 @@ static int fuse_writepages_fill(struct folio *folio,
> >               err = -ENOMEM;
> >               wpa = fuse_writepage_args_alloc();
> >               if (!wpa) {
> > -                     __free_page(tmp_page);
> > +                     folio_put(tmp_folio);
> >                       goto out_unlock;
> >               }
> >               fuse_writepage_add_to_bucket(fc, wpa);
> > @@ -2308,14 +2308,14 @@ static int fuse_writepages_fill(struct folio *folio,
> >       }
> >       folio_start_writeback(folio);
> >
> > -     copy_highpage(tmp_page, &folio->page);
> > -     ap->pages[ap->num_pages] = tmp_page;
> > +     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;
> >
> >       inc_wb_stat(&inode_to_bdi(inode)->wb, WB_WRITEBACK);
> > -     inc_node_page_state(tmp_page, NR_WRITEBACK_TEMP);
> > +     inc_node_page_state(&tmp_folio->page, NR_WRITEBACK_TEMP);
>
> I *think* you can use
>
> node_stat_add_folio(tmp_folio, NR_WRITEBACK_TEMP);
>
> here instead of inc_node_page_state().  Thanks,

I was thinking inc_node_page_state() here would be better for
preserving the API symmetry with the dec_node_page_state() function
that gets called when the writeback gets finished (in
fuse_writepage_finish_stat) - I don't think it's immediately obvious
that node_stat_add_folio() and dec_node_page_state() are inverses of
each other. I don't feel strongly about this though, so i'm happy to
change this to node_stat_add_folio as well.


Thanks,
Joanne

>
> Josef

Re: [PATCH v3 6/9] fuse: convert fuse_writepages_fill() to use a folio for its tmp page

[Josef Bacik]

On Fri, Aug 23, 2024 at 02:38:02PM -0700, Joanne Koong wrote:
> On Fri, Aug 23, 2024 at 12:03 PM Josef Bacik <josef@toxicpanda.com> wrote:
> >
> > On Fri, Aug 23, 2024 at 09:27:27AM -0700, Joanne Koong wrote:
> > > To pave the way for refactoring out the shared logic in
> > > fuse_writepages_fill() and fuse_writepage_locked(), this change converts
> > > the temporary page in fuse_writepages_fill() to use the folio API.
> > >
> > > This is similar to the change in e0887e095a80 ("fuse: Convert
> > > fuse_writepage_locked to take a folio"), which converted the tmp page in
> > > fuse_writepage_locked() to use the folio API.
> > >
> > > inc_node_page_state() is intentionally preserved here instead of
> > > converting to node_stat_add_folio() since it is updating the stat of the
> > > underlying page and to better maintain API symmetry with
> > > dec_node_page_stat() in fuse_writepage_finish_stat().
> > >
> > > No functional changes added.
> > >
> > > Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> > > ---
> > >  fs/fuse/file.c | 14 +++++++-------
> > >  1 file changed, 7 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> > > index a51b0b085616..905b202a7acd 100644
> > > --- a/fs/fuse/file.c
> > > +++ b/fs/fuse/file.c
> > > @@ -2260,7 +2260,7 @@ static int fuse_writepages_fill(struct folio *folio,
> > >       struct inode *inode = data->inode;
> > >       struct fuse_inode *fi = get_fuse_inode(inode);
> > >       struct fuse_conn *fc = get_fuse_conn(inode);
> > > -     struct page *tmp_page;
> > > +     struct folio *tmp_folio;
> > >       int err;
> > >
> > >       if (wpa && fuse_writepage_need_send(fc, &folio->page, ap, data)) {
> > > @@ -2269,8 +2269,8 @@ static int fuse_writepages_fill(struct folio *folio,
> > >       }
> > >
> > >       err = -ENOMEM;
> > > -     tmp_page = alloc_page(GFP_NOFS | __GFP_HIGHMEM);
> > > -     if (!tmp_page)
> > > +     tmp_folio = folio_alloc(GFP_NOFS | __GFP_HIGHMEM, 0);
> > > +     if (!tmp_folio)
> > >               goto out_unlock;
> > >
> > >       /*
> > > @@ -2290,7 +2290,7 @@ static int fuse_writepages_fill(struct folio *folio,
> > >               err = -ENOMEM;
> > >               wpa = fuse_writepage_args_alloc();
> > >               if (!wpa) {
> > > -                     __free_page(tmp_page);
> > > +                     folio_put(tmp_folio);
> > >                       goto out_unlock;
> > >               }
> > >               fuse_writepage_add_to_bucket(fc, wpa);
> > > @@ -2308,14 +2308,14 @@ static int fuse_writepages_fill(struct folio *folio,
> > >       }
> > >       folio_start_writeback(folio);
> > >
> > > -     copy_highpage(tmp_page, &folio->page);
> > > -     ap->pages[ap->num_pages] = tmp_page;
> > > +     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;
> > >
> > >       inc_wb_stat(&inode_to_bdi(inode)->wb, WB_WRITEBACK);
> > > -     inc_node_page_state(tmp_page, NR_WRITEBACK_TEMP);
> > > +     inc_node_page_state(&tmp_folio->page, NR_WRITEBACK_TEMP);
> >
> > I *think* you can use
> >
> > node_stat_add_folio(tmp_folio, NR_WRITEBACK_TEMP);
> >
> > here instead of inc_node_page_state().  Thanks,
> 
> I was thinking inc_node_page_state() here would be better for
> preserving the API symmetry with the dec_node_page_state() function
> that gets called when the writeback gets finished (in
> fuse_writepage_finish_stat) - I don't think it's immediately obvious
> that node_stat_add_folio() and dec_node_page_state() are inverses of
> each other. I don't feel strongly about this though, so i'm happy to
> change this to node_stat_add_folio as well.

Ah yeah that's a good point, probably better to convert those in one shot so
everything is consistent.  Thanks,

Josef

[PATCH v3 7/9] fuse: move folio_start_writeback to after the allocations in fuse_writepage_locked()

[Joanne Koong]

Start writeback on the folio after allocating the fuse_writepage_args
and temporary folio, instead of before the allocations.

This change is to pave the way for a following patch which will refactor
out the shared logic in fuse_writepages_fill() and fuse_writepage_locked().

No functional changes added.

Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
---
 fs/fuse/file.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index 905b202a7acd..3d84cbb1a2d9 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -2060,12 +2060,9 @@ static int fuse_writepage_locked(struct folio *folio)
 	struct folio *tmp_folio;
 	int error = -ENOMEM;
 
-	folio_start_writeback(folio);
-
 	wpa = fuse_writepage_args_alloc();
 	if (!wpa)
 		goto err;
-	ap = &wpa->ia.ap;
 
 	tmp_folio = folio_alloc(GFP_NOFS | __GFP_HIGHMEM, 0);
 	if (!tmp_folio)
@@ -2079,16 +2076,20 @@ static int fuse_writepage_locked(struct folio *folio)
 	fuse_writepage_add_to_bucket(fc, wpa);
 	fuse_write_args_fill(&wpa->ia, wpa->ia.ff, folio_pos(folio), 0);
 
-	folio_copy(tmp_folio, folio);
 	wpa->ia.write.in.write_flags |= FUSE_WRITE_CACHE;
 	wpa->next = NULL;
+	wpa->inode = inode;
+
+	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;
-	ap->args.end = fuse_writepage_end;
-	wpa->inode = inode;
 
 	inc_wb_stat(&inode_to_bdi(inode)->wb, WB_WRITEBACK);
 	node_stat_add_folio(tmp_folio, NR_WRITEBACK_TEMP);
@@ -2109,7 +2110,6 @@ static int fuse_writepage_locked(struct folio *folio)
 	kfree(wpa);
 err:
 	mapping_set_error(folio->mapping, error);
-	folio_end_writeback(folio);
 	return error;
 }
 
-- 
2.43.5

[PATCH v3 8/9] fuse: move fuse file initialization to wpa allocation time

[Joanne Koong]

Before this change, wpa->ia.ff is initialized with an acquired reference
on the fuse file right before it submits the writeback request. If there
are auxiliary writebacks, then the initialization and reference
acquisition needs to also be set before we submit the auxiliary writeback
request.

To make the logic simpler and to pave the way for a subsequent
refactoring of fuse_writepages_fill() and fuse_writepage_locked(), this
change initializes and acquires wpa->ia.ff when the wpa is allocated.

No functional changes added.

Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
---
 fs/fuse/file.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index 3d84cbb1a2d9..2348baf2521c 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -1762,8 +1762,7 @@ static void fuse_writepage_free(struct fuse_writepage_args *wpa)
 	for (i = 0; i < ap->num_pages; i++)
 		__free_page(ap->pages[i]);
 
-	if (wpa->ia.ff)
-		fuse_file_put(wpa->ia.ff, false);
+	fuse_file_put(wpa->ia.ff, false);
 
 	kfree(ap->pages);
 	kfree(wpa);
@@ -1936,7 +1935,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);
 
 		/*
@@ -2155,7 +2153,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);
@@ -2300,6 +2297,7 @@ static int fuse_writepages_fill(struct folio *folio,
 		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->ia.ff = fuse_file_get(data->ff);
 		wpa->next = NULL;
 		ap->args.in_pages = true;
 		ap->args.end = fuse_writepage_end;
-- 
2.43.5

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

[Joanne Koong]

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 | 99 ++++++++++++++++++++++++++++----------------------
 1 file changed, 55 insertions(+), 44 deletions(-)

diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index 2348baf2521c..88f872c02349 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -2047,50 +2047,77 @@ 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);
+	error = -ENOMEM;
+	if (!wpa)
+		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);
@@ -2102,10 +2129,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;
@@ -2285,36 +2312,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->ia.ff = fuse_file_get(data->ff);
-		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;
+	fuse_writepage_args_page_fill(wpa, folio, tmp_folio, ap->num_pages);
 	data->orig_pages[ap->num_pages] = &folio->page;
 
-	inc_wb_stat(&inode_to_bdi(inode)->wb, WB_WRITEBACK);
-	inc_node_page_state(&tmp_folio->page, NR_WRITEBACK_TEMP);
-
 	err = 0;
 	if (data->wpa) {
 		/*
-- 
2.43.5

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

[Josef Bacik]

On Fri, Aug 23, 2024 at 09:27:30AM -0700, Joanne Koong 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 | 99 ++++++++++++++++++++++++++++----------------------
>  1 file changed, 55 insertions(+), 44 deletions(-)
> 
> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> index 2348baf2521c..88f872c02349 100644
> --- a/fs/fuse/file.c
> +++ b/fs/fuse/file.c
> @@ -2047,50 +2047,77 @@ 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);

Same comment here as before.  Thanks,

Josef