stable.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [5.15 REGRESSION v2] iomap: Fix inline extent handling in iomap_readpage
@ 2021-11-11 16:17 Andreas Gruenbacher
  2021-11-12  5:44 ` Christoph Hellwig
  2021-11-17  5:33 ` Darrick J. Wong
  0 siblings, 2 replies; 5+ messages in thread
From: Andreas Gruenbacher @ 2021-11-11 16:17 UTC (permalink / raw)
  To: Christoph Hellwig, Darrick J . Wong
  Cc: linux-xfs, linux-fsdevel, cluster-devel, Andreas Gruenbacher, stable

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.

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);
+
+		if (ret < 0)
+			return ret;
+		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


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [5.15 REGRESSION v2] iomap: Fix inline extent handling in iomap_readpage
  2021-11-11 16:17 [5.15 REGRESSION v2] iomap: Fix inline extent handling in iomap_readpage Andreas Gruenbacher
@ 2021-11-12  5:44 ` Christoph Hellwig
  2021-11-17  5:33 ` Darrick J. Wong
  1 sibling, 0 replies; 5+ messages in thread
From: Christoph Hellwig @ 2021-11-12  5:44 UTC (permalink / raw)
  To: Andreas Gruenbacher
  Cc: Christoph Hellwig, Darrick J . Wong, linux-xfs, linux-fsdevel,
	cluster-devel, stable

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.
> 
> To fix readahead as well, change iomap_readahead_iter to pass on
> iomap_readpage_iter return values less than or equal to zero.

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [5.15 REGRESSION v2] iomap: Fix inline extent handling in iomap_readpage
  2021-11-11 16:17 [5.15 REGRESSION v2] iomap: Fix inline extent handling in iomap_readpage Andreas Gruenbacher
  2021-11-12  5:44 ` Christoph Hellwig
@ 2021-11-17  5:33 ` Darrick J. Wong
  2021-11-17  5:52   ` Gao Xiang
  2021-11-17 10:24   ` Andreas Gruenbacher
  1 sibling, 2 replies; 5+ messages in thread
From: Darrick J. Wong @ 2021-11-17  5:33 UTC (permalink / raw)
  To: Andreas Gruenbacher
  Cc: Christoph Hellwig, linux-xfs, linux-fsdevel, cluster-devel, stable

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?

> 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?

> +
> +		if (ret < 0)
> +			return ret;

...and a comment here explaining that we only support inline data that
ends at EOF?

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
> 

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [5.15 REGRESSION v2] iomap: Fix inline extent handling in iomap_readpage
  2021-11-17  5:33 ` Darrick J. Wong
@ 2021-11-17  5:52   ` Gao Xiang
  2021-11-17 10:24   ` Andreas Gruenbacher
  1 sibling, 0 replies; 5+ messages in thread
From: Gao Xiang @ 2021-11-17  5:52 UTC (permalink / raw)
  To: Darrick J. Wong
  Cc: Andreas Gruenbacher, Christoph Hellwig, linux-xfs, linux-fsdevel,
	cluster-devel, stable

On Tue, Nov 16, 2021 at 09:33:30PM -0800, Darrick J. Wong 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?

For erofs, confirmed. (also yes in the long run...)

Thanks,
Gao Xiang

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [5.15 REGRESSION v2] iomap: Fix inline extent handling in iomap_readpage
  2021-11-17  5:33 ` Darrick J. Wong
  2021-11-17  5:52   ` Gao Xiang
@ 2021-11-17 10:24   ` Andreas Gruenbacher
  1 sibling, 0 replies; 5+ messages in thread
From: Andreas Gruenbacher @ 2021-11-17 10:24 UTC (permalink / raw)
  To: Darrick J. Wong
  Cc: Christoph Hellwig, linux-xfs, linux-fsdevel, cluster-devel, stable

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
> >
>


^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2021-11-17 10:25 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-11 16:17 [5.15 REGRESSION v2] iomap: Fix inline extent handling in iomap_readpage Andreas Gruenbacher
2021-11-12  5:44 ` Christoph Hellwig
2021-11-17  5:33 ` Darrick J. Wong
2021-11-17  5:52   ` Gao Xiang
2021-11-17 10:24   ` Andreas Gruenbacher

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).