Navigation Menu

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

io: use syscalls like copy_file_range in Copy when possible #36817

Closed
mvdan opened this issue Jan 27, 2020 · 16 comments
Closed

io: use syscalls like copy_file_range in Copy when possible #36817

mvdan opened this issue Jan 27, 2020 · 16 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Performance

Comments

@mvdan
Copy link
Member

mvdan commented Jan 27, 2020

@stapelberg just made me aware of the copy_file_range syscall. Quoting the summary:

The copy_file_range() system call performs an in-kernel copy between two file descriptors without the additional cost of transferring data from the kernel to user space and then back into the kernel. It copies up to len bytes of data from the source file descriptor fd_in to the target file descriptor fd_out, overwriting any data that exists within the requested range of the target file.

It's also already supported by the sys repo: https://godoc.org/golang.org/x/sys/unix#CopyFileRange

When calling Copy, given that the conditions below are met:

  • runtime.GOOS == "linux"
  • Linux is version 4.5 or newer (when it was added)
  • The source and destination are *os.File

Then, the syscall would be used to make a faster copy. I presume that we could do something similar on other platforms in the future, if they have similar syscalls.

I think CopyN and CopyBuffer could also be included here, but they are left out initially for the sake of simplicity. For example, CopyBuffer could simply ignore the buffer argument when taking the syscall shortcut, as no user space memory is required.

There is only one tricky detail here, to my mind:

If src implements the WriterTo interface, the copy is implemented by calling src.WriteTo(dst). Otherwise, if dst implements the ReaderFrom interface, the copy is implemented by calling dst.ReadFrom(src).

To not break semantics and backwards compatibility, if either of those methods exists, the syscall shortcut can't be taken. One would rely on the method having a similar syscall shortcut, if needed. Luckily, it seems like os.File implements neither method, so we're in the clear.

@mvdan mvdan added Performance NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Jan 27, 2020
@networkimprov
Copy link

networkimprov commented Jan 27, 2020

Isn't splice(2) and/or sendfile(2) already called by io.Copy?
http://man7.org/linux/man-pages/man2/splice.2.html
http://man7.org/linux/man-pages/man2/sendfile.2.html

copy_file_range(2) is recommended different on a kernel >= 5.3
http://man7.org/linux/man-pages/man2/copy_file_range.2.html

@ianlancetaylor
Copy link
Contributor

splice and sendfile are called by io.Copy via the (*TCPConn).ReadFrom method. I think we could add a (*os.File).ReadFrom method that calls copy_file_range.

@beoran
Copy link

beoran commented Jan 28, 2020

While this can be a good performance improvement, only on kernels > 5.3 cross-file-system copies are supported. A fallback must be implemented in case copy_file_range returns -EXDEV.

@mvdan
Copy link
Member Author

mvdan commented Jan 28, 2020

@ianlancetaylor thanks for the insight. I semeed to remember splice being used for TCP. Feel free to retitle the issue to be about os.File.ReadFrom.

@beoran the original proposal did mention this, but in the context of io.Copy. I'm not sure if there is an agreed upon way to handle fallbacks in io.ReaderFrom. I guess calling io.Copy is one option. This edge case was simpler when we were dealing with the copy funcs.

@ianlancetaylor
Copy link
Contributor

The same fallback issues arise for splice. In that case if the ReadFrom method is unable to use splice, or sendfile, we call genericReadFrom in the net package, which is simply

func genericReadFrom(w io.Writer, r io.Reader) (n int64, err error) {
	// Use wrapper to hide existing r.ReadFrom from io.Copy.
	return io.Copy(writerOnly{w}, r)
}

@mvdan
Copy link
Member Author

mvdan commented Apr 19, 2020

It seems like we generally agree that this is doable, so I'm going to replace NeedsDecision with NeedsFix. If anyone wants to work on this, feel free to send a CL.

@mvdan mvdan added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Apr 19, 2020
@acln0
Copy link
Contributor

acln0 commented Apr 21, 2020

I am going to work on this and send a CL soon.

@gopherbot
Copy link

Change https://golang.org/cl/229101 mentions this issue: os, internal/poll, internal/syscall/unix: use copy_file_range on Linux

xujianhai666 pushed a commit to xujianhai666/go-1 that referenced this issue May 21, 2020
Linux 4.5 introduced (and Linux 5.3 refined) the copy_file_range
system call, which allows file systems the opportunity to implement
copy acceleration techniques. This commit adds support for
copy_file_range(2) to the os package.

Introduce a new ReadFrom method on *os.File, which makes *os.File
implement the io.ReaderFrom interface. If dst and src are both files,
this enables io.Copy(dst, src) to call dst.ReadFrom(src), which, in
turn, will call copy_file_range(2) if possible. If copy_file_range(2)
is not supported by the host kernel, or if either of dst or src
refers to a non-regular file, ReadFrom falls back to the regular
io.Copy code path.

Add internal/poll.CopyFileRange, which acquires locks on the
appropriate poll.FDs and performs the actual work, as well as
internal/syscall/unix.CopyFileRange, which wraps the copy_file_range
system call itself at the lowest level.

