| Message ID | 20250319085125.4039368-1-hsiangkao@linux.alibaba.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] iomap: fix inline data on buffered read | expand |
On Wed, Mar 19, 2025 at 04:51:25PM +0800, Gao Xiang wrote: > 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-and-tested-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> > --- Ugh. I'd hoped ext4 testing would have uncovered such an issue, but now that I think of it, IIRC ext4 isn't fully on iomap yet so wouldn't have provided this coverage. So apologies for the testing oversight on my part and thanks for the fix. For future reference, do you guys have any documentation or whatever to run quick/smoke fstests against EROFS? (I assume this could be reproduced via fstests..?). > v1: https://lore.kernel.org/r/20250319025953.3559299-1-hsiangkao@linux.alibaba.com > change since v1: > - iomap_iter_advance() directly instead of `goto done` > as suggested by Christoph. > > fs/iomap/buffered-io.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c > index d52cfdc299c4..814b7f679486 100644 > --- a/fs/iomap/buffered-io.c > +++ b/fs/iomap/buffered-io.c > @@ -372,9 +372,14 @@ 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; > + return iomap_iter_advance(iter, &length); > + } Technically you could use iomap_iter_advance_full() here, but since length is already defined for other purposes it doesn't really make much difference. LGTM: Reviewed-by: Brian Foster <bfoster@redhat.com> > > /* zero post-eof blocks as the page may be mapped */ > ifs = ifs_alloc(iter->inode, folio, iter->flags); > -- > 2.43.5 >
Hi Brian, On 2025/3/19 19:28, Brian Foster wrote: > On Wed, Mar 19, 2025 at 04:51:25PM +0800, Gao Xiang wrote: >> 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-and-tested-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> >> --- > > Ugh. I'd hoped ext4 testing would have uncovered such an issue, but now > that I think of it, IIRC ext4 isn't fully on iomap yet so wouldn't have > provided this coverage. So apologies for the testing oversight on my > part and thanks for the fix. > > For future reference, do you guys have any documentation or whatever to > run quick/smoke fstests against EROFS? (I assume this could be > reproduced via fstests..?). I don't think any existing testcase of fstests is useful for readonly filesystems like EROFS since EROFS only has read interface so all test cases including regression tests will be integrated into erofs-utils directly. EROFS can be easily tested with its own testcases in erofs-utils: git clone git://git.kernel.org/pub/scm/linux/kernel/git/xiang/erofs-utils.git cd erofs-utils git checkout origin/experimental-tests # for now, I will integrate to main later. ./autogen.sh ./configure make check Without this patch, test cases will just hang. > ... > > Reviewed-by: Brian Foster <bfoster@redhat.com> Thanks. Thanks, Gao Xiang > >> >> /* zero post-eof blocks as the page may be mapped */ >> ifs = ifs_alloc(iter->inode, folio, iter->flags); >> -- >> 2.43.5 >>
On Wed, 19 Mar 2025 16:51:25 +0800, Gao Xiang wrote: > 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(). > > [...] Applied to the vfs-6.15.iomap branch of the vfs/vfs.git tree. Patches in the vfs-6.15.iomap branch should appear in linux-next soon. Please report any outstanding bugs that were missed during review in a new review to the original patch series allowing us to drop it. It's encouraged to provide Acked-bys and Reviewed-bys even though the patch has now been applied. If possible patch trailers will be updated. Note that commit hashes shown below are subject to change due to rebase, trailer updates or similar. If in doubt, please check the listed branch. tree: https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git branch: vfs-6.15.iomap [1/1] iomap: fix inline data on buffered read https://git.kernel.org/vfs/vfs/c/b26816b4e320
Looks good:
Reviewed-by: Christoph Hellwig <hch@lst.de>
diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c index d52cfdc299c4..814b7f679486 100644 --- a/fs/iomap/buffered-io.c +++ b/fs/iomap/buffered-io.c @@ -372,9 +372,14 @@ 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; + return iomap_iter_advance(iter, &length); + } /* 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-and-tested-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> --- v1: https://lore.kernel.org/r/20250319025953.3559299-1-hsiangkao@linux.alibaba.com change since v1: - iomap_iter_advance() directly instead of `goto done` as suggested by Christoph. fs/iomap/buffered-io.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)