diff --git a/daemon/commit.go b/daemon/commit.go index 7bc7b6f25d..7cdf80c775 100644 --- a/daemon/commit.go +++ b/daemon/commit.go @@ -222,6 +222,7 @@ func (daemon *Daemon) exportContainerRw(container *container.Container) (archive archive, err := container.RWLayer.TarStream() if err != nil { + daemon.Unmount(container) // logging is already handled in the `Unmount` function return nil, err } return ioutils.NewReadCloserWrapper(archive, func() error { diff --git a/daemon/graphdriver/aufs/aufs.go b/daemon/graphdriver/aufs/aufs.go index ac0bc5f483..ec9454e72a 100644 --- a/daemon/graphdriver/aufs/aufs.go +++ b/daemon/graphdriver/aufs/aufs.go @@ -29,6 +29,7 @@ import ( "os" "os/exec" "path" + "path/filepath" "strings" "sync" "syscall" @@ -64,21 +65,13 @@ func init() { graphdriver.Register("aufs", Init) } -type data struct { - referenceCount int - path string -} - // Driver contains information about the filesystem mounted. -// root of the filesystem -// sync.Mutex to protect against concurrent modifications -// active maps mount id to the count type Driver struct { - root string - uidMaps []idtools.IDMap - gidMaps []idtools.IDMap - sync.Mutex // Protects concurrent modification to active - active map[string]*data + root string + uidMaps []idtools.IDMap + gidMaps []idtools.IDMap + pathCacheLock sync.Mutex + pathCache map[string]string } // Init returns a new AUFS driver. @@ -111,10 +104,10 @@ func Init(root string, options []string, uidMaps, gidMaps []idtools.IDMap) (grap } a := &Driver{ - root: root, - active: make(map[string]*data), - uidMaps: uidMaps, - gidMaps: gidMaps, + root: root, + uidMaps: uidMaps, + gidMaps: gidMaps, + pathCache: make(map[string]string), } rootUID, rootGID, err := idtools.GetRootUIDGID(uidMaps, gidMaps) @@ -228,9 +221,7 @@ func (a *Driver) Create(id, parent, mountLabel string) error { } } } - a.Lock() - a.active[id] = &data{} - a.Unlock() + return nil } @@ -259,108 +250,91 @@ func (a *Driver) createDirsFor(id string) error { // Remove will unmount and remove the given id. func (a *Driver) Remove(id string) error { - // Protect the a.active from concurrent access - a.Lock() - defer a.Unlock() - - m := a.active[id] - if m != nil { - if m.referenceCount > 0 { - return nil - } - // Make sure the dir is umounted first - if err := a.unmount(m); err != nil { - return err - } + a.pathCacheLock.Lock() + mountpoint, exists := a.pathCache[id] + a.pathCacheLock.Unlock() + if !exists { + mountpoint = a.getMountpoint(id) } - tmpDirs := []string{ - "mnt", - "diff", + if err := a.unmount(mountpoint); err != nil { + // no need to return here, we can still try to remove since the `Rename` will fail below if still mounted + logrus.Debugf("aufs: error while unmounting %s: %v", mountpoint, err) } // Atomically remove each directory in turn by first moving it out of the // way (so that docker doesn't find it anymore) before doing removal of // the whole tree. - for _, p := range tmpDirs { - realPath := path.Join(a.rootPath(), p, id) - tmpPath := path.Join(a.rootPath(), p, fmt.Sprintf("%s-removing", id)) - if err := os.Rename(realPath, tmpPath); err != nil && !os.IsNotExist(err) { - return err - } - defer os.RemoveAll(tmpPath) + tmpMntPath := path.Join(a.mntPath(), fmt.Sprintf("%s-removing", id)) + if err := os.Rename(mountpoint, tmpMntPath); err != nil && !os.IsNotExist(err) { + return err } + defer os.RemoveAll(tmpMntPath) + + tmpDiffpath := path.Join(a.diffPath(), fmt.Sprintf("%s-removing", id)) + if err := os.Rename(a.getDiffPath(id), tmpDiffpath); err != nil && !os.IsNotExist(err) { + return err + } + defer os.RemoveAll(tmpDiffpath) + // Remove the layers file for the id if err := os.Remove(path.Join(a.rootPath(), "layers", id)); err != nil && !os.IsNotExist(err) { return err } - if m != nil { - delete(a.active, id) - } + + a.pathCacheLock.Lock() + delete(a.pathCache, id) + a.pathCacheLock.Unlock() return nil } // Get returns the rootfs path for the id. // This will mount the dir at it's given path func (a *Driver) Get(id, mountLabel string) (string, error) { - // Protect the a.active from concurrent access - a.Lock() - defer a.Unlock() - - m := a.active[id] - if m == nil { - m = &data{} - a.active[id] = m - } - parents, err := a.getParentLayerPaths(id) if err != nil && !os.IsNotExist(err) { return "", err } - // If a dir does not have a parent ( no layers )do not try to mount - // just return the diff path to the data - m.path = path.Join(a.rootPath(), "diff", id) - if len(parents) > 0 { - m.path = path.Join(a.rootPath(), "mnt", id) - if m.referenceCount == 0 { - if err := a.mount(id, m, mountLabel, parents); err != nil { - return "", err - } + a.pathCacheLock.Lock() + m, exists := a.pathCache[id] + a.pathCacheLock.Unlock() + + if !exists { + m = a.getDiffPath(id) + if len(parents) > 0 { + m = a.getMountpoint(id) } } - m.referenceCount++ - return m.path, nil + + // If a dir does not have a parent ( no layers )do not try to mount + // just return the diff path to the data + if len(parents) > 0 { + if err := a.mount(id, m, mountLabel, parents); err != nil { + return "", err + } + } + + a.pathCacheLock.Lock() + a.pathCache[id] = m + a.pathCacheLock.Unlock() + return m, nil } // Put unmounts and updates list of active mounts. func (a *Driver) Put(id string) error { - // Protect the a.active from concurrent access - a.Lock() - defer a.Unlock() + a.pathCacheLock.Lock() + m, exists := a.pathCache[id] + if !exists { + m = a.getMountpoint(id) + a.pathCache[id] = m + } + a.pathCacheLock.Unlock() - m := a.active[id] - if m == nil { - // but it might be still here - if a.Exists(id) { - path := path.Join(a.rootPath(), "mnt", id) - err := Unmount(path) - if err != nil { - logrus.Debugf("Failed to unmount %s aufs: %v", id, err) - } - } - return nil + err := a.unmount(m) + if err != nil { + logrus.Debugf("Failed to unmount %s aufs: %v", id, err) } - if count := m.referenceCount; count > 1 { - m.referenceCount = count - 1 - } else { - ids, _ := getParentIds(a.rootPath(), id) - // We only mounted if there are any parents - if ids != nil && len(ids) > 0 { - a.unmount(m) - } - delete(a.active, id) - } - return nil + return err } // Diff produces an archive of the changes between the specified @@ -443,16 +417,13 @@ func (a *Driver) getParentLayerPaths(id string) ([]string, error) { return layers, nil } -func (a *Driver) mount(id string, m *data, mountLabel string, layers []string) error { +func (a *Driver) mount(id string, target string, mountLabel string, layers []string) error { // If the id is mounted or we get an error return - if mounted, err := a.mounted(m); err != nil || mounted { + if mounted, err := a.mounted(target); err != nil || mounted { return err } - var ( - target = m.path - rw = path.Join(a.rootPath(), "diff", id) - ) + rw := a.getDiffPath(id) if err := a.aufsMount(layers, rw, target, mountLabel); err != nil { return fmt.Errorf("error creating aufs mount to %s: %v", target, err) @@ -460,26 +431,39 @@ func (a *Driver) mount(id string, m *data, mountLabel string, layers []string) e return nil } -func (a *Driver) unmount(m *data) error { - if mounted, err := a.mounted(m); err != nil || !mounted { +func (a *Driver) unmount(mountPath string) error { + if mounted, err := a.mounted(mountPath); err != nil || !mounted { return err } - return Unmount(m.path) + if err := Unmount(mountPath); err != nil { + return err + } + return nil } -func (a *Driver) mounted(m *data) (bool, error) { - var buf syscall.Statfs_t - if err := syscall.Statfs(m.path, &buf); err != nil { - return false, nil - } - return graphdriver.FsMagic(buf.Type) == graphdriver.FsMagicAufs, nil +func (a *Driver) mounted(mountpoint string) (bool, error) { + return graphdriver.Mounted(graphdriver.FsMagicAufs, mountpoint) } // Cleanup aufs and unmount all mountpoints func (a *Driver) Cleanup() error { - for id, m := range a.active { + var dirs []string + if err := filepath.Walk(a.mntPath(), func(path string, info os.FileInfo, err error) error { + if err != nil { + return err + } + if !info.IsDir() { + return nil + } + dirs = append(dirs, path) + return nil + }); err != nil { + return err + } + + for _, m := range dirs { if err := a.unmount(m); err != nil { - logrus.Errorf("Unmounting %s: %s", stringid.TruncateID(id), err) + logrus.Debugf("aufs error unmounting %s: %s", stringid.TruncateID(m), err) } } return mountpk.Unmount(a.root) diff --git a/daemon/graphdriver/aufs/aufs_test.go b/daemon/graphdriver/aufs/aufs_test.go index 0f6d59d054..b0ddf89a2c 100644 --- a/daemon/graphdriver/aufs/aufs_test.go +++ b/daemon/graphdriver/aufs/aufs_test.go @@ -200,7 +200,7 @@ func TestMountedFalseResponse(t *testing.T) { t.Fatal(err) } - response, err := d.mounted(d.active["1"]) + response, err := d.mounted(d.getDiffPath("1")) if err != nil { t.Fatal(err) } @@ -227,7 +227,7 @@ func TestMountedTrueReponse(t *testing.T) { t.Fatal(err) } - response, err := d.mounted(d.active["2"]) + response, err := d.mounted(d.pathCache["2"]) if err != nil { t.Fatal(err) } @@ -293,7 +293,7 @@ func TestRemoveMountedDir(t *testing.T) { t.Fatal("mntPath should not be empty string") } - mounted, err := d.mounted(d.active["2"]) + mounted, err := d.mounted(d.pathCache["2"]) if err != nil { t.Fatal(err) } diff --git a/daemon/graphdriver/aufs/dirs.go b/daemon/graphdriver/aufs/dirs.go index 08f1ffc0ed..eb298d9eeb 100644 --- a/daemon/graphdriver/aufs/dirs.go +++ b/daemon/graphdriver/aufs/dirs.go @@ -46,3 +46,19 @@ func getParentIds(root, id string) ([]string, error) { } return out, s.Err() } + +func (a *Driver) getMountpoint(id string) string { + return path.Join(a.mntPath(), id) +} + +func (a *Driver) mntPath() string { + return path.Join(a.rootPath(), "mnt") +} + +func (a *Driver) getDiffPath(id string) string { + return path.Join(a.diffPath(), id) +} + +func (a *Driver) diffPath() string { + return path.Join(a.rootPath(), "diff") +} diff --git a/daemon/graphdriver/devmapper/deviceset.go b/daemon/graphdriver/devmapper/deviceset.go index 71c214d5d8..cb3bf742a0 100644 --- a/daemon/graphdriver/devmapper/deviceset.go +++ b/daemon/graphdriver/devmapper/deviceset.go @@ -69,9 +69,6 @@ type devInfo struct { Deleted bool `json:"deleted"` devices *DeviceSet - mountCount int - mountPath string - // The global DeviceSet lock guarantees that we serialize all // the calls to libdevmapper (which is not threadsafe), but we // sometimes release that lock while sleeping. In that case @@ -1991,13 +1988,6 @@ func (devices *DeviceSet) DeleteDevice(hash string, syncDelete bool) error { devices.Lock() defer devices.Unlock() - // If mountcount is not zero, that means devices is still in use - // or has not been Put() properly. Fail device deletion. - - if info.mountCount != 0 { - return fmt.Errorf("devmapper: Can't delete device %v as it is still mounted. mntCount=%v", info.Hash, info.mountCount) - } - return devices.deleteDevice(info, syncDelete) } @@ -2116,13 +2106,11 @@ func (devices *DeviceSet) cancelDeferredRemoval(info *devInfo) error { } // Shutdown shuts down the device by unmounting the root. -func (devices *DeviceSet) Shutdown() error { +func (devices *DeviceSet) Shutdown(home string) error { logrus.Debugf("devmapper: [deviceset %s] Shutdown()", devices.devicePrefix) logrus.Debugf("devmapper: Shutting down DeviceSet: %s", devices.root) defer logrus.Debugf("devmapper: [deviceset %s] Shutdown() END", devices.devicePrefix) - var devs []*devInfo - // Stop deletion worker. This should start delivering new events to // ticker channel. That means no new instance of cleanupDeletedDevice() // will run after this call. If one instance is already running at @@ -2139,30 +2127,46 @@ func (devices *DeviceSet) Shutdown() error { // metadata. Hence save this early before trying to deactivate devices. devices.saveDeviceSetMetaData() - for _, info := range devices.Devices { - devs = append(devs, info) + // ignore the error since it's just a best effort to not try to unmount something that's mounted + mounts, _ := mount.GetMounts() + mounted := make(map[string]bool, len(mounts)) + for _, mnt := range mounts { + mounted[mnt.Mountpoint] = true } - devices.Unlock() - for _, info := range devs { - info.lock.Lock() - if info.mountCount > 0 { + if err := filepath.Walk(path.Join(home, "mnt"), func(p string, info os.FileInfo, err error) error { + if err != nil { + return err + } + if !info.IsDir() { + return nil + } + + if mounted[p] { // We use MNT_DETACH here in case it is still busy in some running // container. This means it'll go away from the global scope directly, // and the device will be released when that container dies. - if err := syscall.Unmount(info.mountPath, syscall.MNT_DETACH); err != nil { - logrus.Debugf("devmapper: Shutdown unmounting %s, error: %s", info.mountPath, err) + if err := syscall.Unmount(p, syscall.MNT_DETACH); err != nil { + logrus.Debugf("devmapper: Shutdown unmounting %s, error: %s", p, err) } - - devices.Lock() - if err := devices.deactivateDevice(info); err != nil { - logrus.Debugf("devmapper: Shutdown deactivate %s , error: %s", info.Hash, err) - } - devices.Unlock() } - info.lock.Unlock() + + if devInfo, err := devices.lookupDevice(path.Base(p)); err != nil { + logrus.Debugf("devmapper: Shutdown lookup device %s, error: %s", path.Base(p), err) + } else { + if err := devices.deactivateDevice(devInfo); err != nil { + logrus.Debugf("devmapper: Shutdown deactivate %s , error: %s", devInfo.Hash, err) + } + } + + return nil + }); err != nil && !os.IsNotExist(err) { + devices.Unlock() + return err } + devices.Unlock() + info, _ := devices.lookupDeviceWithLock("") if info != nil { info.lock.Lock() @@ -2202,15 +2206,6 @@ func (devices *DeviceSet) MountDevice(hash, path, mountLabel string) error { devices.Lock() defer devices.Unlock() - if info.mountCount > 0 { - if path != info.mountPath { - return fmt.Errorf("devmapper: Trying to mount devmapper device in multiple places (%s, %s)", info.mountPath, path) - } - - info.mountCount++ - return nil - } - if err := devices.activateDeviceIfNeeded(info, false); err != nil { return fmt.Errorf("devmapper: Error activating devmapper device for '%s': %s", hash, err) } @@ -2234,9 +2229,6 @@ func (devices *DeviceSet) MountDevice(hash, path, mountLabel string) error { return fmt.Errorf("devmapper: Error mounting '%s' on '%s': %s", info.DevName(), path, err) } - info.mountCount = 1 - info.mountPath = path - return nil } @@ -2256,20 +2248,6 @@ func (devices *DeviceSet) UnmountDevice(hash, mountPath string) error { devices.Lock() defer devices.Unlock() - // If there are running containers when daemon crashes, during daemon - // restarting, it will kill running containers and will finally call - // Put() without calling Get(). So info.MountCount may become negative. - // if info.mountCount goes negative, we do the unmount and assign - // it to 0. - - info.mountCount-- - if info.mountCount > 0 { - return nil - } else if info.mountCount < 0 { - logrus.Warnf("devmapper: Mount count of device went negative. Put() called without matching Get(). Resetting count to 0") - info.mountCount = 0 - } - logrus.Debugf("devmapper: Unmount(%s)", mountPath) if err := syscall.Unmount(mountPath, syscall.MNT_DETACH); err != nil { return err @@ -2280,8 +2258,6 @@ func (devices *DeviceSet) UnmountDevice(hash, mountPath string) error { return err } - info.mountPath = "" - return nil } diff --git a/daemon/graphdriver/devmapper/driver.go b/daemon/graphdriver/devmapper/driver.go index c03a7730ed..7de6907c80 100644 --- a/daemon/graphdriver/devmapper/driver.go +++ b/daemon/graphdriver/devmapper/driver.go @@ -108,7 +108,7 @@ func (d *Driver) GetMetadata(id string) (map[string]string, error) { // Cleanup unmounts a device. func (d *Driver) Cleanup() error { - err := d.DeviceSet.Shutdown() + err := d.DeviceSet.Shutdown(d.home) if err2 := mount.Unmount(d.home); err == nil { err = err2 diff --git a/daemon/graphdriver/driver_freebsd.go b/daemon/graphdriver/driver_freebsd.go index be4eb52653..2891a84f3a 100644 --- a/daemon/graphdriver/driver_freebsd.go +++ b/daemon/graphdriver/driver_freebsd.go @@ -1,8 +1,19 @@ package graphdriver +import "syscall" + var ( // Slice of drivers that should be used in an order priority = []string{ "zfs", } ) + +// 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 + if err := syscall.Statfs(mountPath, &buf); err != nil { + return false, err + } + return FsMagic(buf.Type) == fsType, nil +} diff --git a/daemon/graphdriver/driver_linux.go b/daemon/graphdriver/driver_linux.go index e64ab1bfa2..2ab20b01a9 100644 --- a/daemon/graphdriver/driver_linux.go +++ b/daemon/graphdriver/driver_linux.go @@ -42,6 +42,8 @@ const ( FsMagicXfs = FsMagic(0x58465342) // FsMagicZfs filesystem id for Zfs FsMagicZfs = FsMagic(0x2fc12fc1) + // FsMagicOverlay filesystem id for overlay + FsMagicOverlay = FsMagic(0x794C7630) ) var ( @@ -86,3 +88,12 @@ func GetFSMagic(rootpath string) (FsMagic, error) { } return FsMagic(buf.Type), nil } + +// 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 + if err := syscall.Statfs(mountPath, &buf); err != nil { + return false, err + } + return FsMagic(buf.Type) == fsType, nil +} diff --git a/daemon/graphdriver/overlay/overlay.go b/daemon/graphdriver/overlay/overlay.go index 9a521ab5f1..476b7899c9 100644 --- a/daemon/graphdriver/overlay/overlay.go +++ b/daemon/graphdriver/overlay/overlay.go @@ -88,21 +88,13 @@ func (d *naiveDiffDriverWithApply) ApplyDiff(id, parent string, diff archive.Rea // of that. This means all child images share file (but not directory) // data with the parent. -// ActiveMount contains information about the count, path and whether is mounted or not. -// This information is part of the Driver, that contains list of active mounts that are part of this overlay. -type ActiveMount struct { - count int - path string - mounted bool -} - // Driver contains information about the home directory and the list of active mounts that are created using this driver. type Driver struct { - home string - sync.Mutex // Protects concurrent modification to active - active map[string]*ActiveMount - uidMaps []idtools.IDMap - gidMaps []idtools.IDMap + home string + pathCacheLock sync.Mutex + pathCache map[string]string + uidMaps []idtools.IDMap + gidMaps []idtools.IDMap } var backingFs = "" @@ -151,10 +143,10 @@ func Init(home string, options []string, uidMaps, gidMaps []idtools.IDMap) (grap } d := &Driver{ - home: home, - active: make(map[string]*ActiveMount), - uidMaps: uidMaps, - gidMaps: gidMaps, + home: home, + pathCache: make(map[string]string), + uidMaps: uidMaps, + gidMaps: gidMaps, } return NaiveDiffDriverWithApply(d, uidMaps, gidMaps), nil @@ -325,23 +317,14 @@ 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) { - // Protect the d.active from concurrent access - d.Lock() - defer d.Unlock() - - mount := d.active[id] - if mount != nil { - mount.count++ - return mount.path, nil - } - - mount = &ActiveMount{count: 1} - dir := d.dir(id) if _, err := os.Stat(dir); err != nil { return "", err @@ -350,9 +333,10 @@ func (d *Driver) Get(id string, mountLabel string) (string, error) { // If id has a root, just return it rootDir := path.Join(dir, "root") if _, err := os.Stat(rootDir); err == nil { - mount.path = rootDir - d.active[id] = mount - return mount.path, nil + d.pathCacheLock.Lock() + d.pathCache[id] = rootDir + d.pathCacheLock.Unlock() + return rootDir, nil } lowerID, err := ioutil.ReadFile(path.Join(dir, "lower-id")) @@ -388,42 +372,38 @@ func (d *Driver) Get(id string, mountLabel string) (string, error) { if err := os.Chown(path.Join(workDir, "work"), rootUID, rootGID); err != nil { return "", err } - mount.path = mergedDir - mount.mounted = true - d.active[id] = mount - return mount.path, nil + d.pathCacheLock.Lock() + d.pathCache[id] = mergedDir + d.pathCacheLock.Unlock() + + return mergedDir, nil +} + +func (d *Driver) mounted(dir string) (bool, error) { + return graphdriver.Mounted(graphdriver.FsMagicOverlay, dir) } // Put unmounts the mount path created for the give id. func (d *Driver) Put(id string) error { - // Protect the d.active from concurrent access - d.Lock() - defer d.Unlock() + d.pathCacheLock.Lock() + mountpoint, exists := d.pathCache[id] + d.pathCacheLock.Unlock() - mount := d.active[id] - if mount == nil { + if !exists { logrus.Debugf("Put on a non-mounted device %s", id) // but it might be still here if d.Exists(id) { - mergedDir := path.Join(d.dir(id), "merged") - err := syscall.Unmount(mergedDir, 0) - if err != nil { - logrus.Debugf("Failed to unmount %s overlay: %v", id, err) - } + mountpoint = path.Join(d.dir(id), "merged") } - return nil + + d.pathCacheLock.Lock() + d.pathCache[id] = mountpoint + d.pathCacheLock.Unlock() } - mount.count-- - if mount.count > 0 { - return nil - } - - defer delete(d.active, id) - if mount.mounted { - err := syscall.Unmount(mount.path, 0) - if err != nil { + 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 diff --git a/daemon/graphdriver/windows/windows.go b/daemon/graphdriver/windows/windows.go index 2b5b549e20..dd659dad0a 100644 --- a/daemon/graphdriver/windows/windows.go +++ b/daemon/graphdriver/windows/windows.go @@ -13,7 +13,6 @@ import ( "path" "path/filepath" "strings" - "sync" "syscall" "time" @@ -47,10 +46,6 @@ const ( type Driver struct { // info stores the shim driver information info hcsshim.DriverInfo - // Mutex protects concurrent modification to active - sync.Mutex - // active stores references to the activated layers - active map[string]int } var _ graphdriver.DiffGetterDriver = &Driver{} @@ -63,7 +58,6 @@ func InitFilter(home string, options []string, uidMaps, gidMaps []idtools.IDMap) HomeDir: home, Flavour: filterDriver, }, - active: make(map[string]int), } return d, nil } @@ -76,7 +70,6 @@ func InitDiff(home string, options []string, uidMaps, gidMaps []idtools.IDMap) ( HomeDir: home, Flavour: diffDriver, }, - active: make(map[string]int), } return d, nil } @@ -189,9 +182,6 @@ func (d *Driver) Get(id, mountLabel string) (string, error) { logrus.Debugf("WindowsGraphDriver Get() id %s mountLabel %s", id, mountLabel) var dir string - d.Lock() - defer d.Unlock() - rID, err := d.resolveID(id) if err != nil { return "", err @@ -203,16 +193,14 @@ func (d *Driver) Get(id, mountLabel string) (string, error) { return "", err } - if d.active[rID] == 0 { - if err := hcsshim.ActivateLayer(d.info, rID); err != nil { - return "", err - } - if err := hcsshim.PrepareLayer(d.info, rID, layerChain); err != nil { - if err2 := hcsshim.DeactivateLayer(d.info, rID); err2 != nil { - logrus.Warnf("Failed to Deactivate %s: %s", id, err) - } - return "", err + if err := hcsshim.ActivateLayer(d.info, rID); err != nil { + return "", err + } + if err := hcsshim.PrepareLayer(d.info, rID, layerChain); err != nil { + if err2 := hcsshim.DeactivateLayer(d.info, rID); err2 != nil { + logrus.Warnf("Failed to Deactivate %s: %s", id, err) } + return "", err } mountPath, err := hcsshim.GetLayerMountPath(d.info, rID) @@ -223,8 +211,6 @@ func (d *Driver) Get(id, mountLabel string) (string, error) { return "", err } - d.active[rID]++ - // If the layer has a mount path, use that. Otherwise, use the // folder path. if mountPath != "" { @@ -245,22 +231,10 @@ func (d *Driver) Put(id string) error { return err } - d.Lock() - defer d.Unlock() - - if d.active[rID] > 1 { - d.active[rID]-- - } else if d.active[rID] == 1 { - if err := hcsshim.UnprepareLayer(d.info, rID); err != nil { - return err - } - if err := hcsshim.DeactivateLayer(d.info, rID); err != nil { - return err - } - delete(d.active, rID) + if err := hcsshim.UnprepareLayer(d.info, rID); err != nil { + return err } - - return nil + return hcsshim.DeactivateLayer(d.info, rID) } // Cleanup ensures the information the driver stores is properly removed. @@ -270,62 +244,40 @@ func (d *Driver) Cleanup() error { // Diff produces an archive of the changes between the specified // layer and its parent layer which may be "". +// The layer should be mounted when calling this function func (d *Driver) Diff(id, parent string) (_ archive.Archive, err error) { rID, err := d.resolveID(id) if err != nil { return } - // Getting the layer paths must be done outside of the lock. layerChain, err := d.getLayerChain(rID) if err != nil { return } - var undo func() - - d.Lock() - - // To support export, a layer must be activated but not prepared. - if d.info.Flavour == filterDriver { - if d.active[rID] == 0 { - if err = hcsshim.ActivateLayer(d.info, rID); err != nil { - d.Unlock() - return - } - undo = func() { - if err := hcsshim.DeactivateLayer(d.info, rID); err != nil { - logrus.Warnf("Failed to Deactivate %s: %s", rID, err) - } - } - } else { - if err = hcsshim.UnprepareLayer(d.info, rID); err != nil { - d.Unlock() - return - } - undo = func() { - if err := hcsshim.PrepareLayer(d.info, rID, layerChain); err != nil { - logrus.Warnf("Failed to re-PrepareLayer %s: %s", rID, err) - } - } - } + // this is assuming that the layer is unmounted + if err := hcsshim.UnprepareLayer(d.info, rID); err != nil { + return nil, err } - - d.Unlock() + defer func() { + if err := hcsshim.PrepareLayer(d.info, rID, layerChain); err != nil { + logrus.Warnf("Failed to Deactivate %s: %s", rID, err) + } + }() arch, err := d.exportLayer(rID, layerChain) if err != nil { - undo() return } return ioutils.NewReadCloserWrapper(arch, func() error { - defer undo() return arch.Close() }), nil } // Changes produces a list of changes between the specified layer // and its parent layer. If parent is "", then all changes will be ADD changes. +// The layer should be mounted when calling this function func (d *Driver) Changes(id, parent string) ([]archive.Change, error) { rID, err := d.resolveID(id) if err != nil { @@ -336,31 +288,15 @@ func (d *Driver) Changes(id, parent string) ([]archive.Change, error) { return nil, err } - d.Lock() - if d.info.Flavour == filterDriver { - if d.active[rID] == 0 { - if err = hcsshim.ActivateLayer(d.info, rID); err != nil { - d.Unlock() - return nil, err - } - defer func() { - if err := hcsshim.DeactivateLayer(d.info, rID); err != nil { - logrus.Warnf("Failed to Deactivate %s: %s", rID, err) - } - }() - } else { - if err = hcsshim.UnprepareLayer(d.info, rID); err != nil { - d.Unlock() - return nil, err - } - defer func() { - if err := hcsshim.PrepareLayer(d.info, rID, parentChain); err != nil { - logrus.Warnf("Failed to re-PrepareLayer %s: %s", rID, err) - } - }() - } + // this is assuming that the layer is unmounted + if err := hcsshim.UnprepareLayer(d.info, rID); err != nil { + return nil, err } - d.Unlock() + defer func() { + if err := hcsshim.PrepareLayer(d.info, rID, parentChain); err != nil { + logrus.Warnf("Failed to Deactivate %s: %s", rID, err) + } + }() r, err := hcsshim.NewLayerReader(d.info, id, parentChain) if err != nil { @@ -391,6 +327,7 @@ func (d *Driver) Changes(id, parent string) ([]archive.Change, error) { // ApplyDiff extracts the changeset from the given diff into the // layer with the specified id and parent, returning the size of the // new layer in bytes. +// The layer should not be mounted when calling this function func (d *Driver) ApplyDiff(id, parent string, diff archive.Reader) (size int64, err error) { rPId, err := d.resolveID(parent) if err != nil { diff --git a/daemon/graphdriver/zfs/zfs.go b/daemon/graphdriver/zfs/zfs.go index 28a94dd0b5..e92045bd83 100644 --- a/daemon/graphdriver/zfs/zfs.go +++ b/daemon/graphdriver/zfs/zfs.go @@ -22,12 +22,6 @@ import ( "github.com/opencontainers/runc/libcontainer/label" ) -type activeMount struct { - count int - path string - mounted bool -} - type zfsOptions struct { fsName string mountPath string @@ -109,7 +103,6 @@ func Init(base string, opt []string, uidMaps, gidMaps []idtools.IDMap) (graphdri dataset: rootDataset, options: options, filesystemsCache: filesystemsCache, - active: make(map[string]*activeMount), uidMaps: uidMaps, gidMaps: gidMaps, } @@ -166,7 +159,6 @@ type Driver struct { options zfsOptions sync.Mutex // protects filesystem cache against concurrent access filesystemsCache map[string]bool - active map[string]*activeMount uidMaps []idtools.IDMap gidMaps []idtools.IDMap } @@ -302,17 +294,6 @@ 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) { - d.Lock() - defer d.Unlock() - - mnt := d.active[id] - if mnt != nil { - mnt.count++ - return mnt.path, nil - } - - mnt = &activeMount{count: 1} - mountpoint := d.mountPath(id) filesystem := d.zfsPath(id) options := label.FormatMountLabel("", mountLabel) @@ -335,48 +316,29 @@ func (d *Driver) Get(id, mountLabel string) (string, error) { if err := os.Chown(mountpoint, rootUID, rootGID); err != nil { return "", fmt.Errorf("error modifying zfs mountpoint (%s) directory ownership: %v", mountpoint, err) } - mnt.path = mountpoint - mnt.mounted = true - d.active[id] = mnt return mountpoint, nil } // Put removes the existing mountpoint for the given id if it exists. func (d *Driver) Put(id string) error { - d.Lock() - defer d.Unlock() - - mnt := d.active[id] - if mnt == nil { - logrus.Debugf("[zfs] Put on a non-mounted device %s", id) - // but it might be still here - if d.Exists(id) { - err := mount.Unmount(d.mountPath(id)) - if err != nil { - logrus.Debugf("[zfs] Failed to unmount %s zfs fs: %v", id, err) - } - } - return nil + mountpoint := d.mountPath(id) + mounted, err := graphdriver.Mounted(graphdriver.FsMagicZfs, mountpoint) + if err != nil || !mounted { + return err } - mnt.count-- - if mnt.count > 0 { - return nil - } + logrus.Debugf(`[zfs] unmount("%s")`, mountpoint) - defer delete(d.active, id) - if mnt.mounted { - logrus.Debugf(`[zfs] unmount("%s")`, mnt.path) - - if err := mount.Unmount(mnt.path); err != nil { - return fmt.Errorf("error unmounting to %s: %v", mnt.path, err) - } + if err := mount.Unmount(mountpoint); err != nil { + return fmt.Errorf("error unmounting to %s: %v", mountpoint, err) } return nil } // Exists checks to see if the cache entry exists for the given id. func (d *Driver) Exists(id string) bool { + d.Lock() + defer d.Unlock() return d.filesystemsCache[d.zfsPath(id)] == true } diff --git a/integration-cli/benchmark_test.go b/integration-cli/benchmark_test.go new file mode 100644 index 0000000000..647d014d30 --- /dev/null +++ b/integration-cli/benchmark_test.go @@ -0,0 +1,95 @@ +package main + +import ( + "fmt" + "io/ioutil" + "os" + "runtime" + "strings" + "sync" + + "github.com/docker/docker/pkg/integration/checker" + "github.com/go-check/check" +) + +func (s *DockerSuite) BenchmarkConcurrentContainerActions(c *check.C) { + maxConcurrency := runtime.GOMAXPROCS(0) + numIterations := c.N + outerGroup := &sync.WaitGroup{} + outerGroup.Add(maxConcurrency) + chErr := make(chan error, numIterations*2*maxConcurrency) + + for i := 0; i < maxConcurrency; i++ { + go func() { + defer outerGroup.Done() + innerGroup := &sync.WaitGroup{} + innerGroup.Add(2) + + go func() { + defer innerGroup.Done() + for i := 0; i < numIterations; i++ { + args := []string{"run", "-d", defaultSleepImage} + args = append(args, defaultSleepCommand...) + out, _, err := dockerCmdWithError(args...) + if err != nil { + chErr <- fmt.Errorf(out) + return + } + + id := strings.TrimSpace(out) + tmpDir, err := ioutil.TempDir("", "docker-concurrent-test-"+id) + if err != nil { + chErr <- err + return + } + defer os.RemoveAll(tmpDir) + out, _, err = dockerCmdWithError("cp", id+":/tmp", tmpDir) + if err != nil { + chErr <- fmt.Errorf(out) + return + } + + out, _, err = dockerCmdWithError("kill", id) + if err != nil { + chErr <- fmt.Errorf(out) + } + + out, _, err = dockerCmdWithError("start", id) + if err != nil { + chErr <- fmt.Errorf(out) + } + + out, _, err = dockerCmdWithError("kill", id) + if err != nil { + chErr <- fmt.Errorf(out) + } + + // don't do an rm -f here since it can potentially ignore errors from the graphdriver + out, _, err = dockerCmdWithError("rm", id) + if err != nil { + chErr <- fmt.Errorf(out) + } + } + }() + + go func() { + defer innerGroup.Done() + for i := 0; i < numIterations; i++ { + out, _, err := dockerCmdWithError("ps") + if err != nil { + chErr <- fmt.Errorf(out) + } + } + }() + + innerGroup.Wait() + }() + } + + outerGroup.Wait() + close(chErr) + + for err := range chErr { + c.Assert(err, checker.IsNil) + } +} diff --git a/layer/layer.go b/layer/layer.go index 26a82440ea..be3fd8329c 100644 --- a/layer/layer.go +++ b/layer/layer.go @@ -49,6 +49,10 @@ var ( // to be created which would result in a layer depth // greater than the 125 max. ErrMaxDepthExceeded = errors.New("max depth exceeded") + + // ErrNotSupported is used when the action is not supppoted + // on the current platform + ErrNotSupported = errors.New("not support on this platform") ) // ChainID is the content-addressable ID of a layer. diff --git a/layer/mounted_layer.go b/layer/mounted_layer.go index bf662e9a42..36a8eb44ce 100644 --- a/layer/mounted_layer.go +++ b/layer/mounted_layer.go @@ -12,6 +12,7 @@ type mountedLayer struct { mountID string initID string parent *roLayer + path string layerStore *layerStore references map[RWLayer]*referencedRWLayer @@ -131,10 +132,21 @@ func (rl *referencedRWLayer) Mount(mountLabel string) (string, error) { return "", ErrLayerNotRetained } - rl.activityCount++ - return rl.mountedLayer.Mount(mountLabel) + 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 } +// 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() @@ -145,7 +157,11 @@ func (rl *referencedRWLayer) Unmount() error { if rl.activityCount == -1 { return ErrLayerNotRetained } + rl.activityCount-- + if rl.activityCount > 0 { + return nil + } return rl.mountedLayer.Unmount() }