[15/17] writeback: Add for_each_writeback_folio()
Commit Message
From: "Matthew Wilcox (Oracle)" <willy@infradead.org>
Wrap up the iterator with a nice bit of syntactic sugar. Now the
caller doesn't need to know about wbc->err and can just return error,
not knowing that the iterator took care of storing errors correctly.
Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
include/linux/writeback.h | 10 ++++++++++
mm/page-writeback.c | 8 +++-----
2 files changed, 13 insertions(+), 5 deletions(-)
Comments
On Mon 18-12-23 16:35:51, Christoph Hellwig wrote:
> From: "Matthew Wilcox (Oracle)" <willy@infradead.org>
>
> Wrap up the iterator with a nice bit of syntactic sugar. Now the
> caller doesn't need to know about wbc->err and can just return error,
> not knowing that the iterator took care of storing errors correctly.
>
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
Not sure if the trick with 'error' variable isn't a bit too clever for us
;) We'll see how many bugs it will cause in the future... Feel free to add:
Reviewed-by: Jan Kara <jack@suse.cz>
Honza
> +#define for_each_writeback_folio(mapping, wbc, folio, error) \
> + for (folio = writeback_iter_init(mapping, wbc); \
> + folio || ((error = wbc->err), false); \
> + folio = writeback_iter_next(mapping, wbc, folio, error))
> +
> typedef int (*writepage_t)(struct folio *folio, struct writeback_control *wbc,
> void *data);
>
> diff --git a/mm/page-writeback.c b/mm/page-writeback.c
> index 0771f19950081f..fbffd30a9cc93f 100644
> --- a/mm/page-writeback.c
> +++ b/mm/page-writeback.c
> @@ -2463,7 +2463,7 @@ static struct folio *writeback_get_folio(struct address_space *mapping,
> return folio;
> }
>
> -static struct folio *writeback_iter_init(struct address_space *mapping,
> +struct folio *writeback_iter_init(struct address_space *mapping,
> struct writeback_control *wbc)
> {
> if (wbc->range_cyclic)
> @@ -2479,7 +2479,7 @@ static struct folio *writeback_iter_init(struct address_space *mapping,
> return writeback_get_folio(mapping, wbc);
> }
>
> -static struct folio *writeback_iter_next(struct address_space *mapping,
> +struct folio *writeback_iter_next(struct address_space *mapping,
> struct writeback_control *wbc, struct folio *folio, int error)
> {
> unsigned long nr = folio_nr_pages(folio);
> @@ -2557,9 +2557,7 @@ int write_cache_pages(struct address_space *mapping,
> struct folio *folio;
> int error;
>
> - for (folio = writeback_iter_init(mapping, wbc);
> - folio;
> - folio = writeback_iter_next(mapping, wbc, folio, error))
> + for_each_writeback_folio(mapping, wbc, folio, error)
> error = writepage(folio, wbc, data);
>
> return wbc->err;
> --
> 2.39.2
>
On Thu, Dec 21, 2023 at 12:51:49PM +0100, Jan Kara wrote:
> On Mon 18-12-23 16:35:51, Christoph Hellwig wrote:
> > From: "Matthew Wilcox (Oracle)" <willy@infradead.org>
> >
> > Wrap up the iterator with a nice bit of syntactic sugar. Now the
> > caller doesn't need to know about wbc->err and can just return error,
> > not knowing that the iterator took care of storing errors correctly.
> >
> > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
>
> Not sure if the trick with 'error' variable isn't a bit too clever for us
> ;) We'll see how many bugs it will cause in the future...
It's a bit too much syntactic sugar for my taste, but if we want a magic
for macro I can't really see a good way around it. I personally wouldn't
mind a version where the writeback_get_folio moves out of
writeback_iter_init and the pattern would look more like:
writeback_iter_init(mapping, wbc);
while ((folio = writeback_iter_next(mapping, wbc, folio))) {
wbc->err = <do something>
}
return wbc->err;
On Thu 21-12-23 13:29:10, Christoph Hellwig wrote:
> On Thu, Dec 21, 2023 at 12:51:49PM +0100, Jan Kara wrote:
> > On Mon 18-12-23 16:35:51, Christoph Hellwig wrote:
> > > From: "Matthew Wilcox (Oracle)" <willy@infradead.org>
> > >
> > > Wrap up the iterator with a nice bit of syntactic sugar. Now the
> > > caller doesn't need to know about wbc->err and can just return error,
> > > not knowing that the iterator took care of storing errors correctly.
> > >
> > > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> > > Signed-off-by: Christoph Hellwig <hch@lst.de>
> >
> > Not sure if the trick with 'error' variable isn't a bit too clever for us
> > ;) We'll see how many bugs it will cause in the future...
>
> It's a bit too much syntactic sugar for my taste, but if we want a magic
> for macro I can't really see a good way around it. I personally wouldn't
Agreed. The macro is kind of neat but a magic like this tends to bite us in
surprising ways. E.g. if someone breaks out of the loop, things will go
really wrong (missing writeback_finish() call). That would be actually a
good usecase for the cleanup handlers PeterZ has been promoting - we could
make sure writeback_finish() is called whenever we exit the loop block.
> mind a version where the writeback_get_folio moves out of
> writeback_iter_init and the pattern would look more like:
>
> writeback_iter_init(mapping, wbc);
> while ((folio = writeback_iter_next(mapping, wbc, folio))) {
> wbc->err = <do something>
> }
>
> return wbc->err;
That would work for me as well. But I don't feel to strongly about this
either way.
Honza
@@ -368,6 +368,16 @@ int balance_dirty_pages_ratelimited_flags(struct address_space *mapping,
bool wb_over_bg_thresh(struct bdi_writeback *wb);
+struct folio *writeback_iter_init(struct address_space *mapping,
+ struct writeback_control *wbc);
+struct folio *writeback_iter_next(struct address_space *mapping,
+ struct writeback_control *wbc, struct folio *folio, int error);
+
+#define for_each_writeback_folio(mapping, wbc, folio, error) \
+ for (folio = writeback_iter_init(mapping, wbc); \
+ folio || ((error = wbc->err), false); \
+ folio = writeback_iter_next(mapping, wbc, folio, error))
+
typedef int (*writepage_t)(struct folio *folio, struct writeback_control *wbc,
void *data);
@@ -2463,7 +2463,7 @@ static struct folio *writeback_get_folio(struct address_space *mapping,
return folio;
}
-static struct folio *writeback_iter_init(struct address_space *mapping,
+struct folio *writeback_iter_init(struct address_space *mapping,
struct writeback_control *wbc)
{
if (wbc->range_cyclic)
@@ -2479,7 +2479,7 @@ static struct folio *writeback_iter_init(struct address_space *mapping,
return writeback_get_folio(mapping, wbc);
}
-static struct folio *writeback_iter_next(struct address_space *mapping,
+struct folio *writeback_iter_next(struct address_space *mapping,
struct writeback_control *wbc, struct folio *folio, int error)
{
unsigned long nr = folio_nr_pages(folio);
@@ -2557,9 +2557,7 @@ int write_cache_pages(struct address_space *mapping,
struct folio *folio;
int error;
- for (folio = writeback_iter_init(mapping, wbc);
- folio;
- folio = writeback_iter_next(mapping, wbc, folio, error))
+ for_each_writeback_folio(mapping, wbc, folio, error)
error = writepage(folio, wbc, data);
return wbc->err;