diff mbox series

[02/11] writeback: Factor writeback_get_batch() out of write_cache_pages()

Message ID 20231214132544.376574-3-hch@lst.de (mailing list archive)
State New
Headers show
Series [01/11] writeback: Factor out writeback_finish() | expand

Commit Message

Christoph Hellwig Dec. 14, 2023, 1:25 p.m. UTC 
From: "Matthew Wilcox (Oracle)" <willy@infradead.org>

This simple helper will be the basis of the writeback iterator.
To make this work, we need to remember the current index
and end positions in writeback_control.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 include/linux/writeback.h |  2 ++
 mm/page-writeback.c       | 49 +++++++++++++++++++++------------------
 2 files changed, 29 insertions(+), 22 deletions(-)

Comments

Jan Kara Dec. 15, 2023, 2:14 p.m. UTC | #1 
On Thu 14-12-23 14:25:35, Christoph Hellwig wrote:
> From: "Matthew Wilcox (Oracle)" <willy@infradead.org>
> 
> This simple helper will be the basis of the writeback iterator.
> To make this work, we need to remember the current index
> and end positions in writeback_control.
> 
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Just some nits:

> --- a/include/linux/writeback.h
> +++ b/include/linux/writeback.h
> @@ -81,6 +81,8 @@ struct writeback_control {
>  
>  	/* internal fields used by the ->writepages implementation: */
>  	struct folio_batch fbatch;
> +	pgoff_t index;
> +	pgoff_t end;			/* inclusive */
>  	pgoff_t done_index;

I don't think we need to cache 'end' since it isn't used that much. In
writeback_get_batch() we can just compute it locally as:

	if (wbc->range_cyclic)
		end = -1;
	else
		end = wbc->range_end >> PAGE_SHIFT;

and in the termination condition of the loop we can have it like:

	while (wbc->range_cyclic || index <= wbc->range_end >> PAGE_SHIFT)

Also I don't think we need both done_index and index since they are closely
related and when spread over several functions it gets a bit confusing
what's for what. So I'd just remove done_index, use index instead for
setting writeback_index and just reset 'index' to the desired value in
those two cases where we break out of the loop early and thus index !=
done_index.

I'm sorry for nitpicking about these state variables but IMO reducing their
amount actually makes things easier to verify (and thus maintain) when they
are spread over several functions.

								Honza
diff mbox series

Patch

diff --git a/include/linux/writeback.h b/include/linux/writeback.h
index 05e8add4b5ae3c..be960f92ad9dbd 100644
--- a/include/linux/writeback.h
+++ b/include/linux/writeback.h
@@ -81,6 +81,8 @@  struct writeback_control {
 
 	/* internal fields used by the ->writepages implementation: */
 	struct folio_batch fbatch;
+	pgoff_t index;
+	pgoff_t end;			/* inclusive */
 	pgoff_t done_index;
 	int err;
 	unsigned range_whole:1;		/* entire file */
diff --git a/mm/page-writeback.c b/mm/page-writeback.c
index 45309f3b8193f8..5d33e7b468e2cc 100644
--- a/mm/page-writeback.c
+++ b/mm/page-writeback.c
@@ -2378,6 +2378,22 @@  static int writeback_finish(struct address_space *mapping,
 	return wbc->err;
 }
 
+static void writeback_get_batch(struct address_space *mapping,
+		struct writeback_control *wbc)
+{
+	xa_mark_t tag;
+
+	if (wbc->sync_mode == WB_SYNC_ALL || wbc->tagged_writepages)
+		tag = PAGECACHE_TAG_TOWRITE;
+	else
+		tag = PAGECACHE_TAG_DIRTY;
+
+	folio_batch_release(&wbc->fbatch);
+	cond_resched();
+	filemap_get_folios_tag(mapping, &wbc->index, wbc->end, tag,
+			&wbc->fbatch);
+}
+
 /**
  * write_cache_pages - walk the list of dirty pages of the given address space and write all of them.
  * @mapping: address space structure to write
@@ -2414,41 +2430,32 @@  int write_cache_pages(struct address_space *mapping,
 		      void *data)
 {
 	int error;
-	int nr_folios;
-	pgoff_t index;
-	pgoff_t end;		/* Inclusive */
-	xa_mark_t tag;
 
 	if (wbc->range_cyclic) {
-		index = mapping->writeback_index; /* prev offset */
-		end = -1;
+		wbc->index = mapping->writeback_index; /* prev offset */
+		wbc->end = -1;
 	} else {
-		index = wbc->range_start >> PAGE_SHIFT;
-		end = wbc->range_end >> PAGE_SHIFT;
+		wbc->index = wbc->range_start >> PAGE_SHIFT;
+		wbc->end = wbc->range_end >> PAGE_SHIFT;
 		if (wbc->range_start == 0 && wbc->range_end == LLONG_MAX)
 			wbc->range_whole = 1;
 	}
-	if (wbc->sync_mode == WB_SYNC_ALL || wbc->tagged_writepages) {
-		tag_pages_for_writeback(mapping, index, end);
-		tag = PAGECACHE_TAG_TOWRITE;
-	} else {
-		tag = PAGECACHE_TAG_DIRTY;
-	}
+	if (wbc->sync_mode == WB_SYNC_ALL || wbc->tagged_writepages)
+		tag_pages_for_writeback(mapping, wbc->index, wbc->end);
 
-	wbc->done_index = index;
+	wbc->done_index = wbc->index;
 	folio_batch_init(&wbc->fbatch);
 	wbc->err = 0;
 
-	while (index <= end) {
+	while (wbc->index <= wbc->end) {
 		int i;
 
-		nr_folios = filemap_get_folios_tag(mapping, &index, end,
-				tag, &wbc->fbatch);
+		writeback_get_batch(mapping, wbc);
 
-		if (nr_folios == 0)
+		if (wbc->fbatch.nr == 0)
 			break;
 
-		for (i = 0; i < nr_folios; i++) {
+		for (i = 0; i < wbc->fbatch.nr; i++) {
 			struct folio *folio = wbc->fbatch.folios[i];
 			unsigned long nr;
 
@@ -2526,8 +2533,6 @@  int write_cache_pages(struct address_space *mapping,
 			    wbc->sync_mode == WB_SYNC_NONE)
 				return writeback_finish(mapping, wbc, true);
 		}
-		folio_batch_release(&wbc->fbatch);
-		cond_resched();
 	}
 
 	return writeback_finish(mapping, wbc, false);