| Message ID | 20151116120431.GA2860@infradead.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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? -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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? -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
--- 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