Have VFS graphdriver use accelerated in-kernel copy by sargun · Pull Request #35537 · moby/moby

sargun · 2017-11-17T18:27:11Z

- What I did
This change makes the VFS graphdriver use the kernel-accelerated
(copy_file_range) mechanism of copying files, which is able to
leverage reflinks.

The copy package should fulfill all of the capabilities that CopyWithTar
fulfilled as well.

I learned a lot about file system performance on Linux. At least about XFS on machines with reasonably fast I/O, and many cores with lots of memory. The primary barrier here is not disk I/O, as it turns out, but contention inside of the dentry, inode, and allocation group.
- How I did it
I reused the accelerated copy code I had written for the overlay graphdriver.

- How to verify it
I used the existing tests to verify the behaviour.

- Description for the changelog
Use copy_file_range to copy files for VFS on Linux.

cpuguy83 · 2017-11-17T18:37:41Z

daemon/graphdriver/vfs/copy_linux.go

Can we just define dirCopy() as a function in both copy_linux and copy_unsupported instead of setting this var?

This var gets changed in by layer/layer_test.go -- why it changes it, I'm not sure. I would feel comfortable changing it, if I knew why it existed.

fyi, this is because the chrootarchive archiver performs a re-exec which does some shadow magic and the unit test is not prepared for that.

cpuguy83 · 2017-11-17T18:52:07Z

daemon/graphdriver/vfs/driver.go

No need for this one, the functions can call copyDir directly.

Oh nevermind I see the layerstore test is using this... something about nice things...

@cpuguy83 Anything else then?

Is chrootarchive really necessary now, without it then there would be no need to expose this, especially since Linux no longer needs it...

What would we do with the non-windows bits? I can do it, but I'd not want to do it in this PR.

cpuguy83 · 2017-11-17T19:33:09Z

Looks like experimental got stuck in a syscall:

Details

goroutine 4277 [runnable]:
syscall.Syscall(0x4a, 0x13, 0x0, 0x0, 0x0, 0x0, 0x0)
	/usr/local/go/src/syscall/asm_linux_amd64.s:18 +0x5
syscall.Fsync(0x13, 0x10, 0x96cdb8)
	/usr/local/go/src/syscall/zsyscall_linux_amd64.go:492 +0x4a
os.(*File).Sync(0xc4216e04f0, 0x1e53018, 0xc4217ae148)
	/usr/local/go/src/os/file_posix.go:121 +0x5b
github.com/docker/docker/pkg/ioutils.(*atomicFileWriter).Close(0xc421bcef90, 0x0, 0x0)
	/go/src/github.com/docker/docker/pkg/ioutils/fswriters.go:68 +0x81
github.com/docker/docker/pkg/ioutils.AtomicWriteFile(0xc42134cb00, 0x71, 0xc421090000, 0xe57, 0xe57, 0xc400000180, 0xc420f24990, 0x2b899b0)
	/go/src/github.com/docker/docker/pkg/ioutils/fswriters.go:41 +0xcb
github.com/docker/docker/image.(*fs).Set(0xc420477e30, 0xc421090000, 0xe57, 0xe57, 0x0, 0x0, 0x0, 0x0)
	/go/src/github.com/docker/docker/image/fs.go:121 +0x122
github.com/docker/docker/image.(*store).Create(0xc42022f1a0, 0xc421090000, 0xe57, 0xe57, 0x0, 0x0, 0x0, 0x0)
	/go/src/github.com/docker/docker/image/store.go:146 +0x2c8
github.com/docker/docker/daemon.(*Daemon).Commit(0xc4203c3200, 0xc421c28580, 0x40, 0xc421b39d80, 0x0, 0x0, 0x0, 0x0)
	/go/src/github.com/docker/docker/daemon/commit.go:207 +0x696
github.com/docker/docker/builder/dockerfile.(*Builder).commitContainer(0xc4203eb4a0, 0xc4213c66c0, 0xc421c28580, 0x40, 0xc42123cc80, 0x2b9cba0, 0xc4217ddf60)
	/go/src/github.com/docker/docker/builder/dockerfile/internals.go:117 +0x117
