Re: [5.15 REGRESSION v2] iomap: Fix inline extent handling in iomap_readpage

[Andreas Gruenbacher <agruenba@redhat.com>]

On Wed, Nov 17, 2021 at 6:33 AM Darrick J. Wong <djwong@kernel.org> wrote:
> On Thu, Nov 11, 2021 at 05:17:14PM +0100, Andreas Gruenbacher wrote:
> > Before commit 740499c78408 ("iomap: fix the iomap_readpage_actor return
> > value for inline data"), when hitting an IOMAP_INLINE extent,
> > iomap_readpage_actor would report having read the entire page.  Since
> > then, it only reports having read the inline data (iomap->length).
> >
> > This will force iomap_readpage into another iteration, and the
> > filesystem will report an unaligned hole after the IOMAP_INLINE extent.
> > But iomap_readpage_actor (now iomap_readpage_iter) isn't prepared to
> > deal with unaligned extents, it will get things wrong on filesystems
> > with a block size smaller than the page size, and we'll eventually run
> > into the following warning in iomap_iter_advance:
> >
> >   WARN_ON_ONCE(iter->processed > iomap_length(iter));
> >
> > Fix that by changing iomap_readpage_iter to return 0 when hitting an
> > inline extent; this will cause iomap_iter to stop immediately.
>
> I guess this means that we also only support having inline data that
> ends at EOF?  IIRC this is true for the three(?) filesystems that have
> expressed any interest in inline data: yours, ext4, and erofs?

Yes.

> > To fix readahead as well, change iomap_readahead_iter to pass on
> > iomap_readpage_iter return values less than or equal to zero.
> >
> > Fixes: 740499c78408 ("iomap: fix the iomap_readpage_actor return value for inline data")
> > Cc: stable@vger.kernel.org # v5.15+
> > Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
> > ---
> >  fs/iomap/buffered-io.c | 11 +++++++++--
> >  1 file changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
> > index 1753c26c8e76..fe10d8a30f6b 100644
> > --- a/fs/iomap/buffered-io.c
> > +++ b/fs/iomap/buffered-io.c
> > @@ -256,8 +256,13 @@ static loff_t iomap_readpage_iter(const struct iomap_iter *iter,
> >       unsigned poff, plen;
> >       sector_t sector;
> >
> > -     if (iomap->type == IOMAP_INLINE)
> > -             return min(iomap_read_inline_data(iter, page), length);
> > +     if (iomap->type == IOMAP_INLINE) {
> > +             loff_t ret = iomap_read_inline_data(iter, page);
>
> Ew, iomap_read_inline_data returns loff_t.  I think I'll slip in a
> change of return type to ssize_t, if you don't mind?

Really?

> > +
> > +             if (ret < 0)
> > +                     return ret;
>
> ...and a comment here explaining that we only support inline data that
> ends at EOF?

I'll send a separate patch that adds a description for
iomap_read_inline_data and cleans up its return value.

Thanks,
Andreas

> If the answers to all /four/ questions are 'yes', then consider this:
> Reviewed-by: Darrick J. Wong <djwong@kernel.org>
>
> --D
>
> > +             return 0;
> > +     }
> >
> >       /* zero post-eof blocks as the page may be mapped */
> >       iop = iomap_page_create(iter->inode, page);
> > @@ -370,6 +375,8 @@ static loff_t iomap_readahead_iter(const struct iomap_iter *iter,
> >                       ctx->cur_page_in_bio = false;
> >               }
> >               ret = iomap_readpage_iter(iter, ctx, done);
> > +             if (ret <= 0)
> > +                     return ret;
> >       }
> >
> >       return done;
> > --
> > 2.31.1
> >
>