On Mon, Feb 05, 2024 at 10:33:52AM -0500, Brian Foster wrote:
> On Sat, Feb 03, 2024 at 08:11:46AM +0100, Christoph Hellwig wrote:
> > Refactor the code left in write_cache_pages into an iterator that the
> > file system can call to get the next folio for a writeback operation:
> >
> > struct folio *folio = NULL;
> >
> > while ((folio = writeback_iter(mapping, wbc, folio, &error))) {
> > error = <do per-foli writeback>;
> > }
> >
> > The twist here is that the error value is passed by reference, so that
> > the iterator can restore it when breaking out of the loop.
> >
> > Handling of the magic AOP_WRITEPAGE_ACTIVATE value stays outside the
> > iterator and needs is just kept in the write_cache_pages legacy wrapper.
> > in preparation for eventually killing it off.
> >
> > Heavily based on a for_each* based iterator from Matthew Wilcox.
> >
> > Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> > ---
> > include/linux/writeback.h | 4 +
> > mm/page-writeback.c | 192 ++++++++++++++++++++++----------------
> > 2 files changed, 118 insertions(+), 78 deletions(-)
> >
> ...
> > diff --git a/mm/page-writeback.c b/mm/page-writeback.c
> > index 3abb053e70580e..5fe4cdb7dbd61a 100644
> > --- a/mm/page-writeback.c
> > +++ b/mm/page-writeback.c
> ...
> > @@ -2434,69 +2434,68 @@ static struct folio *writeback_get_folio(struct address_space *mapping,
> > }
> >
> > /**
> ...
> > */
> > -int write_cache_pages(struct address_space *mapping,
> > - struct writeback_control *wbc, writepage_t writepage,
> > - void *data)
> > +struct folio *writeback_iter(struct address_space *mapping,
> > + struct writeback_control *wbc, struct folio *folio, int *error)
> > {
> ...
> > + } else {
> > wbc->nr_to_write -= folio_nr_pages(folio);
> >
> > - if (error == AOP_WRITEPAGE_ACTIVATE) {
> > - folio_unlock(folio);
> > - error = 0;
> > - }
> > + WARN_ON_ONCE(*error > 0);
>
> Why the warning on writeback error here? It looks like new behavior, but
> maybe I missed something. Otherwise the factoring LGTM.
Err, sorry.. I glossed over the > 0 check and read it as < 0.
Disregard, this seems reasonable to me as long as we no longer expect
those AOP returns (which I'm not really clear on either, but anyways..):
Reviewed-by: Brian Foster <bfoster@xxxxxxxxxx>
>
> Brian
>
> >
> > /*
> > * For integrity writeback we have to keep going until we have
> > @@ -2510,33 +2509,70 @@ int write_cache_pages(struct address_space *mapping,
> > * wbc->nr_to_write or encounter the first error.
> > */
> > if (wbc->sync_mode == WB_SYNC_ALL) {
> > - if (error && !ret)
> > - ret = error;
> > + if (*error && !wbc->saved_err)
> > + wbc->saved_err = *error;
> > } else {
> > - if (error || wbc->nr_to_write <= 0)
> > + if (*error || wbc->nr_to_write <= 0)
> > goto done;
> > }
> > }
> >
> > - /*
> > - * For range cyclic writeback we need to remember where we stopped so
> > - * that we can continue there next time we are called. If we hit the
> > - * last page and there is more work to be done, wrap back to the start
> > - * of the file.
> > - *
> > - * For non-cyclic writeback we always start looking up at the beginning
> > - * of the file if we are called again, which can only happen due to
> > - * -ENOMEM from the file system.
> > - */
> > - folio_batch_release(&wbc->fbatch);
> > - if (wbc->range_cyclic)
> > - mapping->writeback_index = 0;
> > - return ret;
> > + folio = writeback_get_folio(mapping, wbc);
> > + if (!folio) {
> > + /*
> > + * To avoid deadlocks between range_cyclic writeback and callers
> > + * that hold pages in PageWriteback to aggregate I/O until
> > + * the writeback iteration finishes, we do not loop back to the
> > + * start of the file. Doing so causes a page lock/page
> > + * writeback access order inversion - we should only ever lock
> > + * multiple pages in ascending page->index order, and looping
> > + * back to the start of the file violates that rule and causes
> > + * deadlocks.
> > + */
> > + if (wbc->range_cyclic)
> > + mapping->writeback_index = 0;
> > +
> > + /*
> > + * Return the first error we encountered (if there was any) to
> > + * the caller.
> > + */
> > + *error = wbc->saved_err;
> > + }
> > + return folio;
> >
> > done:
> > folio_batch_release(&wbc->fbatch);
> > if (wbc->range_cyclic)
> > mapping->writeback_index = folio->index + folio_nr_pages(folio);
> > + return NULL;
> > +}
> > +
> > +/**
> > + * 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
> > + * @wbc: subtract the number of written pages from *@wbc->nr_to_write
> > + * @writepage: function called for each page
> > + * @data: data passed to writepage function
> > + *
> > + * Return: %0 on success, negative error code otherwise
> > + *
> > + * Note: please use writeback_iter() instead.
> > + */
> > +int write_cache_pages(struct address_space *mapping,
> > + struct writeback_control *wbc, writepage_t writepage,
> > + void *data)
> > +{
> > + struct folio *folio = NULL;
> > + int error;
> > +
> > + while ((folio = writeback_iter(mapping, wbc, folio, &error))) {
> > + error = writepage(folio, wbc, data);
> > + if (error == AOP_WRITEPAGE_ACTIVATE) {
> > + folio_unlock(folio);
> > + error = 0;
> > + }
> > + }
> > +
> > return error;
> > }
> > EXPORT_SYMBOL(write_cache_pages);
> > --
> > 2.39.2
> >
> >
>
>
