rust
rust copied to clipboard
rust-lang
Less syscalls for the `copy_file_range` probe
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
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
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.
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.
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
EOPNOTSUPPon old RHELs.
The current version should be functionally equivalent to the old code, just that we omit one syscall.
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.
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.
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.
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.
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?
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
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
fnprobe 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