github.com/docker/docker/builder/dockerfile.dispatchRun(0xc4213c66c0, 0xc420addd04, 0xc4203eb4a0, 0x2bafae0, 0xc421af1260, 0xc420feafc0, 0xc42048ff00, 0xc420feafc0, 0xc4217aed00)
	/go/src/github.com/docker/docker/builder/dockerfile/dispatchers.go:356 +0xa7e
github.com/docker/docker/builder/dockerfile.dispatch(0xc4213c66c0, 0xc420addd04, 0xc4203eb4a0, 0x2bafae0, 0xc421af1260, 0xc420feafc0, 0x7fa8b459a008, 0xc42048ff00, 0x0, 0x0)
	/go/src/github.com/docker/docker/builder/dockerfile/evaluator.go:81 +0x457
github.com/docker/docker/builder/dockerfile.(*Builder).dispatchDockerfileWithCancellation(0xc4203eb4a0, 0xc421a77d10, 0x1, 0x1, 0x0, 0x0, 0x0, 0x5c, 0x2bafae0, 0xc421af1260, ...)
	/go/src/github.com/docker/docker/builder/dockerfile/builder.go:322 +0x940
github.com/docker/docker/builder/dockerfile.(*Builder).build(0xc4203eb4a0, 0x2bafae0, 0xc421af1260, 0xc42048f240, 0x0, 0x0, 0x0)
	/go/src/github.com/docker/docker/builder/dockerfile/builder.go:240 +0x3bb
github.com/docker/docker/builder/dockerfile.(*BuildManager).Build(0xc4203b87c0, 0x7fa8b679bba0, 0xc42048f500, 0x2bacf20, 0xc420107280, 0x2b9bd60, 0xc4217ddf40, 0x2b9cba0, 0xc4217ddf60, 0x2b9cba0, ...)
	/go/src/github.com/docker/docker/builder/dockerfile/builder.go:125 +0x4aa
github.com/docker/docker/api/server/backend/build.(*Backend).Build(0xc42085eae0, 0x7fa8b679bba0, 0xc420107380, 0x2bacf20, 0xc420107280, 0x2b9bd60, 0xc4217ddf40, 0x2b9cba0, 0xc4217ddf60, 0x2b9cba0, ...)

Restarting it.

cpuguy83 · 2017-11-17T20:16:46Z

Same failure:

panic: DockerSuite.TestBuildAddAndCopyFileWithWhitespace test timed out after 5m0s [recovered]

cpuguy83 · 2017-11-17T20:38:57Z

It looks like there's some other problems as well.
Janky failed on docker-py tests.

powerpc and z failed on TestCommitHardlink

Details

19:43:33 FAIL: docker_cli_commit_test.go:67: DockerSuite.TestCommitHardlink
19:43:33 
19:43:33 docker_cli_commit_test.go:84:
19:43:33     c.Assert(chunks[1], checker.Contains, chunks[0], check.Commentf("Failed to create hardlink in a container. Expected to find %q in %q", inode, chunks[1:]))
19:43:33 ... obtained string = "\x1b[0;0mfile1\x1b[0m  1466359 \x1b[0;0mfile2\x1b[0m"
19:43:33 ... substring string = "1466358 "
19:43:33 ... Failed to create hardlink in a container. Expected to find "1466358" in ["\x1b[0;0mfile1\x1b[0m  1466359 \x1b[0;0mfile2\x1b[0m"]

cpuguy83 · 2017-11-17T20:39:31Z

These errors are likely showing up now because CI runs on VFS.

sargun · 2017-11-20T22:47:03Z

@cpuguy83 It seems like the hardlink was creating the issue. I can break the hardlink fix into its own separate commit if you want, but otherwise, things look good to me.

cpuguy83 · 2017-11-21T00:02:06Z

Separate commit may be good.
Can we add a unit test for hardlinks as well?

sargun · 2017-11-21T16:29:10Z

There is a test for hard links.

cpuguy83 · 2017-11-21T16:40:34Z

Looks good.
One more nit (outside of splitting the fix for the dir copy hardlinks) is our use of syscall instead of golang.org/x/sys/unix.

sargun · 2017-11-21T18:27:47Z

