From 9d00aedebc25507042c5afd4ab8fc6b333ca7c53 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 23 Jan 2018 17:17:13 -0800 Subject: [PATCH 1/2] devmapper cleanup: improve error msg 1. Make sure it's clear the error is from unmount. 2. Simplify the code a bit to make it more readable. [v2: use errors.Wrap] [v3: use errors.Wrapf] [v4: lowercase the error message] Signed-off-by: Kir Kolyshkin --- daemon/graphdriver/devmapper/driver.go | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/daemon/graphdriver/devmapper/driver.go b/daemon/graphdriver/devmapper/driver.go index e27f3c85cc..6bed6634cf 100644 --- a/daemon/graphdriver/devmapper/driver.go +++ b/daemon/graphdriver/devmapper/driver.go @@ -16,6 +16,7 @@ import ( "github.com/docker/docker/pkg/locker" "github.com/docker/docker/pkg/mount" units "github.com/docker/go-units" + "github.com/pkg/errors" "github.com/sirupsen/logrus" "golang.org/x/sys/unix" ) @@ -121,12 +122,18 @@ func (d *Driver) GetMetadata(id string) (map[string]string, error) { // Cleanup unmounts a device. func (d *Driver) Cleanup() error { err := d.DeviceSet.Shutdown(d.home) + umountErr := mount.RecursiveUnmount(d.home) - if err2 := mount.RecursiveUnmount(d.home); err == nil { - err = err2 + // in case we have two errors, prefer the one from Shutdown() + if err != nil { + return err } - return err + if umountErr != nil { + return errors.Wrapf(umountErr, "error unmounting %s", d.home) + } + + return nil } // CreateReadWrite creates a layer that is writable for use as a container From f1a459229724f5e8e440b49f058167c2eeeb2dc6 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 13 Feb 2018 16:10:53 -0800 Subject: [PATCH 2/2] devmapper.shutdown: optimize Move the "unmount and deactivate" code into a separate method, and optimize it a bit: 1. Do not use filepath.Walk() as there's no requirement to recursively go into every directory under home/mnt; a list of directories in mnt is sufficient. With filepath.Walk(), in case some container will fail to unmount, it'll go through the whole container filesystem which is excessive and useless. 2. Do not use GetMounts() and check if a directory is mounted; just unmount it and ignore "not mounted" error. Note the same error is returned in case of wrong flags set, but as flags are hardcoded we can safely ignore such case. While at it, promote "can't unmount" log level from debug to warning. Signed-off-by: Kir Kolyshkin --- daemon/graphdriver/devmapper/deviceset.go | 72 +++++++++++------------ 1 file changed, 33 insertions(+), 39 deletions(-) diff --git a/daemon/graphdriver/devmapper/deviceset.go b/daemon/graphdriver/devmapper/deviceset.go index 3926648e29..ca6251023b 100644 --- a/daemon/graphdriver/devmapper/deviceset.go +++ b/daemon/graphdriver/devmapper/deviceset.go @@ -2223,6 +2223,38 @@ func (devices *DeviceSet) cancelDeferredRemoval(info *devInfo) error { return err } +func (devices *DeviceSet) unmountAndDeactivateAll(dir string) { + files, err := ioutil.ReadDir(dir) + if err != nil { + logrus.Warnf("devmapper: unmountAndDeactivate: %s", err) + return + } + + for _, d := range files { + if !d.IsDir() { + continue + } + + name := d.Name() + fullname := path.Join(dir, name) + + // 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 := unix.Unmount(fullname, unix.MNT_DETACH); err != nil && err != unix.EINVAL { + logrus.Warnf("devmapper: Shutdown unmounting %s, error: %s", fullname, err) + } + + if devInfo, err := devices.lookupDevice(name); err != nil { + logrus.Debugf("devmapper: Shutdown lookup device %s, error: %s", name, err) + } else { + if err := devices.deactivateDevice(devInfo); err != nil { + logrus.Debugf("devmapper: Shutdown deactivate %s, error: %s", devInfo.Hash, err) + } + } + } +} + // Shutdown shuts down the device by unmounting the root. func (devices *DeviceSet) Shutdown(home string) error { logrus.Debugf("devmapper: [deviceset %s] Shutdown()", devices.devicePrefix) @@ -2244,45 +2276,7 @@ func (devices *DeviceSet) Shutdown(home string) error { // will be killed and we will not get a chance to save deviceset // metadata. Hence save this early before trying to deactivate devices. devices.saveDeviceSetMetaData() - - // 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 - } - - 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 := unix.Unmount(p, unix.MNT_DETACH); err != nil { - logrus.Debugf("devmapper: Shutdown unmounting %s, error: %s", p, err) - } - } - - 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.unmountAndDeactivateAll(path.Join(home, "mnt")) devices.Unlock() info, _ := devices.lookupDeviceWithLock("")