clone ioctl return values

PATCH Christoph Hellwig 4 participants 11 messages Nov 16, 2015 – Nov 18, 2015 VFS

Share & tools

Christoph Hellwig

author

2015-11-16 12:04 UTC #0 lore raw

Hi Darrick,

your new generic/157 xfs test brings up an interesting issue: error returns forthe various clone failure cases. It seems like this case was written fo the XFS case which differs a lot from the error chosen by btrfs and mostly followed by NFS. I'd say it might be a better idea to follow the btrfs example as the btrfs ioctls have been in use for a while. The only shortcoming I see in btrfs is that id doesn't explicitly check for non-directory, non-regular file items as the source. I have to admit I'm kinda surprised that it doesn't blow up, given that NFS instantly did when I removed those checks.

FYI, output from the test on btrfs below:

root/xfstests/results//generic/157.out.bad renamed from generic/157.out +10 ~~-10~~

@@ -2,24 +2,24 @@
 Format and mount
 Create the original files
 Try cross-device reflink
-XFS_IOC_CLONE_RANGE: Invalid cross-device link
+reflink: Invalid cross-device link
 Try unaligned reflink
-XFS_IOC_CLONE_RANGE: Invalid argument
+reflink: Invalid argument
 Try overlapping reflink
-XFS_IOC_CLONE_RANGE: Invalid argument
+reflink: Invalid argument
 Try reflink past EOF
-XFS_IOC_CLONE_RANGE: Invalid argument
+reflink: Invalid argument
 Try to reflink a dir
-XFS_IOC_CLONE_RANGE: Is a directory
+reflink: Is a directory
 Try to reflink a device
-XFS_IOC_CLONE_RANGE: Invalid argument
+/mnt/test/test-157/dev1: No such device or address
 Try to reflink to a dir
-/mnt/test-157/dir1: Is a directory
+/mnt/test/test-157/dir1: Is a directory
 Try to reflink to a device
-XFS_IOC_CLONE_RANGE: Operation not supported
+/mnt/test/test-157/dev1: No such device or address
 Try to reflink to a fifo
-XFS_IOC_CLONE_RANGE: Operation not supported
+reflink: Inappropriate ioctl for device
 Try to reflink an append-only file
-XFS_IOC_CLONE_RANGE: Bad file descriptor
+reflink: Invalid argument
 Reflink two files
 Check scratch fs

Darrick J. Wong

2015-11-17 00:28 UTC #1 lore raw

Quoted 18 lines · Christoph Hellwig

Hi Darrick,
your new generic/157 xfs test brings up an interesting issue: error returns forthe various clone failure cases. It seems like this case was written fo the XFS case which differs a lot from the error chosen by btrfs and mostly followed by NFS. I'd say it might be a better idea to follow the btrfs example as the btrfs ioctls have been in use for a while. The only shortcoming I see in btrfs is that id doesn't explicitly check for non-directory, non-regular file items as the source. I have to admit I'm kinda surprised that it doesn't blow up, given that NFS instantly did when I removed those checks.
FYI, output from the test on btrfs below:

root/xfstests/results//generic/157.out.bad line 5

Quoted 5 lines

root/xfstests/results//generic/157.out.bad renamed from generic/157.out +1 -1

@@ -2,24 +2,24 @@
 Format and mount
 Create the original files
 Try cross-device reflink
-XFS_IOC_CLONE_RANGE: Invalid cross-device link
+reflink: Invalid cross-device link

Hmm, I think you're running an older version of xfsprogs for-next. The "reflink:" perror prefix was changed to the ioctl name (for all three ioctls) per the comment in:
http://oss.sgi.com/archives/xfs/2015-09/msg00338.html

Quoted 6 lines

root/xfstests/results//generic/157.out.bad renamed from generic/157.out +2 -2

 Try unaligned reflink
-XFS_IOC_CLONE_RANGE: Invalid argument
+reflink: Invalid argument
 Try overlapping reflink
-XFS_IOC_CLONE_RANGE: Invalid argument
+reflink: Invalid argument

root/xfstests/results//generic/157.out.bad line 15

Quoted 10 lines

root/xfstests/results//generic/157.out.bad renamed from generic/157.out +3 -3

 Try reflink past EOF
