Skip to content

FileSystem.Unix: CopyFile: use copy_file_range on Linux. - #64264

Merged
adamsitnik merged 7 commits into
dotnet:mainfrom
tmds:cfr
Jan 31, 2022
Merged

FileSystem.Unix: CopyFile: use copy_file_range on Linux.#64264
adamsitnik merged 7 commits into
dotnet:mainfrom
tmds:cfr

Conversation

@tmds

@tmdstmds commented Jan 25, 2022

Copy link
Copy Markdown
Member

@ghostghost added community-contribution Indicates that the PR has been added by a community member area-System.IO labels Jan 25, 2022
@ghost

ghost commented Jan 25, 2022

Copy link
Copy Markdown

Tagging subscribers to this area: @dotnet/area-system-io
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #30156.

@adamsitnik ptal.

cc @lpereira@filipnavara

Author:tmds
Assignees:-
Labels:

area-System.IO, community-contribution

Milestone:-

am11
am11 reviewed Jan 25, 2022
View reviewed changes
Comment threadsrc/native/libs/System.Native/pal_io.c Outdated
filipnavara
filipnavara reviewed Jan 25, 2022
View reviewed changes
Comment threadsrc/libraries/Common/src/Interop/Unix/System.Native/Interop.CopyFile.cs Outdated
jkotas
jkotas reviewed Jan 25, 2022
View reviewed changes
Comment threadsrc/libraries/Common/src/Interop/Unix/System.Native/Interop.CopyFile.cs Outdated
stephentoub
stephentoub reviewed Jan 25, 2022
View reviewed changes
Comment threadsrc/libraries/Common/src/Interop/Unix/System.Native/Interop.CopyFile.cs Outdated
jkotas
jkotas reviewed Jan 25, 2022
View reviewed changes
Comment threadsrc/libraries/Common/src/Interop/Unix/System.Native/Interop.CopyFile.cs Outdated
filipnavara
filipnavara reviewed Jan 25, 2022
View reviewed changes
Comment threadsrc/native/libs/System.Native/pal_io.c
am11
am11 reviewed Jan 25, 2022
View reviewed changes
Comment threadsrc/native/libs/System.Native/pal_io.c
jkotas
jkotas reviewed Jan 26, 2022
View reviewed changes
Comment threadsrc/native/libs/System.Native/pal_io.c Outdated
filipnavara
filipnavara reviewed Jan 26, 2022
View reviewed changes
Comment threadsrc/native/libs/System.Native/pal_io.c Outdated
tmds
tmds commented Jan 26, 2022
View reviewed changes
Comment threadsrc/native/libs/System.Native/pal_io.c
adamsitnik
adamsitnik reviewed Jan 26, 2022
View reviewed changes

@adamsitnikadamsitnik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@tmds could you please provide benchmark numbers that justify the change?

@tmds

tmds commented Jan 27, 2022•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

@tmds could you please provide benchmark numbers that justify the change?

The main benefit is that unlike sendfile this is a file system operation, and the filesystem may optimize for it.

When I copy a file over SMB using the existing .NET sendfile implementation, all the data passes through my machine.

$ time dotnet /tmp/console/bin/Debug/net6.0/console.dll /tmp/share/Fedora.iso /tmp/share/Fedora_sendfile.iso
real	0m29.196s
user	0m0.051s
sys	0m1.748s

With copy_file_range it does not.

$ time ./corerun /tmp/console/bin/Debug/net6.0/console.dll /tmp/share/Fedora.iso /tmp/share/Fedora_cfr.iso
real	0m0.591s
user	0m0.097s
sys	0m0.161s

adamsitnik
adamsitnik reviewed Jan 27, 2022
View reviewed changes
Comment threadsrc/native/libs/System.Native/pal_io.c
Comment threadsrc/native/libs/System.Native/pal_io.c Outdated
adamsitnik
adamsitnik approved these changes Jan 28, 2022
View reviewed changes

@adamsitnikadamsitnik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, big thanks for taking care of all of the edge cases! 👍

@adamsitnikadamsitnik added this to the 7.0.0 milestone Jan 28, 2022
@adamsitnik
adamsitnik merged commit 215c328 into dotnet:mainJan 31, 2022
@adamsitnikadamsitnik added the tenet-performance Performance related issue label Jan 31, 2022
@Thefrank

Thefrank commented Feb 1, 2022

Copy link
Copy Markdown
Contributor

This already got merged, but FreeBSD starting with 13.0-STABLE also has copy_file_range: https://www.freebsd.org/cgi/man.cgi?query=copy_file_range&sektion=2&format=html

@adamsitnikadamsitnik mentioned this pull request Feb 9, 2022
Closed
@ghostghost locked as resolved and limited conversation to collaborators Mar 3, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@stephentoubstephentoubstephentoub left review comments
@jkotasjkotasjkotas left review comments
@adamsitnikadamsitnikadamsitnik approved these changes
+2 more reviewers
@filipnavarafilipnavarafilipnavara approved these changes
@am11am11am11 left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area-System.IOcommunity-contributionIndicates that the PR has been added by a community membertenet-performancePerformance related issue

Projects

None yet

Milestone

7.0.0

Development

Successfully merging this pull request may close these issues.

SystemNative_CopyFile() should use copy_file_range() in Linux

7 participants

@tmds@Thefrank@filipnavara@stephentoub@am11@adamsitnik@jkotas