Use copy_file_range(2) to implement System.Native.CopyFile() #33159

filipnavara · 2020-03-04T15:10:59Z

Revival of #1495 and fix for #31429.

This cleans up the file locking code in PAL and the code paths introduced in dotnet/corefx#39235 to support clonefile API on macOS. Additionally, it introduces support for the copy_file_range API on newer Linux systems:

   The  copy_file_range()  system  call performs an in-kernel
   copy between two file descriptors without the addi‐ tional
   cost of transferring data from the kernel to user space and
   then back into the kernel.

   (...)

   copy_file_range()  gives  filesystems  an opportunity to
   implement "copy acceleration" techniques, such as the use of
   reflinks (i.e., two or more inodes that share pointers to the
   same copy-on-write disk blocks) or server-side-copy (in the
   case of NFS).

filipnavara · 2020-03-04T15:11:52Z

Damn, should have been draft PR.

src/libraries/Native/Unix/System.Native/pal_io.c

filipnavara · 2020-03-04T20:31:09Z

@lpereira I cannot test the Linux part locally at the moment but it seems that at least the macOS part works correctly now, including the new test. The last CI failure is infrastructure issue that would likely go away on re-trigger.

lpereira · 2020-03-04T20:59:02Z

Thanks for looking a this, @filipnavara!

carlossanlop · 2020-04-16T21:16:49Z

@filipnavara I converted it back to draft.

filipnavara · 2020-04-16T21:19:11Z

@carlossanlop Yeah, it is basically done but we need to figure out whether @lpereira's PR will be the master one or this one.

carlossanlop · 2020-04-16T21:19:49Z

I suggest we use @lpereira 's PR as the main one since it has most of the comment history, and we should close this one.

filipnavara · 2020-04-16T21:20:50Z

@carlossanlop Fine with me, but this one is based on top of it and adds more fixes to pass all the tests. I cannot edit someone else's PRs. The fixes are essential to get this in.

carlossanlop · 2020-04-16T21:28:08Z

@lpereira if you're still going to work on this, can you please make sure to apply Filip's changes in your PR?

carlossanlop · 2020-04-23T21:16:51Z

@filipnavara let's make this PR the main one. @lpereira may not be able to continue working on this issue since he moved to another team.

filipnavara · 2020-04-23T21:18:33Z

I will update the description.

This is in preparation to introduce a different method of copying files on Linux, using the copy_file_range(2) system call.

From the man page: The copy_file_range() system call performs an in-kernel copy between two file descriptors without the addi‐ tional cost of transferring data from the kernel to user space and then back into the kernel. (...) copy_file_range() gives filesystems an opportunity to implement "copy acceleration" techniques, such as the use of reflinks (i.e., two or more inodes that share pointers to the same copy-on-write disk blocks) or server-side-copy (in the case of NFS).

There's already code to handle the existence of O_CLOEXEC in the system, among other things, so just call the function to clean things up slightly.

Since all the different copying methods are different across supported operating systems, it's better to move this logic to where the rubber meets the road. This replicates the behavior of the FileStream instance that used to be used by the managed portion of this API.

The input file is now locked correctly with shared lock instead of exclusive one. The output file is locked with exclusive lock to match previous behavior of .NET Core 3.0. Added comment explaining why explict unlocking in necessary before closing the file.

…ange

danmoseley · 2020-08-10T22:22:05Z

@filipnavara I apologize for the delay - I assume this is still waiting for review.

@carlossanlop @jozkee from discussion with @stephentoub I think it's important we take this (or reverse the original break) to resolve a breaking change since 3.1. Could one of you please shepherd this?

danmoseley · 2020-08-10T22:22:34Z

At this point reverting the original change might be safest (?)

cc @stephentoub

filipnavara · 2020-08-10T22:29:11Z

I am open to either finishing this PR or reverting the original one. I wanted to produce some benchmarks to show the benefit of the change but due to many unforseen circumstances I ended up stranded in different countries and recently most of my test hardware was stolen. It may take me a few days to get back some reasonable setup to test the PR but I am committed to doing it.

stephentoub · 2020-08-10T22:34:43Z

@filipnavara, I'm very sorry to hear about all your troubles. Please let us know if we can help with anything.

