writeback cleanups

writeback cleanups

[Christoph Hellwig]

Hi all,

this series has a bunch of cosmetic cleanups for the NFS folio writeback
code.

Diffstat:
 write.c |   54 +++++++++++++++++++-----------------------------------
 1 file changed, 19 insertions(+), 35 deletions(-)

[PATCH 1/5] nfs: fold nfs_page_async_flush into nfs_do_writepage

[Christoph Hellwig]

Fold nfs_page_async_flush into its only caller to clean up the code a
bit.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/nfs/write.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/fs/nfs/write.c b/fs/nfs/write.c
index 23df8b214474..048837a9b1ba 100644
--- a/fs/nfs/write.c
+++ b/fs/nfs/write.c
@@ -632,10 +632,10 @@ static void nfs_write_error(struct nfs_page *req, int error)
  * Find an associated nfs write request, and prepare to flush it out
  * May return an error if the user signalled nfs_wait_on_request().
  */
-static int nfs_page_async_flush(struct folio *folio,
-				struct writeback_control *wbc,
-				struct nfs_pageio_descriptor *pgio)
+static int nfs_do_writepage(struct folio *folio, struct writeback_control *wbc,
+		struct nfs_pageio_descriptor *pgio)
 {
+	nfs_pageio_cond_complete(pgio, folio->index);
 	struct nfs_page *req;
 	int ret = 0;
 
@@ -677,13 +677,6 @@ static int nfs_page_async_flush(struct folio *folio,
 	return 0;
 }
 
-static int nfs_do_writepage(struct folio *folio, struct writeback_control *wbc,
-			    struct nfs_pageio_descriptor *pgio)
-{
-	nfs_pageio_cond_complete(pgio, folio->index);
-	return nfs_page_async_flush(folio, wbc, pgio);
-}
-
 /*
  * Write an mmapped page to the server.
  */
-- 
2.47.2

[PATCH 2/5] nfs: don't return AOP_WRITEPAGE_ACTIVATE from nfs_do_writepage

[Christoph Hellwig]

nfs_do_writepage is a successful return that requires the caller to
unlock the folio.  Using it here requires special casing both in
nfs_do_writepage and nfs_writepages_callback and leaves a land mine in
nfs_wb_folio in case it ever set the flag.  Remove it and just
unconditionally unlock in nfs_writepages_callback.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/nfs/write.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/fs/nfs/write.c b/fs/nfs/write.c
index 048837a9b1ba..3e873c5b1041 100644
--- a/fs/nfs/write.c
+++ b/fs/nfs/write.c
@@ -662,8 +662,6 @@ static int nfs_do_writepage(struct folio *folio, struct writeback_control *wbc,
 		 */
 		if (nfs_error_is_fatal_on_server(ret))
 			goto out_launder;
-		if (wbc->sync_mode == WB_SYNC_NONE)
-			ret = AOP_WRITEPAGE_ACTIVATE;
 		folio_redirty_for_writepage(wbc, folio);
 		nfs_redirty_request(req);
 		pgio->pg_error = 0;
@@ -702,8 +700,7 @@ static int nfs_writepages_callback(struct folio *folio,
 	int ret;
 
 	ret = nfs_do_writepage(folio, wbc, data);
-	if (ret != AOP_WRITEPAGE_ACTIVATE)
-		folio_unlock(folio);
+	folio_unlock(folio);
 	return ret;
 }
 
-- 
2.47.2

[PATCH 3/5] nfs: move the call to nfs_pageio_cond_complete in nfs_do_writepage

[Christoph Hellwig]

Calling a function before the main variable declaration is extremely
confusing, so don't do that.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/nfs/write.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/nfs/write.c b/fs/nfs/write.c
index 3e873c5b1041..4e1d57b63a85 100644
--- a/fs/nfs/write.c
+++ b/fs/nfs/write.c
@@ -635,10 +635,11 @@ static void nfs_write_error(struct nfs_page *req, int error)
 static int nfs_do_writepage(struct folio *folio, struct writeback_control *wbc,
 		struct nfs_pageio_descriptor *pgio)
 {
-	nfs_pageio_cond_complete(pgio, folio->index);
 	struct nfs_page *req;
 	int ret = 0;
 
+	nfs_pageio_cond_complete(pgio, folio->index);
+
 	req = nfs_lock_and_join_requests(folio);
 	if (!req)
 		goto out;
-- 
2.47.2

[PATCH 4/5] nfs: refactor nfs_do_writepage

[Christoph Hellwig]

Use early returns wherever possible to simplify the code.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/nfs/write.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/fs/nfs/write.c b/fs/nfs/write.c
index 4e1d57b63a85..68c5dc061abe 100644
--- a/fs/nfs/write.c
+++ b/fs/nfs/write.c
@@ -636,16 +636,15 @@ static int nfs_do_writepage(struct folio *folio, struct writeback_control *wbc,
 		struct nfs_pageio_descriptor *pgio)
 {
 	struct nfs_page *req;
-	int ret = 0;
+	int ret;
 
 	nfs_pageio_cond_complete(pgio, folio->index);
 
 	req = nfs_lock_and_join_requests(folio);
 	if (!req)
-		goto out;
-	ret = PTR_ERR(req);
+		return 0;
 	if (IS_ERR(req))
-		goto out;
+		return PTR_ERR(req);
 
 	nfs_folio_set_writeback(folio);
 	WARN_ON_ONCE(test_bit(PG_CLEAN, &req->wb_flags));
@@ -655,7 +654,6 @@ static int nfs_do_writepage(struct folio *folio, struct writeback_control *wbc,
 	if (nfs_error_is_fatal_on_server(ret))
 		goto out_launder;
 
-	ret = 0;
 	if (!nfs_pageio_add_request(pgio, req)) {
 		ret = pgio->pg_error;
 		/*
@@ -666,11 +664,12 @@ static int nfs_do_writepage(struct folio *folio, struct writeback_control *wbc,
 		folio_redirty_for_writepage(wbc, folio);
 		nfs_redirty_request(req);
 		pgio->pg_error = 0;
-	} else
-		nfs_add_stats(folio->mapping->host,
-			      NFSIOS_WRITEPAGES, 1);
-out:
-	return ret;
+		return ret;
+	}
+
+	nfs_add_stats(folio->mapping->host, NFSIOS_WRITEPAGES, 1);
+	return 0;
+
 out_launder:
 	nfs_write_error(req, ret);
 	return 0;
-- 
2.47.2

[PATCH 5/5] nfs: use writeback_iter directly

[Christoph Hellwig]

Stop using write_cache_pages and use writeback_iter directly.  This
removes an indirect call per written folio and makes the code easier
to follow.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/nfs/write.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/fs/nfs/write.c b/fs/nfs/write.c
index 68c5dc061abe..374fc6b34c79 100644
--- a/fs/nfs/write.c
+++ b/fs/nfs/write.c
@@ -694,16 +694,6 @@ static int nfs_writepage_locked(struct folio *folio,
 	return err;
 }
 
-static int nfs_writepages_callback(struct folio *folio,
-				   struct writeback_control *wbc, void *data)
-{
-	int ret;
-
-	ret = nfs_do_writepage(folio, wbc, data);
-	folio_unlock(folio);
-	return ret;
-}
-
 static void nfs_io_completion_commit(void *inode)
 {
 	nfs_commit_inode(inode, 0);
@@ -739,11 +729,15 @@ int nfs_writepages(struct address_space *mapping, struct writeback_control *wbc)
 	}
 
 	do {
+		struct folio *folio = NULL;
+
 		nfs_pageio_init_write(&pgio, inode, priority, false,
 				      &nfs_async_write_completion_ops);
 		pgio.pg_io_completion = ioc;
-		err = write_cache_pages(mapping, wbc, nfs_writepages_callback,
-					&pgio);
+		while ((folio = writeback_iter(mapping, wbc, folio, &err))) {
+			err = nfs_do_writepage(folio, wbc, &pgio);
+			folio_unlock(folio);
+		}
 		pgio.pg_error = 0;
 		nfs_pageio_complete(&pgio);
 		if (err == -EAGAIN && mntflags & NFS_MOUNT_SOFTERR)
-- 
2.47.2

Re: [PATCH 3/5] nfs: move the call to nfs_pageio_cond_complete in nfs_do_writepage

[Trond Myklebust]

On Tue, 2025-05-06 at 16:07 +0200, Christoph Hellwig wrote:
> Calling a function before the main variable declaration is extremely
> confusing, so don't do that.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/nfs/write.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/nfs/write.c b/fs/nfs/write.c
> index 3e873c5b1041..4e1d57b63a85 100644
> --- a/fs/nfs/write.c
> +++ b/fs/nfs/write.c
> @@ -635,10 +635,11 @@ static void nfs_write_error(struct nfs_page
> *req, int error)
>  static int nfs_do_writepage(struct folio *folio, struct
> writeback_control *wbc,
>  		struct nfs_pageio_descriptor *pgio)
>  {
> -	nfs_pageio_cond_complete(pgio, folio->index);
>  	struct nfs_page *req;
>  	int ret = 0;
>  
> +	nfs_pageio_cond_complete(pgio, folio->index);
> +
>  	req = nfs_lock_and_join_requests(folio);
>  	if (!req)
>  		goto out;


Maybe collapse this patch and 1/5? 🙂

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com

Re: writeback cleanups

[Trond Myklebust]

On Tue, 2025-05-06 at 16:07 +0200, Christoph Hellwig wrote:
> Hi all,
> 
> this series has a bunch of cosmetic cleanups for the NFS folio
> writeback
> code.
> 
> Diffstat:
>  write.c |   54 +++++++++++++++++++----------------------------------
> -
>  1 file changed, 19 insertions(+), 35 deletions(-)

Looks good, but as I said, Patch 1 & 3 want to be squashed together.

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com

Re: [PATCH 3/5] nfs: move the call to nfs_pageio_cond_complete in nfs_do_writepage

[hch]

On Tue, May 06, 2025 at 03:46:13PM +0000, Trond Myklebust wrote:
> Maybe collapse this patch and 1/5? 🙂

Oops, I didn't realize it was me who messed up the function call
placement.  Yes, this should be fixed at the source.