| Message ID | 20210330134537.423447-1-joel@jms.id.au |
|---|---|
| State | Accepted |
| Delegated to: | Richard Weinberger |
| Headers | show |
| Series | jffs2: Hook up splice_write callback | expand |
On Wed, Mar 31, 2021 at 12:15:37AM +1030, Joel Stanley wrote: > overlayfs using jffs2 as the upper filesystem would fail in some cases > since moving to v5.10. The test case used was to run 'touch' on a file > that exists in the lower fs, causing the modification time to be > updated. It returns EINVAL when the bug is triggered. > > A bisection showed this was introduced in v5.9-rc1, with commit > 36e2c7421f02 ("fs: don't allow splice read/write without explicit ops"). > Reverting that commit restores the expected behaviour. > > Some digging showed that this was due to jffs2 lacking an implementation > of splice_write. (For unknown reasons the warn_unsupported that should > trigger was not displaying any output). > > Adding this patch resolved the issue and the test now passes. Looks good: Reviewed-by: Christoph Hellwig <hch@lst.de>
On Tue, Mar 30, 2021 at 06:17:15PM +0200, Christoph Hellwig wrote: > On Wed, Mar 31, 2021 at 12:15:37AM +1030, Joel Stanley wrote: > > overlayfs using jffs2 as the upper filesystem would fail in some cases > > since moving to v5.10. The test case used was to run 'touch' on a file > > that exists in the lower fs, causing the modification time to be > > updated. It returns EINVAL when the bug is triggered. > > > > A bisection showed this was introduced in v5.9-rc1, with commit > > 36e2c7421f02 ("fs: don't allow splice read/write without explicit ops"). > > Reverting that commit restores the expected behaviour. > > > > Some digging showed that this was due to jffs2 lacking an implementation > > of splice_write. (For unknown reasons the warn_unsupported that should > > trigger was not displaying any output). > > > > Adding this patch resolved the issue and the test now passes. > > Looks good: > > Reviewed-by: Christoph Hellwig <hch@lst.de> The same goes for quite a few other filesystems, actually - at least adfs, affs, bfs, hfs, hfsplus, hostfs, hpfs, minix, omfs, sysv, ufs and vboxsf are in the same boat, and I suspect that ecryptfs and ntfs might be too. Christoph, do you see any problems with doing the same thing for that bunch as well?
On Tue, Mar 30, 2021 at 06:31:00PM +0000, Al Viro wrote: > On Tue, Mar 30, 2021 at 06:17:15PM +0200, Christoph Hellwig wrote: > > On Wed, Mar 31, 2021 at 12:15:37AM +1030, Joel Stanley wrote: > > > overlayfs using jffs2 as the upper filesystem would fail in some cases > > > since moving to v5.10. The test case used was to run 'touch' on a file > > > that exists in the lower fs, causing the modification time to be > > > updated. It returns EINVAL when the bug is triggered. > > > > > > A bisection showed this was introduced in v5.9-rc1, with commit > > > 36e2c7421f02 ("fs: don't allow splice read/write without explicit ops"). > > > Reverting that commit restores the expected behaviour. > > > > > > Some digging showed that this was due to jffs2 lacking an implementation > > > of splice_write. (For unknown reasons the warn_unsupported that should > > > trigger was not displaying any output). > > > > > > Adding this patch resolved the issue and the test now passes. > > > > Looks good: > > > > Reviewed-by: Christoph Hellwig <hch@lst.de> > > The same goes for quite a few other filesystems, actually - at least > adfs, affs, bfs, hfs, hfsplus, hostfs, hpfs, minix, omfs, sysv, ufs > and vboxsf are in the same boat, and I suspect that ecryptfs and ntfs > might be too. > > Christoph, do you see any problems with doing the same thing for that > bunch as well? coda and udf as well, by the look of it...
On Tue, Mar 30, 2021 at 9:45 PM Joel Stanley <joel@jms.id.au> wrote: > > overlayfs using jffs2 as the upper filesystem would fail in some cases > since moving to v5.10. The test case used was to run 'touch' on a file > that exists in the lower fs, causing the modification time to be > updated. It returns EINVAL when the bug is triggered. > > A bisection showed this was introduced in v5.9-rc1, with commit > 36e2c7421f02 ("fs: don't allow splice read/write without explicit ops"). > Reverting that commit restores the expected behaviour. > > Some digging showed that this was due to jffs2 lacking an implementation > of splice_write. (For unknown reasons the warn_unsupported that should > trigger was not displaying any output). > > Adding this patch resolved the issue and the test now passes. > > Fixes: 36e2c7421f02 ("fs: don't allow splice read/write without explicit ops") > Signed-off-by: Joel Stanley <joel@jms.id.au> > --- > fs/jffs2/file.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/fs/jffs2/file.c b/fs/jffs2/file.c > index f8fb89b10227..4fc8cd698d1a 100644 > --- a/fs/jffs2/file.c > +++ b/fs/jffs2/file.c > @@ -57,6 +57,7 @@ const struct file_operations jffs2_file_operations = > .mmap = generic_file_readonly_mmap, > .fsync = jffs2_fsync, > .splice_read = generic_file_splice_read, > + .splice_write = iter_file_splice_write, > }; > > /* jffs2_file_inode_operations */ > -- > 2.30.2 > Verified on g220a openbmc system that the patch fixes the issue. Tested-by: Lei YU <yulei.sh@bytedance.com>
On Tue, Mar 30, 2021 at 06:31:00PM +0000, Al Viro wrote: > The same goes for quite a few other filesystems, actually - at least > adfs, affs, bfs, hfs, hfsplus, hostfs, hpfs, minix, omfs, sysv, ufs > and vboxsf are in the same boat, and I suspect that ecryptfs and ntfs > might be too. > > Christoph, do you see any problems with doing the same thing for that > bunch as well? Linus asked to only fix things up where we get reports, but I think his intent there was more random procfs and debugfs files rather than file systems. So just doing the sweep should be fine.
diff --git a/fs/jffs2/file.c b/fs/jffs2/file.c index f8fb89b10227..4fc8cd698d1a 100644 --- a/fs/jffs2/file.c +++ b/fs/jffs2/file.c @@ -57,6 +57,7 @@ const struct file_operations jffs2_file_operations = .mmap = generic_file_readonly_mmap, .fsync = jffs2_fsync, .splice_read = generic_file_splice_read, + .splice_write = iter_file_splice_write, }; /* jffs2_file_inode_operations */
overlayfs using jffs2 as the upper filesystem would fail in some cases since moving to v5.10. The test case used was to run 'touch' on a file that exists in the lower fs, causing the modification time to be updated. It returns EINVAL when the bug is triggered. A bisection showed this was introduced in v5.9-rc1, with commit 36e2c7421f02 ("fs: don't allow splice read/write without explicit ops"). Reverting that commit restores the expected behaviour. Some digging showed that this was due to jffs2 lacking an implementation of splice_write. (For unknown reasons the warn_unsupported that should trigger was not displaying any output). Adding this patch resolved the issue and the test now passes. Fixes: 36e2c7421f02 ("fs: don't allow splice read/write without explicit ops") Signed-off-by: Joel Stanley <joel@jms.id.au> --- fs/jffs2/file.c | 1 + 1 file changed, 1 insertion(+)