[Bug build/26338] New: io/tst-copy_file_range fails on RHEL 7.8 hosts

[Bug build/26338] New: io/tst-copy_file_range fails on RHEL 7.8 hosts

[romain.geissler at amadeus dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=26338

            Bug ID: 26338
           Summary: io/tst-copy_file_range fails on RHEL 7.8 hosts
           Product: glibc
           Version: 2.33
            Status: UNCONFIRMED
          Severity: normal
          Priority: P2
         Component: build
          Assignee: unassigned at sourceware dot org
          Reporter: romain.geissler at amadeus dot com
                CC: carlos at redhat dot com
  Target Milestone: ---

Hi,

Following an internal mail exchange with folks from Red Hat, I would like to
notify you that right now the test io/tst-copy_file_range is failing on kernel
3.10.0-1127.13.1.el7.x86_64 from RHEL 7.8 (while it used to work on earlier
RHEL 7 kernels).

It looks like on the RHEL kernel the backported syscall copy_file_range has had
different behavior over time in the RHEL 7 kernel, and with RHEL 7.8 it ended
up being deprecated in some use cases. See
https://access.redhat.com/documentation/en-us/red_hat_enterprise_linux/7/html/7.8_release_notes/deprecated_functionality
and this paragraph in particular:

The copy_file_range() call has been disabled on local file systems and in NFS
The copy_file_range() system call on local file systems contains multiple
issues that are difficult to fix. To avoid file corruptions, copy_file_range()
support on local file systems has been disabled in RHEL 7.8. If an application
uses the call in this case, copy_file_range() now returns an ENOSYS error.

It happens that this description from Red Hat is not fully accurate. Actually,
you *CAN* call copy_file_range on a RHEL 7.8 kernel and a local file system, it
will return 0 if the size of the copy is 0 as well, and ENOSYS if the size of
the copy is different from 0.

The code in io/tst-copy_file_range which flags the test as unsupported on the
running system looks like this:

 240   infd = create_temp_file ("tst-copy_file_range-in-", &infile);
 241   outfd = create_temp_file ("tst-copy_file_range-out-", &outfile);
 242   {
 243     ssize_t ret = copy_file_range (infd, NULL, outfd, NULL, 0, 0);
 244     if (ret != 0)
 245       {
 246         if (errno == ENOSYS)
 247           FAIL_UNSUPPORTED ("copy_file_range is not support on this
system");
 248         FAIL_EXIT1 ("copy_file_range probing call: %m");
 249       }
 250   }

So basically on these RHEL 7.8 kernel the first "copy_file_range" with size of
0 are successfull, and the test is *NOT* marked as unsupported, while actually
it is.

May I suggest to cope with this case by ensuring we do copy more than 0 bytes,
and if we get ENOSYS flag the whole test as unsupported ?

Thanks,
Romain

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug build/26338] io/tst-copy_file_range fails on RHEL 7.8 hosts

[adhemerval.zanella at linaro dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=26338

Adhemerval Zanella <adhemerval.zanella at linaro dot org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |adhemerval.zanella at linaro dot o
                   |                            |rg

--- Comment #1 from Adhemerval Zanella <adhemerval.zanella at linaro dot org> ---
(In reply to Romain Geissler from comment #0)
> Hi,
> 
> Following an internal mail exchange with folks from Red Hat, I would like to
> notify you that right now the test io/tst-copy_file_range is failing on
> kernel 3.10.0-1127.13.1.el7.x86_64 from RHEL 7.8 (while it used to work on
> earlier RHEL 7 kernels).
> 
> It looks like on the RHEL kernel the backported syscall copy_file_range has
> had different behavior over time in the RHEL 7 kernel, and with RHEL 7.8 it
> ended up being deprecated in some use cases. See
> https://access.redhat.com/documentation/en-us/red_hat_enterprise_linux/7/
> html/7.8_release_notes/deprecated_functionality and this paragraph in
> particular:
> 
> The copy_file_range() call has been disabled on local file systems and in NFS
> The copy_file_range() system call on local file systems contains multiple
> issues that are difficult to fix. To avoid file corruptions,
> copy_file_range() support on local file systems has been disabled in RHEL
> 7.8. If an application uses the call in this case, copy_file_range() now
> returns an ENOSYS error.
> 
> It happens that this description from Red Hat is not fully accurate.
> Actually, you *CAN* call copy_file_range on a RHEL 7.8 kernel and a local
> file system, it will return 0 if the size of the copy is 0 as well, and
> ENOSYS if the size of the copy is different from 0.

This behavior in misleading and it means that it might not be possible to
check if the syscall is supported without triggering side-effects, since
it seems that ENOSYS is being returned later when the syscall calls the
filesystem callbacks.

> 
> The code in io/tst-copy_file_range which flags the test as unsupported on
> the running system looks like this:
> 
>  240   infd = create_temp_file ("tst-copy_file_range-in-", &infile);
>  241   outfd = create_temp_file ("tst-copy_file_range-out-", &outfile);
>  242   {
>  243     ssize_t ret = copy_file_range (infd, NULL, outfd, NULL, 0, 0);
>  244     if (ret != 0)
>  245       {
>  246         if (errno == ENOSYS)
>  247           FAIL_UNSUPPORTED ("copy_file_range is not support on this
> system");
>  248         FAIL_EXIT1 ("copy_file_range probing call: %m");
>  249       }
>  250   }
> 
> So basically on these RHEL 7.8 kernel the first "copy_file_range" with size
> of 0 are successfull, and the test is *NOT* marked as unsupported, while
> actually it is.
> 
> May I suggest to cope with this case by ensuring we do copy more than 0
> bytes, and if we get ENOSYS flag the whole test as unsupported ?

If it is a RHEL7.8 specific issue due the non-upstream change to disable it
I think the test should keep as-is, since it does indicate non expected
failure in the syscall semantic regarding upstream behaviour.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug build/26338] io/tst-copy_file_range fails on RHEL 7.8 hosts

[carlos at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=26338

--- Comment #2 from Carlos O'Donell <carlos at redhat dot com> ---
These failures are noted here in our release notes per arch:
https://sourceware.org/glibc/wiki/Release/2.32

Where the Red Hat glibc team did testing on RHEL7 we noted that this failed
because the kernel doesn't meet our expecatations.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug build/26338] io/tst-copy_file_range fails on RHEL 7.8 hosts

[fweimer at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=26338

Florian Weimer <fweimer at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |fweimer at redhat dot com

--- Comment #3 from Florian Weimer <fweimer at redhat dot com> ---
I assume the upstream kernel would behave in the same way (succeed with length
0, fail with any other length) if a file system does something so that the
generic splice-based copy_file_range fails, so maybe this is a test bug after
all.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug build/26338] io/tst-copy_file_range fails on RHEL 7.8 hosts

[adhemerval.zanella at linaro dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=26338

--- Comment #4 from Adhemerval Zanella <adhemerval.zanella at linaro dot org> ---
(In reply to Florian Weimer from comment #3)
> I assume the upstream kernel would behave in the same way (succeed with
> length 0, fail with any other length) if a file system does something so
> that the generic splice-based copy_file_range fails, so maybe this is a test
> bug after all.

Skimming through kernel sources it seems that is EOPNOTSUPP is usually the
preferred code instead:

fs/fuse/file.c
3275 static ssize_t __fuse_copy_file_range(struct file *file_in, loff_t pos_in,
3276                                       struct file *file_out, loff_t
pos_out,
3277                                       size_t len, unsigned int flags)
[...]
3302         if (fc->no_copy_file_range)
3303                 return -EOPNOTSUPP;

fs/cifs/cifsfs.c
1207 static ssize_t cifs_copy_file_range(struct file *src_file, loff_t off,
1208                                 struct file *dst_file, loff_t destoff,
1209                                 size_t len, unsigned int flags)
1210 {
1211         unsigned int xid = get_xid();
1212         ssize_t rc;
1213         struct cifsFileInfo *cfile = dst_file->private_data;
1214 
1215         if (cfile->swapfile)
1216                 return -EOPNOTSUPP;
1217 
1218         rc = cifs_file_copychunk_range(xid, src_file, off, dst_file,
destoff,
1219                                         len, flags);
1220         free_xid(xid);
1221 
1222         if (rc == -EOPNOTSUPP || rc == -EXDEV)
1223                 rc = generic_copy_file_range(src_file, off, dst_file,
1224                                              destoff, len, flags);
1225         return rc;
1226 }

fs/fuse/file.c
3275 static ssize_t __fuse_copy_file_range(struct file *file_in, loff_t pos_in,
3276                                       struct file *file_out, loff_t
pos_out,
3277                                       size_t len, unsigned int flags)
3278 {
[...]
3302         if (fc->no_copy_file_range)
3303                 return -EOPNOTSUPP;

fs/nfs/nfs4file.c
132 static ssize_t __nfs4_copy_file_range(struct file *file_in, loff_t pos_in,
133                                       struct file *file_out, loff_t
pos_out,
134                                       size_t count, unsigned int flags)
135 {
[...]
145         if (!nfs_server_capable(file_inode(file_out), NFS_CAP_COPY))
146                 return -EOPNOTSUPP;
147         if (file_inode(file_in) == file_inode(file_out))
148                 return -EOPNOTSUPP;
[..]
161                 if (sync ||
162                         count <= 14 *
NFS_SERVER(file_inode(file_in))->rsize)
163                         return -EOPNOTSUPP;

So I don't think ENOSYS as RHEL 7.8 is doing is the correct way to advertise
the syscall does not work depending of the underlying filesystem (at least it
seems not what upstream is doing).

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug build/26338] io/tst-copy_file_range fails on RHEL 7.8 hosts

[fweimer at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=26338

--- Comment #5 from Florian Weimer <fweimer at redhat dot com> ---
(In reply to Adhemerval Zanella from comment #4)
> So I don't think ENOSYS as RHEL 7.8 is doing is the correct way to advertise
> the syscall does not work depending of the underlying filesystem (at least
> it seems not what upstream is doing).

It would break cp from coreutils, which only recognizes ENOSYS, EINVAL, EBADF,
EXDEV as soft errors.

The EOPNOTSUPPs you listed should not leak into userspace because they trigger
generic splice-based copy_file_range fallback, and splice apparently fails with
EINVAL in case of missing file system support.

It's true that ENOSYS breaks programs that cache the availability of the system
call and assume that the error is parameter-independent. EINVAL might have been
the better choice for that reason.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug build/26338] io/tst-copy_file_range fails on RHEL 7.8 hosts

[adhemerval.zanella at linaro dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=26338

--- Comment #6 from Adhemerval Zanella <adhemerval.zanella at linaro dot org> ---
(In reply to Florian Weimer from comment #5)
> (In reply to Adhemerval Zanella from comment #4)
> > So I don't think ENOSYS as RHEL 7.8 is doing is the correct way to advertise
> > the syscall does not work depending of the underlying filesystem (at least
> > it seems not what upstream is doing).
> 
> It would break cp from coreutils, which only recognizes ENOSYS, EINVAL,
> EBADF, EXDEV as soft errors.
> 
> The EOPNOTSUPPs you listed should not leak into userspace because they
> trigger generic splice-based copy_file_range fallback, and splice apparently
> fails with EINVAL in case of missing file system support.
> 
> It's true that ENOSYS breaks programs that cache the availability of the
> system call and assume that the error is parameter-independent. EINVAL might
> have been the better choice for that reason.

It seems that in very simplified mode the call sequence is:

  copy_file_range
  \_ vfs_copy_file_range
     \_ remap_file_range
     \_ do_copy_file_range
        \_ file_out->f_op->copy_file_range
        \_ generic_copy_file_range


However on do_copy_file_range the generic_copy_file_range is not actually
called is the filesystem one fails. The vfs_copy_file_range does WARN_ON_ONCE
on EOPNOTSUPP, but afaiu it indeed might leak to userspace (although it might
characterize as a bug).

But I agree that EINVAL is indeed a better return code and it is what
generic_copy_file_range does for such cases.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug build/26338] io/tst-copy_file_range fails on RHEL 7.8 hosts

[fweimer at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=26338

--- Comment #7 from Florian Weimer <fweimer at redhat dot com> ---
Most file systems I looked at have something like this:

static ssize_t nfs4_copy_file_range(struct file *file_in, loff_t pos_in,
                                    struct file *file_out, loff_t pos_out,
                                    size_t count, unsigned int flags)
{
        ssize_t ret;

        ret = __nfs4_copy_file_range(file_in, pos_in, file_out, pos_out, count,
                                     flags);
        if (ret == -EOPNOTSUPP || ret == -EXDEV)
                ret = generic_copy_file_range(file_in, pos_in, file_out,
                                              pos_out, count, flags);
        return ret;
}

This masks the EOPNOTSUPP error from the native copy_file_range implementation
and calls generic_copy_file_range even if do_copy_file_range does not.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug build/26338] io/tst-copy_file_range fails on RHEL 7.8 hosts

[adhemerval.zanella at linaro dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=26338

--- Comment #8 from Adhemerval Zanella <adhemerval.zanella at linaro dot org> ---
(In reply to Florian Weimer from comment #7)
> Most file systems I looked at have something like this:
> 
> static ssize_t nfs4_copy_file_range(struct file *file_in, loff_t pos_in,
> 				    struct file *file_out, loff_t pos_out,
> 				    size_t count, unsigned int flags)
> {
> 	ssize_t ret;
> 
> 	ret = __nfs4_copy_file_range(file_in, pos_in, file_out, pos_out, count,
> 				     flags);
> 	if (ret == -EOPNOTSUPP || ret == -EXDEV)
> 		ret = generic_copy_file_range(file_in, pos_in, file_out,
> 					      pos_out, count, flags);
> 	return ret;
> }
> 
> This masks the EOPNOTSUPP error from the native copy_file_range
> implementation and calls generic_copy_file_range even if do_copy_file_range
> does not.

Right, so that's explain the WARN_ON_ONCE on vfs_copy_file_range.

In any case, I still think it is not an expected semantic that kernel returns
ENOSYS for invalid arguments.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug build/26338] io/tst-copy_file_range fails on RHEL 7.8 hosts

[romain.geissler at amadeus dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=26338

--- Comment #9 from Romain Geissler <romain.geissler at amadeus dot com> ---
Hi,

So in the end shall we change the test to try non-zero copy at first ?

Or shall we update it so that each and every calls to "copy_file_range" shall
check the return code ENOSYS and then mark the test as unsupported ?

Or shall we just do nothing, and mention that this test is not supposed to work
correctly on RHEL 7.x kernels ?

Cheers,
Romain

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug build/26338] io/tst-copy_file_range fails on RHEL 7.8 hosts

[fweimer at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=26338

--- Comment #10 from Florian Weimer <fweimer at redhat dot com> ---
(In reply to Romain Geissler from comment #9)
> So in the end shall we change the test to try non-zero copy at first ?
> 
> Or shall we update it so that each and every calls to "copy_file_range"
> shall check the return code ENOSYS and then mark the test as unsupported ?

I think we should change the test to do a non-noop copy because I think it's
required to detect file systems where splice fallback does not work. We should
accept EINVAL and ENOSYS errors there and flag the test as unsupported. Other
errors should remain test failures.

Accepting ENOSYS throughout the test would be more controversial because
argument-dependent ENOSYS *is* iffy, and maybe we should not encourage that.
Doing a first probe call sidesteps the issue.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug build/26338] io/tst-copy_file_range may incorrectly detect availability of copy_file_range file system support

[fweimer at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=26338

Florian Weimer <fweimer at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|io/tst-copy_file_range      |io/tst-copy_file_range may
                   |fails on RHEL 7.8 hosts     |incorrectly detect
                   |                            |availability of
                   |                            |copy_file_range file system
                   |                            |support

-- 
You are receiving this mail because:
You are on the CC list for the bug.