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. > > + struct folio *folio = 0; > ^^ NULL please Fixed. > > ret = writeback_use_writepage(mapping, wbc); > > + if (!ret) > > + ret = wbc->err; > > AFAICT this should not be needed as writeback_iter() made sure wbc->err is > returned when set? Heh. That's a leftover from my above mentioned different attempt at error handling and shouldn't have stayed in.