@cpuguy83 I made the test use golang.org/x/sys/unix, but the old code in copy.go is still using syscall.

sargun · 2017-11-21T20:59:59Z

I don't see the Windows build being scheduled?

thaJeztah · 2017-11-21T21:01:23Z

Let me restart, there was an issue earlier with Jenkins

sargun · 2017-11-21T23:51:10Z

Finally. @cpuguy83 Split the commit. Changed the tests. Should be good to go.

sargun · 2017-11-22T03:25:50Z

Some benchmarks:

reflinks:
time docker create centos:latest echo
3dd564d5823994ed54f8e2d646210b4aebcc7622fb6e35d353483057f9d4dd23

real    0m1.154s
user    0m0.008s
sys     0m0.004s

copy_file_range, no reflinks:
time docker create centos:latest echo
737917a226317df9a2f4f7c46d9ed1e83e32a16f4f827eee5e5730f29e61e441

real    0m1.147s
user    0m0.016s
sys     0m0.004s

legacy:
time docker create centos:latest echo
bd925697d2067a6cde175d6a13b4c25f8c23e926422d32303f06e5084647b045

real    0m1.685s
user    0m0.012s
sys     0m0.008s

sargun · 2017-12-01T03:04:13Z

It seems like users are no longer able to restart their own Jenkins builds. Is this intentional?

sargun · 2017-12-01T03:14:48Z

If it helps, we could merge the non-lolspeed version in, and then I can do another PR with the concurrency stuffs.

cpuguy83

Right now there's a lot of complexity that seems like we could simplify.

There are currently 3 different concurrency primitives in use.
If we can change this to use at most two concurrency primitives (e.g. chan + a waitgroup to simplify chan handling) that will be a good start.

Not sure what to do with the linked list... it's probably ok but don't often see it in go code. Can we use a slice instead?

sargun · 2017-12-01T12:42:30Z

The linked list seems like a better approach than a slice to me because the slice will require constant resizes / reallocations. I don't see any benefit of using a slice, other than potential perf cost.

RE: Concurrency primitives --
Really, there's the waitgroup + chans coordinating the divide and conquer. We could replace some of that by a golang.org/x/sync/errgroup, (https://godoc.org/golang.org/x/sync/errgroup), but I'm not sure how much better that is?

I don't see a better way to deal with the inode references.

sargun · 2017-12-01T12:45:10Z

To me, errgroup would be yet another set of primitives.

cpuguy83 · 2017-12-01T13:02:13Z

There's (multiple) mutex + (multiple) chan + waitgroup.

sargun · 2017-12-01T13:07:00Z

I mean, I can also break out the concurrency code into a separate module, if that helps? I don't see a particularly good way to actually remove many of these unless we start using interface{} in places, or overloading?

Previously, the code would set the mtime on the directories before creating files in the directory itself. This was problematic because it resulted in the mtimes on the directories being incorrectly set. This change makes it so that the mtime is set only _after_ all of the files have been created. Signed-off-by: Sargun Dhillon <sargun@sargun.me>

cpuguy83

LGTM

sargun · 2017-12-04T16:56:55Z

DockerSwarmSuite.TestSwarmLockUnlockCluster failed. Can someone restart janky?

sargun requested a review from dmcgowan as a code owner November 17, 2017 18:27

GordonTheTurtle added the status/0-triage label Nov 17, 2017

cpuguy83 requested changes Nov 17, 2017

View reviewed changes

cpuguy83 added status/2-code-review and removed status/0-triage labels Nov 17, 2017

sargun force-pushed the vfs-use-copy_file_range branch from af1e226 to 65222b7 Compare November 17, 2017 18:49

cpuguy83 reviewed Nov 17, 2017

View reviewed changes

cpuguy83 added the rebuild/experimental label Nov 17, 2017

GordonTheTurtle removed the rebuild/experimental label Nov 17, 2017

sargun force-pushed the vfs-use-copy_file_range branch from 65222b7 to 45d7b26 Compare November 20, 2017 20:27

sargun closed this Nov 21, 2017

sargun force-pushed the vfs-use-copy_file_range branch from 45d7b26 to ab90bc2 Compare November 21, 2017 18:25

