Skip to content

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

Merged
kzys merged 1 commit into
containerd:mainfrom
fuweid:use-io-copy-from-copy-file
Jun 16, 2023
Merged

fs: use io.Copy because go supports CopyFileRange#227
kzys merged 1 commit into
containerd:mainfrom
fuweid:use-io-copy-from-copy-file

Conversation

@fuweid

@fuweid fuweid commented Jun 9, 2023

Copy link
Copy Markdown
Member

@dmcgowan

dmcgowan commented Jun 9, 2023

Copy link
Copy Markdown
Member

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

fuweid commented Jun 9, 2023

Copy link
Copy Markdown
Member Author

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
Comment thread fs/copy.go Outdated
tgtWriter = onlyWriter{tgt}
}

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

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.

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
Copy Markdown
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
@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.

3 participants