| Message ID | 20250319025953.3559299-1-hsiangkao@linux.alibaba.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [-next] iomap: fix inline data on buffered read | expand |
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,
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?
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
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);
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(-)