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

fs: use io.Copy because go supports CopyFileRange #227

Merged
merged 1 commit into from Jun 16, 2023

Conversation

fuweid
Copy link
Member

@fuweid fuweid commented Jun 9, 2023

@dmcgowan
Copy link
Member

dmcgowan commented Jun 9, 2023

I wonder if we could just get rid of this function all together, for unix we still do

func copyFileContent(dst, src *os.File) error {
	buf := bufferPool.Get().(*[]byte)
	_, err := io.CopyBuffer(dst, src, *buf)
	bufferPool.Put(buf)

	return err
}

Based on the Go code though, this buffer will never be used for os.File to os.File copying though
https://github.com/golang/go/blob/master/src/os/file.go#L161

@fuweid fuweid force-pushed the use-io-copy-from-copy-file branch from a747c08 to 577e360 Compare June 9, 2023 05:36
@fuweid
Copy link
Member Author

fuweid commented Jun 9, 2023

I wonder if we could just get rid of this function all together

If the go runtime move the ReadFrom into linux platform, I guess it will be possible.

Updated. Please take a look.

@fuweid fuweid force-pushed the use-io-copy-from-copy-file branch 2 times, most recently from 0be4dfb to 80e9692 Compare June 9, 2023 06:11
fs/copy.go Outdated
tgtWriter = onlyWriter{tgt}
}

buf := bufferPool.Get().(*[]byte)
Copy link
Member

Choose a reason for hiding this comment

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

The buffer didn't even end up getting used before, should we even bother with it rather than checking OS. It could be possible for Go to implement copy optimizations on other platforms as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated. Let's use io.Copy directly.

REF: golang/go@7be3f09

Signed-off-by: Wei Fu <fuweid89@gmail.com>
@fuweid fuweid force-pushed the use-io-copy-from-copy-file branch from 80e9692 to 4b8bec5 Compare June 13, 2023 15:53
@kzys kzys merged commit 1e0d26e into containerd:main Jun 16, 2023
12 checks passed
@fuweid fuweid deleted the use-io-copy-from-copy-file branch June 17, 2023 00:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants