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

[Dave Chinner <david@fromorbit.com>]

On Wed, Jan 05, 2022 at 05:43:54AM -0800, hch@infradead.org wrote:
> On Wed, Jan 05, 2022 at 08:16:05AM +1100, Dave Chinner wrote:
> > > > +	if (ioend->io_inline_bio.bi_iter.bi_sector + (ioend->io_size >> 9) !=
> > > 
> > > This open codes bio_end_sector()
> > 
> > No, it doesn't. The ioend can have chained bios or have others merged
> > and concatenated to the ioend->io_list, so ioend->io_size != length
> > of the first bio in the chain....
> > 
> > > > +	    next->io_inline_bio.bi_iter.bi_sector)
> > > 
> > > But more importantly I don't think just using the inline_bio makes sense
> > > here as the ioend can have multiple bios.  Fortunately we should always
> > > have the last built bio available in ->io_bio.
> > 
> > Except merging chains ioends and modifies the head io_size to
> > account for the chained ioends we add to ioend->io_list. Hence
> > ioend->io_bio is not the last bio in a contiguous ioend chain.
> 
> Indeed.  We could use bio_end_sector on io_bio or this.

Not after we merge the first two contiguous ioends:

Before first merge:

ioend.io_inline_bio.bi_sector	= X
ioend.io_size			= A
bio_end_sector(ioend.io_bio)    = X + A		<<<< correct
ioend.io_list			= <empty>

After first merge:

ioend.io_inline_bio.bi_sector	= X
ioend.io_size			= A + B
bio_end_sector(ioend.io_bio)    = X + A		<<<<<<<< wrong
ioend.io_list			= <merged ioend B,
				   bi_sector	= X + A,
				   io_size	= B,
	correct >>>>>>>		   bio_end_sector() = X + A + B>


Hence if we want to use bio_end_sector(), we've got to jump through
hoops to get to the end of the ioend->io_list to get the io_bio from
that ioend. i.e:

	if (!list_empty(ioend->io_list)) {
		struct iomap_ioend *last = list_last_entry(&ioend->io_list, ...); 

		if (bio_end_sector(last->io_bio) !=
		    next->io_inline_bio.bi_iter.bi_sector)
			return false;
	}
	return true;

That much more opaque than just using bi_sector and ioend->io_size
to directly calculate the last sector of the contiguous ioend chain.
I much prefer the simple, obvious direct ioend maths compared to
having to remember exactly how the the io_list is structured every
time I need to understand what the merging constraints are....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com