At this point, we should revert, and try again for 6.0.

Thanks!

danmoseley · 2020-08-10T23:03:19Z

@stephentoub that means to revert 02ba75a right? Looks like that will need some conflict fixing.

carlossanlop · 2020-08-11T00:02:18Z

@jozkee will work on reverting that change.

danmoseley · 2020-08-11T00:13:41Z

@filipnavara that sounds like an awful experience, I'm sorry to hear about it. We appreciate your work, and it's too bad it didn't quite make this release, but it's unlikely to be wasted.

stephentoub · 2020-09-14T21:28:58Z

@filipnavara, thanks for all your work on this. Seems like this PR should be closed and we can start with a new one from scratch if you're still interested in trying to make this work?

filipnavara · 2020-09-15T04:31:52Z

We can either close it or I can reapply the commits and rebase it. Not sure what is better.

stephentoub · 2020-09-15T11:06:42Z

Thanks, @filipnavara. Let's close this then and start over with a new PR. Hopefully we can find a clean approach that achieves your perf goals while maintaining most of the code in C# and avoiding the managed/native duplication. Thanks!

rainersigwald · 2021-06-01T17:30:03Z

Did this ever get picked up again? This weekend, I wondered if we should have MSBuild do clonefile and then found a few issues where we wouldn't have to because it would be done for us, but the current state is "backed out" and I don't see a work item to track for the "hopefully we can turn it back on in 6.0" part. Am I missing one?

danmoseley · 2021-06-01T20:49:42Z

@rainersigwald it did not.

rainersigwald · 2021-06-01T21:29:55Z

https://twitter.com/filipnavara/status/1399839335256633348?s=20 -- @filipnavara is still thinking about it, but no tracking issue at the moment. Thanks, folks!

filipnavara · 2021-06-02T09:02:53Z

I am planning to revive this along with more unit tests and benchmarks. The primary motivation was to speed up our builds on macOS. It's mostly a convenience thing for us since it's not something that affects the product we ship to customers. I was prioritizing lately PRs that were blocking migration of our product to net6.0, some of which require coordination on different repositories (eg. Xamarin). That said, it is getting on top of my backlog again.

stephentoub · 2021-06-07T15:30:29Z

@adamsitnik, @carlossanlop, @jozkee, @jeffhandley ... one of the issues hit here originally was related to the FileShare functionality we attempt to partially mimic on Unix. As such, whatever decision is made on #44546 will impact work done here.

Dotnet-GitSync-Bot added the area-System.IO label Mar 4, 2020

filipnavara changed the title ~~CI test: Use copy_file_range(2) to implement System.Native.CopyFile()~~ WIP: Use copy_file_range(2) to implement System.Native.CopyFile() Mar 4, 2020

filipnavara force-pushed the pr-1495 branch from 0f38970 to ab8df4f Compare March 4, 2020 17:04

lpereira reviewed Mar 4, 2020

View reviewed changes

src/libraries/Native/Unix/System.Native/pal_io.c Show resolved Hide resolved

lpereira reviewed Mar 4, 2020

View reviewed changes

src/libraries/Native/Unix/System.Native/pal_io.c Show resolved Hide resolved

carlossanlop marked this pull request as draft April 16, 2020 21:15

carlossanlop mentioned this pull request Apr 16, 2020

Use copy_file_range(2) to implement System.Native.CopyFile() #1495

Closed

carlossanlop assigned filipnavara Apr 23, 2020

filipnavara changed the title ~~WIP: Use copy_file_range(2) to implement System.Native.CopyFile()~~ Use copy_file_range(2) to implement System.Native.CopyFile() Apr 23, 2020

lpereira and others added 10 commits April 23, 2020 23:38

System.Native: Break SystemNative_CopyFile() into smaller functions

6fd7753

This is in preparation to introduce a different method of copying files on Linux, using the copy_file_range(2) system call.

System.Native: Cache availability of sendfile() system call

faea5f0

System.Native: Refactor file metadata copying when copying files

08fcce5

System.Native: Use SystemNative_Open() to handle O_CLOEXEC et al

2657f6a