sargun reopened this Nov 21, 2017

sargun force-pushed the vfs-use-copy_file_range branch 2 times, most recently from 0c48441 to a5e7681 Compare November 21, 2017 18:49

thaJeztah added the rebuild/windowsRS1 label Nov 21, 2017

GordonTheTurtle removed the rebuild/windowsRS1 label Nov 21, 2017

sargun force-pushed the vfs-use-copy_file_range branch 2 times, most recently from b40b11a to 5a082b0 Compare December 1, 2017 02:50

sargun force-pushed the vfs-use-copy_file_range branch from 5a082b0 to 77ab7c2 Compare December 1, 2017 03:06

cpuguy83 requested changes Dec 1, 2017

View reviewed changes

sargun force-pushed the vfs-use-copy_file_range branch from 77ab7c2 to 9b16a95 Compare December 1, 2017 17:07

sargun force-pushed the vfs-use-copy_file_range branch from 9b16a95 to 77a2bc3 Compare December 1, 2017 17:12

sargun changed the title ~~Have VFS graphdriver use accelerated in-kernel copy; Make accelerated In kernel copy fast~~ Have VFS graphdriver use accelerated in-kernel copy Dec 1, 2017

sargun mentioned this pull request Dec 1, 2017

Make accelerated In kernel copy fast by increasing concurrency #35676

Closed

GordonTheTurtle assigned LK4D4 Dec 2, 2017

cpuguy83 added the rebuild/janky label Dec 4, 2017

GordonTheTurtle removed the rebuild/janky label Dec 4, 2017

cpuguy83 added the rebuild/z label Dec 4, 2017

GordonTheTurtle removed the rebuild/z label Dec 4, 2017

cpuguy83 approved these changes Dec 4, 2017

View reviewed changes

cpuguy83 added the status/4-merge label Dec 4, 2017

yongtang merged commit 4047ced into moby:master Dec 5, 2017

thaJeztah added the impact/changelog label Dec 5, 2017

sargun deleted the vfs-use-copy_file_range branch January 25, 2018 20:11

redbaron mentioned this pull request Feb 16, 2018

backport recent copy_file_range changes from moby containers/storage#151

Closed

thaJeztah mentioned this pull request Jul 19, 2021

VFS storage driver drops all file capabilities when creating a container #42655

Closed

thaJeztah mentioned this pull request Oct 13, 2021

Fix file capabilites droping in Dockerfile #42934

Merged

thaJeztah mentioned this pull request Nov 14, 2023

graphdriver/copy: faster copy of hard links #46812

Merged

Conversation

sargun commented Nov 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sargun Nov 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cpuguy83 commented Nov 17, 2017

Uh oh!

cpuguy83 commented Nov 17, 2017

Uh oh!

cpuguy83 commented Nov 17, 2017

Uh oh!

cpuguy83 commented Nov 17, 2017

Uh oh!

sargun commented Nov 20, 2017

Uh oh!

cpuguy83 commented Nov 21, 2017

Uh oh!

sargun commented Nov 21, 2017

Uh oh!

cpuguy83 commented Nov 21, 2017

Uh oh!

sargun commented Nov 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sargun commented Nov 21, 2017

Uh oh!

thaJeztah commented Nov 21, 2017

Uh oh!

sargun commented Nov 21, 2017

Uh oh!

sargun commented Nov 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sargun commented Dec 1, 2017

Uh oh!

sargun commented Dec 1, 2017

Uh oh!

cpuguy83 left a comment

Choose a reason for hiding this comment

Uh oh!

sargun commented Dec 1, 2017

Uh oh!

sargun commented Dec 1, 2017

Uh oh!

cpuguy83 commented Dec 1, 2017

Uh oh!

sargun commented Dec 1, 2017

Uh oh!

cpuguy83 left a comment

Choose a reason for hiding this comment

Uh oh!

sargun commented Dec 4, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

sargun commented Nov 17, 2017 •

edited

Loading

sargun Nov 27, 2017 •

edited

Loading

sargun commented Nov 21, 2017 •

edited

Loading

sargun commented Nov 22, 2017 •

edited

Loading