Re: [PATCH 19/19] writeback: simplify writeback iteration

[Jan Kara <jack@suse.cz>]

On Tue 30-01-24 15:16:01, 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.

OK.

> > > +		 */
> > > +		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.

Well, batch release needs to be only here because if writeback_get_folio()
returns NULL, the batch has been already released by it. So what would be
duplicated is only the error assignment. But I'm fine with the version in
the following email and actually somewhat prefer it compared the yet
another variant you've sent.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR