A regression has been reported by Nicolas Boichat, found while using the
copy_file_range syscall to copy a tracefs file. Before commit
5dae222a5ff0 ("vfs: allow copy_file_range to copy across devices") the
kernel would return -EXDEV to userspace when trying to copy a file across
different filesystems. After this commit, the syscall doesn't fail anymore
and instead returns zero (zero bytes copied), as this file's content is
generated on-the-fly and thus reports a size of zero.
This patch restores some cross-filesystem copy restrictions that existed
prior to commit 5dae222a5ff0 ("vfs: allow copy_file_range to copy across
devices"). Filesystems are still allowed to fall-back to the VFS
generic_copy_file_range() implementation, but that has now to be done
explicitly.
nfsd is also modified to fall-back into generic_copy_file_range() in case
vfs_copy_file_range() fails with -EOPNOTSUPP or -EXDEV.
Fixes: 5dae222a5ff0 ("vfs: allow copy_file_range to copy across devices")
Link: https://lore.kernel.org/linux-fsdevel/[email protected]/
Link: https://lore.kernel.org/linux-fsdevel/CANMq1KDZuxir2LM5jOTm0xx+BnvW=ZmpsG47CyHFJwnw7zSX6Q@mail.gmail.com/
Link: https://lore.kernel.org/linux-fsdevel/20210126135012.1.If45b7cdc3ff707bc1efa17f5366057d60603c45f@changeid/
Reported-by: Nicolas Boichat <[email protected]>
Signed-off-by: Luis Henriques <[email protected]>
Reviewed-by: Amir Goldstein <[email protected]>
Tested-by: Olga Kornievskaia <[email protected]>
---
Changes since v8
- Simply added Amir's Reviewed-by and Olga's Tested-by
Changes since v7
- set 'ret' to '-EOPNOTSUPP' before the clone 'if' statement so that the
error returned is always related to the 'copy' operation
Changes since v6
- restored i_sb checks for the clone operation
Changes since v5
- check if ->copy_file_range is NULL before calling it
Changes since v4
- nfsd falls-back to generic_copy_file_range() only *if* it gets -EOPNOTSUPP
or -EXDEV.
Changes since v3
- dropped the COPY_FILE_SPLICE flag
- kept the f_op's checks early in generic_copy_file_checks, implementing
Amir's suggestions
- modified nfsd to use generic_copy_file_range()
Changes since v2
- do all the required checks earlier, in generic_copy_file_checks(),
adding new checks for ->remap_file_range
- new COPY_FILE_SPLICE flag
- don't remove filesystem's fallback to generic_copy_file_range()
- updated commit changelog (and subject)
Changes since v1 (after Amir review)
- restored do_copy_file_range() helper
- return -EOPNOTSUPP if fs doesn't implement CFR
- updated commit description
fs/nfsd/vfs.c | 8 +++++++-
fs/read_write.c | 49 ++++++++++++++++++++++++-------------------------
2 files changed, 31 insertions(+), 26 deletions(-)
diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index 15adf1f6ab21..f54a88b3b4a2 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -569,6 +569,7 @@ __be32 nfsd4_clone_file_range(struct nfsd_file *nf_src, u64 src_pos,
ssize_t nfsd_copy_file_range(struct file *src, u64 src_pos, struct file *dst,
u64 dst_pos, u64 count)
{
+ ssize_t ret;
/*
* Limit copy to 4MB to prevent indefinitely blocking an nfsd
@@ -579,7 +580,12 @@ ssize_t nfsd_copy_file_range(struct file *src, u64 src_pos, struct file *dst,
* limit like this and pipeline multiple COPY requests.
*/
count = min_t(u64, count, 1 << 22);
- return vfs_copy_file_range(src, src_pos, dst, dst_pos, count, 0);
+ ret = vfs_copy_file_range(src, src_pos, dst, dst_pos, count, 0);
+
+ if (ret == -EOPNOTSUPP || ret == -EXDEV)
+ ret = generic_copy_file_range(src, src_pos, dst, dst_pos,
+ count, 0);
+ return ret;
}
__be32 nfsd4_vfs_fallocate(struct svc_rqst *rqstp, struct svc_fh *fhp,
diff --git a/fs/read_write.c b/fs/read_write.c
index 9db7adf160d2..2f0dd73b8b91 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -1395,28 +1395,6 @@ ssize_t generic_copy_file_range(struct file *file_in, loff_t pos_in,
}
EXPORT_SYMBOL(generic_copy_file_range);
-static ssize_t do_copy_file_range(struct file *file_in, loff_t pos_in,
- struct file *file_out, loff_t pos_out,
- size_t len, unsigned int flags)
-{
- /*
- * Although we now allow filesystems to handle cross sb copy, passing
- * a file of the wrong filesystem type to filesystem driver can result
- * in an attempt to dereference the wrong type of ->private_data, so
- * avoid doing that until we really have a good reason. NFS defines
- * several different file_system_type structures, but they all end up
- * using the same ->copy_file_range() function pointer.
- */
- if (file_out->f_op->copy_file_range &&
- file_out->f_op->copy_file_range == file_in->f_op->copy_file_range)
- return file_out->f_op->copy_file_range(file_in, pos_in,
- file_out, pos_out,
- len, flags);
-
- return generic_copy_file_range(file_in, pos_in, file_out, pos_out, len,
- flags);
-}
-
/*
* Performs necessary checks before doing a file copy
*
@@ -1434,6 +1412,25 @@ static int generic_copy_file_checks(struct file *file_in, loff_t pos_in,
loff_t size_in;
int ret;
+ /*
+ * Although we now allow filesystems to handle cross sb copy, passing
+ * a file of the wrong filesystem type to filesystem driver can result
+ * in an attempt to dereference the wrong type of ->private_data, so
+ * avoid doing that until we really have a good reason. NFS defines
+ * several different file_system_type structures, but they all end up
+ * using the same ->copy_file_range() function pointer.
+ */
+ if (file_out->f_op->copy_file_range) {
+ if (file_in->f_op->copy_file_range !=
+ file_out->f_op->copy_file_range)
+ return -EXDEV;
+ } else if (file_in->f_op->remap_file_range) {
+ if (file_inode(file_in)->i_sb != file_inode(file_out)->i_sb)
+ return -EXDEV;
+ } else {
+ return -EOPNOTSUPP;
+ }
+
ret = generic_file_rw_checks(file_in, file_out);
if (ret)
return ret;
@@ -1502,6 +1499,7 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in,
file_start_write(file_out);
+ ret = -EOPNOTSUPP;
/*
* Try cloning first, this is supported by more file systems, and
* more efficient if both clone and copy are supported (e.g. NFS).
@@ -1520,9 +1518,10 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in,
}
}
- ret = do_copy_file_range(file_in, pos_in, file_out, pos_out, len,
- flags);
- WARN_ON_ONCE(ret == -EOPNOTSUPP);
+ if (file_out->f_op->copy_file_range)
+ ret = file_out->f_op->copy_file_range(file_in, pos_in,
+ file_out, pos_out,
+ len, flags);
done:
if (ret > 0) {
fsnotify_access(file_in);
Greeting,
FYI, we noticed the following commit (built with gcc-9):
commit: 94a4dd06a6bbf3978b0bb1dddc2d8ec4e5bcad26 ("[PATCH v9] vfs: fix copy_file_range regression in cross-fs copies")
url: https://github.com/0day-ci/linux/commits/Luis-Henriques/vfs-fix-copy_file_range-regression-in-cross-fs-copies/20210510-170804
base: https://git.kernel.org/cgit/linux/kernel/git/viro/vfs.git for-next
in testcase: xfstests
version: xfstests-x86_64-73c0871-1_20210401
with following parameters:
disk: 4HDD
fs: xfs
test: generic-group-13
ucode: 0x21
test-description: xfstests is a regression test suite for xfs and other files ystems.
test-url: git://git.kernel.org/pub/scm/fs/xfs/xfstests-dev.git
on test machine: 4 threads 1 sockets Intel(R) Core(TM) i3-3220 CPU @ 3.30GHz with 8G memory
caused below changes (please refer to attached dmesg/kmsg for entire log/backtrace):
If you fix the issue, kindly add following tag
Reported-by: kernel test robot <[email protected]>
2021-05-11 11:28:23 export TEST_DIR=/fs/sda1
2021-05-11 11:28:23 export TEST_DEV=/dev/sda1
2021-05-11 11:28:23 export FSTYP=xfs
2021-05-11 11:28:23 export SCRATCH_MNT=/fs/scratch
2021-05-11 11:28:23 mkdir /fs/scratch -p
2021-05-11 11:28:23 export SCRATCH_DEV=/dev/sda4
2021-05-11 11:28:23 export SCRATCH_LOGDEV=/dev/sda2
2021-05-11 11:28:23 sed "s:^:generic/:" //lkp/benchmarks/xfstests/tests/generic-group-13
2021-05-11 11:28:23 ./check generic/260 generic/261 generic/262 generic/263 generic/264 generic/265 generic/266 generic/267 generic/268 generic/269 generic/270 generic/271 generic/272 generic/273 generic/274 generic/275 generic/276 generic/277 generic/278 generic/279
FSTYP -- xfs (debug)
PLATFORM -- Linux/x86_64 lkp-ivb-d02 5.12.0-rc6-00061-g94a4dd06a6bb #1 SMP Tue May 11 00:58:17 CST 2021
MKFS_OPTIONS -- -f -bsize=4096 /dev/sda4
MOUNT_OPTIONS -- /dev/sda4 /fs/scratch
generic/260 [not run] FITRIM not supported on /fs/scratch
generic/261 [not run] Reflink not supported by scratch filesystem type: xfs
generic/262 [not run] Reflink not supported by scratch filesystem type: xfs
generic/263 [failed, exit status 1]- output mismatch (see /lkp/benchmarks/xfstests/results//generic/263.out.bad)
--- tests/generic/263.out 2021-04-01 03:07:08.000000000 +0000
+++ /lkp/benchmarks/xfstests/results//generic/263.out.bad 2021-05-11 11:28:29.773460096 +0000
@@ -1,3 +1,32 @@
QA output created by 263
fsx -N 10000 -o 8192 -l 500000 -r PSIZE -t BSIZE -w BSIZE -Z
-fsx -N 10000 -o 128000 -l 500000 -r PSIZE -t BSIZE -w BSIZE -Z
+Seed set to 1
+main: filesystem does not support clone range, disabling!
+main: filesystem does not support dedupe range, disabling!
+skipping zero size read
...
(Run 'diff -u /lkp/benchmarks/xfstests/tests/generic/263.out /lkp/benchmarks/xfstests/results//generic/263.out.bad' to see the entire diff)
generic/264 [not run] Reflink not supported by scratch filesystem type: xfs
generic/265 [not run] Reflink not supported by scratch filesystem type: xfs
generic/266 [not run] Reflink not supported by scratch filesystem type: xfs
generic/267 [not run] Reflink not supported by scratch filesystem type: xfs
generic/268 [not run] Reflink not supported by scratch filesystem type: xfs
generic/269 48s
generic/270 61s
generic/271 [not run] Reflink not supported by scratch filesystem type: xfs
generic/272 [not run] Reflink not supported by scratch filesystem type: xfs
generic/273 17s
generic/274 14s
generic/275 11s
generic/276 [not run] Reflink not supported by scratch filesystem type: xfs
generic/277 3s
generic/278 [not run] Reflink not supported by scratch filesystem type: xfs
generic/279 [not run] Reflink not supported by scratch filesystem type: xfs
Ran: generic/260 generic/261 generic/262 generic/263 generic/264 generic/265 generic/266 generic/267 generic/268 generic/269 generic/270 generic/271 generic/272 generic/273 generic/274 generic/275 generic/276 generic/277 generic/278 generic/279
Not run: generic/260 generic/261 generic/262 generic/264 generic/265 generic/266 generic/267 generic/268 generic/271 generic/272 generic/276 generic/278 generic/279
Failures: generic/263
Failed 1 of 20 tests
To reproduce:
git clone https://github.com/intel/lkp-tests.git
cd lkp-tests
bin/lkp install job.yaml # job file is attached in this email
bin/lkp split-job --compatible job.yaml # generate the yaml file for lkp run
bin/lkp run generated-yaml-file
---
0DAY/LKP+ Test Infrastructure Open Source Technology Center
https://lists.01.org/hyperkitty/list/[email protected] Intel Corporation
Thanks,
Oliver Sang
kernel test robot <[email protected]> writes:
> Greeting,
>
> FYI, we noticed the following commit (built with gcc-9):
>
> commit: 94a4dd06a6bbf3978b0bb1dddc2d8ec4e5bcad26 ("[PATCH v9] vfs: fix copy_file_range regression in cross-fs copies")
> url: https://github.com/0day-ci/linux/commits/Luis-Henriques/vfs-fix-copy_file_range-regression-in-cross-fs-copies/20210510-170804
> base: https://git.kernel.org/cgit/linux/kernel/git/viro/vfs.git for-next
>
> in testcase: xfstests
> version: xfstests-x86_64-73c0871-1_20210401
> with following parameters:
>
> disk: 4HDD
> fs: xfs
> test: generic-group-13
> ucode: 0x21
>
> test-description: xfstests is a regression test suite for xfs and other files ystems.
> test-url: git://git.kernel.org/pub/scm/fs/xfs/xfstests-dev.git
>
>
> on test machine: 4 threads 1 sockets Intel(R) Core(TM) i3-3220 CPU @ 3.30GHz with 8G memory
>
> caused below changes (please refer to attached dmesg/kmsg for entire log/backtrace):
>
>
>
>
> If you fix the issue, kindly add following tag
> Reported-by: kernel test robot <[email protected]>
>
> 2021-05-11 11:28:23 export TEST_DIR=/fs/sda1
> 2021-05-11 11:28:23 export TEST_DEV=/dev/sda1
> 2021-05-11 11:28:23 export FSTYP=xfs
> 2021-05-11 11:28:23 export SCRATCH_MNT=/fs/scratch
> 2021-05-11 11:28:23 mkdir /fs/scratch -p
> 2021-05-11 11:28:23 export SCRATCH_DEV=/dev/sda4
> 2021-05-11 11:28:23 export SCRATCH_LOGDEV=/dev/sda2
> 2021-05-11 11:28:23 sed "s:^:generic/:" //lkp/benchmarks/xfstests/tests/generic-group-13
> 2021-05-11 11:28:23 ./check generic/260 generic/261 generic/262 generic/263 generic/264 generic/265 generic/266 generic/267 generic/268 generic/269 generic/270 generic/271 generic/272 generic/273 generic/274 generic/275 generic/276 generic/277 generic/278 generic/279
> FSTYP -- xfs (debug)
> PLATFORM -- Linux/x86_64 lkp-ivb-d02 5.12.0-rc6-00061-g94a4dd06a6bb #1 SMP Tue May 11 00:58:17 CST 2021
> MKFS_OPTIONS -- -f -bsize=4096 /dev/sda4
> MOUNT_OPTIONS -- /dev/sda4 /fs/scratch
>
> generic/260 [not run] FITRIM not supported on /fs/scratch
> generic/261 [not run] Reflink not supported by scratch filesystem type: xfs
> generic/262 [not run] Reflink not supported by scratch filesystem type: xfs
> generic/263 [failed, exit status 1]- output mismatch (see /lkp/benchmarks/xfstests/results//generic/263.out.bad)
> --- tests/generic/263.out 2021-04-01 03:07:08.000000000 +0000
> +++ /lkp/benchmarks/xfstests/results//generic/263.out.bad 2021-05-11 11:28:29.773460096 +0000
> @@ -1,3 +1,32 @@
> QA output created by 263
> fsx -N 10000 -o 8192 -l 500000 -r PSIZE -t BSIZE -w BSIZE -Z
> -fsx -N 10000 -o 128000 -l 500000 -r PSIZE -t BSIZE -w BSIZE -Z
> +Seed set to 1
> +main: filesystem does not support clone range, disabling!
> +main: filesystem does not support dedupe range, disabling!
> +skipping zero size read
> ...
> (Run 'diff -u /lkp/benchmarks/xfstests/tests/generic/263.out /lkp/benchmarks/xfstests/results//generic/263.out.bad' to see the entire diff)
> generic/264 [not run] Reflink not supported by scratch filesystem type: xfs
> generic/265 [not run] Reflink not supported by scratch filesystem type: xfs
> generic/266 [not run] Reflink not supported by scratch filesystem type: xfs
> generic/267 [not run] Reflink not supported by scratch filesystem type: xfs
> generic/268 [not run] Reflink not supported by scratch filesystem type: xfs
> generic/269 48s
> generic/270 61s
> generic/271 [not run] Reflink not supported by scratch filesystem type: xfs
> generic/272 [not run] Reflink not supported by scratch filesystem type: xfs
> generic/273 17s
> generic/274 14s
> generic/275 11s
> generic/276 [not run] Reflink not supported by scratch filesystem type: xfs
> generic/277 3s
> generic/278 [not run] Reflink not supported by scratch filesystem type: xfs
> generic/279 [not run] Reflink not supported by scratch filesystem type: xfs
> Ran: generic/260 generic/261 generic/262 generic/263 generic/264 generic/265 generic/266 generic/267 generic/268 generic/269 generic/270 generic/271 generic/272 generic/273 generic/274 generic/275 generic/276 generic/277 generic/278 generic/279
> Not run: generic/260 generic/261 generic/262 generic/264 generic/265 generic/266 generic/267 generic/268 generic/271 generic/272 generic/276 generic/278 generic/279
> Failures: generic/263
> Failed 1 of 20 tests
OK, I see what's going on. There are 2 issues: one with patch and another
one with the test itself.
The CFR syscall should have been disabled in this test but it isn't
because the test tries to copy 1 byte from a zero-sized file:
int
test_copy_range(void)
{
loff_t o1 = 0, o2 = 1;
if (syscall(__NR_copy_file_range, fd, &o1, fd, &o2, 1, 0) == -1 &&
(errno == ENOSYS || errno == EOPNOTSUPP || errno == ENOTTY)) {
if (!quiet)
fprintf(stderr,
"main: filesystem does not support "
"copy range, disabling!\n");
return 0;
}
return 1;
}
The syscall is doing an early '0' return because the file size is < len.
Fixing the kernel should probably be as easy as removing the
short-circuiting check in vfs_copy_file_range():
if (len == 0)
return 0;
This will force the filesystems code to handle '0' size copies but will
also make sure -EOPNOTSUPP is returned in this case.
Alternatively, we could have something like:
if (len == 0) {
if (file_out->f_op->copy_file_range)
return 0;
else
return -EOPNOTSUPP;
}
What do you guys think is the right thing to do?
Additionally, the test should also be fixed with something as the patch
bellow. By making sure we have 1 byte to copy we also ensure the syscall
will return -EOPNOTSUPP, even with the current version of the patch.
Cheers,
--
Luis
diff --git a/ltp/fsx.c b/ltp/fsx.c
index cd0bae55aeb8..97db594ae142 100644
--- a/ltp/fsx.c
+++ b/ltp/fsx.c
@@ -1596,6 +1596,10 @@ int
test_copy_range(void)
{
loff_t o1 = 0, o2 = 1;
+ int ret = 1;
+
+ /* Make sure we have 1 byte to copy */
+ ftruncate(fd, 1);
if (syscall(__NR_copy_file_range, fd, &o1, fd, &o2, 1, 0) == -1 &&
(errno == ENOSYS || errno == EOPNOTSUPP || errno == ENOTTY)) {
@@ -1603,10 +1607,13 @@ test_copy_range(void)
fprintf(stderr,
"main: filesystem does not support "
"copy range, disabling!\n");
- return 0;
+ ret = 0;
}
- return 1;
+ /* Restore file size */
+ ftruncate(fd, 0);
+
+ return ret;
}
void
The early return from copy_file_range when len is zero should check if the
filesystem really implements this syscall, returning -EOPNOTSUPP if it doesn't,
and 0 otherwise.
Reported-by: kernel test robot <[email protected]>
Signed-off-by: Luis Henriques <[email protected]>
---
Hi!
Since I got not feedback, I'm sending a patch that should fix this issue
reported by 0day. Probably this should simply be squashed into v9, I can
send v10 if you prefer that.
Cheers,
--
Luis
fs/read_write.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/read_write.c b/fs/read_write.c
index 9db7adf160d2..24b4bf704765 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -1498,7 +1498,7 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in,
return ret;
if (len == 0)
- return 0;
+ return file_out->f_op->copy_file_range ? 0 : -EOPNOTSUPP;
file_start_write(file_out);
On Fri, May 14, 2021 at 2:03 PM Luis Henriques <[email protected]> wrote:
>
> kernel test robot <[email protected]> writes:
>
> > Greeting,
> >
> > FYI, we noticed the following commit (built with gcc-9):
> >
> > commit: 94a4dd06a6bbf3978b0bb1dddc2d8ec4e5bcad26 ("[PATCH v9] vfs: fix copy_file_range regression in cross-fs copies")
> > url: https://github.com/0day-ci/linux/commits/Luis-Henriques/vfs-fix-copy_file_range-regression-in-cross-fs-copies/20210510-170804
> > base: https://git.kernel.org/cgit/linux/kernel/git/viro/vfs.git for-next
> >
> > in testcase: xfstests
> > version: xfstests-x86_64-73c0871-1_20210401
> > with following parameters:
> >
> > disk: 4HDD
> > fs: xfs
> > test: generic-group-13
> > ucode: 0x21
> >
> > test-description: xfstests is a regression test suite for xfs and other files ystems.
> > test-url: git://git.kernel.org/pub/scm/fs/xfs/xfstests-dev.git
> >
> >
> > on test machine: 4 threads 1 sockets Intel(R) Core(TM) i3-3220 CPU @ 3.30GHz with 8G memory
> >
> > caused below changes (please refer to attached dmesg/kmsg for entire log/backtrace):
> >
> >
> >
> >
> > If you fix the issue, kindly add following tag
> > Reported-by: kernel test robot <[email protected]>
> >
> > 2021-05-11 11:28:23 export TEST_DIR=/fs/sda1
> > 2021-05-11 11:28:23 export TEST_DEV=/dev/sda1
> > 2021-05-11 11:28:23 export FSTYP=xfs
> > 2021-05-11 11:28:23 export SCRATCH_MNT=/fs/scratch
> > 2021-05-11 11:28:23 mkdir /fs/scratch -p
> > 2021-05-11 11:28:23 export SCRATCH_DEV=/dev/sda4
> > 2021-05-11 11:28:23 export SCRATCH_LOGDEV=/dev/sda2
> > 2021-05-11 11:28:23 sed "s:^:generic/:" //lkp/benchmarks/xfstests/tests/generic-group-13
> > 2021-05-11 11:28:23 ./check generic/260 generic/261 generic/262 generic/263 generic/264 generic/265 generic/266 generic/267 generic/268 generic/269 generic/270 generic/271 generic/272 generic/273 generic/274 generic/275 generic/276 generic/277 generic/278 generic/279
> > FSTYP -- xfs (debug)
> > PLATFORM -- Linux/x86_64 lkp-ivb-d02 5.12.0-rc6-00061-g94a4dd06a6bb #1 SMP Tue May 11 00:58:17 CST 2021
> > MKFS_OPTIONS -- -f -bsize=4096 /dev/sda4
> > MOUNT_OPTIONS -- /dev/sda4 /fs/scratch
> >
> > generic/260 [not run] FITRIM not supported on /fs/scratch
> > generic/261 [not run] Reflink not supported by scratch filesystem type: xfs
> > generic/262 [not run] Reflink not supported by scratch filesystem type: xfs
> > generic/263 [failed, exit status 1]- output mismatch (see /lkp/benchmarks/xfstests/results//generic/263.out.bad)
> > --- tests/generic/263.out 2021-04-01 03:07:08.000000000 +0000
> > +++ /lkp/benchmarks/xfstests/results//generic/263.out.bad 2021-05-11 11:28:29.773460096 +0000
> > @@ -1,3 +1,32 @@
> > QA output created by 263
> > fsx -N 10000 -o 8192 -l 500000 -r PSIZE -t BSIZE -w BSIZE -Z
> > -fsx -N 10000 -o 128000 -l 500000 -r PSIZE -t BSIZE -w BSIZE -Z
> > +Seed set to 1
> > +main: filesystem does not support clone range, disabling!
> > +main: filesystem does not support dedupe range, disabling!
> > +skipping zero size read
> > ...
> > (Run 'diff -u /lkp/benchmarks/xfstests/tests/generic/263.out /lkp/benchmarks/xfstests/results//generic/263.out.bad' to see the entire diff)
> > generic/264 [not run] Reflink not supported by scratch filesystem type: xfs
> > generic/265 [not run] Reflink not supported by scratch filesystem type: xfs
> > generic/266 [not run] Reflink not supported by scratch filesystem type: xfs
> > generic/267 [not run] Reflink not supported by scratch filesystem type: xfs
> > generic/268 [not run] Reflink not supported by scratch filesystem type: xfs
> > generic/269 48s
> > generic/270 61s
> > generic/271 [not run] Reflink not supported by scratch filesystem type: xfs
> > generic/272 [not run] Reflink not supported by scratch filesystem type: xfs
> > generic/273 17s
> > generic/274 14s
> > generic/275 11s
> > generic/276 [not run] Reflink not supported by scratch filesystem type: xfs
> > generic/277 3s
> > generic/278 [not run] Reflink not supported by scratch filesystem type: xfs
> > generic/279 [not run] Reflink not supported by scratch filesystem type: xfs
> > Ran: generic/260 generic/261 generic/262 generic/263 generic/264 generic/265 generic/266 generic/267 generic/268 generic/269 generic/270 generic/271 generic/272 generic/273 generic/274 generic/275 generic/276 generic/277 generic/278 generic/279
> > Not run: generic/260 generic/261 generic/262 generic/264 generic/265 generic/266 generic/267 generic/268 generic/271 generic/272 generic/276 generic/278 generic/279
> > Failures: generic/263
> > Failed 1 of 20 tests
>
> OK, I see what's going on. There are 2 issues: one with patch and another
> one with the test itself.
>
> The CFR syscall should have been disabled in this test but it isn't
> because the test tries to copy 1 byte from a zero-sized file:
>
> int
> test_copy_range(void)
> {
> loff_t o1 = 0, o2 = 1;
>
> if (syscall(__NR_copy_file_range, fd, &o1, fd, &o2, 1, 0) == -1 &&
> (errno == ENOSYS || errno == EOPNOTSUPP || errno == ENOTTY)) {
> if (!quiet)
> fprintf(stderr,
> "main: filesystem does not support "
> "copy range, disabling!\n");
> return 0;
> }
>
> return 1;
> }
>
> The syscall is doing an early '0' return because the file size is < len.
>
> Fixing the kernel should probably be as easy as removing the
> short-circuiting check in vfs_copy_file_range():
>
> if (len == 0)
> return 0;
>
> This will force the filesystems code to handle '0' size copies but will
> also make sure -EOPNOTSUPP is returned in this case.
>
Sorry for the late reply.
The solution above is correct.
That is aligned with the behavior of vfs_clone_file_range().
Need to call into the filesystem method also with 0 length
in order to learn about CFR support of this filesystem instance.
> Alternatively, we could have something like:
>
> if (len == 0) {
> if (file_out->f_op->copy_file_range)
> return 0;
> else
> return -EOPNOTSUPP;
> }
>
This does not catch the case of a filesystem driver that has
CFR method but a filesystem instance does not support CFR.
For example, overlayfs with ext4 as upper fs.
> What do you guys think is the right thing to do?
>
> Additionally, the test should also be fixed with something as the patch
> bellow. By making sure we have 1 byte to copy we also ensure the syscall
> will return -EOPNOTSUPP, even with the current version of the patch.
>
I don't think that the test should be fixed.
Thanks,
Amir.
> Cheers,
> --
> Luis
>
> diff --git a/ltp/fsx.c b/ltp/fsx.c
> index cd0bae55aeb8..97db594ae142 100644
> --- a/ltp/fsx.c
> +++ b/ltp/fsx.c
> @@ -1596,6 +1596,10 @@ int
> test_copy_range(void)
> {
> loff_t o1 = 0, o2 = 1;
> + int ret = 1;
> +
> + /* Make sure we have 1 byte to copy */
> + ftruncate(fd, 1);
>
> if (syscall(__NR_copy_file_range, fd, &o1, fd, &o2, 1, 0) == -1 &&
> (errno == ENOSYS || errno == EOPNOTSUPP || errno == ENOTTY)) {
> @@ -1603,10 +1607,13 @@ test_copy_range(void)
> fprintf(stderr,
> "main: filesystem does not support "
> "copy range, disabling!\n");
> - return 0;
> + ret = 0;
> }
>
> - return 1;
> + /* Restore file size */
> + ftruncate(fd, 0);
> +
> + return ret;
> }
>
> void
On Wed, Jun 30, 2021 at 06:46:22PM +0300, Amir Goldstein wrote:
> On Fri, May 14, 2021 at 2:03 PM Luis Henriques <[email protected]> wrote:
> >
> > kernel test robot <[email protected]> writes:
> >
> > > Greeting,
> > >
> > > FYI, we noticed the following commit (built with gcc-9):
> > >
> > > commit: 94a4dd06a6bbf3978b0bb1dddc2d8ec4e5bcad26 ("[PATCH v9] vfs: fix copy_file_range regression in cross-fs copies")
> > > url: https://github.com/0day-ci/linux/commits/Luis-Henriques/vfs-fix-copy_file_range-regression-in-cross-fs-copies/20210510-170804
> > > base: https://git.kernel.org/cgit/linux/kernel/git/viro/vfs.git for-next
> > >
> > > in testcase: xfstests
> > > version: xfstests-x86_64-73c0871-1_20210401
> > > with following parameters:
> > >
> > > disk: 4HDD
> > > fs: xfs
> > > test: generic-group-13
> > > ucode: 0x21
> > >
> > > test-description: xfstests is a regression test suite for xfs and other files ystems.
> > > test-url: git://git.kernel.org/pub/scm/fs/xfs/xfstests-dev.git
> > >
> > >
> > > on test machine: 4 threads 1 sockets Intel(R) Core(TM) i3-3220 CPU @ 3.30GHz with 8G memory
> > >
> > > caused below changes (please refer to attached dmesg/kmsg for entire log/backtrace):
> > >
> > >
> > >
> > >
> > > If you fix the issue, kindly add following tag
> > > Reported-by: kernel test robot <[email protected]>
> > >
> > > 2021-05-11 11:28:23 export TEST_DIR=/fs/sda1
> > > 2021-05-11 11:28:23 export TEST_DEV=/dev/sda1
> > > 2021-05-11 11:28:23 export FSTYP=xfs
> > > 2021-05-11 11:28:23 export SCRATCH_MNT=/fs/scratch
> > > 2021-05-11 11:28:23 mkdir /fs/scratch -p
> > > 2021-05-11 11:28:23 export SCRATCH_DEV=/dev/sda4
> > > 2021-05-11 11:28:23 export SCRATCH_LOGDEV=/dev/sda2
> > > 2021-05-11 11:28:23 sed "s:^:generic/:" //lkp/benchmarks/xfstests/tests/generic-group-13
> > > 2021-05-11 11:28:23 ./check generic/260 generic/261 generic/262 generic/263 generic/264 generic/265 generic/266 generic/267 generic/268 generic/269 generic/270 generic/271 generic/272 generic/273 generic/274 generic/275 generic/276 generic/277 generic/278 generic/279
> > > FSTYP -- xfs (debug)
> > > PLATFORM -- Linux/x86_64 lkp-ivb-d02 5.12.0-rc6-00061-g94a4dd06a6bb #1 SMP Tue May 11 00:58:17 CST 2021
> > > MKFS_OPTIONS -- -f -bsize=4096 /dev/sda4
> > > MOUNT_OPTIONS -- /dev/sda4 /fs/scratch
> > >
> > > generic/260 [not run] FITRIM not supported on /fs/scratch
> > > generic/261 [not run] Reflink not supported by scratch filesystem type: xfs
> > > generic/262 [not run] Reflink not supported by scratch filesystem type: xfs
> > > generic/263 [failed, exit status 1]- output mismatch (see /lkp/benchmarks/xfstests/results//generic/263.out.bad)
> > > --- tests/generic/263.out 2021-04-01 03:07:08.000000000 +0000
> > > +++ /lkp/benchmarks/xfstests/results//generic/263.out.bad 2021-05-11 11:28:29.773460096 +0000
> > > @@ -1,3 +1,32 @@
> > > QA output created by 263
> > > fsx -N 10000 -o 8192 -l 500000 -r PSIZE -t BSIZE -w BSIZE -Z
> > > -fsx -N 10000 -o 128000 -l 500000 -r PSIZE -t BSIZE -w BSIZE -Z
> > > +Seed set to 1
> > > +main: filesystem does not support clone range, disabling!
> > > +main: filesystem does not support dedupe range, disabling!
> > > +skipping zero size read
> > > ...
> > > (Run 'diff -u /lkp/benchmarks/xfstests/tests/generic/263.out /lkp/benchmarks/xfstests/results//generic/263.out.bad' to see the entire diff)
> > > generic/264 [not run] Reflink not supported by scratch filesystem type: xfs
> > > generic/265 [not run] Reflink not supported by scratch filesystem type: xfs
> > > generic/266 [not run] Reflink not supported by scratch filesystem type: xfs
> > > generic/267 [not run] Reflink not supported by scratch filesystem type: xfs
> > > generic/268 [not run] Reflink not supported by scratch filesystem type: xfs
> > > generic/269 48s
> > > generic/270 61s
> > > generic/271 [not run] Reflink not supported by scratch filesystem type: xfs
> > > generic/272 [not run] Reflink not supported by scratch filesystem type: xfs
> > > generic/273 17s
> > > generic/274 14s
> > > generic/275 11s
> > > generic/276 [not run] Reflink not supported by scratch filesystem type: xfs
> > > generic/277 3s
> > > generic/278 [not run] Reflink not supported by scratch filesystem type: xfs
> > > generic/279 [not run] Reflink not supported by scratch filesystem type: xfs
> > > Ran: generic/260 generic/261 generic/262 generic/263 generic/264 generic/265 generic/266 generic/267 generic/268 generic/269 generic/270 generic/271 generic/272 generic/273 generic/274 generic/275 generic/276 generic/277 generic/278 generic/279
> > > Not run: generic/260 generic/261 generic/262 generic/264 generic/265 generic/266 generic/267 generic/268 generic/271 generic/272 generic/276 generic/278 generic/279
> > > Failures: generic/263
> > > Failed 1 of 20 tests
> >
> > OK, I see what's going on. There are 2 issues: one with patch and another
> > one with the test itself.
> >
> > The CFR syscall should have been disabled in this test but it isn't
> > because the test tries to copy 1 byte from a zero-sized file:
> >
> > int
> > test_copy_range(void)
> > {
> > loff_t o1 = 0, o2 = 1;
> >
> > if (syscall(__NR_copy_file_range, fd, &o1, fd, &o2, 1, 0) == -1 &&
> > (errno == ENOSYS || errno == EOPNOTSUPP || errno == ENOTTY)) {
> > if (!quiet)
> > fprintf(stderr,
> > "main: filesystem does not support "
> > "copy range, disabling!\n");
> > return 0;
> > }
> >
> > return 1;
> > }
> >
> > The syscall is doing an early '0' return because the file size is < len.
> >
> > Fixing the kernel should probably be as easy as removing the
> > short-circuiting check in vfs_copy_file_range():
> >
> > if (len == 0)
> > return 0;
> >
> > This will force the filesystems code to handle '0' size copies but will
> > also make sure -EOPNOTSUPP is returned in this case.
> >
>
> Sorry for the late reply.
> The solution above is correct.
> That is aligned with the behavior of vfs_clone_file_range().
> Need to call into the filesystem method also with 0 length
> in order to learn about CFR support of this filesystem instance.
Yep, this makes sense (I've seen you're detailed explanation in the other
thread -- thanks!). I'll send out v11 in a sec.
Cheers,
--
Lu?s
> > Alternatively, we could have something like:
> >
> > if (len == 0) {
> > if (file_out->f_op->copy_file_range)
> > return 0;
> > else
> > return -EOPNOTSUPP;
> > }
> >
>
> This does not catch the case of a filesystem driver that has
> CFR method but a filesystem instance does not support CFR.
> For example, overlayfs with ext4 as upper fs.
>
> > What do you guys think is the right thing to do?
> >
> > Additionally, the test should also be fixed with something as the patch
> > bellow. By making sure we have 1 byte to copy we also ensure the syscall
> > will return -EOPNOTSUPP, even with the current version of the patch.
> >
>
> I don't think that the test should be fixed.
>
> Thanks,
> Amir.
>
> > Cheers,
> > --
> > Luis
> >
> > diff --git a/ltp/fsx.c b/ltp/fsx.c
> > index cd0bae55aeb8..97db594ae142 100644
> > --- a/ltp/fsx.c
> > +++ b/ltp/fsx.c
> > @@ -1596,6 +1596,10 @@ int
> > test_copy_range(void)
> > {
> > loff_t o1 = 0, o2 = 1;
> > + int ret = 1;
> > +
> > + /* Make sure we have 1 byte to copy */
> > + ftruncate(fd, 1);
> >
> > if (syscall(__NR_copy_file_range, fd, &o1, fd, &o2, 1, 0) == -1 &&
> > (errno == ENOSYS || errno == EOPNOTSUPP || errno == ENOTTY)) {
> > @@ -1603,10 +1607,13 @@ test_copy_range(void)
> > fprintf(stderr,
> > "main: filesystem does not support "
> > "copy range, disabling!\n");
> > - return 0;
> > + ret = 0;
> > }
> >
> > - return 1;
> > + /* Restore file size */
> > + ftruncate(fd, 0);
> > +
> > + return ret;
> > }
> >
> > void