| Message ID | 20230626173521.459345-9-willy@infradead.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Convert write_cache_pages() to an iterator | expand |
> + for (;;) { > + folio = writeback_get_next(mapping, wbc); > + if (!folio) > + return 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)); > + return folio; Same minor nitpick, why not: while ((folio = writeback_get_next(mapping, wbc)) { wbc->done_index = folio->index; folio_lock(folio); if (likely(should_writeback_folio(mapping, wbc, folio))) { trace_wbc_writepage(wbc, inode_to_bdi(mapping->host)); break; } folio_unlock(folio); } return folio; ?
On Mon, Jun 26, 2023 at 09:34:17PM -0700, Christoph Hellwig wrote: > > + for (;;) { > > + folio = writeback_get_next(mapping, wbc); > > + if (!folio) > > + return 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)); > > + return folio; > > Same minor nitpick, why not: > > > while ((folio = writeback_get_next(mapping, wbc)) { > wbc->done_index = folio->index; > > folio_lock(folio); > if (likely(should_writeback_folio(mapping, wbc, folio))) { > trace_wbc_writepage(wbc, inode_to_bdi(mapping->host)); > break; > } > folio_unlock(folio); > } > > return folio; > > ? Because we end up having to call writeback_finish() somewhere, and it's neater to do it here than in either writeback_get_next() or both callers of writeback_get_folio().
diff --git a/mm/page-writeback.c b/mm/page-writeback.c index 18f332611a52..659df2b5c7c0 100644 --- a/mm/page-writeback.c +++ b/mm/page-writeback.c @@ -2430,6 +2430,27 @@ static bool should_writeback_folio(struct address_space *mapping, return true; } +static struct folio *writeback_get_folio(struct address_space *mapping, + struct writeback_control *wbc) +{ + struct folio *folio; + + for (;;) { + folio = writeback_get_next(mapping, wbc); + if (!folio) + return 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)); + return folio; +} + static struct folio *writeback_iter_init(struct address_space *mapping, struct writeback_control *wbc) { @@ -2449,7 +2470,7 @@ static struct folio *writeback_iter_init(struct address_space *mapping, folio_batch_init(&wbc->fbatch); wbc->err = 0; - return writeback_get_next(mapping, wbc); + return writeback_get_folio(mapping, wbc); } /** @@ -2492,17 +2513,7 @@ int write_cache_pages(struct address_space *mapping, for (folio = writeback_iter_init(mapping, wbc); folio; - folio = writeback_get_next(mapping, wbc)) { - wbc->done_index = folio->index; - - folio_lock(folio); - if (!should_writeback_folio(mapping, wbc, folio)) { - folio_unlock(folio); - continue; - } - - trace_wbc_writepage(wbc, inode_to_bdi(mapping->host)); - + folio = writeback_get_folio(mapping, wbc)) { error = writepage(folio, wbc, data); if (unlikely(error)) { /*
Move the loop for should-we-write-this-folio to its own function. Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> --- mm/page-writeback.c | 35 +++++++++++++++++++++++------------ 1 file changed, 23 insertions(+), 12 deletions(-)