| Message ID | 20240203071147.862076-14-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/13] writeback: remove a duplicate prototype for tag_pages_for_writeback | expand |
On Sat 03-02-24 08:11:47, Christoph Hellwig wrote: > From: "Matthew Wilcox (Oracle)" <willy@infradead.org> > > Use the new writeback_iter() directly instead of indirecting > through a callback. > > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> > [hch: ported to the while based iter style] > Signed-off-by: Christoph Hellwig <hch@lst.de> Looks good. Feel free to add: Reviewed-by: Jan Kara <jack@suse.cz> I've just noticed one preexisting problem which was made more visible by your reshuffling: > +static int writeback_use_writepage(struct address_space *mapping, > + struct writeback_control *wbc) > { > - struct address_space *mapping = data; > - int ret = mapping->a_ops->writepage(&folio->page, wbc); > - mapping_set_error(mapping, ret); > - return ret; > + struct folio *folio = NULL; > + struct blk_plug plug; > + int err; > + > + blk_start_plug(&plug); > + while ((folio = writeback_iter(mapping, wbc, folio, &err))) { > + err = mapping->a_ops->writepage(&folio->page, wbc); > + mapping_set_error(mapping, err); So if ->writepage() returns AOP_WRITEPAGE_ACTIVATE, we shouldn't call mapping_set_error() because that's just going to confuse the error tracking. > + if (err == AOP_WRITEPAGE_ACTIVATE) { > + folio_unlock(folio); > + err = 0; > + } > + } > + blk_finish_plug(&plug); > + > + return err; > } Honza
On Sat, Feb 03, 2024 at 08:11:47AM +0100, Christoph Hellwig wrote: > From: "Matthew Wilcox (Oracle)" <willy@infradead.org> > > Use the new writeback_iter() directly instead of indirecting > through a callback. > > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> > [hch: ported to the while based iter style] > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- LGTM, modulo Jan's comments: Reviewed-by: Brian Foster <bfoster@redhat.com> > mm/page-writeback.c | 31 +++++++++++++++++++------------ > 1 file changed, 19 insertions(+), 12 deletions(-) > > diff --git a/mm/page-writeback.c b/mm/page-writeback.c > index 5fe4cdb7dbd61a..53ff2d8219ddb6 100644 > --- a/mm/page-writeback.c > +++ b/mm/page-writeback.c > @@ -2577,13 +2577,25 @@ int write_cache_pages(struct address_space *mapping, > } > EXPORT_SYMBOL(write_cache_pages); > > -static int writepage_cb(struct folio *folio, struct writeback_control *wbc, > - void *data) > +static int writeback_use_writepage(struct address_space *mapping, > + struct writeback_control *wbc) > { > - struct address_space *mapping = data; > - int ret = mapping->a_ops->writepage(&folio->page, wbc); > - mapping_set_error(mapping, ret); > - return ret; > + struct folio *folio = NULL; > + struct blk_plug plug; > + int err; > + > + blk_start_plug(&plug); > + while ((folio = writeback_iter(mapping, wbc, folio, &err))) { > + err = mapping->a_ops->writepage(&folio->page, wbc); > + mapping_set_error(mapping, err); > + if (err == AOP_WRITEPAGE_ACTIVATE) { > + folio_unlock(folio); > + err = 0; > + } > + } > + blk_finish_plug(&plug); > + > + return err; > } > > int do_writepages(struct address_space *mapping, struct writeback_control *wbc) > @@ -2599,12 +2611,7 @@ int do_writepages(struct address_space *mapping, struct writeback_control *wbc) > if (mapping->a_ops->writepages) { > ret = mapping->a_ops->writepages(mapping, wbc); > } else if (mapping->a_ops->writepage) { > - struct blk_plug plug; > - > - blk_start_plug(&plug); > - ret = write_cache_pages(mapping, wbc, writepage_cb, > - mapping); > - blk_finish_plug(&plug); > + ret = writeback_use_writepage(mapping, wbc); > } else { > /* deal with chardevs and other special files */ > ret = 0; > -- > 2.39.2 >
diff --git a/mm/page-writeback.c b/mm/page-writeback.c index 5fe4cdb7dbd61a..53ff2d8219ddb6 100644 --- a/mm/page-writeback.c +++ b/mm/page-writeback.c @@ -2577,13 +2577,25 @@ int write_cache_pages(struct address_space *mapping, } EXPORT_SYMBOL(write_cache_pages); -static int writepage_cb(struct folio *folio, struct writeback_control *wbc, - void *data) +static int writeback_use_writepage(struct address_space *mapping, + struct writeback_control *wbc) { - struct address_space *mapping = data; - int ret = mapping->a_ops->writepage(&folio->page, wbc); - mapping_set_error(mapping, ret); - return ret; + struct folio *folio = NULL; + struct blk_plug plug; + int err; + + blk_start_plug(&plug); + while ((folio = writeback_iter(mapping, wbc, folio, &err))) { + err = mapping->a_ops->writepage(&folio->page, wbc); + mapping_set_error(mapping, err); + if (err == AOP_WRITEPAGE_ACTIVATE) { + folio_unlock(folio); + err = 0; + } + } + blk_finish_plug(&plug); + + return err; } int do_writepages(struct address_space *mapping, struct writeback_control *wbc) @@ -2599,12 +2611,7 @@ int do_writepages(struct address_space *mapping, struct writeback_control *wbc) if (mapping->a_ops->writepages) { ret = mapping->a_ops->writepages(mapping, wbc); } else if (mapping->a_ops->writepage) { - struct blk_plug plug; - - blk_start_plug(&plug); - ret = write_cache_pages(mapping, wbc, writepage_cb, - mapping); - blk_finish_plug(&plug); + ret = writeback_use_writepage(mapping, wbc); } else { /* deal with chardevs and other special files */ ret = 0;