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

[PATCH v2 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

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: refactor out shared logic in fuse_writepages_fill() and
    fuse_writepage_locked()
  fuse: tidy up error paths in fuse_writepages_fill() and
    fuse_writepage_locked()

 fs/fuse/file.c | 192 ++++++++++++++++++++++++++-----------------------
 1 file changed, 102 insertions(+), 90 deletions(-)

-- 
2.43.5

[PATCH v2 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 v2 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 v2 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 v2 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 v2 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 | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index 8a9b6e8dbd1b..147645d7e5d9 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,21 +2355,25 @@ 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);
-	if (!data.orig_pages)
+	if (!data.orig_pages) {
+		fuse_file_put(data.ff, false);
 		return -ENOMEM;
+	}
 
 	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);
+
+	fuse_file_put(data.ff, false);
 
 	kfree(data.orig_pages);
 	return err;
-- 
2.43.5

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

[Miklos Szeredi]

On Thu, 22 Aug 2024 at 01:25, Joanne Koong <joannelkoong@gmail.com> wrote:

> @@ -2361,21 +2355,25 @@ 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);
> -       if (!data.orig_pages)
> +       if (!data.orig_pages) {
> +               fuse_file_put(data.ff, false);

I'd prefer a cleanup label at the end of the function.

Thanks,
Miklos

[PATCH v2 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 147645d7e5d9..113b7429a818 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

[PATCH v2 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 113b7429a818..812b3d043b26 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 v2 8/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 | 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;
+	}
 
 	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) {
-- 
2.43.5

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

[Miklos Szeredi]

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;

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

[Joanne Koong]

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

[PATCH v2 9/9] fuse: tidy up error paths in fuse_writepages_fill() and fuse_writepage_locked()

[Joanne Koong]

Tidy up the error paths in fuse_writepages_fill() and
fuse_writepage_locked() to be easier to read / less cluttered.

No functional changes added.

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

diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index fe8ae19587fb..0a3a92ef645d 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -2098,16 +2098,19 @@ static int fuse_writepage_locked(struct folio *folio)
 	struct fuse_args_pages *ap;
 	struct folio *tmp_folio;
 	struct fuse_file *ff;
-	int error = -ENOMEM;
+	int error;
 
 	tmp_folio = folio_alloc(GFP_NOFS | __GFP_HIGHMEM, 0);
-	if (!tmp_folio)
+	if (!tmp_folio) {
+		error = -ENOMEM;
 		goto err;
+	}
 
-	error = -EIO;
 	ff = fuse_write_file_get(fi);
-	if (!ff)
+	if (!ff) {
+		error = -EIO;
 		goto err_nofile;
+	}
 
 	wpa = fuse_writepage_args_setup(folio, ff);
 	if (!wpa) {
@@ -2287,17 +2290,18 @@ static int fuse_writepages_fill(struct folio *folio,
 	struct fuse_inode *fi = get_fuse_inode(inode);
 	struct fuse_conn *fc = get_fuse_conn(inode);
 	struct folio *tmp_folio;
-	int err;
+	int err = 0;
 
 	if (wpa && fuse_writepage_need_send(fc, &folio->page, ap, data)) {
 		fuse_writepages_send(data);
 		data->wpa = NULL;
 	}
 
-	err = -ENOMEM;
 	tmp_folio = folio_alloc(GFP_NOFS | __GFP_HIGHMEM, 0);
-	if (!tmp_folio)
+	if (!tmp_folio) {
+		err = -ENOMEM;
 		goto out_unlock;
+	}
 
 	/*
 	 * The page must not be redirtied until the writeout is completed
@@ -2313,10 +2317,10 @@ static int fuse_writepages_fill(struct folio *folio,
 	 * under writeback, so we can release the page lock.
 	 */
 	if (data->wpa == NULL) {
-		err = -ENOMEM;
 		wpa = fuse_writepage_args_setup(folio, data->ff);
 		if (!wpa) {
 			folio_put(tmp_folio);
+			err = -ENOMEM;
 			goto out_unlock;
 		}
 		fuse_file_get(wpa->ia.ff);
@@ -2329,7 +2333,6 @@ static int fuse_writepages_fill(struct folio *folio,
 
 	data->orig_pages[ap->num_pages] = &folio->page;
 
-	err = 0;
 	if (data->wpa) {
 		/*
 		 * Protected by fi->lock against concurrent access by
-- 
2.43.5

Re: [PATCH v2 9/9] fuse: tidy up error paths in fuse_writepages_fill() and fuse_writepage_locked()

[Miklos Szeredi]

On Thu, 22 Aug 2024 at 01:25, Joanne Koong <joannelkoong@gmail.com> wrote:
>
> Tidy up the error paths in fuse_writepages_fill() and
> fuse_writepage_locked() to be easier to read / less cluttered.
>
> No functional changes added.
>
> Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> ---
>  fs/fuse/file.c | 21 ++++++++++++---------
>  1 file changed, 12 insertions(+), 9 deletions(-)
>
> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> index fe8ae19587fb..0a3a92ef645d 100644
> --- a/fs/fuse/file.c
> +++ b/fs/fuse/file.c
> @@ -2098,16 +2098,19 @@ static int fuse_writepage_locked(struct folio *folio)
>         struct fuse_args_pages *ap;
>         struct folio *tmp_folio;
>         struct fuse_file *ff;
> -       int error = -ENOMEM;
> +       int error;
>
>         tmp_folio = folio_alloc(GFP_NOFS | __GFP_HIGHMEM, 0);
> -       if (!tmp_folio)
> +       if (!tmp_folio) {
> +               error = -ENOMEM;
>                 goto err;
> +       }

Same comment as previous patch.

Thanks,
Miklos

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

[Miklos Szeredi]

On Thu, 22 Aug 2024 at 01:25, Joanne Koong <joannelkoong@gmail.com> wrote:
>
> 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

Please also run fsx  (originally fsx-linux) from the xfstest suite
which stresses the I/O subsystem very well.

Thanks,
Miklos