-XFS_IOC_CLONE_RANGE: Invalid argument
+reflink: Invalid argument
 Try to reflink a dir
-XFS_IOC_CLONE_RANGE: Is a directory
+reflink: Is a directory
 
 Try to reflink a device
-XFS_IOC_CLONE_RANGE: Invalid argument
+/mnt/test/test-157/dev1: No such device or address

Huh. How did you get -ENODEV here? I ran this on 4.3 and got -EINVAL.

root/xfstests/results//generic/157.out.bad line 17

Quoted 3 lines

root/xfstests/results//generic/157.out.bad renamed from generic/157.out +1 -1
 Try to reflink to a dir
-/mnt/test-157/dir1: Is a directory
+/mnt/test/test-157/dir1: Is a directory

Drat, will fix.

root/xfstests/results//generic/157.out.bad line 19

Quoted 3 lines

root/xfstests/results//generic/157.out.bad renamed from generic/157.out +1 -1
 Try to reflink to a device
-XFS_IOC_CLONE_RANGE: Operation not supported
+/mnt/test/test-157/dev1: No such device or address

Same here as the other device case.

root/xfstests/results//generic/157.out.bad line 21

Quoted 3 lines

root/xfstests/results//generic/157.out.bad renamed from generic/157.out +1 -1
 Try to reflink to a fifo
-XFS_IOC_CLONE_RANGE: Operation not supported
+reflink: Inappropriate ioctl for device

Errrgh, the golden output of this test reflects the changes to the input checking in Anna/Peng's copy_file_range/clone_file_range patches.

So, I guess the question is, should I reset the golden output to whatever btrfs spits out before that patchset, and we'll consider the alterations to be bugs/regressions/whatever that ought to be fixed in their patches?

root/xfstests/results//generic/157.out.bad line 23

Quoted 3 lines

root/xfstests/results//generic/157.out.bad renamed from generic/157.out +1 -1
 Try to reflink an append-only file
-XFS_IOC_CLONE_RANGE: Bad file descriptor
+reflink: Invalid argument

Same as above.

--D

Quoted 2 lines

root/xfstests/results//generic/157.out.bad renamed from generic/157.out +0 -0
 Reflink two files
Check scratch fs

Christoph Hellwig

author

2015-11-17 10:54 UTC #2 lore raw

Quoted 5 lines · Darrick J. Wong

Review
Quoted 3 lines
Try to reflink a device
(quoted diff) +1 -1
-XFS_IOC_CLONE_RANGE: Invalid argument
+/mnt/test/test-157/dev1: No such device or address
Huh. How did you get -ENODEV here? I ran this on 4.3 and got -EINVAL.

This is current 4.4-rc. The error seems accidental I think.

Quoted 6 lines

Errrgh, the golden output of this test reflects the changes to the input checking in Anna/Peng's copy_file_range/clone_file_range patches.
So, I guess the question is, should I reset the golden output to whatever btrfs spits out before that patchset, and we'll consider the alterations to be bugs/regressions/whatever that ought to be fixed in their patches?

Some bits in btrfs don't seem kosher. But it would be good to explicitly send patches for btrfs to adopt to what might make more sense, and then follow it in the other implementations.

Chris Mason

2015-11-17 13:58 UTC #3 lore raw

Quoted 12 lines · Christoph Hellwig

Quoted 5 lines · Darrick J. Wong
Review
Quoted 3 lines
Try to reflink a device
(quoted diff) +1 -1
-XFS_IOC_CLONE_RANGE: Invalid argument
+/mnt/test/test-157/dev1: No such device or address
Huh. How did you get -ENODEV here? I ran this on 4.3 and got -EINVAL.
This is current 4.4-rc. The error seems accidental I think.
Quoted 3 lines
Errrgh, the golden output of this test reflects the changes to the input checking in Anna/Peng's copy_file_range/clone_file_range patches.

Quoted 7 lines

Quoted 3 lines
So, I guess the question is, should I reset the golden output to whatever btrfs spits out before that patchset, and we'll consider the alterations to be bugs/regressions/whatever that ought to be fixed in their patches?
Some bits in btrfs don't seem kosher. But it would be good to explicitly send patches for btrfs to adopt to what might make more sense, and then follow it in the other implementations.

