Less syscalls for the `copy_file_range` probe

[tbu-]

If it's obvious from the actual syscall results themselves that the syscall is supported or unsupported, don't do an extra syscall with an invalid file descriptor.

CC #122052

[rustbot]

r? @joboet

rustbot has assigned @joboet. They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[rust-log-analyzer]

The job mingw-check-tidy failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

Getting action download info
Download action repository 'msys2/[email protected]' (SHA:cc11e9188b693c2b100158c3322424c4cc1dadea)
Download action repository 'actions/checkout@v4' (SHA:b4ffde65f46336ab88eb53be808477a3936bae11)
Download action repository 'actions/upload-artifact@v3' (SHA:a8a3f3ad30e3422c9c7b888a15615d19a852ae32)
Complete job name: PR - mingw-check-tidy
git config --global core.autocrlf false
shell: /usr/bin/bash --noprofile --norc -e -o pipefail {0}
---
COPY scripts/sccache.sh /scripts/
RUN sh /scripts/sccache.sh

COPY host-x86_64/mingw-check/reuse-requirements.txt /tmp/
RUN pip3 install --no-deps --no-cache-dir --require-hashes -r /tmp/reuse-requirements.txt \
    && pip3 install virtualenv
COPY host-x86_64/mingw-check/validate-toolstate.sh /scripts/
COPY host-x86_64/mingw-check/validate-error-codes.sh /scripts/

# NOTE: intentionally uses python2 for x.py so we can test it still works.
# NOTE: intentionally uses python2 for x.py so we can test it still works.
# validate-toolstate only runs in our CI, so it's ok for it to only support python3.
ENV SCRIPT TIDY_PRINT_DIFF=1 python2.7 ../x.py test \
           --stage 0 src/tools/tidy tidyselftest --extra-checks=py:lint
# This file is autogenerated by pip-compile with Python 3.10
# by the following command:
#
#    pip-compile --allow-unsafe --generate-hashes reuse-requirements.in
---

#10 [5/8] COPY host-x86_64/mingw-check/reuse-requirements.txt /tmp/
#10 DONE 0.0s

#11 [6/8] RUN pip3 install --no-deps --no-cache-dir --require-hashes -r /tmp/reuse-requirements.txt     && pip3 install virtualenv
#11 0.660   Downloading binaryornot-0.4.4-py2.py3-none-any.whl (9.0 kB)
#11 0.676 Collecting boolean-py==4.0
#11 0.683   Downloading boolean.py-4.0-py3-none-any.whl (25 kB)
#11 0.700 Collecting chardet==5.1.0
---
#11 3.779 Building wheels for collected packages: reuse
#11 3.780   Building wheel for reuse (pyproject.toml): started
#11 4.111   Building wheel for reuse (pyproject.toml): finished with status 'done'
#11 4.112   Created wheel for reuse: filename=reuse-1.1.0-cp310-cp310-manylinux_2_35_x86_64.whl size=181117 sha256=f5f58750481f69515c2c0d1d503daf565e2565c370d07fc6aeb95fe3498b4269
#11 4.112   Stored in directory: /tmp/pip-ephem-wheel-cache-gu2n2pos/wheels/c2/3c/b9/1120c2ab4bd82694f7e6f0537dc5b9a085c13e2c69a8d0c76d
#11 4.115 Installing collected packages: boolean-py, binaryornot, setuptools, reuse, python-debian, markupsafe, license-expression, jinja2, chardet
#11 4.137   Attempting uninstall: setuptools
#11 4.138     Found existing installation: setuptools 59.6.0
#11 4.139     Not uninstalling setuptools at /usr/lib/python3/dist-packages, outside environment /usr
#11 4.139     Not uninstalling setuptools at /usr/lib/python3/dist-packages, outside environment /usr
#11 4.139     Can't uninstall 'setuptools'. No files were found to uninstall.
#11 4.806 Successfully installed binaryornot-0.4.4 boolean-py-4.0 chardet-5.1.0 jinja2-3.1.2 license-expression-30.0.0 markupsafe-2.1.1 python-debian-0.1.49 reuse-1.1.0 setuptools-66.0.0
#11 4.806 WARNING: Running pip as the 'root' user can result in broken permissions and conflicting behaviour with the system package manager. It is recommended to use a virtual environment instead: https://pip.pypa.io/warnings/venv
#11 5.326 Collecting virtualenv
#11 5.374   Downloading virtualenv-20.25.1-py3-none-any.whl (3.8 MB)
#11 5.565      ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 3.8/3.8 MB 20.1 MB/s eta 0:00:00
#11 5.616 Collecting platformdirs<5,>=3.9.1
#11 5.623   Downloading platformdirs-4.2.0-py3-none-any.whl (17 kB)
#11 5.642 Collecting distlib<1,>=0.3.7
#11 5.649   Downloading distlib-0.3.8-py2.py3-none-any.whl (468 kB)
#11 5.692 Collecting filelock<4,>=3.12.2
#11 5.699   Downloading filelock-3.13.1-py3-none-any.whl (11 kB)
#11 5.699   Downloading filelock-3.13.1-py3-none-any.whl (11 kB)
#11 5.783 Installing collected packages: distlib, platformdirs, filelock, virtualenv
#11 5.946 Successfully installed distlib-0.3.8 filelock-3.13.1 platformdirs-4.2.0 virtualenv-20.25.1
#11 DONE 6.0s

