On Wed, Jan 05, 2022 at 05:43:54AM -0800, hch@xxxxxxxxxxxxx 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@xxxxxxxxxxxxx
