From 2b52cbdf3e9bf1f2ee63a6bfe893125fc94a3882 Mon Sep 17 00:00:00 2001 From: Michael Crosby Date: Mon, 2 May 2016 15:44:20 -0700 Subject: [PATCH 1/9] Restore ref count Signed-off-by: Michael Crosby Upstream-commit: 009ee16beff4f6d3607fa251019908cc72ce0a34 Component: engine --- .../engine/daemon/graphdriver/counter.go | 59 +++++++++++++++---- .../daemon/graphdriver/devmapper/driver.go | 18 +++--- .../daemon/graphdriver/overlay/overlay.go | 33 ++++------- .../engine/daemon/graphdriver/zfs/zfs.go | 14 ++--- .../libcontainerd/client_liverestore_linux.go | 2 - .../client_shutdownrestore_linux.go | 41 ------------- 6 files changed, 75 insertions(+), 92 deletions(-) delete mode 100644 components/engine/libcontainerd/client_shutdownrestore_linux.go diff --git a/components/engine/daemon/graphdriver/counter.go b/components/engine/daemon/graphdriver/counter.go index 572fc9be47..8440a457ea 100644 --- a/components/engine/daemon/graphdriver/counter.go +++ b/components/engine/daemon/graphdriver/counter.go @@ -1,32 +1,69 @@ package graphdriver -import "sync" +import ( + "sync" + + "github.com/docker/docker/pkg/mount" +) + +type minfo struct { + check bool + count int +} // RefCounter is a generic counter for use by graphdriver Get/Put calls type RefCounter struct { - counts map[string]int + counts map[string]*minfo mu sync.Mutex } // NewRefCounter returns a new RefCounter func NewRefCounter() *RefCounter { - return &RefCounter{counts: make(map[string]int)} + return &RefCounter{counts: make(map[string]*minfo)} } // Increment increaes the ref count for the given id and returns the current count -func (c *RefCounter) Increment(id string) int { +func (c *RefCounter) Increment(path string) int { c.mu.Lock() - c.counts[id]++ - count := c.counts[id] + m := c.counts[path] + if m == nil { + m = &minfo{check: true} + c.counts[path] = m + } + // if we are checking this path for the first time check to make sure + // if it was already mounted on the system and make sure we have a correct ref + // count if it is mounted as it is in use. + if !m.check { + m.check = true + mntd, _ := mount.Mounted(path) + if mntd { + m.count++ + } + } + m.count++ c.mu.Unlock() - return count + return m.count } // Decrement decreases the ref count for the given id and returns the current count -func (c *RefCounter) Decrement(id string) int { +func (c *RefCounter) Decrement(path string) int { c.mu.Lock() - c.counts[id]-- - count := c.counts[id] + m := c.counts[path] + if m == nil { + m = &minfo{check: true} + c.counts[path] = m + } + // if we are checking this path for the first time check to make sure + // if it was already mounted on the system and make sure we have a correct ref + // count if it is mounted as it is in use. + if !m.check { + m.check = true + mntd, _ := mount.Mounted(path) + if mntd { + m.count++ + } + } + m.count-- c.mu.Unlock() - return count + return m.count } diff --git a/components/engine/daemon/graphdriver/devmapper/driver.go b/components/engine/daemon/graphdriver/devmapper/driver.go index 7cd90e924a..85a28b59ed 100644 --- a/components/engine/daemon/graphdriver/devmapper/driver.go +++ b/components/engine/daemon/graphdriver/devmapper/driver.go @@ -160,35 +160,35 @@ func (d *Driver) Remove(id string) error { // Get mounts a device with given id into the root filesystem func (d *Driver) Get(id, mountLabel string) (string, error) { mp := path.Join(d.home, "mnt", id) - if count := d.ctr.Increment(id); count > 1 { + if count := d.ctr.Increment(mp); count > 1 { return mp, nil } uid, gid, err := idtools.GetRootUIDGID(d.uidMaps, d.gidMaps) if err != nil { - d.ctr.Decrement(id) + d.ctr.Decrement(mp) return "", err } // Create the target directories if they don't exist if err := idtools.MkdirAllAs(path.Join(d.home, "mnt"), 0755, uid, gid); err != nil && !os.IsExist(err) { - d.ctr.Decrement(id) + d.ctr.Decrement(mp) return "", err } if err := idtools.MkdirAs(mp, 0755, uid, gid); err != nil && !os.IsExist(err) { - d.ctr.Decrement(id) + d.ctr.Decrement(mp) return "", err } // Mount the device if err := d.DeviceSet.MountDevice(id, mp, mountLabel); err != nil { - d.ctr.Decrement(id) + d.ctr.Decrement(mp) return "", err } rootFs := path.Join(mp, "rootfs") if err := idtools.MkdirAllAs(rootFs, 0755, uid, gid); err != nil && !os.IsExist(err) { - d.ctr.Decrement(id) + d.ctr.Decrement(mp) d.DeviceSet.UnmountDevice(id, mp) return "", err } @@ -198,7 +198,7 @@ func (d *Driver) Get(id, mountLabel string) (string, error) { // Create an "id" file with the container/image id in it to help reconstruct this in case // of later problems if err := ioutil.WriteFile(idFile, []byte(id), 0600); err != nil { - d.ctr.Decrement(id) + d.ctr.Decrement(mp) d.DeviceSet.UnmountDevice(id, mp) return "", err } @@ -209,10 +209,10 @@ func (d *Driver) Get(id, mountLabel string) (string, error) { // Put unmounts a device and removes it. func (d *Driver) Put(id string) error { - if count := d.ctr.Decrement(id); count > 0 { + mp := path.Join(d.home, "mnt", id) + if count := d.ctr.Decrement(mp); count > 0 { return nil } - mp := path.Join(d.home, "mnt", id) err := d.DeviceSet.UnmountDevice(id, mp) if err != nil { logrus.Errorf("devmapper: Error unmounting device %s: %s", id, err) diff --git a/components/engine/daemon/graphdriver/overlay/overlay.go b/components/engine/daemon/graphdriver/overlay/overlay.go index a03a5acea6..6cb597fa13 100644 --- a/components/engine/daemon/graphdriver/overlay/overlay.go +++ b/components/engine/daemon/graphdriver/overlay/overlay.go @@ -340,6 +340,10 @@ func (d *Driver) Get(id string, mountLabel string) (string, error) { if _, err := os.Stat(dir); err != nil { return "", err } + mergedDir := path.Join(dir, "merged") + if count := d.ctr.Increment(mergedDir); count > 1 { + return mergedDir, nil + } // If id has a root, just return it rootDir := path.Join(dir, "root") @@ -357,40 +361,24 @@ func (d *Driver) Get(id string, mountLabel string) (string, error) { lowerDir := path.Join(d.dir(string(lowerID)), "root") upperDir := path.Join(dir, "upper") workDir := path.Join(dir, "work") - mergedDir := path.Join(dir, "merged") - - if count := d.ctr.Increment(id); count > 1 { - return mergedDir, nil - } opts := fmt.Sprintf("lowerdir=%s,upperdir=%s,workdir=%s", lowerDir, upperDir, workDir) - // if it's mounted already, just return - mounted, err := d.mounted(mergedDir) - if err != nil { - d.ctr.Decrement(id) - return "", err - } - if mounted { - d.ctr.Decrement(id) - return mergedDir, nil - } - if err := syscall.Mount("overlay", mergedDir, "overlay", 0, label.FormatMountLabel(opts, mountLabel)); err != nil { - d.ctr.Decrement(id) + d.ctr.Decrement(mergedDir) return "", fmt.Errorf("error creating overlay mount to %s: %v", mergedDir, err) } // chown "workdir/work" to the remapped root UID/GID. Overlay fs inside a // user namespace requires this to move a directory from lower to upper. rootUID, rootGID, err := idtools.GetRootUIDGID(d.uidMaps, d.gidMaps) if err != nil { - d.ctr.Decrement(id) + d.ctr.Decrement(mergedDir) syscall.Unmount(mergedDir, 0) return "", err } if err := os.Chown(path.Join(workDir, "work"), rootUID, rootGID); err != nil { - d.ctr.Decrement(id) + d.ctr.Decrement(mergedDir) syscall.Unmount(mergedDir, 0) return "", err } @@ -408,13 +396,14 @@ func (d *Driver) mounted(dir string) (bool, error) { // Put unmounts the mount path created for the give id. func (d *Driver) Put(id string) error { - if count := d.ctr.Decrement(id); count > 0 { - return nil - } d.pathCacheLock.Lock() mountpoint, exists := d.pathCache[id] d.pathCacheLock.Unlock() + if count := d.ctr.Decrement(mountpoint); count > 0 { + return nil + } + if !exists { logrus.Debugf("Put on a non-mounted device %s", id) // but it might be still here diff --git a/components/engine/daemon/graphdriver/zfs/zfs.go b/components/engine/daemon/graphdriver/zfs/zfs.go index ffde8a545f..1ce71a68d1 100644 --- a/components/engine/daemon/graphdriver/zfs/zfs.go +++ b/components/engine/daemon/graphdriver/zfs/zfs.go @@ -307,7 +307,7 @@ func (d *Driver) Remove(id string) error { // Get returns the mountpoint for the given id after creating the target directories if necessary. func (d *Driver) Get(id, mountLabel string) (string, error) { mountpoint := d.mountPath(id) - if count := d.ctr.Increment(id); count > 1 { + if count := d.ctr.Increment(mountpoint); count > 1 { return mountpoint, nil } @@ -317,17 +317,17 @@ func (d *Driver) Get(id, mountLabel string) (string, error) { rootUID, rootGID, err := idtools.GetRootUIDGID(d.uidMaps, d.gidMaps) if err != nil { - d.ctr.Decrement(id) + d.ctr.Decrement(mountpoint) return "", err } // Create the target directories if they don't exist if err := idtools.MkdirAllAs(mountpoint, 0755, rootUID, rootGID); err != nil { - d.ctr.Decrement(id) + d.ctr.Decrement(mountpoint) return "", err } if err := mount.Mount(filesystem, mountpoint, "zfs", options); err != nil { - d.ctr.Decrement(id) + d.ctr.Decrement(mountpoint) return "", fmt.Errorf("error creating zfs mount of %s to %s: %v", filesystem, mountpoint, err) } @@ -335,7 +335,7 @@ func (d *Driver) Get(id, mountLabel string) (string, error) { // permissions instead of the remapped root uid:gid (if user namespaces are enabled): if err := os.Chown(mountpoint, rootUID, rootGID); err != nil { mount.Unmount(mountpoint) - d.ctr.Decrement(id) + d.ctr.Decrement(mountpoint) return "", fmt.Errorf("error modifying zfs mountpoint (%s) directory ownership: %v", mountpoint, err) } @@ -344,10 +344,10 @@ func (d *Driver) Get(id, mountLabel string) (string, error) { // Put removes the existing mountpoint for the given id if it exists. func (d *Driver) Put(id string) error { - if count := d.ctr.Decrement(id); count > 0 { + mountpoint := d.mountPath(id) + if count := d.ctr.Decrement(mountpoint); count > 0 { return nil } - mountpoint := d.mountPath(id) mounted, err := graphdriver.Mounted(graphdriver.FsMagicZfs, mountpoint) if err != nil || !mounted { return err diff --git a/components/engine/libcontainerd/client_liverestore_linux.go b/components/engine/libcontainerd/client_liverestore_linux.go index 2d6c2b257f..83b1a96fd4 100644 --- a/components/engine/libcontainerd/client_liverestore_linux.go +++ b/components/engine/libcontainerd/client_liverestore_linux.go @@ -1,5 +1,3 @@ -// +build experimental - package libcontainerd import ( diff --git a/components/engine/libcontainerd/client_shutdownrestore_linux.go b/components/engine/libcontainerd/client_shutdownrestore_linux.go deleted file mode 100644 index 52ea2a6180..0000000000 --- a/components/engine/libcontainerd/client_shutdownrestore_linux.go +++ /dev/null @@ -1,41 +0,0 @@ -// +build !experimental - -package libcontainerd - -import ( - "syscall" - "time" - - "github.com/Sirupsen/logrus" -) - -func (clnt *client) Restore(containerID string, options ...CreateOption) error { - w := clnt.getOrCreateExitNotifier(containerID) - defer w.close() - cont, err := clnt.getContainerdContainer(containerID) - if err == nil && cont.Status != "stopped" { - clnt.lock(cont.Id) - container := clnt.newContainer(cont.BundlePath) - container.systemPid = systemPid(cont) - clnt.appendContainer(container) - clnt.unlock(cont.Id) - - if err := clnt.Signal(containerID, int(syscall.SIGTERM)); err != nil { - logrus.Errorf("error sending sigterm to %v: %v", containerID, err) - } - select { - case <-time.After(10 * time.Second): - if err := clnt.Signal(containerID, int(syscall.SIGKILL)); err != nil { - logrus.Errorf("error sending sigkill to %v: %v", containerID, err) - } - select { - case <-time.After(2 * time.Second): - case <-w.wait(): - return nil - } - case <-w.wait(): - return nil - } - } - return clnt.setExited(containerID) -} From 8f4e229dc91d442d4ad8834e4c4202b464801105 Mon Sep 17 00:00:00 2001 From: Michael Crosby Date: Mon, 2 May 2016 16:36:20 -0700 Subject: [PATCH 2/9] Remove overlay pathCache Signed-off-by: Michael Crosby Upstream-commit: 290be017c54ed99466339529bf8683ee00930c28 Component: engine --- .../daemon/graphdriver/overlay/overlay.go | 82 ++++++------------- 1 file changed, 24 insertions(+), 58 deletions(-) diff --git a/components/engine/daemon/graphdriver/overlay/overlay.go b/components/engine/daemon/graphdriver/overlay/overlay.go index 6cb597fa13..e2b76b39b1 100644 --- a/components/engine/daemon/graphdriver/overlay/overlay.go +++ b/components/engine/daemon/graphdriver/overlay/overlay.go @@ -9,7 +9,6 @@ import ( "os" "os/exec" "path" - "sync" "syscall" "github.com/Sirupsen/logrus" @@ -92,12 +91,10 @@ func (d *naiveDiffDriverWithApply) ApplyDiff(id, parent string, diff archive.Rea // Driver contains information about the home directory and the list of active mounts that are created using this driver. type Driver struct { - home string - pathCacheLock sync.Mutex - pathCache map[string]string - uidMaps []idtools.IDMap - gidMaps []idtools.IDMap - ctr *graphdriver.RefCounter + home string + uidMaps []idtools.IDMap + gidMaps []idtools.IDMap + ctr *graphdriver.RefCounter } func init() { @@ -141,11 +138,10 @@ func Init(home string, options []string, uidMaps, gidMaps []idtools.IDMap) (grap } d := &Driver{ - home: home, - pathCache: make(map[string]string), - uidMaps: uidMaps, - gidMaps: gidMaps, - ctr: graphdriver.NewRefCounter(), + home: home, + uidMaps: uidMaps, + gidMaps: gidMaps, + ctr: graphdriver.NewRefCounter(), } return NaiveDiffDriverWithApply(d, uidMaps, gidMaps), nil @@ -328,14 +324,11 @@ func (d *Driver) Remove(id string) error { if err := os.RemoveAll(d.dir(id)); err != nil && !os.IsNotExist(err) { return err } - d.pathCacheLock.Lock() - delete(d.pathCache, id) - d.pathCacheLock.Unlock() return nil } // Get creates and mounts the required file system for the given id and returns the mount path. -func (d *Driver) Get(id string, mountLabel string) (string, error) { +func (d *Driver) Get(id string, mountLabel string) (s string, err error) { dir := d.dir(id) if _, err := os.Stat(dir); err != nil { return "", err @@ -344,13 +337,16 @@ func (d *Driver) Get(id string, mountLabel string) (string, error) { if count := d.ctr.Increment(mergedDir); count > 1 { return mergedDir, nil } + defer func() { + if err != nil { + d.ctr.Decrement(mergedDir) + syscall.Unmount(mergedDir, 0) + } + }() // If id has a root, just return it rootDir := path.Join(dir, "root") if _, err := os.Stat(rootDir); err == nil { - d.pathCacheLock.Lock() - d.pathCache[id] = rootDir - d.pathCacheLock.Unlock() return rootDir, nil } @@ -358,35 +354,24 @@ func (d *Driver) Get(id string, mountLabel string) (string, error) { if err != nil { return "", err } - lowerDir := path.Join(d.dir(string(lowerID)), "root") - upperDir := path.Join(dir, "upper") - workDir := path.Join(dir, "work") - - opts := fmt.Sprintf("lowerdir=%s,upperdir=%s,workdir=%s", lowerDir, upperDir, workDir) - + var ( + lowerDir = path.Join(d.dir(string(lowerID)), "root") + upperDir = path.Join(dir, "upper") + workDir = path.Join(dir, "work") + opts = fmt.Sprintf("lowerdir=%s,upperdir=%s,workdir=%s", lowerDir, upperDir, workDir) + ) if err := syscall.Mount("overlay", mergedDir, "overlay", 0, label.FormatMountLabel(opts, mountLabel)); err != nil { - d.ctr.Decrement(mergedDir) return "", fmt.Errorf("error creating overlay mount to %s: %v", mergedDir, err) } // chown "workdir/work" to the remapped root UID/GID. Overlay fs inside a // user namespace requires this to move a directory from lower to upper. rootUID, rootGID, err := idtools.GetRootUIDGID(d.uidMaps, d.gidMaps) if err != nil { - d.ctr.Decrement(mergedDir) - syscall.Unmount(mergedDir, 0) return "", err } - if err := os.Chown(path.Join(workDir, "work"), rootUID, rootGID); err != nil { - d.ctr.Decrement(mergedDir) - syscall.Unmount(mergedDir, 0) return "", err } - - d.pathCacheLock.Lock() - d.pathCache[id] = mergedDir - d.pathCacheLock.Unlock() - return mergedDir, nil } @@ -396,31 +381,12 @@ func (d *Driver) mounted(dir string) (bool, error) { // Put unmounts the mount path created for the give id. func (d *Driver) Put(id string) error { - d.pathCacheLock.Lock() - mountpoint, exists := d.pathCache[id] - d.pathCacheLock.Unlock() - + mountpoint := path.Join(d.dir(id), "merged") if count := d.ctr.Decrement(mountpoint); count > 0 { return nil } - - if !exists { - logrus.Debugf("Put on a non-mounted device %s", id) - // but it might be still here - if d.Exists(id) { - mountpoint = path.Join(d.dir(id), "merged") - } - - d.pathCacheLock.Lock() - d.pathCache[id] = mountpoint - d.pathCacheLock.Unlock() - } - - if mounted, err := d.mounted(mountpoint); mounted || err != nil { - if err = syscall.Unmount(mountpoint, 0); err != nil { - logrus.Debugf("Failed to unmount %s overlay: %v", id, err) - } - return err + if err := syscall.Unmount(mountpoint, 0); err != nil { + logrus.Debugf("Failed to unmount %s overlay: %v", id, err) } return nil } From e6822e5504bfabd5c65429baf87066020cf3ec0a Mon Sep 17 00:00:00 2001 From: Michael Crosby Date: Mon, 2 May 2016 17:22:11 -0700 Subject: [PATCH 3/9] Remove restart test This test is not applicable anymore now that containers are not stopped when the daemon is restored. Signed-off-by: Michael Crosby Upstream-commit: 31e903b0e17d01a4240f7890218a80088d32658c Component: engine --- .../engine/daemon/graphdriver/counter.go | 4 +- .../integration-cli/docker_cli_daemon_test.go | 29 ------- .../engine/libcontainerd/client_linux.go | 77 ++++++++++++++++- .../libcontainerd/client_liverestore_linux.go | 83 ------------------- 4 files changed, 78 insertions(+), 115 deletions(-) delete mode 100644 components/engine/libcontainerd/client_liverestore_linux.go diff --git a/components/engine/daemon/graphdriver/counter.go b/components/engine/daemon/graphdriver/counter.go index 8440a457ea..1b8e20a767 100644 --- a/components/engine/daemon/graphdriver/counter.go +++ b/components/engine/daemon/graphdriver/counter.go @@ -27,7 +27,7 @@ func (c *RefCounter) Increment(path string) int { c.mu.Lock() m := c.counts[path] if m == nil { - m = &minfo{check: true} + m = &minfo{} c.counts[path] = m } // if we are checking this path for the first time check to make sure @@ -50,7 +50,7 @@ func (c *RefCounter) Decrement(path string) int { c.mu.Lock() m := c.counts[path] if m == nil { - m = &minfo{check: true} + m = &minfo{} c.counts[path] = m } // if we are checking this path for the first time check to make sure diff --git a/components/engine/integration-cli/docker_cli_daemon_test.go b/components/engine/integration-cli/docker_cli_daemon_test.go index c3fa3edd01..71fd581822 100644 --- a/components/engine/integration-cli/docker_cli_daemon_test.go +++ b/components/engine/integration-cli/docker_cli_daemon_test.go @@ -1616,35 +1616,6 @@ func (s *DockerDaemonSuite) TestRunContainerWithBridgeNone(c *check.C) { check.Commentf("The network interfaces in container should be the same with host when --net=host when bridge network is disabled: %s", out)) } -// os.Kill should kill daemon ungracefully, leaving behind container mounts. -// A subsequent daemon restart shoud clean up said mounts. -func (s *DockerDaemonSuite) TestCleanupMountsAfterDaemonKill(c *check.C) { - testRequires(c, NotExperimentalDaemon) - c.Assert(s.d.StartWithBusybox(), check.IsNil) - - out, err := s.d.Cmd("run", "-d", "busybox", "top") - c.Assert(err, check.IsNil, check.Commentf("Output: %s", out)) - id := strings.TrimSpace(out) - c.Assert(s.d.cmd.Process.Signal(os.Kill), check.IsNil) - mountOut, err := ioutil.ReadFile("/proc/self/mountinfo") - c.Assert(err, check.IsNil, check.Commentf("Output: %s", mountOut)) - - // container mounts should exist even after daemon has crashed. - comment := check.Commentf("%s should stay mounted from older daemon start:\nDaemon root repository %s\n%s", id, s.d.folder, mountOut) - c.Assert(strings.Contains(string(mountOut), id), check.Equals, true, comment) - - // restart daemon. - if err := s.d.Restart(); err != nil { - c.Fatal(err) - } - - // Now, container mounts should be gone. - mountOut, err = ioutil.ReadFile("/proc/self/mountinfo") - c.Assert(err, check.IsNil, check.Commentf("Output: %s", mountOut)) - comment = check.Commentf("%s is still mounted from older daemon start:\nDaemon root repository %s\n%s", id, s.d.folder, mountOut) - c.Assert(strings.Contains(string(mountOut), id), check.Equals, false, comment) -} - func (s *DockerDaemonSuite) TestDaemonRestartWithContainerRunning(t *check.C) { if err := s.d.StartWithBusybox(); err != nil { t.Fatal(err) diff --git a/components/engine/libcontainerd/client_linux.go b/components/engine/libcontainerd/client_linux.go index 6422eb619e..165597b9a6 100644 --- a/components/engine/libcontainerd/client_linux.go +++ b/components/engine/libcontainerd/client_linux.go @@ -13,7 +13,7 @@ import ( containerd "github.com/docker/containerd/api/grpc/types" "github.com/docker/docker/pkg/idtools" "github.com/docker/docker/pkg/mount" - "github.com/opencontainers/specs/specs-go" + specs "github.com/opencontainers/specs/specs-go" "golang.org/x/net/context" ) @@ -380,6 +380,81 @@ func (clnt *client) getOrCreateExitNotifier(containerID string) *exitNotifier { return w } +func (clnt *client) restore(cont *containerd.Container, options ...CreateOption) (err error) { + clnt.lock(cont.Id) + defer clnt.unlock(cont.Id) + + logrus.Debugf("restore container %s state %s", cont.Id, cont.Status) + + containerID := cont.Id + if _, err := clnt.getContainer(containerID); err == nil { + return fmt.Errorf("container %s is already active", containerID) + } + + defer func() { + if err != nil { + clnt.deleteContainer(cont.Id) + } + }() + + container := clnt.newContainer(cont.BundlePath, options...) + container.systemPid = systemPid(cont) + + var terminal bool + for _, p := range cont.Processes { + if p.Pid == InitFriendlyName { + terminal = p.Terminal + } + } + + iopipe, err := container.openFifos(terminal) + if err != nil { + return err + } + + if err := clnt.backend.AttachStreams(containerID, *iopipe); err != nil { + return err + } + + clnt.appendContainer(container) + + err = clnt.backend.StateChanged(containerID, StateInfo{ + CommonStateInfo: CommonStateInfo{ + State: StateRestore, + Pid: container.systemPid, + }}) + + if err != nil { + return err + } + + if event, ok := clnt.remote.pastEvents[containerID]; ok { + // This should only be a pause or resume event + if event.Type == StatePause || event.Type == StateResume { + return clnt.backend.StateChanged(containerID, StateInfo{ + CommonStateInfo: CommonStateInfo{ + State: event.Type, + Pid: container.systemPid, + }}) + } + + logrus.Warnf("unexpected backlog event: %#v", event) + } + + return nil +} + +func (clnt *client) Restore(containerID string, options ...CreateOption) error { + cont, err := clnt.getContainerdContainer(containerID) + if err == nil && cont.Status != "stopped" { + if err := clnt.restore(cont, options...); err != nil { + logrus.Errorf("error restoring %s: %v", containerID, err) + } + return nil + } + return clnt.setExited(containerID) +} + type exitNotifier struct { id string client *client diff --git a/components/engine/libcontainerd/client_liverestore_linux.go b/components/engine/libcontainerd/client_liverestore_linux.go deleted file mode 100644 index 83b1a96fd4..0000000000 --- a/components/engine/libcontainerd/client_liverestore_linux.go +++ /dev/null @@ -1,83 +0,0 @@ -package libcontainerd - -import ( - "fmt" - - "github.com/Sirupsen/logrus" - containerd "github.com/docker/containerd/api/grpc/types" -) - -func (clnt *client) restore(cont *containerd.Container, options ...CreateOption) (err error) { - clnt.lock(cont.Id) - defer clnt.unlock(cont.Id) - - logrus.Debugf("restore container %s state %s", cont.Id, cont.Status) - - containerID := cont.Id - if _, err := clnt.getContainer(containerID); err == nil { - return fmt.Errorf("container %s is already active", containerID) - } - - defer func() { - if err != nil { - clnt.deleteContainer(cont.Id) - } - }() - - container := clnt.newContainer(cont.BundlePath, options...) - container.systemPid = systemPid(cont) - - var terminal bool - for _, p := range cont.Processes { - if p.Pid == InitFriendlyName { - terminal = p.Terminal - } - } - - iopipe, err := container.openFifos(terminal) - if err != nil { - return err - } - - if err := clnt.backend.AttachStreams(containerID, *iopipe); err != nil { - return err - } - - clnt.appendContainer(container) - - err = clnt.backend.StateChanged(containerID, StateInfo{ - CommonStateInfo: CommonStateInfo{ - State: StateRestore, - Pid: container.systemPid, - }}) - - if err != nil { - return err - } - - if event, ok := clnt.remote.pastEvents[containerID]; ok { - // This should only be a pause or resume event - if event.Type == StatePause || event.Type == StateResume { - return clnt.backend.StateChanged(containerID, StateInfo{ - CommonStateInfo: CommonStateInfo{ - State: event.Type, - Pid: container.systemPid, - }}) - } - - logrus.Warnf("unexpected backlog event: %#v", event) - } - - return nil -} - -func (clnt *client) Restore(containerID string, options ...CreateOption) error { - cont, err := clnt.getContainerdContainer(containerID) - if err == nil && cont.Status != "stopped" { - if err := clnt.restore(cont, options...); err != nil { - logrus.Errorf("error restoring %s: %v", containerID, err) - } - return nil - } - return clnt.setExited(containerID) -} From 50e99151eb2a017f63d295c1a14888b41928d171 Mon Sep 17 00:00:00 2001 From: Michael Crosby Date: Fri, 6 May 2016 12:04:26 -0700 Subject: [PATCH 4/9] Add fast path for fsmagic supported drivers For things that we can check if they are mounted by using their fsmagic we should use that and for others do it the slow way. Signed-off-by: Michael Crosby Upstream-commit: 1ba05cdb6ade7e3abd4c4c3221b5e27645460111 Component: engine --- .../engine/daemon/graphdriver/counter.go | 27 ++++++++-------- .../daemon/graphdriver/devmapper/driver.go | 2 +- .../engine/daemon/graphdriver/driver_linux.go | 32 +++++++++++++++++++ .../daemon/graphdriver/overlay/overlay.go | 2 +- .../engine/daemon/graphdriver/zfs/zfs.go | 2 +- 5 files changed, 49 insertions(+), 16 deletions(-) diff --git a/components/engine/daemon/graphdriver/counter.go b/components/engine/daemon/graphdriver/counter.go index 1b8e20a767..2de80b7e1e 100644 --- a/components/engine/daemon/graphdriver/counter.go +++ b/components/engine/daemon/graphdriver/counter.go @@ -1,10 +1,6 @@ package graphdriver -import ( - "sync" - - "github.com/docker/docker/pkg/mount" -) +import "sync" type minfo struct { check bool @@ -13,13 +9,20 @@ type minfo struct { // RefCounter is a generic counter for use by graphdriver Get/Put calls type RefCounter struct { - counts map[string]*minfo - mu sync.Mutex + counts map[string]*minfo + mu sync.Mutex + checker Checker } // NewRefCounter returns a new RefCounter -func NewRefCounter() *RefCounter { - return &RefCounter{counts: make(map[string]*minfo)} +func NewRefCounter(c Checker) *RefCounter { + if c == nil { + c = &defaultChecker{} + } + return &RefCounter{ + checker: c, + counts: make(map[string]*minfo), + } } // Increment increaes the ref count for the given id and returns the current count @@ -35,8 +38,7 @@ func (c *RefCounter) Increment(path string) int { // count if it is mounted as it is in use. if !m.check { m.check = true - mntd, _ := mount.Mounted(path) - if mntd { + if c.checker.IsMounted(path) { m.count++ } } @@ -58,8 +60,7 @@ func (c *RefCounter) Decrement(path string) int { // count if it is mounted as it is in use. if !m.check { m.check = true - mntd, _ := mount.Mounted(path) - if mntd { + if c.checker.IsMounted(path) { m.count++ } } diff --git a/components/engine/daemon/graphdriver/devmapper/driver.go b/components/engine/daemon/graphdriver/devmapper/driver.go index 85a28b59ed..9b58dba66d 100644 --- a/components/engine/daemon/graphdriver/devmapper/driver.go +++ b/components/engine/daemon/graphdriver/devmapper/driver.go @@ -47,7 +47,7 @@ func Init(home string, options []string, uidMaps, gidMaps []idtools.IDMap) (grap home: home, uidMaps: uidMaps, gidMaps: gidMaps, - ctr: graphdriver.NewRefCounter(), + ctr: graphdriver.NewRefCounter(nil), } return graphdriver.NewNaiveDiffDriver(d, uidMaps, gidMaps), nil diff --git a/components/engine/daemon/graphdriver/driver_linux.go b/components/engine/daemon/graphdriver/driver_linux.go index 2ab20b01a9..10a1abcf0d 100644 --- a/components/engine/daemon/graphdriver/driver_linux.go +++ b/components/engine/daemon/graphdriver/driver_linux.go @@ -5,6 +5,8 @@ package graphdriver import ( "path/filepath" "syscall" + + "github.com/docker/docker/pkg/mount" ) const ( @@ -89,6 +91,36 @@ func GetFSMagic(rootpath string) (FsMagic, error) { return FsMagic(buf.Type), nil } +// Checker makes checks on specified filesystems. +type Checker interface { + // IsMounted returns true if the provided path is mounted for the specific checker + IsMounted(path string) bool +} + +// NewFsChecker returns a checker configured for the provied FsMagic +func NewFsChecker(t FsMagic) Checker { + return &fsChecker{ + t: t, + } +} + +type fsChecker struct { + t FsMagic +} + +func (c *fsChecker) IsMounted(path string) bool { + m, _ := Mounted(c.t, path) + return m +} + +type defaultChecker struct { +} + +func (c *defaultChecker) IsMounted(path string) bool { + m, _ := mount.Mounted(path) + return m +} + // Mounted checks if the given path is mounted as the fs type func Mounted(fsType FsMagic, mountPath string) (bool, error) { var buf syscall.Statfs_t diff --git a/components/engine/daemon/graphdriver/overlay/overlay.go b/components/engine/daemon/graphdriver/overlay/overlay.go index e2b76b39b1..1abab6b265 100644 --- a/components/engine/daemon/graphdriver/overlay/overlay.go +++ b/components/engine/daemon/graphdriver/overlay/overlay.go @@ -141,7 +141,7 @@ func Init(home string, options []string, uidMaps, gidMaps []idtools.IDMap) (grap home: home, uidMaps: uidMaps, gidMaps: gidMaps, - ctr: graphdriver.NewRefCounter(), + ctr: graphdriver.NewRefCounter(graphdriver.NewFsChecker(graphdriver.FsMagicOverlay)), } return NaiveDiffDriverWithApply(d, uidMaps, gidMaps), nil diff --git a/components/engine/daemon/graphdriver/zfs/zfs.go b/components/engine/daemon/graphdriver/zfs/zfs.go index 1ce71a68d1..7a4ec1d5cf 100644 --- a/components/engine/daemon/graphdriver/zfs/zfs.go +++ b/components/engine/daemon/graphdriver/zfs/zfs.go @@ -105,7 +105,7 @@ func Init(base string, opt []string, uidMaps, gidMaps []idtools.IDMap) (graphdri filesystemsCache: filesystemsCache, uidMaps: uidMaps, gidMaps: gidMaps, - ctr: graphdriver.NewRefCounter(), + ctr: graphdriver.NewRefCounter(graphdriver.NewFsChecker(graphdriver.FsMagicZfs)), } return graphdriver.NewNaiveDiffDriver(d, uidMaps, gidMaps), nil } From 987aa6f79e143a4326e3fa4056d873634484703c Mon Sep 17 00:00:00 2001 From: Michael Crosby Date: Fri, 6 May 2016 13:09:45 -0700 Subject: [PATCH 5/9] Add reference counting to aufs Signed-off-by: Michael Crosby Upstream-commit: 5b6b8df0c1b5a54ae9a717810eedf9fc971e1321 Component: engine --- components/engine/daemon/graphdriver/aufs/aufs.go | 8 ++++++++ components/engine/daemon/graphdriver/counter.go | 3 --- .../engine/daemon/graphdriver/devmapper/driver.go | 2 +- components/engine/daemon/graphdriver/driver.go | 6 ++++++ components/engine/daemon/graphdriver/driver_linux.go | 12 ++++++------ 5 files changed, 21 insertions(+), 10 deletions(-) diff --git a/components/engine/daemon/graphdriver/aufs/aufs.go b/components/engine/daemon/graphdriver/aufs/aufs.go index 83380e1143..044351b7ee 100644 --- a/components/engine/daemon/graphdriver/aufs/aufs.go +++ b/components/engine/daemon/graphdriver/aufs/aufs.go @@ -70,6 +70,7 @@ type Driver struct { root string uidMaps []idtools.IDMap gidMaps []idtools.IDMap + ctr *graphdriver.RefCounter pathCacheLock sync.Mutex pathCache map[string]string } @@ -108,6 +109,7 @@ func Init(root string, options []string, uidMaps, gidMaps []idtools.IDMap) (grap uidMaps: uidMaps, gidMaps: gidMaps, pathCache: make(map[string]string), + ctr: graphdriver.NewRefCounter(graphdriver.NewFsChecker(graphdriver.FsMagicAufs)), } rootUID, rootGID, err := idtools.GetRootUIDGID(uidMaps, gidMaps) @@ -320,6 +322,9 @@ func (a *Driver) Get(id, mountLabel string) (string, error) { m = a.getMountpoint(id) } } + if count := a.ctr.Increment(m); count > 1 { + return m, nil + } // If a dir does not have a parent ( no layers )do not try to mount // just return the diff path to the data @@ -344,6 +349,9 @@ func (a *Driver) Put(id string) error { a.pathCache[id] = m } a.pathCacheLock.Unlock() + if count := a.ctr.Decrement(m); count > 0 { + return nil + } err := a.unmount(m) if err != nil { diff --git a/components/engine/daemon/graphdriver/counter.go b/components/engine/daemon/graphdriver/counter.go index 2de80b7e1e..5ea604f5b6 100644 --- a/components/engine/daemon/graphdriver/counter.go +++ b/components/engine/daemon/graphdriver/counter.go @@ -16,9 +16,6 @@ type RefCounter struct { // NewRefCounter returns a new RefCounter func NewRefCounter(c Checker) *RefCounter { - if c == nil { - c = &defaultChecker{} - } return &RefCounter{ checker: c, counts: make(map[string]*minfo), diff --git a/components/engine/daemon/graphdriver/devmapper/driver.go b/components/engine/daemon/graphdriver/devmapper/driver.go index 9b58dba66d..38fa3ece70 100644 --- a/components/engine/daemon/graphdriver/devmapper/driver.go +++ b/components/engine/daemon/graphdriver/devmapper/driver.go @@ -47,7 +47,7 @@ func Init(home string, options []string, uidMaps, gidMaps []idtools.IDMap) (grap home: home, uidMaps: uidMaps, gidMaps: gidMaps, - ctr: graphdriver.NewRefCounter(nil), + ctr: graphdriver.NewRefCounter(graphdriver.NewDefaultChecker()), } return graphdriver.NewNaiveDiffDriver(d, uidMaps, gidMaps), nil diff --git a/components/engine/daemon/graphdriver/driver.go b/components/engine/daemon/graphdriver/driver.go index 495bac2cf5..79f6789f99 100644 --- a/components/engine/daemon/graphdriver/driver.go +++ b/components/engine/daemon/graphdriver/driver.go @@ -113,6 +113,12 @@ type FileGetCloser interface { Close() error } +// Checker makes checks on specified filesystems. +type Checker interface { + // IsMounted returns true if the provided path is mounted for the specific checker + IsMounted(path string) bool +} + func init() { drivers = make(map[string]InitFunc) } diff --git a/components/engine/daemon/graphdriver/driver_linux.go b/components/engine/daemon/graphdriver/driver_linux.go index 10a1abcf0d..70b2ce22f1 100644 --- a/components/engine/daemon/graphdriver/driver_linux.go +++ b/components/engine/daemon/graphdriver/driver_linux.go @@ -91,12 +91,6 @@ func GetFSMagic(rootpath string) (FsMagic, error) { return FsMagic(buf.Type), nil } -// Checker makes checks on specified filesystems. -type Checker interface { - // IsMounted returns true if the provided path is mounted for the specific checker - IsMounted(path string) bool -} - // NewFsChecker returns a checker configured for the provied FsMagic func NewFsChecker(t FsMagic) Checker { return &fsChecker{ @@ -113,6 +107,12 @@ func (c *fsChecker) IsMounted(path string) bool { return m } +// NewDefaultChecker returns a check that parses /proc/mountinfo to check +// if the specified path is mounted. +func NewDefaultChecker() Checker { + return &defaultChecker{} +} + type defaultChecker struct { } From 49f1314f0be0847f281c9186a0e14ded369f50f1 Mon Sep 17 00:00:00 2001 From: Michael Crosby Date: Fri, 6 May 2016 14:54:28 -0700 Subject: [PATCH 6/9] Remove ref counting from layer store Signed-off-by: Michael Crosby Upstream-commit: e19499710e3728433cdc8348e985f9a825cb2336 Component: engine --- components/engine/layer/layer_store.go | 8 +-- components/engine/layer/layer_test.go | 17 ----- components/engine/layer/migration_test.go | 13 ---- components/engine/layer/mounted_layer.go | 81 +---------------------- 4 files changed, 3 insertions(+), 116 deletions(-) diff --git a/components/engine/layer/layer_store.go b/components/engine/layer/layer_store.go index f18aff2145..f25ef91ac9 100644 --- a/components/engine/layer/layer_store.go +++ b/components/engine/layer/layer_store.go @@ -502,15 +502,9 @@ func (ls *layerStore) ReinitRWLayer(l RWLayer) error { ls.mountL.Lock() defer ls.mountL.Unlock() - m, ok := ls.mounts[l.Name()] - if !ok { + if _, ok := ls.mounts[l.Name()]; !ok { return ErrMountDoesNotExist } - - if err := m.incActivityCount(l); err != nil { - return err - } - return nil } diff --git a/components/engine/layer/layer_test.go b/components/engine/layer/layer_test.go index 85687cebb4..2e4fab5c86 100644 --- a/components/engine/layer/layer_test.go +++ b/components/engine/layer/layer_test.go @@ -400,14 +400,11 @@ func TestStoreRestore(t *testing.T) { if err := ioutil.WriteFile(filepath.Join(path, "testfile.txt"), []byte("nothing here"), 0644); err != nil { t.Fatal(err) } - assertActivityCount(t, m, 1) if err := m.Unmount(); err != nil { t.Fatal(err) } - assertActivityCount(t, m, 0) - ls2, err := NewStoreFromGraphDriver(ls.(*layerStore).store, ls.(*layerStore).driver) if err != nil { t.Fatal(err) @@ -438,20 +435,15 @@ func TestStoreRestore(t *testing.T) { t.Fatalf("Unexpected path %s, expected %s", mountPath, path) } - assertActivityCount(t, m2, 1) - if mountPath, err := m2.Mount(""); err != nil { t.Fatal(err) } else if path != mountPath { t.Fatalf("Unexpected path %s, expected %s", mountPath, path) } - assertActivityCount(t, m2, 2) if err := m2.Unmount(); err != nil { t.Fatal(err) } - assertActivityCount(t, m2, 1) - b, err := ioutil.ReadFile(filepath.Join(path, "testfile.txt")) if err != nil { t.Fatal(err) @@ -464,8 +456,6 @@ func TestStoreRestore(t *testing.T) { t.Fatal(err) } - assertActivityCount(t, m2, 0) - if metadata, err := ls2.ReleaseRWLayer(m2); err != nil { t.Fatal(err) } else if len(metadata) != 0 { @@ -674,13 +664,6 @@ func assertReferences(t *testing.T, references ...Layer) { } } -func assertActivityCount(t *testing.T, l RWLayer, expected int) { - rl := l.(*referencedRWLayer) - if rl.activityCount != expected { - t.Fatalf("Unexpected activity count %d, expected %d", rl.activityCount, expected) - } -} - func TestRegisterExistingLayer(t *testing.T) { ls, _, cleanup := newTestStore(t) defer cleanup() diff --git a/components/engine/layer/migration_test.go b/components/engine/layer/migration_test.go index 7e2e2a6489..50ea6407bb 100644 --- a/components/engine/layer/migration_test.go +++ b/components/engine/layer/migration_test.go @@ -380,8 +380,6 @@ func TestMountMigration(t *testing.T) { Kind: archive.ChangeAdd, }) - assertActivityCount(t, rwLayer1, 1) - if _, err := ls.CreateRWLayer("migration-mount", layer1.ChainID(), "", nil, nil); err == nil { t.Fatal("Expected error creating mount with same name") } else if err != ErrMountNameConflict { @@ -401,16 +399,10 @@ func TestMountMigration(t *testing.T) { t.Fatal(err) } - assertActivityCount(t, rwLayer2, 1) - assertActivityCount(t, rwLayer1, 1) - if _, err := rwLayer2.Mount(""); err != nil { t.Fatal(err) } - assertActivityCount(t, rwLayer2, 2) - assertActivityCount(t, rwLayer1, 1) - if metadata, err := ls.Release(layer1); err != nil { t.Fatal(err) } else if len(metadata) > 0 { @@ -420,8 +412,6 @@ func TestMountMigration(t *testing.T) { if err := rwLayer1.Unmount(); err != nil { t.Fatal(err) } - assertActivityCount(t, rwLayer2, 2) - assertActivityCount(t, rwLayer1, 0) if _, err := ls.ReleaseRWLayer(rwLayer1); err != nil { t.Fatal(err) @@ -430,9 +420,6 @@ func TestMountMigration(t *testing.T) { if err := rwLayer2.Unmount(); err != nil { t.Fatal(err) } - if _, err := ls.ReleaseRWLayer(rwLayer2); err == nil { - t.Fatal("Expected error deleting active mount") - } if err := rwLayer2.Unmount(); err != nil { t.Fatal(err) } diff --git a/components/engine/layer/mounted_layer.go b/components/engine/layer/mounted_layer.go index 5a07fd08ea..62189c8c20 100644 --- a/components/engine/layer/mounted_layer.go +++ b/components/engine/layer/mounted_layer.go @@ -2,7 +2,6 @@ package layer import ( "io" - "sync" "github.com/docker/docker/pkg/archive" ) @@ -83,106 +82,30 @@ func (ml *mountedLayer) hasReferences() bool { return len(ml.references) > 0 } -func (ml *mountedLayer) incActivityCount(ref RWLayer) error { - rl, ok := ml.references[ref] - if !ok { - return ErrLayerNotRetained - } - - if err := rl.acquire(); err != nil { - return err - } - return nil -} - func (ml *mountedLayer) deleteReference(ref RWLayer) error { - rl, ok := ml.references[ref] - if !ok { + if _, ok := ml.references[ref]; !ok { return ErrLayerNotRetained } - - if err := rl.release(); err != nil { - return err - } delete(ml.references, ref) - return nil } func (ml *mountedLayer) retakeReference(r RWLayer) { if ref, ok := r.(*referencedRWLayer); ok { - ref.activityCount = 0 ml.references[ref] = ref } } type referencedRWLayer struct { *mountedLayer - - activityL sync.Mutex - activityCount int -} - -func (rl *referencedRWLayer) acquire() error { - rl.activityL.Lock() - defer rl.activityL.Unlock() - - rl.activityCount++ - - return nil -} - -func (rl *referencedRWLayer) release() error { - rl.activityL.Lock() - defer rl.activityL.Unlock() - - if rl.activityCount > 0 { - return ErrActiveMount - } - - rl.activityCount = -1 - - return nil } func (rl *referencedRWLayer) Mount(mountLabel string) (string, error) { - rl.activityL.Lock() - defer rl.activityL.Unlock() - - if rl.activityCount == -1 { - return "", ErrLayerNotRetained - } - - if rl.activityCount > 0 { - rl.activityCount++ - return rl.path, nil - } - - m, err := rl.mountedLayer.Mount(mountLabel) - if err == nil { - rl.activityCount++ - rl.path = m - } - return m, err + return rl.mountedLayer.Mount(mountLabel) } // Unmount decrements the activity count and unmounts the underlying layer // Callers should only call `Unmount` once per call to `Mount`, even on error. func (rl *referencedRWLayer) Unmount() error { - rl.activityL.Lock() - defer rl.activityL.Unlock() - - if rl.activityCount == 0 { - return ErrNotMounted - } - if rl.activityCount == -1 { - return ErrLayerNotRetained - } - - rl.activityCount-- - if rl.activityCount > 0 { - return nil - } - return rl.mountedLayer.Unmount() } From bfa2eac6731e2f893a3e8d7539fca8613e104314 Mon Sep 17 00:00:00 2001 From: Michael Crosby Date: Tue, 10 May 2016 11:42:03 -0700 Subject: [PATCH 7/9] Add windows graph driver ref counter Signed-off-by: Michael Crosby Upstream-commit: 4bac8bce985b20d68422ef93de3adf4931b2dee5 Component: engine --- .../daemon/graphdriver/windows/windows.go | 33 ++++++++++++++++++- 1 file changed, 32 insertions(+), 1 deletion(-) diff --git a/components/engine/daemon/graphdriver/windows/windows.go b/components/engine/daemon/graphdriver/windows/windows.go index b90b69b668..648ce16d67 100644 --- a/components/engine/daemon/graphdriver/windows/windows.go +++ b/components/engine/daemon/graphdriver/windows/windows.go @@ -15,11 +15,11 @@ import ( "path/filepath" "strconv" "strings" + "sync" "syscall" "time" "unsafe" - "github.com/Microsoft/go-winio" "github.com/Microsoft/go-winio/archive/tar" "github.com/Microsoft/go-winio/backuptar" "github.com/Microsoft/hcsshim" @@ -31,6 +31,7 @@ import ( "github.com/docker/docker/pkg/longpath" "github.com/docker/docker/pkg/reexec" "github.com/docker/docker/pkg/system" + "github.com/docker/docker/vendor/src/github.com/Microsoft/go-winio" "github.com/vbatts/tar-split/tar/storage" ) @@ -43,10 +44,22 @@ func init() { reexec.Register("docker-windows-write-layer", writeLayer) } +type checker struct { +} + +func (c *checker) IsMounted(path string) bool { + return false +} + // Driver represents a windows graph driver. type Driver struct { // info stores the shim driver information info hcsshim.DriverInfo + ctr *graphdriver.RefCounter + // it is safe for windows to use a cache here because it does not support + // restoring containers when the daemon dies. + cacheMu sync.Mutex + cache map[string]string } func isTP5OrOlder() bool { @@ -61,6 +74,8 @@ func InitFilter(home string, options []string, uidMaps, gidMaps []idtools.IDMap) HomeDir: home, Flavour: filterDriver, }, + cache: make(map[string]string), + ctr: graphdriver.NewRefCounter(&checker{}), } return d, nil } @@ -211,17 +226,23 @@ func (d *Driver) Get(id, mountLabel string) (string, error) { if err != nil { return "", err } + if count := d.ctr.Increment(rID); count > 1 { + return d.cache[rID], nil + } // Getting the layer paths must be done outside of the lock. layerChain, err := d.getLayerChain(rID) if err != nil { + d.ctr.Decrement(rID) return "", err } if err := hcsshim.ActivateLayer(d.info, rID); err != nil { + d.ctr.Decrement(rID) return "", err } if err := hcsshim.PrepareLayer(d.info, rID, layerChain); err != nil { + d.ctr.Decrement(rID) if err2 := hcsshim.DeactivateLayer(d.info, rID); err2 != nil { logrus.Warnf("Failed to Deactivate %s: %s", id, err) } @@ -230,11 +251,15 @@ func (d *Driver) Get(id, mountLabel string) (string, error) { mountPath, err := hcsshim.GetLayerMountPath(d.info, rID) if err != nil { + d.ctr.Decrement(rID) if err2 := hcsshim.DeactivateLayer(d.info, rID); err2 != nil { logrus.Warnf("Failed to Deactivate %s: %s", id, err) } return "", err } + d.cacheMu.Lock() + d.cache[rID] = mountPath + d.cacheMu.Unlock() // If the layer has a mount path, use that. Otherwise, use the // folder path. @@ -255,6 +280,12 @@ func (d *Driver) Put(id string) error { if err != nil { return err } + if count := d.ctr.Decrement(rID); count > 0 { + return nil + } + d.cacheMu.Lock() + delete(d.cache, rID) + d.cacheMu.Unlock() if err := hcsshim.UnprepareLayer(d.info, rID); err != nil { return err From de303611553bd10536871d0090af8cdc9996cbee Mon Sep 17 00:00:00 2001 From: Michael Crosby Date: Mon, 16 May 2016 11:07:03 -0700 Subject: [PATCH 8/9] Fix overlay use of rootdir and defer Check for the rootDir first because the mergeDir may not exist if root is present. Also fix unmounting in the defer to make sure it does not have a refcount. Signed-off-by: Michael Crosby Upstream-commit: 36a82c20321936a71b30fcfde8bc6c76d6cc8d1f Component: engine --- .../daemon/graphdriver/overlay/overlay.go | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/components/engine/daemon/graphdriver/overlay/overlay.go b/components/engine/daemon/graphdriver/overlay/overlay.go index 1abab6b265..d532e44037 100644 --- a/components/engine/daemon/graphdriver/overlay/overlay.go +++ b/components/engine/daemon/graphdriver/overlay/overlay.go @@ -333,23 +333,22 @@ func (d *Driver) Get(id string, mountLabel string) (s string, err error) { if _, err := os.Stat(dir); err != nil { return "", err } + // If id has a root, just return it + rootDir := path.Join(dir, "root") + if _, err := os.Stat(rootDir); err == nil { + return rootDir, nil + } mergedDir := path.Join(dir, "merged") if count := d.ctr.Increment(mergedDir); count > 1 { return mergedDir, nil } defer func() { if err != nil { - d.ctr.Decrement(mergedDir) - syscall.Unmount(mergedDir, 0) + if c := d.ctr.Decrement(mergedDir); c <= 0 { + syscall.Unmount(mergedDir, 0) + } } }() - - // If id has a root, just return it - rootDir := path.Join(dir, "root") - if _, err := os.Stat(rootDir); err == nil { - return rootDir, nil - } - lowerID, err := ioutil.ReadFile(path.Join(dir, "lower-id")) if err != nil { return "", err From 2736991181d8a457d6532ce03cc85f0476cebda5 Mon Sep 17 00:00:00 2001 From: Michael Crosby Date: Tue, 17 May 2016 13:20:28 -0700 Subject: [PATCH 9/9] Remove mountedLayer Mount and Unmount Signed-off-by: Michael Crosby Upstream-commit: 8bb4d31b10e4c3abee9ca92535461859bbf25d46 Component: engine --- components/engine/daemon/daemon.go | 5 ----- .../engine/daemon/graphdriver/windows/windows.go | 2 +- components/engine/daemon/graphdriver/zfs/zfs.go | 2 +- .../engine/distribution/xfer/download_test.go | 4 ---- components/engine/layer/layer.go | 1 - components/engine/layer/layer_store.go | 13 ------------- components/engine/layer/layer_test.go | 5 +---- components/engine/layer/mounted_layer.go | 12 ++---------- 8 files changed, 5 insertions(+), 39 deletions(-) diff --git a/components/engine/daemon/daemon.go b/components/engine/daemon/daemon.go index dcaba0db65..bed1881a3e 100644 --- a/components/engine/daemon/daemon.go +++ b/components/engine/daemon/daemon.go @@ -277,11 +277,6 @@ func (daemon *Daemon) restore() error { defer wg.Done() rm := c.RestartManager(false) if c.IsRunning() || c.IsPaused() { - // Fix activityCount such that graph mounts can be unmounted later - if err := daemon.layerStore.ReinitRWLayer(c.RWLayer); err != nil { - logrus.Errorf("Failed to ReinitRWLayer for %s due to %s", c.ID, err) - return - } if err := daemon.containerd.Restore(c.ID, libcontainerd.WithRestartManager(rm)); err != nil { logrus.Errorf("Failed to restore with containerd: %q", err) return diff --git a/components/engine/daemon/graphdriver/windows/windows.go b/components/engine/daemon/graphdriver/windows/windows.go index 648ce16d67..3490c006fb 100644 --- a/components/engine/daemon/graphdriver/windows/windows.go +++ b/components/engine/daemon/graphdriver/windows/windows.go @@ -20,6 +20,7 @@ import ( "time" "unsafe" + "github.com/Microsoft/go-winio" "github.com/Microsoft/go-winio/archive/tar" "github.com/Microsoft/go-winio/backuptar" "github.com/Microsoft/hcsshim" @@ -31,7 +32,6 @@ import ( "github.com/docker/docker/pkg/longpath" "github.com/docker/docker/pkg/reexec" "github.com/docker/docker/pkg/system" - "github.com/docker/docker/vendor/src/github.com/Microsoft/go-winio" "github.com/vbatts/tar-split/tar/storage" ) diff --git a/components/engine/daemon/graphdriver/zfs/zfs.go b/components/engine/daemon/graphdriver/zfs/zfs.go index 7a4ec1d5cf..a9e40d3431 100644 --- a/components/engine/daemon/graphdriver/zfs/zfs.go +++ b/components/engine/daemon/graphdriver/zfs/zfs.go @@ -105,7 +105,7 @@ func Init(base string, opt []string, uidMaps, gidMaps []idtools.IDMap) (graphdri filesystemsCache: filesystemsCache, uidMaps: uidMaps, gidMaps: gidMaps, - ctr: graphdriver.NewRefCounter(graphdriver.NewFsChecker(graphdriver.FsMagicZfs)), + ctr: graphdriver.NewRefCounter(graphdriver.NewDefaultChecker()), } return graphdriver.NewNaiveDiffDriver(d, uidMaps, gidMaps), nil } diff --git a/components/engine/distribution/xfer/download_test.go b/components/engine/distribution/xfer/download_test.go index 5a38e3f038..330882f24f 100644 --- a/components/engine/distribution/xfer/download_test.go +++ b/components/engine/distribution/xfer/download_test.go @@ -121,10 +121,6 @@ func (ls *mockLayerStore) GetMountID(string) (string, error) { return "", errors.New("not implemented") } -func (ls *mockLayerStore) ReinitRWLayer(layer.RWLayer) error { - return errors.New("not implemented") -} - func (ls *mockLayerStore) Cleanup() error { return nil } diff --git a/components/engine/layer/layer.go b/components/engine/layer/layer.go index 5100fe2dee..5d3b8c672a 100644 --- a/components/engine/layer/layer.go +++ b/components/engine/layer/layer.go @@ -174,7 +174,6 @@ type Store interface { CreateRWLayer(id string, parent ChainID, mountLabel string, initFunc MountInit, storageOpt map[string]string) (RWLayer, error) GetRWLayer(id string) (RWLayer, error) GetMountID(id string) (string, error) - ReinitRWLayer(l RWLayer) error ReleaseRWLayer(RWLayer) ([]Metadata, error) Cleanup() error diff --git a/components/engine/layer/layer_store.go b/components/engine/layer/layer_store.go index f25ef91ac9..8c3d0a4911 100644 --- a/components/engine/layer/layer_store.go +++ b/components/engine/layer/layer_store.go @@ -495,19 +495,6 @@ func (ls *layerStore) GetMountID(id string) (string, error) { return mount.mountID, nil } -// ReinitRWLayer reinitializes a given mount to the layerstore, specifically -// initializing the usage count. It should strictly only be used in the -// daemon's restore path to restore state of live containers. -func (ls *layerStore) ReinitRWLayer(l RWLayer) error { - ls.mountL.Lock() - defer ls.mountL.Unlock() - - if _, ok := ls.mounts[l.Name()]; !ok { - return ErrMountDoesNotExist - } - return nil -} - func (ls *layerStore) ReleaseRWLayer(l RWLayer) ([]Metadata, error) { ls.mountL.Lock() defer ls.mountL.Unlock() diff --git a/components/engine/layer/layer_test.go b/components/engine/layer/layer_test.go index 2e4fab5c86..8e6817c96a 100644 --- a/components/engine/layer/layer_test.go +++ b/components/engine/layer/layer_test.go @@ -174,10 +174,7 @@ func getCachedLayer(l Layer) *roLayer { } func getMountLayer(l RWLayer) *mountedLayer { - if rl, ok := l.(*referencedRWLayer); ok { - return rl.mountedLayer - } - return l.(*mountedLayer) + return l.(*referencedRWLayer).mountedLayer } func createMetadata(layers ...Layer) []Metadata { diff --git a/components/engine/layer/mounted_layer.go b/components/engine/layer/mounted_layer.go index 62189c8c20..add33d9f19 100644 --- a/components/engine/layer/mounted_layer.go +++ b/components/engine/layer/mounted_layer.go @@ -49,14 +49,6 @@ func (ml *mountedLayer) Parent() Layer { return nil } -func (ml *mountedLayer) Mount(mountLabel string) (string, error) { - return ml.layerStore.driver.Get(ml.mountID, mountLabel) -} - -func (ml *mountedLayer) Unmount() error { - return ml.layerStore.driver.Put(ml.mountID) -} - func (ml *mountedLayer) Size() (int64, error) { return ml.layerStore.driver.DiffSize(ml.mountID, ml.cacheParent()) } @@ -101,11 +93,11 @@ type referencedRWLayer struct { } func (rl *referencedRWLayer) Mount(mountLabel string) (string, error) { - return rl.mountedLayer.Mount(mountLabel) + return rl.layerStore.driver.Get(rl.mountedLayer.mountID, mountLabel) } // Unmount decrements the activity count and unmounts the underlying layer // Callers should only call `Unmount` once per call to `Mount`, even on error. func (rl *referencedRWLayer) Unmount() error { - return rl.mountedLayer.Unmount() + return rl.layerStore.driver.Put(rl.mountedLayer.mountID) }