Dave Howells doesn't like the indirect function call imposed by
write_cache_pages(), so refactor it into an iterator. I took the
opportunity to add the ability to iterate a folio_batch without having
an external variable.
This is against next-20230623. If you try to apply it on top of a tree
which doesn't include the pagevec removal series, IT WILL CRASH because
it won't reinitialise folio_batch->i and the iteration will index out
of bounds.
I have a feeling the 'done' parameter could have a better name, but I
can't think what it might be.
Matthew Wilcox (Oracle) (12):
writeback: Factor out writeback_finish()
writeback: Factor writeback_get_batch() out of write_cache_pages()
writeback: Factor should_writeback_folio() out of write_cache_pages()
writeback: Simplify the loops in write_cache_pages()
pagevec: Add ability to iterate a queue
writeback: Use the folio_batch queue iterator
writeback: Factor writeback_iter_init() out of write_cache_pages()
writeback: Factor writeback_get_folio() out of write_cache_pages()
writeback: Factor writeback_iter_next() out of write_cache_pages()
writeback: Add for_each_writeback_folio()
iomap: Convert iomap_writepages() to use for_each_writeback_folio()
writeback: Remove a use of write_cache_pages() from do_writepages()
fs/iomap/buffered-io.c | 14 +-
include/linux/pagevec.h | 18 +++
include/linux/writeback.h | 22 ++-
mm/page-writeback.c | 310 +++++++++++++++++++++-----------------
4 files changed, 216 insertions(+), 148 deletions(-)
--
2.39.2
Reduce write_cache_pages() by about 30 lines; much of it is commentary,
but it all bundles nicely into an obvious function.
Signed-off-by: Matthew Wilcox (Oracle) <[email protected]>
---
mm/page-writeback.c | 60 +++++++++++++++++++++++++--------------------
1 file changed, 33 insertions(+), 27 deletions(-)
diff --git a/mm/page-writeback.c b/mm/page-writeback.c
index 67c7f1564727..54f2972dab45 100644
--- a/mm/page-writeback.c
+++ b/mm/page-writeback.c
@@ -2394,6 +2394,37 @@ static void writeback_get_batch(struct address_space *mapping,
&wbc->fbatch);
}
+static bool should_writeback_folio(struct address_space *mapping,
+ struct writeback_control *wbc, struct folio *folio)
+{
+ /*
+ * Folio truncated or invalidated. We can freely skip it then,
+ * even for data integrity operations: the folio has disappeared
+ * concurrently, so there could be no real expectation of this
+ * data integrity operation even if there is now a new, dirty
+ * folio at the same pagecache index.
+ */
+ if (unlikely(folio->mapping != mapping))
+ return false;
+
+ /* Did somebody write it for us? */
+ if (!folio_test_dirty(folio))
+ return false;
+
+ if (folio_test_writeback(folio)) {
+ if (wbc->sync_mode != WB_SYNC_NONE)
+ folio_wait_writeback(folio);
+ else
+ return false;
+ }
+
+ BUG_ON(folio_test_writeback(folio));
+ if (!folio_clear_dirty_for_io(folio))
+ return false;
+
+ return true;
+}
+
/**
* 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
@@ -2461,38 +2492,13 @@ int write_cache_pages(struct address_space *mapping,
wbc->done_index = folio->index;
folio_lock(folio);
-
- /*
- * Page truncated or invalidated. We can freely skip it
- * then, even for data integrity operations: the page
- * has disappeared concurrently, so there could be no
- * real expectation of this data integrity operation
- * even if there is now a new, dirty page at the same
- * pagecache address.
- */
- if (unlikely(folio->mapping != mapping)) {
-continue_unlock:
+ if (!should_writeback_folio(mapping, wbc, folio)) {
folio_unlock(folio);
continue;
}
- if (!folio_test_dirty(folio)) {
- /* someone wrote it for us */
- goto continue_unlock;
- }
-
- if (folio_test_writeback(folio)) {
- if (wbc->sync_mode != WB_SYNC_NONE)
- folio_wait_writeback(folio);
- else
- goto continue_unlock;
- }
-
- BUG_ON(folio_test_writeback(folio));
- if (!folio_clear_dirty_for_io(folio))
- goto continue_unlock;
-
trace_wbc_writepage(wbc, inode_to_bdi(mapping->host));
+
error = writepage(folio, wbc, data);
if (unlikely(error)) {
/*
--
2.39.2
Make it return the first folio in the batch so that we can use it
in a typical for() pattern.
Signed-off-by: Matthew Wilcox (Oracle) <[email protected]>
---
mm/page-writeback.c | 48 ++++++++++++++++++++++++---------------------
1 file changed, 26 insertions(+), 22 deletions(-)
diff --git a/mm/page-writeback.c b/mm/page-writeback.c
index f782b48c5b0c..18f332611a52 100644
--- a/mm/page-writeback.c
+++ b/mm/page-writeback.c
@@ -2430,6 +2430,28 @@ static bool should_writeback_folio(struct address_space *mapping,
return true;
}
+static struct folio *writeback_iter_init(struct address_space *mapping,
+ struct writeback_control *wbc)
+{
+ if (wbc->range_cyclic) {
+ wbc->index = mapping->writeback_index; /* prev offset */
+ wbc->end = -1;
+ } else {
+ 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, wbc->index, wbc->end);
+
+ wbc->done_index = wbc->index;
+ folio_batch_init(&wbc->fbatch);
+ wbc->err = 0;
+
+ return writeback_get_next(mapping, wbc);
+}
+
/**
* 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
@@ -2465,30 +2487,12 @@ int write_cache_pages(struct address_space *mapping,
struct writeback_control *wbc, writepage_t writepage,
void *data)
{
+ struct folio *folio;
int error;
- if (wbc->range_cyclic) {
- wbc->index = mapping->writeback_index; /* prev offset */
- wbc->end = -1;
- } else {
- 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, wbc->index, wbc->end);
-
- wbc->done_index = wbc->index;
- folio_batch_init(&wbc->fbatch);
- wbc->err = 0;
-
- for (;;) {
- struct folio *folio = writeback_get_next(mapping, wbc);
-
- if (!folio)
- break;
-
+ for (folio = writeback_iter_init(mapping, wbc);
+ folio;
+ folio = writeback_get_next(mapping, wbc)) {
wbc->done_index = folio->index;
folio_lock(folio);
--
2.39.2
Collapse the two nested loops into one. This is needed as a step
towards turning this into an iterator.
---
mm/page-writeback.c | 94 ++++++++++++++++++++++-----------------------
1 file changed, 47 insertions(+), 47 deletions(-)
diff --git a/mm/page-writeback.c b/mm/page-writeback.c
index 54f2972dab45..68f28eeb15ed 100644
--- a/mm/page-writeback.c
+++ b/mm/page-writeback.c
@@ -2461,6 +2461,7 @@ int write_cache_pages(struct address_space *mapping,
void *data)
{
int error;
+ int i = 0;
if (wbc->range_cyclic) {
wbc->index = mapping->writeback_index; /* prev offset */
@@ -2478,65 +2479,64 @@ int write_cache_pages(struct address_space *mapping,
folio_batch_init(&wbc->fbatch);
wbc->err = 0;
- while (wbc->index <= wbc->end) {
- int i;
-
- writeback_get_batch(mapping, wbc);
+ for (;;) {
+ struct folio *folio;
+ if (i == wbc->fbatch.nr) {
+ writeback_get_batch(mapping, wbc);
+ i = 0;
+ }
if (wbc->fbatch.nr == 0)
break;
- for (i = 0; i < wbc->fbatch.nr; i++) {
- struct folio *folio = wbc->fbatch.folios[i];
+ folio = wbc->fbatch.folios[i++];
- wbc->done_index = folio->index;
+ wbc->done_index = folio->index;
- folio_lock(folio);
- if (!should_writeback_folio(mapping, wbc, folio)) {
- folio_unlock(folio);
- continue;
- }
+ folio_lock(folio);
+ if (!should_writeback_folio(mapping, wbc, folio)) {
+ folio_unlock(folio);
+ continue;
+ }
- trace_wbc_writepage(wbc, inode_to_bdi(mapping->host));
-
- error = writepage(folio, wbc, data);
- if (unlikely(error)) {
- /*
- * Handle errors according to the type of
- * writeback. There's no need to continue for
- * background writeback. Just push done_index
- * past this page so media errors won't choke
- * writeout for the entire file. For integrity
- * writeback, we must process the entire dirty
- * set regardless of errors because the fs may
- * still have state to clear for each page. In
- * that case we continue processing and return
- * the first error.
- */
- if (error == AOP_WRITEPAGE_ACTIVATE) {
- folio_unlock(folio);
- error = 0;
- } else if (wbc->sync_mode != WB_SYNC_ALL) {
- wbc->err = error;
- wbc->done_index = folio->index +
- folio_nr_pages(folio);
- return writeback_finish(mapping,
- wbc, true);
- }
- if (!wbc->err)
- wbc->err = error;
- }
+ trace_wbc_writepage(wbc, inode_to_bdi(mapping->host));
+ error = writepage(folio, wbc, data);
+ if (unlikely(error)) {
/*
- * We stop writing back only if we are not doing
- * integrity sync. In case of integrity sync we have to
- * keep going until we have written all the pages
- * we tagged for writeback prior to entering this loop.
+ * Handle errors according to the type of
+ * writeback. There's no need to continue for
+ * background writeback. Just push done_index
+ * past this page so media errors won't choke
+ * writeout for the entire file. For integrity
+ * writeback, we must process the entire dirty
+ * set regardless of errors because the fs may
+ * still have state to clear for each page. In
+ * that case we continue processing and return
+ * the first error.
*/
- if (--wbc->nr_to_write <= 0 &&
- wbc->sync_mode == WB_SYNC_NONE)
+ if (error == AOP_WRITEPAGE_ACTIVATE) {
+ folio_unlock(folio);
+ error = 0;
+ } else if (wbc->sync_mode != WB_SYNC_ALL) {
+ wbc->err = error;
+ wbc->done_index = folio->index +
+ folio_nr_pages(folio);
return writeback_finish(mapping, wbc, true);
+ }
+ if (!wbc->err)
+ wbc->err = error;
}
+
+ /*
+ * We stop writing back only if we are not doing
+ * integrity sync. In case of integrity sync we have to
+ * keep going until we have written all the pages
+ * we tagged for writeback prior to entering this loop.
+ */
+ if (--wbc->nr_to_write <= 0 &&
+ wbc->sync_mode == WB_SYNC_NONE)
+ return writeback_finish(mapping, wbc, true);
}
return writeback_finish(mapping, wbc, false);
--
2.39.2
On Mon, Jun 26, 2023 at 06:35:09PM +0100, Matthew Wilcox (Oracle) wrote:
> Dave Howells doesn't like the indirect function call imposed by
> write_cache_pages(), so refactor it into an iterator. I took the
> opportunity to add the ability to iterate a folio_batch without having
> an external variable.
>
> This is against next-20230623. If you try to apply it on top of a tree
> which doesn't include the pagevec removal series, IT WILL CRASH because
> it won't reinitialise folio_batch->i and the iteration will index out
> of bounds.
>
> I have a feeling the 'done' parameter could have a better name, but I
> can't think what it might be.
Heh, I actually have a local series with a minor subset of some of the
cleanups, but without the iterator conversion.
> + if (folio_test_writeback(folio)) {
> + if (wbc->sync_mode != WB_SYNC_NONE)
> + folio_wait_writeback(folio);
> + else
> + return false;
> + }
Please reorder this to avoid the else and return earlier while you're
at it:
if (folio_test_writeback(folio)) {
if (wbc->sync_mode == WB_SYNC_NONE)
return false;
folio_wait_writeback(folio);
}
(that's what actually got me started on my little cleanup spree while
checking some details of the writeback waiting..)
> + BUG_ON(folio_test_writeback(folio));
> + if (!folio_clear_dirty_for_io(folio))
> + return false;
> +
> + return true;
..
return folio_clear_dirty_for_io(folio);
?
On Mon, Jun 26, 2023 at 06:35:13PM +0100, Matthew Wilcox (Oracle) wrote:
> Collapse the two nested loops into one. This is needed as a step
> towards turning this into an iterator.
> ---
> mm/page-writeback.c | 94 ++++++++++++++++++++++-----------------------
> 1 file changed, 47 insertions(+), 47 deletions(-)
>
> diff --git a/mm/page-writeback.c b/mm/page-writeback.c
> index 54f2972dab45..68f28eeb15ed 100644
> --- a/mm/page-writeback.c
> +++ b/mm/page-writeback.c
> @@ -2461,6 +2461,7 @@ int write_cache_pages(struct address_space *mapping,
> void *data)
> {
> int error;
> + int i = 0;
>
> if (wbc->range_cyclic) {
> wbc->index = mapping->writeback_index; /* prev offset */
> @@ -2478,65 +2479,64 @@ int write_cache_pages(struct address_space *mapping,
> folio_batch_init(&wbc->fbatch);
> wbc->err = 0;
>
> + for (;;) {
> + struct folio *folio;
>
> + if (i == wbc->fbatch.nr) {
> + writeback_get_batch(mapping, wbc);
> + i = 0;
> + }
> if (wbc->fbatch.nr == 0)
> break;
> + folio = wbc->fbatch.folios[i++];
Did you consider moving what is currently the "i" local variable
into strut writeback_control as well? Then writeback_get_batch
could return the current folio, and we could hae a much nicer loop
here by moving all of the above into writeback_get_batch:
while ((folio = writeback_get_batch(mapping, wbc))) {
(and yes, writeback_get_batch probably needs a better name with that)
On Mon, Jun 26, 2023 at 09:30:07PM -0700, Christoph Hellwig wrote:
> On Mon, Jun 26, 2023 at 06:35:16PM +0100, Matthew Wilcox (Oracle) wrote:
> > + for (folio = writeback_iter_init(mapping, wbc);
> > + folio;
> > + folio = writeback_get_next(mapping, wbc)) {
>
> Ok that's another way to structure it. Guess I should look over the
> whole series first..
That beeing said. Given that writeback_iter_init calls
writeback_get_next anyway,
writeback_iter_init(mapping, wbc);
while ((folio = writeback_get_next(mapping, wbc)))
still feels a little easier to follow to be. No hard feelings either
way, just an observation.
On Mon, Jun 26, 2023 at 06:35:16PM +0100, Matthew Wilcox (Oracle) wrote:
> + for (folio = writeback_iter_init(mapping, wbc);
> + folio;
> + folio = writeback_get_next(mapping, wbc)) {
Ok that's another way to structure it. Guess I should look over the
whole series first..
Do you have this on a branch somewhere?
David
On Mon, Jun 26, 2023 at 09:31:56PM -0700, Christoph Hellwig wrote:
> On Mon, Jun 26, 2023 at 09:30:07PM -0700, Christoph Hellwig wrote:
> > On Mon, Jun 26, 2023 at 06:35:16PM +0100, Matthew Wilcox (Oracle) wrote:
> > > + for (folio = writeback_iter_init(mapping, wbc);
> > > + folio;
> > > + folio = writeback_get_next(mapping, wbc)) {
> >
> > Ok that's another way to structure it. Guess I should look over the
> > whole series first..
Perhaps ... it's a little hard to decide which of your comments
are worth replying to, and which are obviated by later realisations.
> That beeing said. Given that writeback_iter_init calls
> writeback_get_next anyway,
>
> writeback_iter_init(mapping, wbc);
> while ((folio = writeback_get_next(mapping, wbc)))
>
> still feels a little easier to follow to be. No hard feelings either
> way, just an observation.
I had it structured that way originally, but we need to pass in 'error'
to the get_next, and it's better if we also pass in 'folio', which means
that the user then needs to initialise error to 0 and folio to NULL
before using the macro, and that all felt a bit "You're holding it wrong".
On Mon, Jun 26, 2023 at 09:12:07PM -0700, Christoph Hellwig wrote:
> > + if (folio_test_writeback(folio)) {
> > + if (wbc->sync_mode != WB_SYNC_NONE)
> > + folio_wait_writeback(folio);
> > + else
> > + return false;
> > + }
>
> Please reorder this to avoid the else and return earlier while you're
> at it:
>
> if (folio_test_writeback(folio)) {
> if (wbc->sync_mode == WB_SYNC_NONE)
> return false;
> folio_wait_writeback(folio);
> }
Sure, that makes sense.
> (that's what actually got me started on my little cleanup spree while
> checking some details of the writeback waiting..)
This might be a good point to share that I'm considering (eventually)
not taking the folio lock here.
My plan looks something like this (not fully baked):
truncation (and similar) paths currently lock the folio, They would both
lock the folio _and_ claim that they were doing writeback on the folio.
Filesystems would receive the folio from the writeback iterator with
the writeback flag already set.
This allows, eg, folio mapping/unmapping to take place completely
independent of writeback. That seems like a good thing; I can't see
why the two should be connected.
> > + BUG_ON(folio_test_writeback(folio));
> > + if (!folio_clear_dirty_for_io(folio))
> > + return false;
> > +
> > + return true;
>
> ..
>
> return folio_clear_dirty_for_io(folio);
>
> ?
I did consider that, but there's a nice symmetry to the code the way it's
currently written, and that took precedence in my mind over "fewer lines
of code". There's nothing intrinsic about folio_clear_dirty_for_io()
being the last condition to be checked (is there? We have to
redirty_for_io if we decide to not start writeback), so it seemed to
make sense to leave space to add more conditions.
On Tue, Jun 27, 2023 at 12:16:34PM +0100, Matthew Wilcox wrote:
> This might be a good point to share that I'm considering (eventually)
> not taking the folio lock here.
>
> My plan looks something like this (not fully baked):
>
> truncation (and similar) paths currently lock the folio, They would both
> lock the folio _and_ claim that they were doing writeback on the folio.
>
> Filesystems would receive the folio from the writeback iterator with
> the writeback flag already set.
>
>
> This allows, eg, folio mapping/unmapping to take place completely
> independent of writeback. That seems like a good thing; I can't see
> why the two should be connected.
Ah, i_size is a problem. With an extending write, i_size is updated
while holding the folio lock. If we're writing back a partial folio,
we zero the tail. That must not race with an extending write. So
either we'd need to take both the folio lock & wb_lock when updating
i_size, or we'd need to take both the lock and wb_lock when writing
back the last page of a file.
On Tue, Jun 27, 2023 at 11:53:02AM +0100, David Howells wrote:
> Do you have this on a branch somewhere?
I just pushed it out to https://git.infradead.org/users/willy/pagecache.git/shortlog/refs/heads/writeback-iter
Running it through xfstests now. This includes one of Christoph's
suggestions, a build fix for Linus's tree, and a bugfix I noticed last
night, so it's not quite the same as the emails that were sent out in
this thread. I doubt it'll be what I send out for v2 either.
I'm looking at afs writeback now.
Matthew Wilcox <[email protected]> wrote:
> I'm looking at afs writeback now.
:-)
> fs/iomap/buffered-io.c | 14 +-
> include/linux/pagevec.h | 18 +++
> include/linux/writeback.h | 22 ++-
> mm/page-writeback.c | 310 +++++++++++++++++++++-----------------
> 4 files changed, 216 insertions(+), 148 deletions(-)
Documentation/mm/writeback.rst too please.
David
On Wed, Jun 28, 2023 at 09:03:10PM +0100, David Howells wrote:
> Matthew Wilcox <[email protected]> wrote:
>
> > I'm looking at afs writeback now.
>
> :-)
>
> > fs/iomap/buffered-io.c | 14 +-
> > include/linux/pagevec.h | 18 +++
> > include/linux/writeback.h | 22 ++-
> > mm/page-writeback.c | 310 +++++++++++++++++++++-----------------
> > 4 files changed, 216 insertions(+), 148 deletions(-)
>
> Documentation/mm/writeback.rst too please.
$ ls Documentation/mm/w*
ls: cannot access 'Documentation/mm/w*': No such file or directory
$ git grep writeback Documentation/mm
Documentation/mm/multigen_lru.rst:do not require TLB flushes; clean pages do not require writeback.
Documentation/mm/page_migration.rst:2. Ensure that writeback is complete.
Documentation/mm/page_migration.rst:15. Queued up writeback on the new page is triggered.
Documentation/mm/physical_memory.rst:``nr_writeback_throttled``
Documentation/mm/physical_memory.rst: Number of pages written while reclaim is throttled waiting for writeback.
Or are you suggesting I write a new piece of kernel documentation?
If so, I respectfully decline. I've updated the kernel-doc included
in Documentation/core-api/mm-api.rst and I think that's all I can
reasonably be asked to do.
Just curious where this did end up. I have some changes that could use
or conflict with this depending on your view. They aren't time critical
yt, but if we're going to the road in this series I'd appreciate if we'd
get it done rather sooner than later :)
On Wed, Jun 28, 2023 at 08:31:05PM +0100, Matthew Wilcox wrote:
> On Tue, Jun 27, 2023 at 11:53:02AM +0100, David Howells wrote:
> > Do you have this on a branch somewhere?
>
> I just pushed it out to https://git.infradead.org/users/willy/pagecache.git/shortlog/refs/heads/writeback-iter
>
> Running it through xfstests now. This includes one of Christoph's
> suggestions, a build fix for Linus's tree, and a bugfix I noticed last
> night, so it's not quite the same as the emails that were sent out in
> this thread. I doubt it'll be what I send out for v2 either.
So it turns out thіs version still applies fine and tests fine with
latest mainline.
I've put up a slight tweak here:
http://git.infradead.org/users/hch/misc.git/shortlog/refs/heads/writeback-iter
this moves and documents the new fields in struct writeback_control
and drops the iomap patch for now as it has conflicts in the VFS tree
in this merge window.
Do you want me to send this version out, or do you want to take over or
is there a good reason not to progress with it?