| Message ID | 20231218153553.807799-16-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/17] writeback: fix done_index when hitting the wbc->nr_to_write | expand |
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
diff --git a/include/linux/writeback.h b/include/linux/writeback.h index 195393981ccb5c..1c1a543070c17b 100644 --- a/include/linux/writeback.h +++ b/include/linux/writeback.h @@ -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); 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;