Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[beta] always disable copy_file_range to avoid EOVERFLOW errors #79008

Merged
merged 1 commit into from
Nov 15, 2020

Conversation

the8472
Copy link
Member

@the8472 the8472 commented Nov 12, 2020

A bigger hammer as alternative to #79007

Pro: will certainly fix the issue
Cons: will disable copy_file_range for everyone

Resolves #78979

@rust-highfive
Copy link
Collaborator

r? @withoutboats

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive
Copy link
Collaborator

⚠️ Warning ⚠️

  • Pull requests are usually filed against the master branch for this repo, but this one is against beta. Please double check that you specified the right target!

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 12, 2020
@Mark-Simulacrum
Copy link
Member

r? @Mark-Simulacrum

This is going to just be a (possible) performance hit, right? Do we have numbers on it (perhaps from a past PR)? I am wondering basically if the fix here is worth the performance loss; maybe we should gate the disable on e.g. mips or something like that.

cc @pietroalbini this or #79007 will likely want to land into 1.48

@the8472
Copy link
Member Author

the8472 commented Nov 12, 2020

This is going to just be a (possible) performance hit, right? Do we have numbers on it (perhaps from a past PR)? I am wondering basically if the fix here is worth the performance loss; maybe we should gate the disable on e.g. mips or something like that.

It's not just performance. copy_file_range can also save disk space if you're on btrfs or xfs by creating reflink copies, That might also be relevant for anything running in certain container setups.

As for the performance numbers:

# userspace copy

test io::tests::bench_file_to_file_copy         ... bench:      21,002 ns/iter (+/- 750) = 6240 MB/s    [ext4]
test io::tests::bench_file_to_file_copy         ... bench:      35,704 ns/iter (+/- 1,108) = 3671 MB/s  [btrfs]

# copy_file_range

test io::tests::bench_file_to_file_copy         ... bench:      14,745 ns/iter (+/- 519) = 8889 MB/s    [ext4]
test io::tests::bench_file_to_file_copy         ... bench:       6,128 ns/iter (+/- 227) = 21389 MB/s   [btrfs]

@Mark-Simulacrum
Copy link
Member

Ok, the performance impact is not too big and this is the most minimal patch, so I think it's the right thing to backport to 1.48 and 1.49 (once that branches).

@bors r+ rollup=never p=1

@bors
Copy link
Contributor

bors commented Nov 14, 2020

📌 Commit d19e2de has been approved by Mark-Simulacrum

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 14, 2020
@mati865
Copy link
Contributor

mati865 commented Nov 14, 2020

Ok, the performance impact is not too big and this is the most minimal patch

I find this statement a bit confusing when looking at the results.
With BTRFS (and very likely XFS) it's quite a performance difference and delaying reflink support is a pity.

That said it's not a regression/performance degradation but an optimisation delayed by few more weeks (with proper reason) so the decision is understandable.

@Mark-Simulacrum
Copy link
Member

Yes, and ultimately (AFAIK) use of such file systems is somewhat unusual, though not entirely unexpected. I also somewhat expect that the specialization here is likely not used by cases where it is absolutely critical to get this behavior - those likely want to call the relevant APIs directly. Once we get more assurance that this all works and roll it out for a while we may provide more guarantees here

@bors
Copy link
Contributor

bors commented Nov 15, 2020

⌛ Testing commit d19e2de with merge a238a370d9a199d57826b528681c572fa0929f03...

@bors
Copy link
Contributor

bors commented Nov 15, 2020

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 15, 2020
@Mark-Simulacrum
Copy link
Member

@bors retry spurious apple lldb error

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 15, 2020
@bors
Copy link
Contributor

bors commented Nov 15, 2020

⌛ Testing commit d19e2de with merge 61bc95a...

@bors
Copy link
Contributor

bors commented Nov 15, 2020

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing 61bc95a to beta...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 15, 2020
@bors bors merged commit 61bc95a into rust-lang:beta Nov 15, 2020
@rustbot rustbot added this to the 1.48.0 milestone Nov 15, 2020
@Mark-Simulacrum Mark-Simulacrum added beta-accepted Accepted for backporting to the compiler in the beta channel. beta-nominated Nominated for backporting to the compiler in the beta channel. labels Nov 17, 2020
@Mark-Simulacrum
Copy link
Member

beta-nominating & accepting this for backport (really forward port) to 1.49, which has just branched.

@the8472
Copy link
Member Author

the8472 commented Nov 17, 2020

Do you need a separate PR for that?

@Mark-Simulacrum
Copy link
Member

No, I will pick it up as a part of regular beta backports, likely sometime in the next week or two.

@Mark-Simulacrum Mark-Simulacrum removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Dec 8, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 8, 2020
…ulacrum

[beta] backports

* [beta] always disable copy_file_range to avoid EOVERFLOW errors rust-lang#79008
* Enforce no-move rule of ReentrantMutex using Pin and fix UB in stdio rust-lang#77801
* bootstrap: use the same version number for rustc and cargo rust-lang#79133
* [beta] Revert "Enable ASLR for windows-gnu" rust-lang#79141
* [beta] revert rust-lang#78790, vendor libtest for rustc-src rust-lang#79571
* Mirror centos vault to S3 rust-lang#79435
*  [beta] Update cargo rust-lang#79739

This also bumps to non-dev stable compiler.

r? `@ghost`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta-accepted Accepted for backporting to the compiler in the beta channel. merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants