| Message ID | 20231214132544.376574-10-hch@lst.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [01/11] writeback: Factor out writeback_finish() | expand |
On Thu, Dec 14, 2023 at 02:25:42PM +0100, Christoph Hellwig wrote: > From: "Matthew Wilcox (Oracle)" <willy@infradead.org> > > Pull the post-processing of the writepage_t callback into a > separate function. That means changing writeback_finish() to > return NULL, and writeback_get_next() to call writeback_finish() > when we naturally run out of folios. > > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > mm/page-writeback.c | 89 +++++++++++++++++++++++---------------------- > 1 file changed, 46 insertions(+), 43 deletions(-) > > diff --git a/mm/page-writeback.c b/mm/page-writeback.c > index b0accca1f4bfa7..4fae912f7a86e2 100644 > --- a/mm/page-writeback.c > +++ b/mm/page-writeback.c > @@ -2360,7 +2360,7 @@ void tag_pages_for_writeback(struct address_space *mapping, > } > EXPORT_SYMBOL(tag_pages_for_writeback); > > -static int writeback_finish(struct address_space *mapping, > +static struct folio *writeback_finish(struct address_space *mapping, > struct writeback_control *wbc, bool done) > { > folio_batch_release(&wbc->fbatch); > @@ -2375,7 +2375,7 @@ static int writeback_finish(struct address_space *mapping, > if (wbc->range_cyclic || (wbc->range_whole && wbc->nr_to_write > 0)) > mapping->writeback_index = wbc->done_index; > > - return wbc->err; > + return NULL; The series looks reasonable to me on a first pass, but this stood out to me as really odd. I was initially wondering if it made sense to use an ERR_PTR() here or something, but on further staring it kind of seems like this is better off being factored out of the internal iteration paths. Untested and Probably Broken patch (based on this one) below as a quick reference, but just a thought... BTW it would be nicer to just drop the ->done field entirely as Jan has already suggested, but I just stuffed it in wbc for simplicity. Brian --- 8< --- diff --git a/include/linux/writeback.h b/include/linux/writeback.h index be960f92ad9d..0babca17a2c0 100644 --- a/include/linux/writeback.h +++ b/include/linux/writeback.h @@ -84,6 +84,7 @@ struct writeback_control { pgoff_t index; pgoff_t end; /* inclusive */ pgoff_t done_index; + bool done; int err; unsigned range_whole:1; /* entire file */ diff --git a/mm/page-writeback.c b/mm/page-writeback.c index 4fae912f7a86..3ee2058a2559 100644 --- a/mm/page-writeback.c +++ b/mm/page-writeback.c @@ -2360,8 +2360,8 @@ void tag_pages_for_writeback(struct address_space *mapping, } EXPORT_SYMBOL(tag_pages_for_writeback); -static struct folio *writeback_finish(struct address_space *mapping, - struct writeback_control *wbc, bool done) +static int writeback_iter_finish(struct address_space *mapping, + struct writeback_control *wbc) { folio_batch_release(&wbc->fbatch); @@ -2370,12 +2370,12 @@ static struct folio *writeback_finish(struct address_space *mapping, * wrap the index back to the start of the file for the next * time we are called. */ - if (wbc->range_cyclic && !done) + if (wbc->range_cyclic && !wbc->done) wbc->done_index = 0; if (wbc->range_cyclic || (wbc->range_whole && wbc->nr_to_write > 0)) mapping->writeback_index = wbc->done_index; - return NULL; + return wbc->err; } static struct folio *writeback_get_next(struct address_space *mapping, @@ -2434,19 +2434,16 @@ static struct folio *writeback_get_folio(struct address_space *mapping, { struct folio *folio; - for (;;) { - folio = writeback_get_next(mapping, wbc); - if (!folio) - return writeback_finish(mapping, wbc, false); + while ((folio = writeback_get_next(mapping, wbc)) != NULL) { wbc->done_index = folio->index; - folio_lock(folio); if (likely(should_writeback_folio(mapping, wbc, folio))) break; folio_unlock(folio); } - trace_wbc_writepage(wbc, inode_to_bdi(mapping->host)); + if (folio) + trace_wbc_writepage(wbc, inode_to_bdi(mapping->host)); return folio; } @@ -2466,6 +2463,7 @@ static struct folio *writeback_iter_init(struct address_space *mapping, tag_pages_for_writeback(mapping, wbc->index, wbc->end); wbc->done_index = wbc->index; + wbc->done = false; folio_batch_init(&wbc->fbatch); wbc->err = 0; @@ -2494,7 +2492,8 @@ static struct folio *writeback_iter_next(struct address_space *mapping, } else if (wbc->sync_mode != WB_SYNC_ALL) { wbc->err = error; wbc->done_index = folio->index + nr; - return writeback_finish(mapping, wbc, true); + wbc->done = true; + return NULL; } if (!wbc->err) wbc->err = error; @@ -2507,8 +2506,10 @@ static struct folio *writeback_iter_next(struct address_space *mapping, * to entering this loop. */ wbc->nr_to_write -= nr; - if (wbc->nr_to_write <= 0 && wbc->sync_mode == WB_SYNC_NONE) - return writeback_finish(mapping, wbc, true); + if (wbc->nr_to_write <= 0 && wbc->sync_mode == WB_SYNC_NONE) { + wbc->done = true; + return NULL; + } return writeback_get_folio(mapping, wbc); } @@ -2557,7 +2558,7 @@ int write_cache_pages(struct address_space *mapping, error = writepage(folio, wbc, data); } - return wbc->err; + return writeback_iter_finish(mapping, wbc); } EXPORT_SYMBOL(write_cache_pages);
On Fri, Dec 15, 2023 at 09:17:05AM -0500, Brian Foster wrote: > + while ((folio = writeback_get_next(mapping, wbc)) != NULL) { > wbc->done_index = folio->index; > folio_lock(folio); > if (likely(should_writeback_folio(mapping, wbc, folio))) > break; > folio_unlock(folio); > } > > + if (folio) > + trace_wbc_writepage(wbc, inode_to_bdi(mapping->host)); > return folio; I posted a very similar transformation in reply to willy's first posting of the series :) I guess that's 2:1 now and I might as well go ahead and do something like that now :)
diff --git a/mm/page-writeback.c b/mm/page-writeback.c index b0accca1f4bfa7..4fae912f7a86e2 100644 --- a/mm/page-writeback.c +++ b/mm/page-writeback.c @@ -2360,7 +2360,7 @@ void tag_pages_for_writeback(struct address_space *mapping, } EXPORT_SYMBOL(tag_pages_for_writeback); -static int writeback_finish(struct address_space *mapping, +static struct folio *writeback_finish(struct address_space *mapping, struct writeback_control *wbc, bool done) { folio_batch_release(&wbc->fbatch); @@ -2375,7 +2375,7 @@ static int writeback_finish(struct address_space *mapping, if (wbc->range_cyclic || (wbc->range_whole && wbc->nr_to_write > 0)) mapping->writeback_index = wbc->done_index; - return wbc->err; + return NULL; } static struct folio *writeback_get_next(struct address_space *mapping, @@ -2437,7 +2437,7 @@ static struct folio *writeback_get_folio(struct address_space *mapping, for (;;) { folio = writeback_get_next(mapping, wbc); if (!folio) - return NULL; + return writeback_finish(mapping, wbc, false); wbc->done_index = folio->index; folio_lock(folio); @@ -2472,6 +2472,47 @@ 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 writeback_control *wbc, struct folio *folio, int error) +{ + unsigned long nr = folio_nr_pages(folio); + + if (unlikely(error)) { + /* + * Handle errors according to the type of writeback. + * There's no need to continue for background writeback. + * Just push done_index past this folio so media + * errors won't choke writeout for the entire file. + * For integrity writeback, we must process the entire + * dirty set regardless of errors because the fs may + * still have state to clear for each folio. In that + * case we continue processing and return the first error. + */ + if (error == AOP_WRITEPAGE_ACTIVATE) { + folio_unlock(folio); + error = 0; + } else if (wbc->sync_mode != WB_SYNC_ALL) { + wbc->err = error; + wbc->done_index = folio->index + nr; + return writeback_finish(mapping, wbc, true); + } + if (!wbc->err) + wbc->err = error; + } + + /* + * We stop writing back only if we are not doing integrity + * sync. In case of integrity sync we have to keep going until + * we have written all the folios we tagged for writeback prior + * to entering this loop. + */ + wbc->nr_to_write -= nr; + if (wbc->nr_to_write <= 0 && wbc->sync_mode == WB_SYNC_NONE) + return writeback_finish(mapping, wbc, true); + + return writeback_get_folio(mapping, wbc); +} + /** * 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 @@ -2512,49 +2553,11 @@ int write_cache_pages(struct address_space *mapping, for (folio = writeback_iter_init(mapping, wbc); folio; - folio = writeback_get_folio(mapping, wbc)) { - unsigned long nr; - + folio = writeback_iter_next(mapping, wbc, folio, error)) { error = writepage(folio, wbc, data); - nr = folio_nr_pages(folio); - if (unlikely(error)) { - /* - * Handle errors according to the type of - * writeback. There's no need to continue for - * background writeback. Just push done_index - * past this page so media errors won't choke - * writeout for the entire file. For integrity - * writeback, we must process the entire dirty - * set regardless of errors because the fs may - * still have state to clear for each page. In - * that case we continue processing and return - * the first error. - */ - if (error == AOP_WRITEPAGE_ACTIVATE) { - folio_unlock(folio); - error = 0; - } else if (wbc->sync_mode != WB_SYNC_ALL) { - wbc->err = error; - wbc->done_index = folio->index + nr; - return writeback_finish(mapping, wbc, true); - } - if (!wbc->err) - wbc->err = error; - } - - /* - * We stop writing back only if we are not doing - * integrity sync. In case of integrity sync we have to - * keep going until we have written all the pages - * we tagged for writeback prior to entering this loop. - */ - wbc->nr_to_write -= nr; - if (wbc->nr_to_write <= 0 && - wbc->sync_mode == WB_SYNC_NONE) - return writeback_finish(mapping, wbc, true); } - return writeback_finish(mapping, wbc, false); + return wbc->err; } EXPORT_SYMBOL(write_cache_pages);