#12 [7/8] COPY host-x86_64/mingw-check/validate-toolstate.sh /scripts/
#12 DONE 0.0s
---
DirectMap4k:      190400 kB
DirectMap2M:     8198144 kB
DirectMap1G:    10485760 kB
##[endgroup]
Executing TIDY_PRINT_DIFF=1 python2.7 ../x.py test            --stage 0 src/tools/tidy tidyselftest --extra-checks=py:lint
+ TIDY_PRINT_DIFF=1 python2.7 ../x.py test --stage 0 src/tools/tidy tidyselftest --extra-checks=py:lint
    Finished dev [unoptimized] target(s) in 0.03s
##[endgroup]
downloading https://ci-artifacts.rust-lang.org/rustc-builds-alt/80399064afa4a2cd153f30d02c25f7ea0383ed65/rust-dev-nightly-x86_64-unknown-linux-gnu.tar.xz
extracting /checkout/obj/build/cache/llvm-80399064afa4a2cd153f30d02c25f7ea0383ed65-true/rust-dev-nightly-x86_64-unknown-linux-gnu.tar.xz to /checkout/obj/build/x86_64-unknown-linux-gnu/ci-llvm
---
   Compiling tidy v0.1.0 (/checkout/src/tools/tidy)
    Finished release [optimized] target(s) in 26.74s
##[endgroup]
fmt check
##[error]Diff in /checkout/library/std/src/sys/pal/unix/kernel_copy.rs at line 626:
                                 // supported and some other error (ENOSYS or
                                 // EPERM) if it's not available.
                                 let result = unsafe {
-                                    cvt(copy_file_range(INVALID_FD, ptr::null_mut(), INVALID_FD, ptr::null_mut(), 1, 0))
+                                    cvt(copy_file_range(
+                                        INVALID_FD,
+                                        INVALID_FD,
+                                        ptr::null_mut(),
+                                        1,
+                                        0,
+                                        0,
+                                    ))
                                 };
 
