| Message ID | 20231218153553.807799-14-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:49, Christoph Hellwig wrote: > From: "Matthew Wilcox (Oracle)" <willy@infradead.org> > > Move the loop for should-we-write-this-folio to its own function. > > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> > Signed-off-by: Christoph Hellwig <hch@lst.de> Looks good to me. Feel free to add: Reviewed-by: Jan Kara <jack@suse.cz> But I'd note that the call stack depth of similarly called helper functions (with more to come later in the series) is getting a bit confusing. Maybe we should inline writeback_get_next() into its single caller writeback_get_folio() to reduce confusion a bit... Honza > +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; > + 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) > { > @@ -2455,7 +2474,7 @@ static struct folio *writeback_iter_init(struct address_space *mapping, > > wbc->err = 0; > folio_batch_init(&wbc->fbatch); > - return writeback_get_next(mapping, wbc); > + return writeback_get_folio(mapping, wbc); > } > > /** > @@ -2498,17 +2517,9 @@ int write_cache_pages(struct address_space *mapping, > > for (folio = writeback_iter_init(mapping, wbc); > folio; > - folio = writeback_get_next(mapping, wbc)) { > + folio = writeback_get_folio(mapping, wbc)) { > unsigned long nr; > > - folio_lock(folio); > - if (!should_writeback_folio(mapping, wbc, folio)) { > - folio_unlock(folio); > - continue; > - } > - > - trace_wbc_writepage(wbc, inode_to_bdi(mapping->host)); > - > error = writepage(folio, wbc, data); > nr = folio_nr_pages(folio); > wbc->nr_to_write -= nr; > -- > 2.39.2 >
On Thu, Dec 21, 2023 at 12:41:53PM +0100, Jan Kara wrote: > But I'd note that the call stack depth of similarly called helper functions > (with more to come later in the series) is getting a bit confusing. Maybe > we should inline writeback_get_next() into its single caller > writeback_get_folio() to reduce confusion a bit... I just hacked that up based on the fully applied series and that looks good to me.
On Thu 21-12-23 13:25:35, Christoph Hellwig wrote: > On Thu, Dec 21, 2023 at 12:41:53PM +0100, Jan Kara wrote: > > But I'd note that the call stack depth of similarly called helper functions > > (with more to come later in the series) is getting a bit confusing. Maybe > > we should inline writeback_get_next() into its single caller > > writeback_get_folio() to reduce confusion a bit... > > I just hacked that up based on the fully applied series and that looks > good to me. Yeah, cleanup on top works for me so that you don't have to rebase. Honza
diff --git a/mm/page-writeback.c b/mm/page-writeback.c index efcfffa800884d..9d37dd5e58ffb6 100644 --- a/mm/page-writeback.c +++ b/mm/page-writeback.c @@ -2442,6 +2442,25 @@ 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; + 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) { @@ -2455,7 +2474,7 @@ static struct folio *writeback_iter_init(struct address_space *mapping, wbc->err = 0; folio_batch_init(&wbc->fbatch); - return writeback_get_next(mapping, wbc); + return writeback_get_folio(mapping, wbc); } /** @@ -2498,17 +2517,9 @@ int write_cache_pages(struct address_space *mapping, for (folio = writeback_iter_init(mapping, wbc); folio; - folio = writeback_get_next(mapping, wbc)) { + folio = writeback_get_folio(mapping, wbc)) { unsigned long nr; - folio_lock(folio); - if (!should_writeback_folio(mapping, wbc, folio)) { - folio_unlock(folio); - continue; - } - - trace_wbc_writepage(wbc, inode_to_bdi(mapping->host)); - error = writepage(folio, wbc, data); nr = folio_nr_pages(folio); wbc->nr_to_write -= nr;