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

Add fallback for copy_file_range EXDEV error #3756

Merged
merged 2 commits into from Sep 7, 2022

Conversation

Xist12gh
Copy link
Contributor

@Xist12gh Xist12gh commented Sep 4, 2022

Fixes #3654

Add fallback to other copy routines in case of error in previous attempt.
I tried to keep it generic, but for now it targets EXDEV error in copy_file_range explicitly.

For efficiency sake, I kept the tiered approach to available kernel routines which was previously selected by compile flags.
To do the fallback in runtime I had to duplicate some code. The effort to refactor this went a bit above the scope of a bugfix. At least for now it keeps the change readily accessible for review.
Added some comments for the one trying to clean this up.

Functional changes:
Previously:
Try one of copy_file_range or sendfile64 in this order selected during compile time, if it fails exit tr_sys_path_copy
Use user-space copy if neither was available during compile time

Now:
First try copy_file_range if available during compile time.
Try sendfile64 if available in case of previous EXDEV or when copy_file_range wasn't available.
Then try user-space copy if neither kernel routine was available during compile time or previous EXDEV errno was set

Copy link
Member

@ckerr ckerr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FTBFS:

/usr/home/user/buildAgent/work/1f3bff43dd55cfe9/libtransmission/file-posix.cc:491:5: error: unknown type name 'error_t'; did you mean 'errno_t'?

@kisol-pw
Copy link

kisol-pw commented Sep 5, 2022

Xist12gh here:
hm, as I feared, too environment specific.
well, let's not get overambitious and go with the traditional int then.
I'll push an updated when I get off work in a few hours.

@Xist12gh
Copy link
Contributor Author

Xist12gh commented Sep 5, 2022

Updated PR

  • rebased to 72a1696
  • switched to traditional int for errno
  • fixed unneeded logging of EXDEV even though the fallback catches it, somehow slipped the original testing

Let's see if this works now.

If nobody beats me to it, I'll probably refactor the duplicated parts in a later PR once this one proves to run stable.

Xist12gh and others added 2 commits September 6, 2022 13:31
…ssion#3654)

* allow fallback to other copy routines based on errno

* tiered kernel copy routines are tried in runtime now when available
@ckerr ckerr merged commit 32a4709 into transmission:main Sep 7, 2022
ckerr added a commit that referenced this pull request Sep 7, 2022
@ckerr ckerr mentioned this pull request Sep 7, 2022
ckerr added a commit that referenced this pull request Sep 7, 2022
* fixup! Add fallback for copy_file_range EXDEV error (#3756)

* fixup! refactor: use readability-identifier-naming in clang-tidy (#3784)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

Invalid cross-device: copy_file_range changed in Kernel 5.18
3 participants