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

Christoph Hellwig
2024-01-30T14:16:06Z

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.