Btrfs does check for directories, but we should really be checking for regular files too. In the end, we only copy extents that would correspond with regular files, so we're sneaking by.

-chris

Christoph Hellwig

author

2015-11-17 15:22 UTC #4 lore raw

Quoted 7 lines · Chris Mason

Quoted 7 lines
Quoted 6 lines
Errrgh, the golden output of this test reflects the changes to the input checking in Anna/Peng's copy_file_range/clone_file_range patches.
So, I guess the question is, should I reset the golden output to whatever btrfs spits out before that patchset, and we'll consider the alterations to be bugs/regressions/whatever that ought to be fixed in their patches?

Quoted 7 lines

Quoted 3 lines
Some bits in btrfs don't seem kosher. But it would be good to explicitly send patches for btrfs to adopt to what might make more sense, and then follow it in the other implementations.
Btrfs does check for directories, but we should really be checking for regular files too. In the end, we only copy extents that would correspond with regular files, so we're sneaking by.

Yes, I saw that. So so far I'd suggest something like the following for btrfs:

- return EBADFD for missing read/wite permissions
- return EINVAL for wrong non-directory file types as the source fd

And then make the test case and other implementations match this.

Does this sound like a plan?

Al Viro

maintainer

2015-11-17 15:33 UTC #5 lore raw

Quoted 4 lines · Christoph Hellwig

Yes, I saw that. So so far I'd suggest something like the following for btrfs:
- return EBADFD for missing read/wite permissions

Yowwwch... What the hell does that have to do with STREAMS? Or are you using EBADFD as "nobody uses that error value anyway, let's assign it whatever meaning we need"?

Besides, that'll be confused with EBADF all the time. I strongly recommend against that.

Christoph Hellwig

author

2015-11-17 15:36 UTC #6 lore raw

Quoted 5 lines · Al Viro

Quoted 3 lines · Christoph Hellwig
Yes, I saw that. So so far I'd suggest something like the following for btrfs:

Quoted 8 lines

Quoted 1 line
- return EBADFD for missing read/wite permissions
Yowwwch... What the hell does that have to do with STREAMS? Or are you using EBADFD as "nobody uses that error value anyway, let's assign it whatever meaning we need"?
Besides, that'll be confused with EBADF all the time. I strongly recommend against that.

Yes, I meant EBADF. That's what we normally use for missing FMODE_READ/WRITE or fget failures, so why would this call be different from everything else?

Chris Mason

2015-11-17 18:43 UTC #7 lore raw

In reply to Christoph Hellwig

Quoted 12 lines · Christoph Hellwig

Quoted 11 lines · Chris Mason
Quoted 10 lines
Quoted 6 lines
Errrgh, the golden output of this test reflects the changes to the input checking in Anna/Peng's copy_file_range/clone_file_range patches.
So, I guess the question is, should I reset the golden output to whatever btrfs spits out before that patchset, and we'll consider the alterations to be bugs/regressions/whatever that ought to be fixed in their patches?
Some bits in btrfs don't seem kosher. But it would be good to explicitly send patches for btrfs to adopt to what might make more sense, and then follow it in the other implementations.

Quoted 8 lines

Quoted 3 lines
Btrfs does check for directories, but we should really be checking for regular files too. In the end, we only copy extents that would correspond with regular files, so we're sneaking by.
Yes, I saw that. So so far I'd suggest something like the following for btrfs:
- return EBADFD for missing read/wite permissions

Why not -EPERM? I don't have strong feelings about picking errnos, as long as we're consistent, I'm not worried.

Quoted 2 lines

- return EINVAL for wrong non-directory file types as the source fd

Ack.

-chris

Darrick J. Wong

2015-11-18 03:02 UTC #8 lore raw

In reply to Christoph Hellwig

Quoted 12 lines · Christoph Hellwig

Quoted 11 lines · Chris Mason
Quoted 10 lines
Quoted 6 lines
Errrgh, the golden output of this test reflects the changes to the input checking in Anna/Peng's copy_file_range/clone_file_range patches.
So, I guess the question is, should I reset the golden output to whatever btrfs spits out before that patchset, and we'll consider the alterations to be bugs/regressions/whatever that ought to be fixed in their patches?
Some bits in btrfs don't seem kosher. But it would be good to explicitly send patches for btrfs to adopt to what might make more sense, and then follow it in the other implementations.