There's already code to handle the existence of O_CLOEXEC in the system, among other things, so just call the function to clean things up slightly.

System.IO.FileSystem: Add test for File.Copy onto locked file

2ed8d78

Remove unnecessary double loop.

f2ac619

Update the loop in CopyFile_CopyFileRange to account for file size ch…

f5fcc86

…ange

filipnavara force-pushed the pr-1495 branch from 952f431 to f5fcc86 Compare April 23, 2020 21:43

jaredpar mentioned this pull request Apr 24, 2020

OSX machines are de-provisioned during CI / PR runs leading to failures #34472

Closed

filipnavara marked this pull request as ready for review April 24, 2020 06:19

stephentoub added this to the 5.0.0 milestone Jun 28, 2020

danmoseley mentioned this pull request Aug 8, 2020

File.Copy no longer respects FileShare.None on Linux/Mac in 5.0 #31429

Closed

danmoseley requested a review from carlossanlop August 10, 2020 22:20

danmoseley requested a review from jozkee August 10, 2020 22:22

danmoseley modified the milestones: 5.0.0, Future Sep 1, 2020

stephentoub closed this Sep 15, 2020

ghost locked as resolved and limited conversation to collaborators Dec 10, 2020

filipnavara deleted the pr-1495 branch April 21, 2021 10:54

dotnet unlocked this conversation Jun 2, 2021

ghost locked as resolved and limited conversation to collaborators Jul 7, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use copy_file_range(2) to implement System.Native.CopyFile() #33159

Use copy_file_range(2) to implement System.Native.CopyFile() #33159

filipnavara commented Mar 4, 2020 •

edited

Loading

filipnavara commented Mar 4, 2020

filipnavara commented Mar 4, 2020

lpereira commented Mar 4, 2020

carlossanlop commented Apr 16, 2020

filipnavara commented Apr 16, 2020

carlossanlop commented Apr 16, 2020

filipnavara commented Apr 16, 2020 •

edited

Loading

carlossanlop commented Apr 16, 2020

carlossanlop commented Apr 23, 2020

filipnavara commented Apr 23, 2020

danmoseley commented Aug 10, 2020

danmoseley commented Aug 10, 2020

filipnavara commented Aug 10, 2020

stephentoub commented Aug 10, 2020

danmoseley commented Aug 10, 2020

carlossanlop commented Aug 11, 2020

danmoseley commented Aug 11, 2020

stephentoub commented Sep 14, 2020

filipnavara commented Sep 15, 2020

stephentoub commented Sep 15, 2020

rainersigwald commented Jun 1, 2021

danmoseley commented Jun 1, 2021

rainersigwald commented Jun 1, 2021

filipnavara commented Jun 2, 2021

stephentoub commented Jun 7, 2021

Use copy_file_range(2) to implement System.Native.CopyFile() #33159

Use copy_file_range(2) to implement System.Native.CopyFile() #33159

Conversation

filipnavara commented Mar 4, 2020 • edited Loading

filipnavara commented Mar 4, 2020

filipnavara commented Mar 4, 2020

lpereira commented Mar 4, 2020

carlossanlop commented Apr 16, 2020

filipnavara commented Apr 16, 2020

carlossanlop commented Apr 16, 2020

filipnavara commented Apr 16, 2020 • edited Loading

carlossanlop commented Apr 16, 2020

carlossanlop commented Apr 23, 2020

filipnavara commented Apr 23, 2020

danmoseley commented Aug 10, 2020

danmoseley commented Aug 10, 2020

filipnavara commented Aug 10, 2020

stephentoub commented Aug 10, 2020

danmoseley commented Aug 10, 2020

carlossanlop commented Aug 11, 2020

danmoseley commented Aug 11, 2020

stephentoub commented Sep 14, 2020

filipnavara commented Sep 15, 2020

stephentoub commented Sep 15, 2020

rainersigwald commented Jun 1, 2021

danmoseley commented Jun 1, 2021

rainersigwald commented Jun 1, 2021

filipnavara commented Jun 2, 2021

stephentoub commented Jun 7, 2021

filipnavara commented Mar 4, 2020 •

edited

Loading

filipnavara commented Apr 16, 2020 •

edited

Loading