On Wed, Nov 17, 2021 at 6:33 AM Darrick J. Wong <[email protected]> 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: [email protected] # v5.15+
> > Signed-off-by: Andreas Gruenbacher <[email protected]>
> > ---
> > 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 <[email protected]>
>
> --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
> >
>