Rework file layout in internal/syscall/unix to accomodate the
additional system call numbers needed for copy_file_range.
Merge these definitions with the ones used by getrandom(2) into
sysnum_linux_$GOARCH.go files.

A note on additional optimizations: if dst and src both refer to pipes
in the invocation dst.ReadFrom(src), we could, in theory, use the
existing splice(2) code in package internal/poll to splice directly
from src to dst. Attempting this runs into trouble with the poller,
however. If we call splice(src, dst) and see EAGAIN, we cannot know
if it came from src not being ready for reading or dst not being
ready for writing. The write end of src and the read end of dst are
not under our control, so we cannot reliably use the poller to wait
for readiness. Therefore, it seems infeasible to use the new ReadFrom
method to splice between pipes directly. In conclusion, for now, the
only optimization enabled by the new ReadFrom method on *os.File is
the copy_file_range optimization.

Fixes golang#36817.

Change-Id: I696372639fa0cdf704e3f65414f7321fc7d30adb
Reviewed-on: https://go-review.googlesource.com/c/go/+/229101
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@gopherbot
Copy link

Change https://golang.org/cl/238864 mentions this issue: doc/go1.15: mention consequence of os.File.ReadFrom

gopherbot pushed a commit that referenced this issue Jun 25, 2020
Now that we've added a os.File.ReadFrom method, io.CopyBuffer to a
os.File will no longer use the provided buffer.

For #16474
For #36817
For #37419

Change-Id: I79a3bf778ff93eab88e88dd9ecbb8c7ea101e868
Reviewed-on: https://go-review.googlesource.com/c/go/+/238864
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
@ianlancetaylor
Copy link
Contributor

There is a discussion at https://lore.kernel.org/linux-fsdevel/20210126233840.GG4626@dread.disaster.area/T/#m05753578c7f7882f6e9ffe01f981bc223edef2b0 that suggests that perhaps the Go standard library should not use copy_file_range after all.

CC @acln0

@acln0
Copy link
Contributor

acln0 commented Feb 1, 2021

Spooky. It seems like we should remove all of the copy_file_range code indeed. I can send a CL soon-ish if you'd like me to, but I don't know if the tree is open yet.

Now that we have (*os.File).ReadFrom and we can't remove it anymore, maybe we can look into hooking up the existing splice code, as the e-mails suggest. But I think we should start with just removing all the copy_file_range code and leaving a generic io.Copy forwarder behind in (*os.File).ReadFrom.

@beoran
Copy link

beoran commented Feb 1, 2021

Yes, from that discussion on the Linux kernel mailing list, it seem to me that copy_file_range seems to be a Linux system call that is hard to use correctly in the general case, since it's not easy to fall back to other system calls in case it fails. Getting rid of it might cost us some performance but does have the benefit of simplifying code and lowering the requirements for the Linux kernel version.
EDIT: Could someone please reopen this issue? Thanks!

@mvdan mvdan reopened this Feb 1, 2021
@mvdan
Copy link
Member Author

mvdan commented Feb 1, 2021

@beoran reopened.

If a future Linux release makes copy_file_range easier to use correctly, we could always try using it again.

@gopherbot
Copy link

Change https://golang.org/cl/291989 mentions this issue: internal/poll: if copy_file_range returns 0, assume it failed

@ianlancetaylor
Copy link
Contributor

I've opened an issue for the recently discovered problem at #44272. Let's use that for this specific problem rather than this general issue.

gopherbot pushed a commit that referenced this issue Feb 16, 2021
On current Linux kernels copy_file_range does not correctly handle
files in certain special file systems, such as /proc. For those file
systems it fails to copy any data and returns zero. This breaks Go's
io.Copy for those files.

Fix the problem by assuming that if copy_file_range returns 0 the
first time it is called on a file, that that file is not supported.
In that case fall back to just using read. This will force an extra
system call when using io.Copy to copy a zero-sized normal file,
but at least it will work correctly.

For #36817
Fixes #44272

Change-Id: I02e81872cb70fda0ce5485e2ea712f219132e614
Reviewed-on: https://go-review.googlesource.com/c/go/+/291989
Trust: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>
@gopherbot
Copy link

Change https://golang.org/cl/292289 mentions this issue: [release-branch.go1.15] internal/poll: if copy_file_range returns 0, assume it failed

gopherbot pushed a commit that referenced this issue Feb 16, 2021
…assume it failed

On current Linux kernels copy_file_range does not correctly handle
files in certain special file systems, such as /proc. For those file
systems it fails to copy any data and returns zero. This breaks Go's
io.Copy for those files.

Fix the problem by assuming that if copy_file_range returns 0 the
first time it is called on a file, that that file is not supported.
In that case fall back to just using read. This will force an extra
system call when using io.Copy to copy a zero-sized normal file,
but at least it will work correctly.

For #36817
For #44272
Fixes #44273

Change-Id: I02e81872cb70fda0ce5485e2ea712f219132e614
Reviewed-on: https://go-review.googlesource.com/c/go/+/291989
Trust: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>
(cherry picked from commit 30641e3)
Reviewed-on: https://go-review.googlesource.com/c/go/+/292289
@golang golang locked and limited conversation to collaborators Feb 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Performance
Projects
None yet
Development

No branches or pull requests

6 participants