clone ioctl return values

clone ioctl return values

[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:


--- tests/generic/157.out	2015-11-14 07:56:31.000000000 +0000
+++ /root/xfstests/results//generic/157.out.bad	2015-11-16 11:58:52.879078894 +0000
@@ -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

Re: clone ioctl return values

[Darrick J. Wong]

On Mon, Nov 16, 2015 at 04:04:31AM -0800, Christoph Hellwig wrote:
> 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:
> 
> 
> --- tests/generic/157.out	2015-11-14 07:56:31.000000000 +0000
> +++ /root/xfstests/results//generic/157.out.bad	2015-11-16 11:58:52.879078894 +0000
> @@ -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

>  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

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

>  Try to reflink to a dir
> -/mnt/test-157/dir1: Is a directory
> +/mnt/test/test-157/dir1: Is a directory

Drat, will fix.

>  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.

>  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?

>  Try to reflink an append-only file
> -XFS_IOC_CLONE_RANGE: Bad file descriptor
> +reflink: Invalid argument

Same as above.

--D

>  Reflink two files
>  Check scratch fs

Re: clone ioctl return values

[Christoph Hellwig]

On Mon, Nov 16, 2015 at 04:28:22PM -0800, Darrick J. Wong wrote:
> >  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.

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

> 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.

Re: clone ioctl return values

[Chris Mason]

On Tue, Nov 17, 2015 at 02:54:33AM -0800, Christoph Hellwig wrote:
> On Mon, Nov 16, 2015 at 04:28:22PM -0800, Darrick J. Wong wrote:
> > >  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.
> 
> This is current 4.4-rc.  The error seems accidental I think.
> 
> > 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.

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

Re: clone ioctl return values

[Christoph Hellwig]

On Mon, Nov 16, 2015 at 04:28:22PM -0800, Darrick J. Wong wrote:
> > -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
> 
> 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.

Re: clone ioctl return values

[Christoph Hellwig]

On Tue, Nov 17, 2015 at 08:57:45AM -0500, Chris Mason wrote:
> > > 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.
> 
> 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?

Re: clone ioctl return values

[Al Viro]

On Tue, Nov 17, 2015 at 07:22:52AM -0800, Christoph Hellwig wrote:

> 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.

Re: clone ioctl return values

[Christoph Hellwig]

On Tue, Nov 17, 2015 at 03:33:20PM +0000, Al Viro wrote:
> On Tue, Nov 17, 2015 at 07:22:52AM -0800, Christoph Hellwig wrote:
> 
> > 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.

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?

Re: clone ioctl return values

[Chris Mason]

On Tue, Nov 17, 2015 at 07:22:52AM -0800, Christoph Hellwig wrote:
> On Tue, Nov 17, 2015 at 08:57:45AM -0500, Chris Mason wrote:
> > > > 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.
> > 
> > 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.

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

Ack.

-chris

Re: clone ioctl return values

[Darrick J. Wong]

On Tue, Nov 17, 2015 at 07:22:52AM -0800, Christoph Hellwig wrote:
> On Tue, Nov 17, 2015 at 08:57:45AM -0500, Chris Mason wrote:
> > > > 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.
> > 
> > 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.

>  - 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

> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

Re: clone ioctl return values

[Christoph Hellwig]

On Tue, Nov 17, 2015 at 07:01:17PM -0800, Darrick J. Wong wrote:
> 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.

> 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.

> 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.