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

[Christoph Hellwig]

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.