Fix copy_file_range usage for files > 2GB #114

cpuguy83 · 2018-05-16T14:28:23Z

copy_file_range (or any Linux syscall) will only ever copy 2GB at a
time. In this case there is no error returned from the system call.

This fix uses copy_file_range in a loop until either 0 bytes are copied,
or the desired amount has been copied (st.Size()).

cpuguy83 · 2018-05-16T14:30:06Z

ping @stevvooe @dmcgowan @AkihiroSuda @Random-Liu

This is a nasty gotcha. Considering this a P0 for moby/moby.
I suspect cri-containerd volume copying and containerd's image unpack is broken for any directories that contain files > 2GB.

cpuguy83 · 2018-05-16T14:31:09Z

fs/copy_linux.go


-	if int64(n) != st.Size() {
-		return errors.Wrapf(err, "short copy: %d of %d", int64(n), st.Size())


Note that this errors.Wrap is incorrect and always returns nil.

justincormack · 2018-05-16T14:37:29Z

fs/copy_linux.go

-		bufferPool.Put(buf)
-		return err
-	}
+	first := true


you arent setting this to false later

Fixed. It's probably useless, but figured why not have the extra protection.

justincormack · 2018-05-16T14:39:16Z

fs/copy_linux.go

+		first = false
+		written += int64(n)
+	}
+	if written != size {


this surely cant happen?

Likely not, but I do break if n == 0.

i.e., I guess only if there was a bug in the kernel in the go syscall implementation.

its not really worth checking for random kernel bugs

justincormack · 2018-05-16T14:44:31Z

fs/copy_linux.go

-	}
+	first := true
+	for written < size {
+		n, err := unix.CopyFileRange(int(src.Fd()), nil, int(dst.Fd()), nil, int(size-written), 0)


you shouldnt cast size-written to int, it could be larger. you will need a check for is it >2G and then write a smaller amount given the length arg is an int.

(the syscall doesnt take an int here so in C you would be able to pass a 64 bit value)

estesp

LGTM

crosbymichael · 2018-05-16T16:18:28Z

fs/copy_linux.go

@@ -43,28 +43,41 @@ func copyFileInfo(fi os.FileInfo, name string) error {
 	return nil
 }

+const maxCopySize = 2147483647 // max 32bit integer


Could we just use math.MaxInt32 here?

Certainly. Didn't want to add the import just to get one number.

kolyshkin · 2018-05-16T17:22:18Z

fs/copy_linux.go

+
+	for written < size {
+		var desired int
+		if size-written > math.MaxInt32 {


this size-written looks ugly without spaces, but apparently it's gofmt way. So I'd suggest to rewrite it as

desired := size - written if desired > math.MaxInt32 { desired=int(math.MaxInt32) }

kolyshkin · 2018-05-16T18:03:50Z

fs/copy_linux.go

+
+	for written < size {
+		var desired int
+		if size-written > math.MaxInt32 {


OK in fact I think we should not do something like this instead:

sfd := int(src.Fd()) dfd := int(dst.Fd()) size := st.Size() for size > 0 { n, err := unix.CopyFileRange(sfd, nil, dfd, nil, int(size), 0) if err != nil { // ... } size -= n }

This way we don't have any knowledge of how big the size can be, but just relying on the fact that the syscall returns the number of bytes actually copied.

Note that in here int(size) can indeed be negative, but it is internally used as size_t, ie an unsigned type.

Yeah, I was wondering why it didn't fail before when I did this. This makes sense.

Random-Liu · 2018-05-16T22:32:52Z

LGTM. Will update the vendor once this is merged.

kolyshkin · 2018-05-17T16:18:01Z

fs/copy_test.go

@@ -46,6 +46,45 @@ func TestCopyDirectoryWithLocalSymlink(t *testing.T) {
 	}
 }

+// TestCopyWithLargeFile tests copying a file whose size is > 32bit.


Maybe size > 2^32 bytes

kolyshkin

LGTM; a nit in a comment (s/perrmission/permission/)

copy_file_range (or any Linux syscall) will only ever copy 2GB at a time. In this case there is no error returned from the system call. This fix uses copy_file_range in a loop until either 0 bytes are copied, or the desired amount has been copied (st.Size()). Signed-off-by: Brian Goff <[email protected]>

dmcgowan · 2018-05-21T21:26:18Z

fs/copy_test.go

@@ -46,6 +46,45 @@ func TestCopyDirectoryWithLocalSymlink(t *testing.T) {
 	}
 }

+// TestCopyWithLargeFile tests copying a file whose size > 2^32 bytes.
+func TestCopyWithLargeFile(t *testing.T) {


I'll consider this for a follow up, but skipping on testing.Short() may be a good idea

dmcgowan · 2018-05-21T21:27:56Z

fs/copy_test.go

@@ -46,6 +46,45 @@ func TestCopyDirectoryWithLocalSymlink(t *testing.T) {
 	}
 }

+// TestCopyWithLargeFile tests copying a file whose size > 2^32 bytes.
+func TestCopyWithLargeFile(t *testing.T) {
+	dataR, dataW := io.Pipe()


I think a pseudorandom limit reader would make it cleaner

dmcgowan · 2018-05-21T21:28:41Z

Some comments on test, I will address those in a follow up. Thanks for getting this in.

LGTM

cpuguy83 mentioned this pull request May 16, 2018

Populating volume from container seems to be broken for files bigger than 2Gb docker/for-mac#2905

Closed

3 tasks

cpuguy83 commented May 16, 2018

View reviewed changes

cpuguy83 force-pushed the fix_copy_file_range_usage branch from 2735bca to 3293598 Compare May 16, 2018 14:36

justincormack reviewed May 16, 2018

View reviewed changes

cpuguy83 force-pushed the fix_copy_file_range_usage branch from 3293598 to 1e30e55 Compare May 16, 2018 14:38

justincormack suggested changes May 16, 2018

View reviewed changes

cpuguy83 force-pushed the fix_copy_file_range_usage branch from 1e30e55 to 6a20386 Compare May 16, 2018 14:39

justincormack reviewed May 16, 2018

View reviewed changes

cpuguy83 force-pushed the fix_copy_file_range_usage branch 2 times, most recently from 2399682 to 1b54a75 Compare May 16, 2018 15:06

justincormack approved these changes May 16, 2018

View reviewed changes

cpuguy83 force-pushed the fix_copy_file_range_usage branch from 1b54a75 to c8b9d21 Compare May 16, 2018 15:37

estesp approved these changes May 16, 2018

View reviewed changes

AkihiroSuda approved these changes May 16, 2018

View reviewed changes

crosbymichael reviewed May 16, 2018

View reviewed changes

crosbymichael requested a review from dmcgowan May 16, 2018 16:19

cpuguy83 force-pushed the fix_copy_file_range_usage branch from c8b9d21 to f77b648 Compare May 16, 2018 16:37

kolyshkin suggested changes May 16, 2018

View reviewed changes

cpuguy83 force-pushed the fix_copy_file_range_usage branch 2 times, most recently from edbcbea to b652955 Compare May 17, 2018 13:36

kolyshkin reviewed May 17, 2018

View reviewed changes

cpuguy83 force-pushed the fix_copy_file_range_usage branch from b652955 to 7a7385b Compare May 17, 2018 16:20

kolyshkin approved these changes May 17, 2018

View reviewed changes

cpuguy83 force-pushed the fix_copy_file_range_usage branch from 7a7385b to afba265 Compare May 17, 2018 16:41

dmcgowan reviewed May 21, 2018

View reviewed changes

dmcgowan merged commit 7333bda into containerd:master May 21, 2018

cpuguy83 deleted the fix_copy_file_range_usage branch May 21, 2018 21:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix copy_file_range usage for files > 2GB #114

Fix copy_file_range usage for files > 2GB #114

cpuguy83 commented May 16, 2018

cpuguy83 commented May 16, 2018

cpuguy83 May 16, 2018

justincormack May 16, 2018

cpuguy83 May 16, 2018

justincormack May 16, 2018

cpuguy83 May 16, 2018

cpuguy83 May 16, 2018

justincormack May 16, 2018

justincormack May 16, 2018

justincormack May 16, 2018

cpuguy83 May 16, 2018

estesp left a comment

crosbymichael May 16, 2018

cpuguy83 May 16, 2018

cpuguy83 May 16, 2018

kolyshkin May 16, 2018

kolyshkin May 16, 2018

cpuguy83 May 16, 2018

cpuguy83 May 17, 2018

Random-Liu commented May 16, 2018

kolyshkin May 17, 2018

cpuguy83 May 17, 2018

kolyshkin left a comment

dmcgowan May 21, 2018

dmcgowan May 21, 2018

dmcgowan commented May 21, 2018


		if int64(n) != st.Size() {
		return errors.Wrapf(err, "short copy: %d of %d", int64(n), st.Size())

Fix copy_file_range usage for files > 2GB #114

Fix copy_file_range usage for files > 2GB #114

Conversation

cpuguy83 commented May 16, 2018

cpuguy83 commented May 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

estesp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Random-Liu commented May 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kolyshkin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmcgowan commented May 21, 2018