| Message ID | 20240125085758.2393327-7-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/19] writeback: fix done_index when hitting the wbc->nr_to_write | expand |
On Thu, Jan 25, 2024 at 09:57:45AM +0100, Christoph Hellwig wrote: > From: "Matthew Wilcox (Oracle)" <willy@infradead.org> > > Instead of having a 'done' variable that controls the nested loops, > have a writeback_finish() that can be returned directly. This involves > keeping more things in writeback_control, but it's just moving stuff > allocated on the stack to being allocated slightly earlier on the stack. > > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> > [hch: heavily rebased, reordered and commented struct writeback_control] > Signed-off-by: Christoph Hellwig <hch@lst.de> > Reviewed-by: Jan Kara <jack@suse.cz> > Acked-by: Dave Chinner <dchinner@redhat.com> > --- > include/linux/writeback.h | 6 +++ > mm/page-writeback.c | 79 ++++++++++++++++++++------------------- > 2 files changed, 47 insertions(+), 38 deletions(-) > ... > diff --git a/mm/page-writeback.c b/mm/page-writeback.c > index 437745a511c634..fcd90a176d806c 100644 > --- a/mm/page-writeback.c > +++ b/mm/page-writeback.c ... > @@ -2419,22 +2437,23 @@ int write_cache_pages(struct address_space *mapping, > } else { > tag = PAGECACHE_TAG_DIRTY; > } > - done_index = index; > - while (!done && (index <= end)) { > + > + folio_batch_init(&wbc->fbatch); > + wbc->err = 0; > + > + while (index <= end) { > int i; > > nr_folios = filemap_get_folios_tag(mapping, &index, end, > - tag, &fbatch); > + tag, &wbc->fbatch); > > if (nr_folios == 0) > break; > > for (i = 0; i < nr_folios; i++) { > - struct folio *folio = fbatch.folios[i]; > + struct folio *folio = wbc->fbatch.folios[i]; > unsigned long nr; > > - done_index = folio->index; > - > folio_lock(folio); > > /* > @@ -2481,6 +2500,9 @@ int write_cache_pages(struct address_space *mapping, > folio_unlock(folio); > error = 0; > } > + JFYI: whitespace damage on the above line. > + if (error && !wbc->err) > + wbc->err = error; > Also what happened to the return of the above "first error encountered" for the WB_SYNC_ALL case? Is that not needed for some reason (and so the comment just below might require an update)? Brian > /* > * For integrity sync we have to keep going until we > @@ -2496,38 +2518,19 @@ int write_cache_pages(struct address_space *mapping, > * off and media errors won't choke writeout for the > * entire file. > */ > - if (error && !ret) > - ret = error; > - if (wbc->sync_mode == WB_SYNC_NONE) { > - if (ret || wbc->nr_to_write <= 0) { > - done_index = folio->index + nr; > - done = 1; > - break; > - } > + if (wbc->sync_mode == WB_SYNC_NONE && > + (wbc->err || wbc->nr_to_write <= 0)) { > + writeback_finish(mapping, wbc, > + folio->index + nr); > + return error; > } > } > - folio_batch_release(&fbatch); > + folio_batch_release(&wbc->fbatch); > cond_resched(); > } > > - /* > - * 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. > - */ > - if (wbc->range_cyclic) { > - if (done) > - mapping->writeback_index = done_index; > - else > - mapping->writeback_index = 0; > - } > - > - return ret; > + writeback_finish(mapping, wbc, 0); > + return 0; > } > EXPORT_SYMBOL(write_cache_pages); > > -- > 2.39.2 > >
On Mon, Jan 29, 2024 at 03:13:47PM -0500, Brian Foster wrote: > > @@ -2481,6 +2500,9 @@ int write_cache_pages(struct address_space *mapping, > > folio_unlock(folio); > > error = 0; > > } > > + > > JFYI: whitespace damage on the above line. Thanks, fixed. > > > + if (error && !wbc->err) > > + wbc->err = error; > > > > Also what happened to the return of the above "first error encountered" > for the WB_SYNC_ALL case? Is that not needed for some reason (and so the > comment just below might require an update)? No, this got broken during the various rebases (and is fixed again later in the series). We need to return wbc->err from write_cache_pages at this stage, I'll fix it.
On Tue, Jan 30, 2024 at 03:04:59PM +0100, Christoph Hellwig wrote: > On Mon, Jan 29, 2024 at 03:13:47PM -0500, Brian Foster wrote: > > > @@ -2481,6 +2500,9 @@ int write_cache_pages(struct address_space *mapping, > > > folio_unlock(folio); > > > error = 0; > > > } > > > + > > > > JFYI: whitespace damage on the above line. > > Thanks, fixed. > > > > > > + if (error && !wbc->err) > > > + wbc->err = error; > > > > > > > Also what happened to the return of the above "first error encountered" > > for the WB_SYNC_ALL case? Is that not needed for some reason (and so the > > comment just below might require an update)? > > No, this got broken during the various rebases (and is fixed again later > in the series). We need to return wbc->err from write_cache_pages at > this stage, I'll fix it. > Ok, I noticed it was added back once I got to more of the iter abstraction bits and so figured it was a transient/unintentional thing. The above tweak makes sense to me. FWIW, I haven't stared at the final patch long enough to have a strong opinion. I tend to agree with Jan that the error handling logic in the current series is a little wonky in that it's one of those things I'd have to go read the implementation every time to remember what it does, but the broader changes all seem reasonable to me. So for patches 1-18 and with the above tweak: Reviewed-by: Brian Foster <bfoster@redhat.com>
diff --git a/include/linux/writeback.h b/include/linux/writeback.h index 4b8cf9e4810bad..7d60a68fa4ea47 100644 --- a/include/linux/writeback.h +++ b/include/linux/writeback.h @@ -11,6 +11,7 @@ #include <linux/flex_proportions.h> #include <linux/backing-dev-defs.h> #include <linux/blk_types.h> +#include <linux/pagevec.h> struct bio; @@ -40,6 +41,7 @@ enum writeback_sync_modes { * in a manner such that unspecified fields are set to zero. */ struct writeback_control { + /* public fields that can be set and/or consumed by the caller: */ long nr_to_write; /* Write this many pages, and decrement this for each page written */ long pages_skipped; /* Pages which were not written */ @@ -77,6 +79,10 @@ struct writeback_control { */ struct swap_iocb **swap_plug; + /* internal fields used by the ->writepages implementation: */ + struct folio_batch fbatch; + int err; + #ifdef CONFIG_CGROUP_WRITEBACK struct bdi_writeback *wb; /* wb this writeback is issued under */ struct inode *inode; /* inode being written out */ diff --git a/mm/page-writeback.c b/mm/page-writeback.c index 437745a511c634..fcd90a176d806c 100644 --- a/mm/page-writeback.c +++ b/mm/page-writeback.c @@ -2360,6 +2360,29 @@ void tag_pages_for_writeback(struct address_space *mapping, } EXPORT_SYMBOL(tag_pages_for_writeback); +static void writeback_finish(struct address_space *mapping, + struct writeback_control *wbc, pgoff_t done_index) +{ + folio_batch_release(&wbc->fbatch); + + /* + * 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. + */ + if (wbc->range_cyclic) { + if (wbc->err || wbc->nr_to_write <= 0) + mapping->writeback_index = done_index; + else + mapping->writeback_index = 0; + } +} + /** * 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 @@ -2395,17 +2418,12 @@ int write_cache_pages(struct address_space *mapping, struct writeback_control *wbc, writepage_t writepage, void *data) { - int ret = 0; - int done = 0; int error; - struct folio_batch fbatch; int nr_folios; pgoff_t index; pgoff_t end; /* Inclusive */ - pgoff_t done_index; xa_mark_t tag; - folio_batch_init(&fbatch); if (wbc->range_cyclic) { index = mapping->writeback_index; /* prev offset */ end = -1; @@ -2419,22 +2437,23 @@ int write_cache_pages(struct address_space *mapping, } else { tag = PAGECACHE_TAG_DIRTY; } - done_index = index; - while (!done && (index <= end)) { + + folio_batch_init(&wbc->fbatch); + wbc->err = 0; + + while (index <= end) { int i; nr_folios = filemap_get_folios_tag(mapping, &index, end, - tag, &fbatch); + tag, &wbc->fbatch); if (nr_folios == 0) break; for (i = 0; i < nr_folios; i++) { - struct folio *folio = fbatch.folios[i]; + struct folio *folio = wbc->fbatch.folios[i]; unsigned long nr; - done_index = folio->index; - folio_lock(folio); /* @@ -2481,6 +2500,9 @@ int write_cache_pages(struct address_space *mapping, folio_unlock(folio); error = 0; } + + if (error && !wbc->err) + wbc->err = error; /* * For integrity sync we have to keep going until we @@ -2496,38 +2518,19 @@ int write_cache_pages(struct address_space *mapping, * off and media errors won't choke writeout for the * entire file. */ - if (error && !ret) - ret = error; - if (wbc->sync_mode == WB_SYNC_NONE) { - if (ret || wbc->nr_to_write <= 0) { - done_index = folio->index + nr; - done = 1; - break; - } + if (wbc->sync_mode == WB_SYNC_NONE && + (wbc->err || wbc->nr_to_write <= 0)) { + writeback_finish(mapping, wbc, + folio->index + nr); + return error; } } - folio_batch_release(&fbatch); + folio_batch_release(&wbc->fbatch); cond_resched(); } - /* - * 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. - */ - if (wbc->range_cyclic) { - if (done) - mapping->writeback_index = done_index; - else - mapping->writeback_index = 0; - } - - return ret; + writeback_finish(mapping, wbc, 0); + return 0; } EXPORT_SYMBOL(write_cache_pages);