[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240207084224.o6nn4l7owwhzb5e3@quack3>
Date: Wed, 7 Feb 2024 09:42:24 +0100
From: Jan Kara <jack@...e.cz>
To: Brian Foster <bfoster@...hat.com>
Cc: Christoph Hellwig <hch@....de>, linux-mm@...ck.org,
Matthew Wilcox <willy@...radead.org>, Jan Kara <jack@...e.com>,
David Howells <dhowells@...hat.com>,
Christian Brauner <brauner@...nel.org>,
"Darrick J. Wong" <djwong@...nel.org>, linux-xfs@...r.kernel.org,
linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 12/13] writeback: add a writeback iterator
On Tue 06-02-24 09:54:01, Brian Foster wrote:
> 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@....de>
> > > ---
> > > 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@...hat.com>
So my understanding is that AOP_WRITEPAGE_ACTIVATE should be now handled
directly by the caller of ->writepage hook and not by writeback_iter()
which is the reason why the warning is here.
Honza
--
Jan Kara <jack@...e.com>
SUSE Labs, CR
Powered by blists - more mailing lists