From 85f5db8154abea921c5e511941b6ccc368fdf400 Mon Sep 17 00:00:00 2001 From: Sargun Dhillon Date: Tue, 21 Nov 2017 10:11:43 -0800 Subject: [PATCH 1/4] Fix copying hardlinks in graphdriver/copy Previously, graphdriver/copy would improperly copy hardlinks as just regular files. This patch changes that behaviour, and instead the code now keeps track of inode numbers, and if it sees the same inode number again during the copy loop, it hardlinks it, instead of copying it. Signed-off-by: Sargun Dhillon Upstream-commit: b467f8b2ef21dc2239dcd136a29283ea6c3a0aee Component: engine --- .../engine/daemon/graphdriver/copy/copy.go | 14 +++++++++ .../daemon/graphdriver/copy/copy_test.go | 31 +++++++++++++++++++ 2 files changed, 45 insertions(+) diff --git a/components/engine/daemon/graphdriver/copy/copy.go b/components/engine/daemon/graphdriver/copy/copy.go index 8ec458d6a4..7ecdc09dc1 100644 --- a/components/engine/daemon/graphdriver/copy/copy.go +++ b/components/engine/daemon/graphdriver/copy/copy.go @@ -106,11 +106,19 @@ func copyXattr(srcPath, dstPath, attr string) error { return nil } +type fileID struct { + dev uint64 + ino uint64 +} + // DirCopy copies or hardlinks the contents of one directory to another, // properly handling xattrs, and soft links func DirCopy(srcDir, dstDir string, copyMode Mode) error { copyWithFileRange := true copyWithFileClone := true + // This is a map of source file inodes to dst file paths + copiedFiles := make(map[fileID]string) + err := filepath.Walk(srcDir, func(srcPath string, f os.FileInfo, err error) error { if err != nil { return err @@ -136,15 +144,21 @@ func DirCopy(srcDir, dstDir string, copyMode Mode) error { switch f.Mode() & os.ModeType { case 0: // Regular file + id := fileID{dev: stat.Dev, ino: stat.Ino} if copyMode == Hardlink { isHardlink = true if err2 := os.Link(srcPath, dstPath); err2 != nil { return err2 } + } else if hardLinkDstPath, ok := copiedFiles[id]; ok { + if err2 := os.Link(hardLinkDstPath, dstPath); err2 != nil { + return err2 + } } else { if err2 := copyRegular(srcPath, dstPath, f, ©WithFileRange, ©WithFileClone); err2 != nil { return err2 } + copiedFiles[id] = dstPath } case os.ModeDir: diff --git a/components/engine/daemon/graphdriver/copy/copy_test.go b/components/engine/daemon/graphdriver/copy/copy_test.go index 6976503e18..eb91e9d238 100644 --- a/components/engine/daemon/graphdriver/copy/copy_test.go +++ b/components/engine/daemon/graphdriver/copy/copy_test.go @@ -9,6 +9,8 @@ import ( "path/filepath" "testing" + "golang.org/x/sys/unix" + "github.com/docker/docker/pkg/parsers/kernel" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -65,3 +67,32 @@ func doCopyTest(t *testing.T, copyWithFileRange, copyWithFileClone *bool) { require.NoError(t, err) assert.Equal(t, buf, readBuf) } + +func TestCopyHardlink(t *testing.T) { + var srcFile1FileInfo, srcFile2FileInfo, dstFile1FileInfo, dstFile2FileInfo unix.Stat_t + + srcDir, err := ioutil.TempDir("", "srcDir") + require.NoError(t, err) + defer os.RemoveAll(srcDir) + + dstDir, err := ioutil.TempDir("", "dstDir") + require.NoError(t, err) + defer os.RemoveAll(dstDir) + + srcFile1 := filepath.Join(srcDir, "file1") + srcFile2 := filepath.Join(srcDir, "file2") + dstFile1 := filepath.Join(dstDir, "file1") + dstFile2 := filepath.Join(dstDir, "file2") + require.NoError(t, ioutil.WriteFile(srcFile1, []byte{}, 0777)) + require.NoError(t, os.Link(srcFile1, srcFile2)) + + assert.NoError(t, DirCopy(srcDir, dstDir, Content)) + + require.NoError(t, unix.Stat(srcFile1, &srcFile1FileInfo)) + require.NoError(t, unix.Stat(srcFile2, &srcFile2FileInfo)) + require.Equal(t, srcFile1FileInfo.Ino, srcFile2FileInfo.Ino) + + require.NoError(t, unix.Stat(dstFile1, &dstFile1FileInfo)) + require.NoError(t, unix.Stat(dstFile2, &dstFile2FileInfo)) + assert.Equal(t, dstFile1FileInfo.Ino, dstFile2FileInfo.Ino) +} From 608a03b9d5dca298fd9bdd948f37f02b98f56a60 Mon Sep 17 00:00:00 2001 From: Sargun Dhillon Date: Tue, 21 Nov 2017 10:29:27 -0800 Subject: [PATCH 2/4] Have VFS graphdriver use accelerated in-kernel copy This change makes the VFS graphdriver use the kernel-accelerated (copy_file_range) mechanism of copying files, which is able to leverage reflinks. Signed-off-by: Sargun Dhillon Upstream-commit: d2b71b26604370620630d8d3f35aba75ae474f3f Component: engine --- .../engine/daemon/graphdriver/copy/copy.go | 33 ++++++++++++------- .../daemon/graphdriver/copy/copy_test.go | 2 +- .../daemon/graphdriver/overlay/overlay.go | 4 +-- .../daemon/graphdriver/vfs/copy_linux.go | 9 +++++ .../graphdriver/vfs/copy_unsupported.go | 9 +++++ .../engine/daemon/graphdriver/vfs/driver.go | 7 ++-- components/engine/layer/layer_test.go | 2 +- 7 files changed, 47 insertions(+), 19 deletions(-) create mode 100644 components/engine/daemon/graphdriver/vfs/copy_linux.go create mode 100644 components/engine/daemon/graphdriver/vfs/copy_unsupported.go diff --git a/components/engine/daemon/graphdriver/copy/copy.go b/components/engine/daemon/graphdriver/copy/copy.go index 7ecdc09dc1..bb658c43ef 100644 --- a/components/engine/daemon/graphdriver/copy/copy.go +++ b/components/engine/daemon/graphdriver/copy/copy.go @@ -113,7 +113,9 @@ type fileID struct { // DirCopy copies or hardlinks the contents of one directory to another, // properly handling xattrs, and soft links -func DirCopy(srcDir, dstDir string, copyMode Mode) error { +// +// Copying xattrs can be opted out of by passing false for copyXattrs. +func DirCopy(srcDir, dstDir string, copyMode Mode, copyXattrs bool) error { copyWithFileRange := true copyWithFileClone := true // This is a map of source file inodes to dst file paths @@ -206,16 +208,10 @@ func DirCopy(srcDir, dstDir string, copyMode Mode) error { return err } - if err := copyXattr(srcPath, dstPath, "security.capability"); err != nil { - return err - } - - // We need to copy this attribute if it appears in an overlay upper layer, as - // this function is used to copy those. It is set by overlay if a directory - // is removed and then re-created and should not inherit anything from the - // same dir in the lower dir. - if err := copyXattr(srcPath, dstPath, "trusted.overlay.opaque"); err != nil { - return err + if copyXattrs { + if err := doCopyXattrs(srcPath, dstPath); err != nil { + return err + } } isSymlink := f.Mode()&os.ModeSymlink != 0 @@ -246,3 +242,18 @@ func DirCopy(srcDir, dstDir string, copyMode Mode) error { }) return err } + +func doCopyXattrs(srcPath, dstPath string) error { + if err := copyXattr(srcPath, dstPath, "security.capability"); err != nil { + return err + } + + // We need to copy this attribute if it appears in an overlay upper layer, as + // this function is used to copy those. It is set by overlay if a directory + // is removed and then re-created and should not inherit anything from the + // same dir in the lower dir. + if err := copyXattr(srcPath, dstPath, "trusted.overlay.opaque"); err != nil { + return err + } + return nil +} diff --git a/components/engine/daemon/graphdriver/copy/copy_test.go b/components/engine/daemon/graphdriver/copy/copy_test.go index eb91e9d238..ce99536e01 100644 --- a/components/engine/daemon/graphdriver/copy/copy_test.go +++ b/components/engine/daemon/graphdriver/copy/copy_test.go @@ -86,7 +86,7 @@ func TestCopyHardlink(t *testing.T) { require.NoError(t, ioutil.WriteFile(srcFile1, []byte{}, 0777)) require.NoError(t, os.Link(srcFile1, srcFile2)) - assert.NoError(t, DirCopy(srcDir, dstDir, Content)) + assert.NoError(t, DirCopy(srcDir, dstDir, Content, false)) require.NoError(t, unix.Stat(srcFile1, &srcFile1FileInfo)) require.NoError(t, unix.Stat(srcFile2, &srcFile2FileInfo)) diff --git a/components/engine/daemon/graphdriver/overlay/overlay.go b/components/engine/daemon/graphdriver/overlay/overlay.go index 853318de59..f22d884944 100644 --- a/components/engine/daemon/graphdriver/overlay/overlay.go +++ b/components/engine/daemon/graphdriver/overlay/overlay.go @@ -330,7 +330,7 @@ func (d *Driver) Create(id, parent string, opts *graphdriver.CreateOpts) (retErr return err } - return copy.DirCopy(parentUpperDir, upperDir, copy.Content) + return copy.DirCopy(parentUpperDir, upperDir, copy.Content, true) } func (d *Driver) dir(id string) string { @@ -446,7 +446,7 @@ func (d *Driver) ApplyDiff(id string, parent string, diff io.Reader) (size int64 } }() - if err = copy.DirCopy(parentRootDir, tmpRootDir, copy.Hardlink); err != nil { + if err = copy.DirCopy(parentRootDir, tmpRootDir, copy.Hardlink, true); err != nil { return 0, err } diff --git a/components/engine/daemon/graphdriver/vfs/copy_linux.go b/components/engine/daemon/graphdriver/vfs/copy_linux.go new file mode 100644 index 0000000000..a632d353e3 --- /dev/null +++ b/components/engine/daemon/graphdriver/vfs/copy_linux.go @@ -0,0 +1,9 @@ +// +build linux + +package vfs + +import "github.com/docker/docker/daemon/graphdriver/copy" + +func dirCopy(srcDir, dstDir string) error { + return copy.DirCopy(srcDir, dstDir, copy.Content, false) +} diff --git a/components/engine/daemon/graphdriver/vfs/copy_unsupported.go b/components/engine/daemon/graphdriver/vfs/copy_unsupported.go new file mode 100644 index 0000000000..fcc4b691a0 --- /dev/null +++ b/components/engine/daemon/graphdriver/vfs/copy_unsupported.go @@ -0,0 +1,9 @@ +// +build !linux + +package vfs + +import "github.com/docker/docker/pkg/chrootarchive" + +func dirCopy(srcDir, dstDir string) error { + return chrootarchive.NewArchiver(nil).CopyWithTar(srcDir, dstDir) +} diff --git a/components/engine/daemon/graphdriver/vfs/driver.go b/components/engine/daemon/graphdriver/vfs/driver.go index 610476fd88..a85d6a7cf0 100644 --- a/components/engine/daemon/graphdriver/vfs/driver.go +++ b/components/engine/daemon/graphdriver/vfs/driver.go @@ -7,7 +7,6 @@ import ( "github.com/docker/docker/daemon/graphdriver" "github.com/docker/docker/daemon/graphdriver/quota" - "github.com/docker/docker/pkg/chrootarchive" "github.com/docker/docker/pkg/containerfs" "github.com/docker/docker/pkg/idtools" "github.com/docker/docker/pkg/system" @@ -16,8 +15,8 @@ import ( ) var ( - // CopyWithTar defines the copy method to use. - CopyWithTar = chrootarchive.NewArchiver(nil).CopyWithTar + // CopyDir defines the copy method to use. + CopyDir = dirCopy ) func init() { @@ -133,7 +132,7 @@ func (d *Driver) create(id, parent string, size uint64) error { if err != nil { return fmt.Errorf("%s: %s", parent, err) } - return CopyWithTar(parentDir.Path(), dir) + return CopyDir(parentDir.Path(), dir) } func (d *Driver) dir(id string) string { diff --git a/components/engine/layer/layer_test.go b/components/engine/layer/layer_test.go index 6936faeb22..f632d44b90 100644 --- a/components/engine/layer/layer_test.go +++ b/components/engine/layer/layer_test.go @@ -23,7 +23,7 @@ import ( func init() { graphdriver.ApplyUncompressedLayer = archive.UnpackLayer defaultArchiver := archive.NewDefaultArchiver() - vfs.CopyWithTar = defaultArchiver.CopyWithTar + vfs.CopyDir = defaultArchiver.CopyWithTar } func newVFSGraphDriver(td string) (graphdriver.Driver, error) { From 877f5d0f1fdfc9bb8d19ff4cfcc8235c1f2130f1 Mon Sep 17 00:00:00 2001 From: Sargun Dhillon Date: Tue, 21 Nov 2017 19:11:48 -0800 Subject: [PATCH 3/4] Fix bug, where copy_file_range was still calling legacy copy There was a small issue here, where it copied the data using traditional mechanisms, even when copy_file_range was successful. Signed-off-by: Sargun Dhillon Upstream-commit: 0eac562281782257e6f69d58bcbc13fa889f1759 Component: engine --- components/engine/daemon/graphdriver/copy/copy.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/components/engine/daemon/graphdriver/copy/copy.go b/components/engine/daemon/graphdriver/copy/copy.go index bb658c43ef..3046089512 100644 --- a/components/engine/daemon/graphdriver/copy/copy.go +++ b/components/engine/daemon/graphdriver/copy/copy.go @@ -65,7 +65,7 @@ func copyRegular(srcPath, dstPath string, fileinfo os.FileInfo, copyWithFileRang // as the ioctl may not have been available (therefore EINVAL) if err == unix.EXDEV || err == unix.ENOSYS { *copyWithFileRange = false - } else if err != nil { + } else { return err } } From 20a2865e5342e3faa387138e6c52e2bd3f957525 Mon Sep 17 00:00:00 2001 From: Sargun Dhillon Date: Thu, 23 Nov 2017 16:31:31 -0800 Subject: [PATCH 4/4] Fix setting mtimes on directories 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 Upstream-commit: 77a2bc3e5bbc9be3fe166ed8321b7cd04e7bd097 Component: engine --- .../engine/daemon/graphdriver/copy/copy.go | 25 +++++- .../daemon/graphdriver/copy/copy_test.go | 85 ++++++++++++++++++- 2 files changed, 106 insertions(+), 4 deletions(-) diff --git a/components/engine/daemon/graphdriver/copy/copy.go b/components/engine/daemon/graphdriver/copy/copy.go index 3046089512..7a98bec8ba 100644 --- a/components/engine/daemon/graphdriver/copy/copy.go +++ b/components/engine/daemon/graphdriver/copy/copy.go @@ -11,6 +11,7 @@ package copy */ import "C" import ( + "container/list" "fmt" "io" "os" @@ -111,6 +112,11 @@ type fileID struct { ino uint64 } +type dirMtimeInfo struct { + dstPath *string + stat *syscall.Stat_t +} + // DirCopy copies or hardlinks the contents of one directory to another, // properly handling xattrs, and soft links // @@ -118,9 +124,11 @@ type fileID struct { func DirCopy(srcDir, dstDir string, copyMode Mode, copyXattrs bool) error { copyWithFileRange := true copyWithFileClone := true + // This is a map of source file inodes to dst file paths copiedFiles := make(map[fileID]string) + dirsToSetMtimes := list.New() err := filepath.Walk(srcDir, func(srcPath string, f os.FileInfo, err error) error { if err != nil { return err @@ -226,7 +234,9 @@ func DirCopy(srcDir, dstDir string, copyMode Mode, copyXattrs bool) error { // system.Chtimes doesn't support a NOFOLLOW flag atm // nolint: unconvert - if !isSymlink { + if f.IsDir() { + dirsToSetMtimes.PushFront(&dirMtimeInfo{dstPath: &dstPath, stat: stat}) + } else if !isSymlink { aTime := time.Unix(int64(stat.Atim.Sec), int64(stat.Atim.Nsec)) mTime := time.Unix(int64(stat.Mtim.Sec), int64(stat.Mtim.Nsec)) if err := system.Chtimes(dstPath, aTime, mTime); err != nil { @@ -240,7 +250,18 @@ func DirCopy(srcDir, dstDir string, copyMode Mode, copyXattrs bool) error { } return nil }) - return err + if err != nil { + return err + } + for e := dirsToSetMtimes.Front(); e != nil; e = e.Next() { + mtimeInfo := e.Value.(*dirMtimeInfo) + ts := []syscall.Timespec{mtimeInfo.stat.Atim, mtimeInfo.stat.Mtim} + if err := system.LUtimesNano(*mtimeInfo.dstPath, ts); err != nil { + return err + } + } + + return nil } func doCopyXattrs(srcPath, dstPath string) error { diff --git a/components/engine/daemon/graphdriver/copy/copy_test.go b/components/engine/daemon/graphdriver/copy/copy_test.go index ce99536e01..d21699114d 100644 --- a/components/engine/daemon/graphdriver/copy/copy_test.go +++ b/components/engine/daemon/graphdriver/copy/copy_test.go @@ -3,17 +3,20 @@ package copy import ( + "fmt" "io/ioutil" "math/rand" "os" "path/filepath" + "syscall" "testing" - - "golang.org/x/sys/unix" + "time" "github.com/docker/docker/pkg/parsers/kernel" + "github.com/docker/docker/pkg/system" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "golang.org/x/sys/unix" ) func TestIsCopyFileRangeSyscallAvailable(t *testing.T) { @@ -47,6 +50,84 @@ func TestCopyWithoutRange(t *testing.T) { doCopyTest(t, ©WithFileRange, ©WithFileClone) } +func TestCopyDir(t *testing.T) { + srcDir, err := ioutil.TempDir("", "srcDir") + require.NoError(t, err) + populateSrcDir(t, srcDir, 3) + + dstDir, err := ioutil.TempDir("", "testdst") + require.NoError(t, err) + defer os.RemoveAll(dstDir) + + assert.NoError(t, DirCopy(srcDir, dstDir, Content, false)) + require.NoError(t, filepath.Walk(srcDir, func(srcPath string, f os.FileInfo, err error) error { + if err != nil { + return err + } + + // Rebase path + relPath, err := filepath.Rel(srcDir, srcPath) + require.NoError(t, err) + if relPath == "." { + return nil + } + + dstPath := filepath.Join(dstDir, relPath) + require.NoError(t, err) + + // If we add non-regular dirs and files to the test + // then we need to add more checks here. + dstFileInfo, err := os.Lstat(dstPath) + require.NoError(t, err) + + srcFileSys := f.Sys().(*syscall.Stat_t) + dstFileSys := dstFileInfo.Sys().(*syscall.Stat_t) + + t.Log(relPath) + if srcFileSys.Dev == dstFileSys.Dev { + assert.NotEqual(t, srcFileSys.Ino, dstFileSys.Ino) + } + // Todo: check size, and ctim is not equal + /// on filesystems that have granular ctimes + assert.Equal(t, srcFileSys.Mode, dstFileSys.Mode) + assert.Equal(t, srcFileSys.Uid, dstFileSys.Uid) + assert.Equal(t, srcFileSys.Gid, dstFileSys.Gid) + assert.Equal(t, srcFileSys.Mtim, dstFileSys.Mtim) + + return nil + })) +} + +func randomMode(baseMode int) os.FileMode { + for i := 0; i < 7; i++ { + baseMode = baseMode | (1&rand.Intn(2))<