On Tue, Jan 30, 2024 at 03:16:01PM +0100, Christoph Hellwig wrote:
> On Tue, Jan 30, 2024 at 11:46:05AM +0100, Jan Kara wrote:
> > Looking at it now I'm thinking whether we would not be better off to
> > completely dump the 'error' argument of writeback_iter() /
> > writeback_iter_next() and just make all .writepage implementations set
> > wbc->err directly. But that means touching all the ~20 writepage
> > implementations we still have...
>
> Heh. I actually had an earlier version that looked at wbc->err in
> the ->writepages callers. But it felt a bit too ugly.
>
> > > + */
> > > + if (wbc->sync_mode == WB_SYNC_NONE &&
> > > + (wbc->err || wbc->nr_to_write <= 0))
> > > + goto finish;
> >
> > I think it would be a bit more comprehensible if we replace the goto with:
> > folio_batch_release(&wbc->fbatch);
> > if (wbc->range_cyclic)
> > mapping->writeback_index =
> > folio->index + folio_nr_pages(folio);
> > *error = wbc->err;
> > return NULL;
>
> I agree that keeping the logic on when to break and when to set the
> writeback_index is good, but duplicating the batch release and error
> assignment seems a bit suboptimal. Let me know what you think of the
> alternatіve variant below.
And now for real:
diff --git a/mm/page-writeback.c b/mm/page-writeback.c
index d8fcbac2d72310..3e4aa5bfe75819 100644
--- a/mm/page-writeback.c
+++ b/mm/page-writeback.c
@@ -2475,10 +2475,23 @@ struct folio *writeback_iter(struct address_space *mapping,
* For background writeback just push done_index past this folio
* so that we can just restart where we left off and media
* errors won't choke writeout for the entire file.
+ *
+ * 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->sync_mode == WB_SYNC_NONE &&
- (wbc->err || wbc->nr_to_write <= 0))
+ (wbc->err || wbc->nr_to_write <= 0)) {
+ if (wbc->range_cyclic)
+ mapping->writeback_index =
+ folio->index + folio_nr_pages(folio);
goto finish;
+ }
} else {
if (wbc->range_cyclic)
wbc->index = mapping->writeback_index; /* prev offset */
@@ -2492,31 +2505,15 @@ struct folio *writeback_iter(struct address_space *mapping,
}
folio = writeback_get_folio(mapping, wbc);
- if (!folio)
+ if (!folio) {
+ if (wbc->range_cyclic)
+ mapping->writeback_index = 0;
goto finish;
+ }
return folio;
finish:
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) {
- WARN_ON_ONCE(wbc->sync_mode != WB_SYNC_NONE);
- if (wbc->err || wbc->nr_to_write <= 0)
- mapping->writeback_index =
- folio->index + folio_nr_pages(folio);
- else
- mapping->writeback_index = 0;
- }
*error = wbc->err;
return NULL;
}
