Re: [PATCH] iomap: Address soft lockup in iomap_finish_ioend()

["Darrick J. Wong" <djwong@kernel.org>]

On Tue, Jan 04, 2022 at 10:14:27AM -0800, hch@infradead.org wrote:
> On Tue, Jan 04, 2022 at 06:08:24PM +0000, Matthew Wilcox wrote:
> > I think it's fine to put in a fix like this now that's readily
> > backportable.  For folios, I can't help but think we want a
> > restructuring to iterate per-extent first, then per-folio and finally
> > per-sector instead of the current model where we iterate per folio,
> > looking up the extent for each sector.
> 
> We don't look up the extent for each sector.  We look up the extent
> once and then add as much of it as we can to the bio until either the
> bio is full or the extent ends.  In the first case we then allocate
> a new bio and add it to the ioend.

Can we track the number of folios that have been bio_add_folio'd to the
iomap_ioend, and make iomap_can_add_to_ioend return false when the
number of folios reaches some threshold?  I think that would solve the
problem of overly large ioends while not splitting folios across ioends
unnecessarily.

As for where to put a cond_resched() call, I think we'd need to change
iomap_ioend_can_merge to avoid merging two ioends if their folio count
exceeds the same(?) threshold, and then one could put the cond_resched
after each iomap_finish_ioend call in iomap_finish_ioends, and declare
that iomap_finish_ioends cannot be called from atomic context.

I forget if anyone ever benchmarked the actual overhead of cond_resched,
but if my dim memory serves, it's not cheap but also not expensive.

Limiting each ioend to (say) 16k folios and not letting small ioends
merge into something bigger than that for the completion seems (to me
anyway) a balance between stalling out on marking pages after huge IOs
vs. losing the ability to coalesce xfs_end_ioend calls when a contiguous
range of file range has been written back but the backing isn't.

<shrug> That's just my ENOCOFFEE reaction, hopefully that wasn't total
nonsense.

--D

> > Particularly for the kind of case Trond is talking about here; when we
> > want to fsync(), as long as the entire folio is Uptodate, we want to
> > write the entire thing back.  Doing it in portions and merging them back
> > together seems like a lot of wasted effort.
> 
> Writing everything together should be the common case.