[PATCH] iomap: fix iomap_read_end() for already uptodate folios

[PATCH] iomap: fix iomap_read_end() for already uptodate folios

[Joanne Koong]

There are some cases where when iomap_read_end() is called, the folio
may already have been marked uptodate. For example, if the iomap block
needed zeroing, then the folio may have been marked uptodate after the
zeroing.

iomap_read_end() should unlock the folio instead of calling
folio_end_read(), which is how these cases were handled prior to commit
f8eaf79406fe ("iomap: simplify ->read_folio_range() error handling for
reads"). Calling folio_end_read() on an uptodate folio leads to buggy
behavior where marking an already uptodate folio as uptodate will XOR it
to be marked nonuptodate.

Fixes: f8eaf79406fe ("iomap: simplify ->read_folio_range() error handling for reads")
Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
Reported-by: Matthew Wilcox <willy@infradead.org>
---
This is a fix for commit f8eaf79406fe in the 'vfs-6.19.iomap' branch. It
would be great if this could get folded up into that original commit, if it's
not too late to do so.

Thanks,
Joanne
---
 fs/iomap/buffered-io.c | 37 +++++++++++++++++++------------------
 1 file changed, 19 insertions(+), 18 deletions(-)

diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
index 0475d949e5a0..a5d6e838b801 100644
--- a/fs/iomap/buffered-io.c
+++ b/fs/iomap/buffered-io.c
@@ -455,29 +455,30 @@ static void iomap_read_end(struct folio *folio, size_t bytes_submitted)
 
 	if (ifs) {
 		bool end_read, uptodate;
+		size_t bytes_not_submitted;
 
 		spin_lock_irq(&ifs->state_lock);
 		if (!ifs->read_bytes_pending) {
 			WARN_ON_ONCE(bytes_submitted);
-			end_read = true;
-		} else {
-			/*
-			 * Subtract any bytes that were initially accounted to
-			 * read_bytes_pending but skipped for IO. The +1
-			 * accounts for the bias we added in iomap_read_init().
-			 */
-			size_t bytes_not_submitted = folio_size(folio) + 1 -
-					bytes_submitted;
-			ifs->read_bytes_pending -= bytes_not_submitted;
-			/*
-			 * If !ifs->read_bytes_pending, this means all pending
-			 * reads by the IO helper have already completed, which
-			 * means we need to end the folio read here. If
-			 * ifs->read_bytes_pending != 0, the IO helper will end
-			 * the folio read.
-			 */
-			end_read = !ifs->read_bytes_pending;
+			spin_unlock_irq(&ifs->state_lock);
+			folio_unlock(folio);
+			return;
 		}
+
+		/*
+		 * Subtract any bytes that were initially accounted to
+		 * read_bytes_pending but skipped for IO. The +1 accounts for
+		 * the bias we added in iomap_read_init().
+		 */
+		bytes_not_submitted = folio_size(folio) + 1 - bytes_submitted;
+		ifs->read_bytes_pending -= bytes_not_submitted;
+		/*
+		 * If !ifs->read_bytes_pending, this means all pending reads by
+		 * the IO helper have already completed, which means we need to
+		 * end the folio read here. If ifs->read_bytes_pending != 0,
+		 * the IO helper will end the folio read.
+		 */
+		end_read = !ifs->read_bytes_pending;
 		if (end_read)
 			uptodate = ifs_is_fully_uptodate(folio, ifs);
 		spin_unlock_irq(&ifs->state_lock);
-- 
2.47.3

Re: [PATCH] iomap: fix iomap_read_end() for already uptodate folios

[Christoph Hellwig]

On Mon, Nov 17, 2025 at 04:44:21PM -0800, Joanne Koong wrote:
> There are some cases where when iomap_read_end() is called, the folio
> may already have been marked uptodate. For example, if the iomap block
> needed zeroing, then the folio may have been marked uptodate after the
> zeroing.
> 
> iomap_read_end() should unlock the folio instead of calling
> folio_end_read(), which is how these cases were handled prior to commit
> f8eaf79406fe ("iomap: simplify ->read_folio_range() error handling for
> reads"). Calling folio_end_read() on an uptodate folio leads to buggy
> behavior where marking an already uptodate folio as uptodate will XOR it
> to be marked nonuptodate.
> 
> Fixes: f8eaf79406fe ("iomap: simplify ->read_folio_range() error handling for reads")
> Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> Reported-by: Matthew Wilcox <willy@infradead.org>
> ---
> This is a fix for commit f8eaf79406fe in the 'vfs-6.19.iomap' branch. It
> would be great if this could get folded up into that original commit, if it's
> not too late to do so.
> 
> Thanks,
> Joanne
> ---
>  fs/iomap/buffered-io.c | 37 +++++++++++++++++++------------------
>  1 file changed, 19 insertions(+), 18 deletions(-)
> 
> diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
> index 0475d949e5a0..a5d6e838b801 100644
> --- a/fs/iomap/buffered-io.c
> +++ b/fs/iomap/buffered-io.c
> @@ -455,29 +455,30 @@ static void iomap_read_end(struct folio *folio, size_t bytes_submitted)
>  
>  	if (ifs) {
>  		bool end_read, uptodate;
> +		size_t bytes_not_submitted;
>  
>  		spin_lock_irq(&ifs->state_lock);
>  		if (!ifs->read_bytes_pending) {
>  			WARN_ON_ONCE(bytes_submitted);
> +			spin_unlock_irq(&ifs->state_lock);
> +			folio_unlock(folio);
> +			return;
>  		}
> +
> +		/*
> +		 * Subtract any bytes that were initially accounted to
> +		 * read_bytes_pending but skipped for IO. The +1 accounts for
> +		 * the bias we added in iomap_read_init().
> +		 */
> +		bytes_not_submitted = folio_size(folio) + 1 - bytes_submitted;
> +		ifs->read_bytes_pending -= bytes_not_submitted;
> +		/*

Very nitpicky comment:  I'd do away with the bytes_not_submitted
variable:

		ifs->read_bytes_pending -=
			(folio_size(folio) + 1 - bytes_submitted);

and keep an empty line before the start of the next block comment after
this.

Otherwise looks good;

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

Re: [PATCH] iomap: fix iomap_read_end() for already uptodate folios

[Matthew Wilcox]

On Mon, Nov 17, 2025 at 04:44:21PM -0800, Joanne Koong wrote:
> There are some cases where when iomap_read_end() is called, the folio
> may already have been marked uptodate. For example, if the iomap block
> needed zeroing, then the folio may have been marked uptodate after the
> zeroing.
> 
> iomap_read_end() should unlock the folio instead of calling
> folio_end_read(), which is how these cases were handled prior to commit
> f8eaf79406fe ("iomap: simplify ->read_folio_range() error handling for
> reads"). Calling folio_end_read() on an uptodate folio leads to buggy
> behavior where marking an already uptodate folio as uptodate will XOR it
> to be marked nonuptodate.
> 
> Fixes: f8eaf79406fe ("iomap: simplify ->read_folio_range() error handling for reads")
> Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> Reported-by: Matthew Wilcox <willy@infradead.org>

Tested-by: Matthew Wilcox (Oracle) <willy@infradead.org>

At least, it gets past generic/008.  I'll let the xfstests run continue;
no news is good news.

Re: [PATCH] iomap: fix iomap_read_end() for already uptodate folios

[Joanne Koong]

On Mon, Nov 17, 2025 at 9:51 PM Christoph Hellwig <hch@infradead.org> wrote:
>
> On Mon, Nov 17, 2025 at 04:44:21PM -0800, Joanne Koong wrote:
> > There are some cases where when iomap_read_end() is called, the folio
> > may already have been marked uptodate. For example, if the iomap block
> > needed zeroing, then the folio may have been marked uptodate after the
> > zeroing.
> >
> > iomap_read_end() should unlock the folio instead of calling
> > folio_end_read(), which is how these cases were handled prior to commit
> > f8eaf79406fe ("iomap: simplify ->read_folio_range() error handling for
> > reads"). Calling folio_end_read() on an uptodate folio leads to buggy
> > behavior where marking an already uptodate folio as uptodate will XOR it
> > to be marked nonuptodate.
> >
> > Fixes: f8eaf79406fe ("iomap: simplify ->read_folio_range() error handling for reads")
> > Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> > Reported-by: Matthew Wilcox <willy@infradead.org>
> > ---
> > This is a fix for commit f8eaf79406fe in the 'vfs-6.19.iomap' branch. It
> > would be great if this could get folded up into that original commit, if it's
> > not too late to do so.
> >
> > Thanks,
> > Joanne
> > ---
> >  fs/iomap/buffered-io.c | 37 +++++++++++++++++++------------------
> >  1 file changed, 19 insertions(+), 18 deletions(-)
> >
> > diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
> > index 0475d949e5a0..a5d6e838b801 100644
> > --- a/fs/iomap/buffered-io.c
> > +++ b/fs/iomap/buffered-io.c
> > @@ -455,29 +455,30 @@ static void iomap_read_end(struct folio *folio, size_t bytes_submitted)
> >
> >       if (ifs) {
> >               bool end_read, uptodate;
> > +             size_t bytes_not_submitted;
> >
> >               spin_lock_irq(&ifs->state_lock);
> >               if (!ifs->read_bytes_pending) {
> >                       WARN_ON_ONCE(bytes_submitted);
> > +                     spin_unlock_irq(&ifs->state_lock);
> > +                     folio_unlock(folio);
> > +                     return;
> >               }
> > +
> > +             /*
> > +              * Subtract any bytes that were initially accounted to
> > +              * read_bytes_pending but skipped for IO. The +1 accounts for
> > +              * the bias we added in iomap_read_init().
> > +              */
> > +             bytes_not_submitted = folio_size(folio) + 1 - bytes_submitted;
> > +             ifs->read_bytes_pending -= bytes_not_submitted;
> > +             /*
>
> Very nitpicky comment:  I'd do away with the bytes_not_submitted
> variable:
>
>                 ifs->read_bytes_pending -=
>                         (folio_size(folio) + 1 - bytes_submitted);
>
> and keep an empty line before the start of the next block comment after
> this.

I'll submit v2 today with these changes. Thanks for reviewing this.

>
> Otherwise looks good;
>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
>