Quoted 8 lines

Quoted 3 lines
Btrfs does check for directories, but we should really be checking for regular files too. In the end, we only copy extents that would correspond with regular files, so we're sneaking by.
Yes, I saw that. So so far I'd suggest something like the following for btrfs:
- return EBADFD for missing read/wite permissions

Current btrfs returns EINVAL if the file isn't open for writing or is append-only. I think EBADF captures the situation here better and it's what Anna is pushing for copy_file_range. We can make the change, but generic/157 will fail on old kernels if we do this.

I don't have a problem with 157 failing on old kernels; we can backport the change to them, or note that ioctls are murky anyway.

Quoted 5 lines

- return EINVAL for wrong non-directory file types as the source fd
And then make the test case and other implementations match this.
Does this sound like a plan?

Yes.

One other thing I noticed -- prior to Anna's patchset, trying to invoke the reflink ioctl with a device, pipe, or socket as the destination could return a variety of error codes (-ENOTTY, -EINVAL, -ENOIOCTLCMD, etc.) which has all been replaced with -EOPNOTSUPP. That seems like a reasonable direction to take the test case.

Does this seem like a reasonable addition to the plan?

Should invalid inputs to the dedupe ioctl return the same error codes as the same invalid inputs to the reflink ioctl? I've been working on patches to hoist EXTENT_SAME to the VFS.

--D

Quoted 5 lines

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Christoph Hellwig

author

2015-11-18 15:11 UTC #9 lore raw

Quoted 4 lines · Darrick J. Wong

Current btrfs returns EINVAL if the file isn't open for writing or is append-only. I think EBADF captures the situation here better and it's what Anna is pushing for copy_file_range. We can make the change, but generic/157 will fail on old kernels if we do this.

As the old errno was rather wrong compared to similar syscalls I'd say don't worry about this.

Quoted 7 lines

One other thing I noticed -- prior to Anna's patchset, trying to invoke the reflink ioctl with a device, pipe, or socket as the destination could return a variety of error codes (-ENOTTY, -EINVAL, -ENOIOCTLCMD, etc.) which has all been replaced with -EOPNOTSUPP. That seems like a reasonable direction to take the test case.
Does this seem like a reasonable addition to the plan?

-ENOIOCTLCMD should never reach userspace, this is a clear bug. -EINVAL also seems wrong, but ENOTTY is perfectly valid for an ioctl based implementation. So I'd say we should allow ENOTTY or -EOPNOTSUPP in the test case by using filters, and ensure btrfs and nfs only return those in 4.4 and maybe with backports to stable.

Quoted 3 lines

Should invalid inputs to the dedupe ioctl return the same error codes as the same invalid inputs to the reflink ioctl? I've been working on patches to hoist EXTENT_SAME to the VFS.

I think so. EXTENT_SAME is so similar to clone that it should almost be a flag for it in the low level API.

Christoph Hellwig

author

2015-11-17 15:12 UTC #10 lore raw

In reply to Darrick J. Wong

Quoted 9 lines · Darrick J. Wong

Quoted 8 lines

root/xfstests/results//generic/157.out.bad renamed from generic/157.out +3 -3

-XFS_IOC_CLONE_RANGE: Invalid argument
+reflink: Invalid argument
 Try reflink past EOF
-XFS_IOC_CLONE_RANGE: Invalid argument
+reflink: Invalid argument
 Try to reflink a dir
-XFS_IOC_CLONE_RANGE: Is a directory
+reflink: Is a directory

Quoted 5 lines

Review
Quoted 3 lines
Try to reflink a device
(quoted diff) +1 -1
-XFS_IOC_CLONE_RANGE: Invalid argument
+/mnt/test/test-157/dev1: No such device or address
Huh. How did you get -ENODEV here? I ran this on 4.3 and got -EINVAL.

Turns out this is because my system doesn't have the ramdisk driver built into the kernel, so opening your block device with major 8, minor 0 already fails. Might be better to create a char device with major 1, minor 3 (dev null) as that should always be present.