Received-SPF: pass (google.com: domain of linux-kernel+bounces-1095-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Fri, 15 Dec 2023 09:17:05 -0500
From: Brian Foster <bfoster@redhat.com>
To: Christoph Hellwig <hch@lst.de>
Cc: linux-mm@kvack.org, "Matthew Wilcox (Oracle)" <willy@infradead.org>,
	linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
	Jan Kara <jack@suse.com>, David Howells <dhowells@redhat.com>
Subject: Re: [PATCH 09/11] writeback: Factor writeback_iter_next() out of
 write_cache_pages()
Message-ID: <ZXxf4TB5YU8huiz1@bfoster>
References: <20231214132544.376574-1-hch@lst.de>
 <20231214132544.376574-10-hch@lst.de>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20231214132544.376574-10-hch@lst.de>

On Thu, Dec 14, 2023 at 02:25:42PM +0100, Christoph Hellwig wrote:
> From: "Matthew Wilcox (Oracle)" <willy@infradead.org>
> 
> Pull the post-processing of the writepage_t callback into a
> separate function.  That means changing writeback_finish() to
> return NULL, and writeback_get_next() to call writeback_finish()
> when we naturally run out of folios.
> 
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  mm/page-writeback.c | 89 +++++++++++++++++++++++----------------------
>  1 file changed, 46 insertions(+), 43 deletions(-)
> 
> diff --git a/mm/page-writeback.c b/mm/page-writeback.c
> index b0accca1f4bfa7..4fae912f7a86e2 100644
> --- a/mm/page-writeback.c
> +++ b/mm/page-writeback.c
> @@ -2360,7 +2360,7 @@ void tag_pages_for_writeback(struct address_space *mapping,
>  }
>  EXPORT_SYMBOL(tag_pages_for_writeback);
>  
> -static int writeback_finish(struct address_space *mapping,
> +static struct folio *writeback_finish(struct address_space *mapping,
>  		struct writeback_control *wbc, bool done)
>  {
>  	folio_batch_release(&wbc->fbatch);
> @@ -2375,7 +2375,7 @@ static int writeback_finish(struct address_space *mapping,
>  	if (wbc->range_cyclic || (wbc->range_whole && wbc->nr_to_write > 0))
>  		mapping->writeback_index = wbc->done_index;
>  
> -	return wbc->err;
> +	return NULL;

The series looks reasonable to me on a first pass, but this stood out to
me as really odd. I was initially wondering if it made sense to use an
ERR_PTR() here or something, but on further staring it kind of seems
like this is better off being factored out of the internal iteration
paths. Untested and Probably Broken patch (based on this one) below as a
quick reference, but just a thought...

BTW it would be nicer to just drop the ->done field entirely as Jan has
already suggested, but I just stuffed it in wbc for simplicity.

Brian

--- 8< ---

diff --git a/include/linux/writeback.h b/include/linux/writeback.h
index be960f92ad9d..0babca17a2c0 100644
--- a/include/linux/writeback.h
+++ b/include/linux/writeback.h
@@ -84,6 +84,7 @@ struct writeback_control {
 	pgoff_t index;
 	pgoff_t end;			/* inclusive */
 	pgoff_t done_index;
+	bool done;
 	int err;
 	unsigned range_whole:1;		/* entire file */
 
diff --git a/mm/page-writeback.c b/mm/page-writeback.c
index 4fae912f7a86..3ee2058a2559 100644
--- a/mm/page-writeback.c
+++ b/mm/page-writeback.c
@@ -2360,8 +2360,8 @@ void tag_pages_for_writeback(struct address_space *mapping,
 }
 EXPORT_SYMBOL(tag_pages_for_writeback);
 
-static struct folio *writeback_finish(struct address_space *mapping,
-		struct writeback_control *wbc, bool done)
+static int writeback_iter_finish(struct address_space *mapping,
+		struct writeback_control *wbc)
 {
 	folio_batch_release(&wbc->fbatch);
 
@@ -2370,12 +2370,12 @@ static struct folio *writeback_finish(struct address_space *mapping,
 	 * wrap the index back to the start of the file for the next
 	 * time we are called.
 	 */
-	if (wbc->range_cyclic && !done)
+	if (wbc->range_cyclic && !wbc->done)
 		wbc->done_index = 0;
 	if (wbc->range_cyclic || (wbc->range_whole && wbc->nr_to_write > 0))
 		mapping->writeback_index = wbc->done_index;
 
-	return NULL;
+	return wbc->err;
 }
 
 static struct folio *writeback_get_next(struct address_space *mapping,
@@ -2434,19 +2434,16 @@ static struct folio *writeback_get_folio(struct address_space *mapping,
 {
 	struct folio *folio;
 
-	for (;;) {
-		folio = writeback_get_next(mapping, wbc);
-		if (!folio)
-			return writeback_finish(mapping, wbc, false);
+	while ((folio = writeback_get_next(mapping, wbc)) != NULL) {
 		wbc->done_index = folio->index;
-
 		folio_lock(folio);
 		if (likely(should_writeback_folio(mapping, wbc, folio)))
 			break;
 		folio_unlock(folio);
 	}
 
-	trace_wbc_writepage(wbc, inode_to_bdi(mapping->host));
+	if (folio)
+		trace_wbc_writepage(wbc, inode_to_bdi(mapping->host));
 	return folio;
 }
 
@@ -2466,6 +2463,7 @@ static struct folio *writeback_iter_init(struct address_space *mapping,
 		tag_pages_for_writeback(mapping, wbc->index, wbc->end);
 
 	wbc->done_index = wbc->index;
+	wbc->done = false;
 	folio_batch_init(&wbc->fbatch);
 	wbc->err = 0;
 
@@ -2494,7 +2492,8 @@ static struct folio *writeback_iter_next(struct address_space *mapping,
 		} else if (wbc->sync_mode != WB_SYNC_ALL) {
 			wbc->err = error;
 			wbc->done_index = folio->index + nr;
-			return writeback_finish(mapping, wbc, true);
+			wbc->done = true;
+			return NULL;
 		}
 		if (!wbc->err)
 			wbc->err = error;
@@ -2507,8 +2506,10 @@ static struct folio *writeback_iter_next(struct address_space *mapping,
 	 * to entering this loop.
 	 */
 	wbc->nr_to_write -= nr;
-	if (wbc->nr_to_write <= 0 && wbc->sync_mode == WB_SYNC_NONE)
-		return writeback_finish(mapping, wbc, true);
+	if (wbc->nr_to_write <= 0 && wbc->sync_mode == WB_SYNC_NONE) {
+		wbc->done = true;
+		return NULL;
+	}
 
 	return writeback_get_folio(mapping, wbc);
 }
@@ -2557,7 +2558,7 @@ int write_cache_pages(struct address_space *mapping,
 		error = writepage(folio, wbc, data);
 	}
 
-	return wbc->err;
+	return writeback_iter_finish(mapping, wbc);
 }
 EXPORT_SYMBOL(write_cache_pages);