[PATCH -next] iomap: fix inline data on buffered read

[PATCH -next] iomap: fix inline data on buffered read

[Gao Xiang]

Previously, iomap_readpage_iter() returning 0 would break out of the
loops of iomap_readahead_iter(), which is what iomap_read_inline_data()
relies on.

However, commit d9dc477ff6a2 ("iomap: advance the iter directly on
buffered read") changes this behavior without calling
iomap_iter_advance(), which causes EROFS to get stuck in
iomap_readpage_iter().

It seems iomap_iter_advance() cannot be called in
iomap_read_inline_data() because of the iomap_write_begin() path, so
handle this in iomap_readpage_iter() instead.

Reported-by: Bo Liu <liubo03@inspur.com>
Fixes: d9dc477ff6a2 ("iomap: advance the iter directly on buffered read")
Cc: Brian Foster <bfoster@redhat.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: "Darrick J. Wong" <djwong@kernel.org>
Cc: Christian Brauner <brauner@kernel.org>
Signed-off-by: Gao Xiang <hsiangkao@linux.alibaba.com>
---
 fs/iomap/buffered-io.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
index d52cfdc299c4..2d6e1e3be747 100644
--- a/fs/iomap/buffered-io.c
+++ b/fs/iomap/buffered-io.c
@@ -372,9 +372,15 @@ static int iomap_readpage_iter(struct iomap_iter *iter,
 	struct iomap_folio_state *ifs;
 	size_t poff, plen;
 	sector_t sector;
+	int ret;
 
-	if (iomap->type == IOMAP_INLINE)
-		return iomap_read_inline_data(iter, folio);
+	if (iomap->type == IOMAP_INLINE) {
+		ret = iomap_read_inline_data(iter, folio);
+		if (ret)
+			return ret;
+		plen = length;
+		goto done;
+	}
 
 	/* zero post-eof blocks as the page may be mapped */
 	ifs = ifs_alloc(iter->inode, folio, iter->flags);
-- 
2.43.5

Re: [PATCH -next] iomap: fix inline data on buffered read

[Bo Liu (刘波)-浪潮信息]

[-- Attachment #1: Type: text/plain, Size: 907 bytes --]

>Previously, iomap_readpage_iter() returning 0 would break out of the loops
of
>iomap_readahead_iter(), which is what iomap_read_inline_data() relies on.
>
>However, commit d9dc477ff6a2 ("iomap: advance the iter directly on buffered
>read") changes this behavior without calling iomap_iter_advance(), which
>causes EROFS to get stuck in iomap_readpage_iter().
>
>It seems iomap_iter_advance() cannot be called in
>iomap_read_inline_data() because of the iomap_write_begin() path, so handle
>this in iomap_readpage_iter() instead.
>
>Reported-by: Bo Liu <liubo03@inspur.com>
>Fixes: d9dc477ff6a2 ("iomap: advance the iter directly on buffered read")
>Cc: Brian Foster <bfoster@redhat.com>
>Cc: Christoph Hellwig <hch@lst.de>
>Cc: "Darrick J. Wong" <djwong@kernel.org>
>Cc: Christian Brauner <brauner@kernel.org>
>Signed-off-by: Gao Xiang <hsiangkao@linux.alibaba.com>

Tested-by: Bo Liu <liubo03@inspur.com>

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 3777 bytes --]

Re: [PATCH -next] iomap: fix inline data on buffered read

[Christoph Hellwig]

I'd move the iomap_iter_advance into iomap_read_inline_data, just like
we've pushed it down as far as possible elsewhere, e.g. something like
the patch below.  Although with that having size and length puzzles
me a bit, so maybe someone more familar with the code could figure
out why we need both, how they can be different and either document
or eliminate that.

diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
index d52cfdc299c4..7858c8834144 100644
--- a/fs/iomap/buffered-io.c
+++ b/fs/iomap/buffered-io.c
@@ -332,15 +332,15 @@ struct iomap_readpage_ctx {
  * Only a single IOMAP_INLINE extent is allowed at the end of each file.
  * Returns zero for success to complete the read, or the usual negative errno.
  */
-static int iomap_read_inline_data(const struct iomap_iter *iter,
-		struct folio *folio)
+static int iomap_read_inline_data(struct iomap_iter *iter, struct folio *folio)
 {
 	const struct iomap *iomap = iomap_iter_srcmap(iter);
 	size_t size = i_size_read(iter->inode) - iomap->offset;
+	loff_t length = iomap_length(iter);
 	size_t offset = offset_in_folio(folio, iomap->offset);
 
 	if (folio_test_uptodate(folio))
-		return 0;
+		goto advance;
 
 	if (WARN_ON_ONCE(size > iomap->length))
 		return -EIO;
@@ -349,7 +349,8 @@ static int iomap_read_inline_data(const struct iomap_iter *iter,
 
 	folio_fill_tail(folio, offset, iomap->inline_data, size);
 	iomap_set_range_uptodate(folio, offset, folio_size(folio) - offset);
-	return 0;
+advance:
+	return iomap_iter_advance(iter, &length);
 }
 
 static inline bool iomap_block_needs_zeroing(const struct iomap_iter *iter,

Re: [PATCH -next] iomap: fix inline data on buffered read

[Christoph Hellwig]

On Wed, Mar 19, 2025 at 09:17:30AM +0100, Christoph Hellwig wrote:
> I'd move the iomap_iter_advance into iomap_read_inline_data, just like
> we've pushed it down as far as possible elsewhere, e.g. something like
> the patch below.  Although with that having size and length puzzles
> me a bit, so maybe someone more familar with the code could figure
> out why we need both, how they can be different and either document
> or eliminate that.

... and this doesn't even compile because it breaks write_begin.
So we'll need to keep it in the caller, but maybe without the
goto and just do the plain advance on length?

Re: [PATCH -next] iomap: fix inline data on buffered read

[Gao Xiang]

Hi Christoph,

On 2025/3/19 16:23, Christoph Hellwig wrote:
> On Wed, Mar 19, 2025 at 09:17:30AM +0100, Christoph Hellwig wrote:
>> I'd move the iomap_iter_advance into iomap_read_inline_data, just like
>> we've pushed it down as far as possible elsewhere, e.g. something like
>> the patch below.  Although with that having size and length puzzles
>> me a bit, so maybe someone more familar with the code could figure
>> out why we need both, how they can be different and either document
>> or eliminate that.
> 
> ... and this doesn't even compile because it breaks write_begin.
> So we'll need to keep it in the caller, but maybe without the
> goto and just do the plain advance on length?

Yeah, I was just writing an email to your previous reply:

I think iomap_write_begin_inline() will break if
iomap_iter_advance() is in iomap_read_inline_data().

Because:
   iomap_write_iter
      iomap_write_begin
        iomap_write_begin_inline
          iomap_read_inline_data
             iomap_iter_advance		# 1
      copy_folio_from_iter_atomic
      iomap_write_end
      ...
      iomap_iter_advance			# 1

I will do a plain advance as your suggested instead, but commit
"iomap: advance the iter directly on buffered read" makes EROFS
unusable, and I think gfs2 too.  It needs be fixed now.

Thanks,
Gao Xiang

Thanks,
Gao Xiang