-                                if matches!(result.map_err(|e| e.raw_os_error()), Err(Some(EBADF))) {
+                                if matches!(result.map_err(|e| e.raw_os_error()), Err(Some(EBADF)))
+                                {
                                     HAS_COPY_FILE_RANGE.store(AVAILABLE, Ordering::Relaxed);
                                     HAS_COPY_FILE_RANGE.store(UNAVAILABLE, Ordering::Relaxed);
                                     HAS_COPY_FILE_RANGE.store(UNAVAILABLE, Ordering::Relaxed);
Running `"/checkout/obj/build/x86_64-unknown-linux-gnu/rustfmt/bin/rustfmt" "--config-path" "/checkout" "--edition" "2021" "--unstable-features" "--skip-children" "--check" "/checkout/library/std/src/sys/pal/wasi/io.rs" "/checkout/library/std/src/sys/pal/wasi/mod.rs" "/checkout/library/std/src/sys/pal/wasi/net.rs" "/checkout/library/std/src/sys/pal/wasi/stdio.rs" "/checkout/library/std/src/sys/pal/wasi/os.rs" "/checkout/library/std/src/sys/pal/hermit/thread.rs" "/checkout/library/std/src/sys/pal/hermit/args.rs" "/checkout/library/std/src/sys/pal/hermit/env.rs" "/checkout/library/std/src/sys/pal/hermit/fs.rs" "/checkout/library/std/src/sys/pal/hermit/time.rs" "/checkout/library/std/src/sys/pal/hermit/fd.rs" "/checkout/library/std/src/sys/pal/hermit/futex.rs" "/checkout/library/std/src/sys/pal/hermit/thread_local_dtor.rs" "/checkout/library/std/src/sys/pal/hermit/mod.rs" "/checkout/library/std/src/sys/pal/hermit/net.rs" "/checkout/library/std/src/sys/pal/hermit/stdio.rs" "/checkout/library/std/src/sys/pal/hermit/os.rs" "/checkout/library/std/src/sys/pal/hermit/alloc.rs" "/checkout/library/std/src/sys/pal/mod.rs" "/checkout/library/std/src/sys/pal/unix/rand.rs" "/checkout/library/std/src/sys/pal/unix/thread_local_key.rs" "/checkout/library/std/src/sys/pal/unix/process/process_unix.rs" "/checkout/library/std/src/sys/pal/unix/process/process_common.rs" "/checkout/library/std/src/sys/pal/unix/process/process_unix/tests.rs" "/checkout/library/std/src/sys/pal/unix/process/process_unsupported/wait_status.rs" "/checkout/library/std/src/sys/pal/unix/process/process_unsupported/wait_status/tests.rs" "/checkout/library/std/src/sys/pal/unix/process/mod.rs" "/checkout/library/std/src/sys/pal/unix/process/process_fuchsia.rs" "/checkout/library/std/src/sys/pal/unix/process/zircon.rs" "/checkout/library/std/src/sys/pal/unix/process/process_common/tests.rs" "/checkout/library/std/src/sys/pal/unix/process/process_vxworks.rs" "/checkout/library/std/src/sys/pal/unix/process/process_unsupported.rs" "/checkout/library/std/src/sys/pal/unix/thread.rs" "/checkout/library/std/src/sys/pal/unix/args.rs" "/checkout/library/std/src/sys/pal/unix/l4re.rs" "/checkout/library/std/src/sys/pal/unix/weak.rs" "/checkout/library/std/src/sys/pal/unix/env.rs" "/checkout/library/std/src/sys/pal/unix/fs.rs" "/checkout/library/std/src/sys/pal/unix/time.rs" "/checkout/library/std/src/sys/pal/unix/fd.rs" "/checkout/library/std/src/sys/pal/unix/io.rs" "/checkout/library/std/src/sys/pal/unix/futex.rs" "/checkout/library/std/src/sys/pal/unix/thread_local_dtor.rs" "/checkout/library/std/src/sys/pal/unix/kernel_copy.rs" "/checkout/library/std/src/sys/pal/unix/mod.rs" "/checkout/library/std/src/sys/pal/unix/android.rs" "/checkout/library/std/src/sys/pal/unix/os/tests.rs" "/checkout/library/std/src/sys/pal/unix/net.rs" "/checkout/library/std/src/sys/pal/unix/fd/tests.rs" "/checkout/library/std/src/sys/pal/unix/kernel_copy/tests.rs" "/checkout/library/std/src/sys/pal/unix/stack_overflow.rs" "/checkout/library/std/src/sys/pal/unix/stdio.rs" "/checkout/library/std/src/sys/pal/unix/thread_parking/darwin.rs" "/checkout/library/std/src/sys/pal/unix/thread_parking/pthread.rs" "/checkout/library/std/src/sys/pal/unix/thread_parking/mod.rs" "/checkout/library/std/src/sys/pal/unix/thread_parking/netbsd.rs" "/checkout/library/std/src/sys/pal/unix/os.rs" "/checkout/library/std/src/sys/pal/unix/pipe.rs" "/checkout/library/std/src/sys/pal/unix/alloc.rs" "/checkout/library/std/src/sys/pal/xous/thread_local_key.rs" "/checkout/library/std/src/sys/pal/xous/thread.rs" "/checkout/library/std/src/sys/pal/xous/net/dns.rs" "/checkout/library/std/src/sys/pal/xous/net/tcplistener.rs" "/checkout/library/std/src/sys/pal/wasi/fd.rs"` failed.
If you're running `tidy`, try again with `--bless`. Or, if you just want to format code, run `./x.py fmt` instead.
  local time: Wed Mar  6 11:39:11 UTC 2024
  network time: Wed, 06 Mar 2024 11:39:11 GMT
##[error]Process completed with exit code 1.
Post job cleanup.

[tbu-]

@rustbot ready

[tbu-]

@rustbot ready

[the8472]

This seems unnecessarily complicated because the probe is only done once per program lifetime and stored in a static.

And it obscures some edge-cases such as the EOPNOTSUPP on old RHELs.

[tbu-]

This seems unnecessarily complicated because the probe is only done once per program lifetime and stored in a static.

This basically just uses the first copy_file_range syscall to observe that it exists.

And it obscures some edge-cases such as the EOPNOTSUPP on old RHELs.

The current version should be functionally equivalent to the old code, just that we omit one syscall.

[the8472]

This basically just uses the first copy_file_range syscall to observe that it exists.

I meant the control flow complexity/case-analysis/nesting/readability of the code. Shaving off extra syscall per program lifetime doesn't seem like a great motivation for that.

The current version should be functionally equivalent to the old code, just that we omit one syscall.

That is not correct. If I recall correctly RHEL would fail with ENOSYS on some files but with EOPNOTSUPP on others. Which means depending on which files you're hitting it might never execute the probe and keep trying clone_file_range.

With the explicit probe upfront only the expected error code (EBADF) would lead to real calls.

And missing this case is exactly what I mean by the new code being more complex.

[tbu-]

That is not correct. If I recall correctly RHEL would fail with ENOSYS on some files but with EOPNOTSUPP on others. Which means depending on which files you're hitting it might never execute the probe and keep trying clone_file_range.

With the explicit probe upfront only the expected error code (EBADF) would lead to real calls.

And missing this case is exactly what I mean by the new code being more complex.

I was trying to implement exactly what the comments in the code described. To me, it's bad that they were apparently trying to cover a case it didn't describe. I'll update the comments when I update the code.

I meant the control flow complexity/case-analysis/nesting/readability of the code. Shaving off extra syscall per program lifetime doesn't seem seem like a great motivation for that.

I can optimize for low control flow complexity/case-analysis/nesting if you prefer that. The very concept of using the first call to determine syscall availability sounds sane to me. We did the same in earlier PRs, e.g. #53725.

[the8472]

I was trying to implement exactly what the comments in the code described. To me, it's bad that they were apparently trying to cover a case it didn't describe.

You're right, the comments could have been more detailed. Though there's a hint about the issue further down in the general error path. You probably should update that comment too since it refers to an "initial probe" that this PR is removing.

But ultimately the old code is a whitelist, it checks for an expected EBADF indicating the happy path under controlled conditions. Note that the old probe isn't even looking for EPERM/ENOSYS. The new code tries to interpret the more complicated results of a sad path using the syscall under less controlled conditions before probing. These additional dependencies introduce a point of failure that wasn't there before.

I can optimize for low control flow complexity/case-analysis/nesting if you prefer that. The very concept of using the first call to determine syscall availability sounds sane to me. We did the same in earlier PRs, e.g. https://github.com/rust-lang/rust/pull/53725.

Well, that was a LoC reduction and is a fairly simple bool. This one is more complicated.

[joboet]

These additional dependencies introduce a point of failure that wasn't there before.

That's not right, there aren't any new points of failure. We only return an error when the operation has succeeded before and use the fallback in all other cases.

What's changed is how many syscalls we do before falling back. I'd argue that it is better to try copy_file_range unless we know for certain that it doesn't work, as it means that the code will be able to take advantage of kernel-side improvements without changes.

I don't personally think it's too complicated, but that's quite subjective.

[workingjubilee]

If we aren't completely breaking on the aforementioned dodgy-ass kernels, is there any harm in having a somewhat dubious behavior when confronted with a kernel containing dubious patches?

[joboet]

I'm no longer convinced that this is worth the extra complexity to get rid of just one extra syscall over the lifetime of the program.

With that said, this change is totally correct, because even if we guess the wrong reason an error code, the only bad thing that will happen is that we use the fallback in more cases than necessary or call an unavailable copy_file_range again.

Leaving this up to @the8472, you know this code best. r? @the8472

[tbu-]

I'd be happier with this with the following two changes:

• lift the probe code up a few nesting levels by putting it into a fn

• probe on ENOSYS | EPERM | EOPNOTSUPP

Both done.

I don't particularly like the probe on ENOSYS, but since it only happens on old kernels, it's not that bad.

@rustbot ready

[tbu-]

Changed the comment.

@rustbot ready

[the8472]

@bors r+

[bors]

:pushpin: Commit 7d67ee5aba849939f64ed821a757518f211620a7 has been approved by the8472

It is now in the queue for this repository.

[bors]

:hourglass: Testing commit 7d67ee5aba849939f64ed821a757518f211620a7 with merge b0925697fdaec1bcddec2732bb3cf7f918b18745...

[bors]

:sunny: Test successful - checks-actions Approved by: the8472 Pushing b0925697fdaec1bcddec2732bb3cf7f918b18745 to master...

[rust-timer]

Finished benchmarking commit (b0925697fdaec1bcddec2732bb3cf7f918b18745): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.5%	[0.4%, 0.9%]	7
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.8%	[-1.8%, -1.8%]	1
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (secondary 4.0%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	4.0%	[4.0%, 4.0%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

Results (primary 0.0%, secondary 0.0%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.0%	[0.0%, 0.0%]	1
Regressions ❌ (secondary)	0.0%	[0.0%, 0.0%]	14
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.0%	[0.0%, 0.0%]	1

Bootstrap: 670.153s -> 670.186s (0.00%) Artifact size: 315.71 MiB -> 315.55 MiB (-0.05%)

[the8472]

Looks like the affected benchmarks have been bi-stable recently.