Merge pull request #262 from kolyshkin/18.09-backport-aufs-lock

[18.09 backport ENGCORE-830] aufs optimizations #39107
Upstream-commit: 95d8b395a981bf09e9c10eac06efed9ed44ee456
Component: engine

components/engine/container/container_unix.go

@@ -175,8 +175,8 @@ func (container *Container) HasMountFor(path string) bool {
 	return false
 }
 
-// UnmountIpcMount uses the provided unmount function to unmount shm if it was mounted
-func (container *Container) UnmountIpcMount(unmount func(pth string) error) error {
+// UnmountIpcMount unmounts shm if it was mounted
+func (container *Container) UnmountIpcMount() error {
 	if container.HasMountFor("/dev/shm") {
 		return nil
 	}
@@ -190,10 +190,8 @@ func (container *Container) UnmountIpcMount(unmount func(pth string) error) erro
 	if shmPath == "" {
 		return nil
 	}
-	if err = unmount(shmPath); err != nil && !os.IsNotExist(err) {
-		if mounted, mErr := mount.Mounted(shmPath); mounted || mErr != nil {
-			return errors.Wrapf(err, "umount %s", shmPath)
-		}
+	if err = mount.Unmount(shmPath); err != nil && !os.IsNotExist(err) {
+		return err
 	}
 	return nil
 }
@@ -383,7 +381,8 @@ func (container *Container) DetachAndUnmount(volumeEventLog func(name, action st
 
 	for _, mountPath := range mountPaths {
 		if err := mount.Unmount(mountPath); err != nil {
-			logrus.Warnf("%s unmountVolumes: Failed to do lazy umount fo volume '%s': %v", container.ID, mountPath, err)
+			logrus.WithError(err).WithField("container", container.ID).
+				Warn("Unable to unmount")
 		}
 	}
 	return container.UnmountVolumes(volumeEventLog)

components/engine/container/container_windows.go

@@ -22,7 +22,7 @@ const (
 
 // UnmountIpcMount unmounts Ipc related mounts.
 // This is a NOOP on windows.
-func (container *Container) UnmountIpcMount(unmount func(pth string) error) error {
+func (container *Container) UnmountIpcMount() error {
 	return nil
 }
 

components/engine/daemon/container_operations_unix.go

@@ -351,10 +351,6 @@ func killProcessDirectly(cntr *container.Container) error {
 	return nil
 }
 
-func detachMounted(path string) error {
-	return unix.Unmount(path, unix.MNT_DETACH)
-}
-
 func isLinkable(child *container.Container) bool {
 	// A container is linkable only if it belongs to the default network
 	_, ok := child.NetworkSettings.Networks[runconfig.DefaultDaemonNetworkMode().NetworkName()]

components/engine/daemon/container_operations_windows.go

@@ -78,10 +78,6 @@ func (daemon *Daemon) mountVolumes(container *container.Container) error {
 	return nil
 }
 
-func detachMounted(path string) error {
-	return nil
-}
-
 func (daemon *Daemon) setupSecretDir(c *container.Container) (setupErr error) {
 	if len(c.SecretReferences) == 0 {
 		return nil

components/engine/daemon/graphdriver/aufs/aufs.go

@@ -43,7 +43,7 @@ import (
 	"github.com/docker/docker/pkg/directory"
 	"github.com/docker/docker/pkg/idtools"
 	"github.com/docker/docker/pkg/locker"
-	mountpk "github.com/docker/docker/pkg/mount"
+	"github.com/docker/docker/pkg/mount"
 	"github.com/docker/docker/pkg/system"
 	rsystem "github.com/opencontainers/runc/libcontainer/system"
 	"github.com/opencontainers/selinux/go-selinux/label"
@@ -72,7 +72,6 @@ func init() {
 
 // Driver contains information about the filesystem mounted.
 type Driver struct {
-	sync.Mutex
 	root          string
 	uidMaps       []idtools.IDMap
 	gidMaps       []idtools.IDMap
@@ -81,6 +80,7 @@ type Driver struct {
 	pathCache     map[string]string
 	naiveDiff     graphdriver.DiffDriver
 	locker        *locker.Locker
+	mntL          sync.Mutex
 }
 
 // Init returns a new AUFS driver.
@@ -327,11 +327,11 @@ func (a *Driver) Remove(id string) error {
 			break
 		}
 
-		if err != unix.EBUSY {
-			return errors.Wrapf(err, "aufs: unmount error: %s", mountpoint)
+		if errors.Cause(err) != unix.EBUSY {
+			return errors.Wrap(err, "aufs: unmount error")
 		}
 		if retries >= 5 {
-			return errors.Wrapf(err, "aufs: unmount error after retries: %s", mountpoint)
+			return errors.Wrap(err, "aufs: unmount error after retries")
 		}
 		// If unmount returns EBUSY, it could be a transient error. Sleep and retry.
 		retries++
@@ -437,7 +437,7 @@ func (a *Driver) Put(id string) error {
 
 	err := a.unmount(m)
 	if err != nil {
-		logger.Debugf("Failed to unmount %s aufs: %v", id, err)
+		logger.WithError(err).WithField("method", "Put()").Warn()
 	}
 	return err
 }
@@ -547,9 +547,6 @@ func (a *Driver) getParentLayerPaths(id string) ([]string, error) {
 }
 
 func (a *Driver) mount(id string, target string, mountLabel string, layers []string) error {
-	a.Lock()
-	defer a.Unlock()
-
 	// If the id is mounted or we get an error return
 	if mounted, err := a.mounted(target); err != nil || mounted {
 		return err
@@ -564,9 +561,6 @@ func (a *Driver) mount(id string, target string, mountLabel string, layers []str
 }
 
 func (a *Driver) unmount(mountPath string) error {
-	a.Lock()
-	defer a.Unlock()
-
 	if mounted, err := a.mounted(mountPath); err != nil || !mounted {
 		return err
 	}
@@ -579,32 +573,29 @@ func (a *Driver) mounted(mountpoint string) (bool, error) {
 
 // Cleanup aufs and unmount all mountpoints
 func (a *Driver) Cleanup() error {
-	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
+	dir := a.mntPath()
+	files, err := ioutil.ReadDir(dir)
+	if err != nil {
+		return errors.Wrap(err, "aufs readdir error")
 	}
+	for _, f := range files {
+		if !f.IsDir() {
+			continue
+		}
+
+		m := path.Join(dir, f.Name())
 
-	for _, m := range dirs {
 		if err := a.unmount(m); err != nil {
-			logger.Debugf("error unmounting %s: %s", m, err)
+			logger.WithError(err).WithField("method", "Cleanup()").Warn()
 		}
 	}
-	return mountpk.RecursiveUnmount(a.root)
+	return mount.RecursiveUnmount(a.root)
 }
 
 func (a *Driver) aufsMount(ro []string, rw, target, mountLabel string) (err error) {
 	defer func() {
 		if err != nil {
-			Unmount(target)
+			mount.Unmount(target)
 		}
 	}()
 
@@ -632,14 +623,29 @@ func (a *Driver) aufsMount(ro []string, rw, target, mountLabel string) (err erro
 		opts += ",dirperm1"
 	}
 	data := label.FormatMountLabel(fmt.Sprintf("%s,%s", string(b[:bp]), opts), mountLabel)
-	if err = mount("none", target, "aufs", 0, data); err != nil {
+	a.mntL.Lock()
+	err = unix.Mount("none", target, "aufs", 0, data)
+	a.mntL.Unlock()
+	if err != nil {
+		err = errors.Wrap(err, "mount target="+target+" data="+data)
 		return
 	}
 
-	for ; index < len(ro); index++ {
-		layer := fmt.Sprintf(":%s=ro+wh", ro[index])
-		data := label.FormatMountLabel(fmt.Sprintf("append%s", layer), mountLabel)
-		if err = mount("none", target, "aufs", unix.MS_REMOUNT, data); err != nil {
+	for index < len(ro) {
+		bp = 0
+		for ; index < len(ro); index++ {
+			layer := fmt.Sprintf("append:%s=ro+wh,", ro[index])
+			if bp+len(layer) > len(b) {
+				break
+			}
+			bp += copy(b[bp:], layer)
+		}
+		data := label.FormatMountLabel(string(b[:bp]), mountLabel)
+		a.mntL.Lock()
+		err = unix.Mount("none", target, "aufs", unix.MS_REMOUNT, data)
+		a.mntL.Unlock()
+		if err != nil {
+			err = errors.Wrap(err, "mount target="+target+" flags=MS_REMOUNT data="+data)
 			return
 		}
 	}
@@ -666,7 +672,7 @@ func useDirperm() bool {
 		defer os.RemoveAll(union)
 
 		opts := fmt.Sprintf("br:%s,dirperm1,xino=/dev/shm/aufs.xino", base)
-		if err := mount("none", union, "aufs", 0, opts); err != nil {
+		if err := unix.Mount("none", union, "aufs", 0, opts); err != nil {
 			return
 		}
 		enableDirperm = true

components/engine/daemon/graphdriver/aufs/mount.go

@@ -4,14 +4,38 @@ package aufs // import "github.com/docker/docker/daemon/graphdriver/aufs"
 
 import (
 	"os/exec"
+	"syscall"
 
-	"golang.org/x/sys/unix"
+	"github.com/docker/docker/pkg/mount"
 )
 
 // Unmount the target specified.
 func Unmount(target string) error {
-	if err := exec.Command("auplink", target, "flush").Run(); err != nil {
-		logger.WithError(err).Warnf("Couldn't run auplink before unmount %s", target)
+	const (
+		EINVAL  = 22 // if auplink returns this,
+		retries = 3  // retry a few times
+	)
+
+	for i := 0; ; i++ {
+		out, err := exec.Command("auplink", target, "flush").CombinedOutput()
+		if err == nil {
+			break
+		}
+		rc := 0
+		if exiterr, ok := err.(*exec.ExitError); ok {
+			if status, ok := exiterr.Sys().(syscall.WaitStatus); ok {
+				rc = status.ExitStatus()
+			}
+		}
+		if i >= retries || rc != EINVAL {
+			logger.WithError(err).WithField("method", "Unmount").Warnf("auplink flush failed: %s", out)
+			break
+		}
+		// auplink failed to find target in /proc/self/mounts because
+		// kernel can't guarantee continuity while reading from it
+		// while mounts table is being changed
+		logger.Debugf("auplink flush error (retrying %d/%d): %s", i+1, retries, out)
 	}
-	return unix.Unmount(target, 0)
+
+	return mount.Unmount(target)
 }

components/engine/daemon/graphdriver/aufs/mount_linux.go

@@ -1,7 +0,0 @@
-package aufs // import "github.com/docker/docker/daemon/graphdriver/aufs"
-
-import "golang.org/x/sys/unix"
-
-func mount(source string, target string, fstype string, flags uintptr, data string) error {
-	return unix.Mount(source, target, fstype, flags, data)
-}

components/engine/daemon/graphdriver/aufs/mount_unsupported.go

@@ -1,12 +0,0 @@
-// +build !linux
-
-package aufs // import "github.com/docker/docker/daemon/graphdriver/aufs"
-
-import "errors"
-
-// MsRemount declared to specify a non-linux system mount.
-const MsRemount = 0
-
-func mount(source string, target string, fstype string, flags uintptr, data string) (err error) {
-	return errors.New("mount is not implemented on this platform")
-}

components/engine/daemon/graphdriver/btrfs/btrfs.go

@@ -178,7 +178,7 @@ func (d *Driver) Cleanup() error {
 	}
 
 	if umountErr != nil {
-		return errors.Wrapf(umountErr, "error unmounting %s", d.home)
+		return umountErr
 	}
 
 	return nil

components/engine/daemon/graphdriver/devmapper/deviceset.go

@@ -1200,7 +1200,7 @@ func (devices *DeviceSet) growFS(info *devInfo) error {
 	options = joinMountOptions(options, devices.mountOptions)
 
 	if err := mount.Mount(info.DevName(), fsMountPoint, devices.BaseDeviceFilesystem, options); err != nil {
-		return fmt.Errorf("Error mounting '%s' on '%s' (fstype='%s' options='%s'): %s\n%v", info.DevName(), fsMountPoint, devices.BaseDeviceFilesystem, options, err, string(dmesg.Dmesg(256)))
+		return errors.Wrapf(err, "Failed to mount; dmesg: %s", string(dmesg.Dmesg(256)))
 	}
 
 	defer unix.Unmount(fsMountPoint, unix.MNT_DETACH)
@@ -2381,7 +2381,7 @@ func (devices *DeviceSet) MountDevice(hash, path, mountLabel string) error {
 	options = joinMountOptions(options, label.FormatMountLabel("", mountLabel))
 
 	if err := mount.Mount(info.DevName(), path, fstype, options); err != nil {
-		return fmt.Errorf("devmapper: Error mounting '%s' on '%s' (fstype='%s' options='%s'): %s\n%v", info.DevName(), path, fstype, options, err, string(dmesg.Dmesg(256)))
+		return errors.Wrapf(err, "Failed to mount; dmesg: %s", string(dmesg.Dmesg(256)))
 	}
 
 	if fstype == "xfs" && devices.xfsNospaceRetries != "" {

components/engine/daemon/graphdriver/devmapper/driver.go

@@ -16,7 +16,6 @@ import (
 	"github.com/docker/docker/pkg/locker"
 	"github.com/docker/docker/pkg/mount"
 	"github.com/docker/go-units"
-	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
 	"golang.org/x/sys/unix"
 )
@@ -129,11 +128,7 @@ func (d *Driver) Cleanup() error {
 		return err
 	}
 
-	if umountErr != nil {
-		return errors.Wrapf(umountErr, "error unmounting %s", d.home)
-	}
-
-	return nil
+	return umountErr
 }
 
 // CreateReadWrite creates a layer that is writable for use as a container

components/engine/daemon/graphdriver/zfs/zfs.go

@@ -19,6 +19,7 @@ import (
 	"github.com/docker/docker/pkg/parsers"
 	"github.com/mistifyio/go-zfs"
 	"github.com/opencontainers/selinux/go-selinux/label"
+	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
 	"golang.org/x/sys/unix"
 )
@@ -390,7 +391,7 @@ func (d *Driver) Get(id, mountLabel string) (_ containerfs.ContainerFS, retErr e
 	}
 
 	if err := mount.Mount(filesystem, mountpoint, "zfs", options); err != nil {
-		return nil, fmt.Errorf("error creating zfs mount of %s to %s: %v", filesystem, mountpoint, err)
+		return nil, errors.Wrap(err, "error creating zfs mount")
 	}
 
 	// this could be our first mount after creation of the filesystem, and the root dir may still have root

components/engine/daemon/start.go

@@ -229,7 +229,7 @@ func (daemon *Daemon) containerStart(container *container.Container, checkpoint
 func (daemon *Daemon) Cleanup(container *container.Container) {
 	daemon.releaseNetwork(container)
 
-	if err := container.UnmountIpcMount(detachMounted); err != nil {
+	if err := container.UnmountIpcMount(); err != nil {
 		logrus.Warnf("%s cleanup: failed to unmount IPC: %s", container.ID, err)
 	}
 

components/engine/pkg/mount/mount.go

@@ -2,12 +2,46 @@ package mount // import "github.com/docker/docker/pkg/mount"
 
 import (
 	"sort"
+	"strconv"
 	"strings"
-	"syscall"
 
 	"github.com/sirupsen/logrus"
 )
 
+// mountError records an error from mount or unmount operation
+type mountError struct {
+	op             string
+	source, target string
+	flags          uintptr
+	data           string
+	err            error
+}
+
+func (e *mountError) Error() string {
+	out := e.op + " "
+
+	if e.source != "" {
+		out += e.source + ":" + e.target
+	} else {
+		out += e.target
+	}
+
+	if e.flags != uintptr(0) {
+		out += ", flags: 0x" + strconv.FormatUint(uint64(e.flags), 16)
+	}
+	if e.data != "" {
+		out += ", data: " + e.data
+	}
+
+	out += ": " + e.err.Error()
+	return out
+}
+
+// Cause returns the underlying cause of the error
+func (e *mountError) Cause() error {
+	return e.err
+}
+
 // FilterFunc is a type defining a callback function
 // to filter out unwanted entries. It takes a pointer
 // to an Info struct (not fully populated, currently
@@ -89,12 +123,7 @@ func ForceMount(device, target, mType, options string) error {
 // Unmount lazily unmounts a filesystem on supported platforms, otherwise
 // does a normal unmount.
 func Unmount(target string) error {
-	err := unmount(target, mntDetach)
-	if err == syscall.EINVAL {
-		// ignore "not mounted" error
-		err = nil
-	}
-	return err
+	return unmount(target, mntDetach)
 }
 
 // RecursiveUnmount unmounts the target and all mounts underneath, starting with
@@ -114,25 +143,14 @@ func RecursiveUnmount(target string) error {
 		logrus.Debugf("Trying to unmount %s", m.Mountpoint)
 		err = unmount(m.Mountpoint, mntDetach)
 		if err != nil {
-			// If the error is EINVAL either this whole package is wrong (invalid flags passed to unmount(2)) or this is
-			// not a mountpoint (which is ok in this case).
-			// Meanwhile calling `Mounted()` is very expensive.
-			//
-			// We've purposefully used `syscall.EINVAL` here instead of `unix.EINVAL` to avoid platform branching
-			// Since `EINVAL` is defined for both Windows and Linux in the `syscall` package (and other platforms),
-			//   this is nicer than defining a custom value that we can refer to in each platform file.
-			if err == syscall.EINVAL {
-				continue
-			}
-			if i == len(mounts)-1 {
+			if i == len(mounts)-1 { // last mount
 				if mounted, e := Mounted(m.Mountpoint); e != nil || mounted {
 					return err
 				}
-				continue
+			} else {
+				// This is some submount, we can ignore this error for now, the final unmount will fail if this is a real problem
+				logrus.WithError(err).Warnf("Failed to unmount submount %s", m.Mountpoint)
 			}
-			// This is some submount, we can ignore this error for now, the final unmount will fail if this is a real problem
-			logrus.WithError(err).Warnf("Failed to unmount submount %s", m.Mountpoint)
-			continue
 		}
 
 		logrus.Debugf("Unmounted %s", m.Mountpoint)

components/engine/pkg/mount/mounter_freebsd.go

@@ -11,11 +11,9 @@ package mount // import "github.com/docker/docker/pkg/mount"
 import "C"
 
 import (
-	"fmt"
 	"strings"
+	"syscall"
 	"unsafe"
-
-	"golang.org/x/sys/unix"
 )
 
 func allocateIOVecs(options []string) []C.struct_iovec {
@@ -49,12 +47,13 @@ func mount(device, target, mType string, flag uintptr, data string) error {
 	}
 
 	if errno := C.nmount(&rawOptions[0], C.uint(len(options)), C.int(flag)); errno != 0 {
-		reason := C.GoString(C.strerror(*C.__error()))
-		return fmt.Errorf("Failed to call nmount: %s", reason)
+		return &mountError{
+			op:     "mount",
+			source: device,
+			target: target,
+			flags:  flag,
+			err:    syscall.Errno(errno),
+		}
 	}
 	return nil
 }
-
-func unmount(target string, flag int) error {
-	return unix.Unmount(target, flag)
-}

components/engine/pkg/mount/mounter_linux.go

@@ -33,25 +33,42 @@ func mount(device, target, mType string, flags uintptr, data string) error {
 		// Initial call applying all non-propagation flags for mount
 		// or remount with changed data
 		if err := unix.Mount(device, target, mType, oflags, data); err != nil {
-			return err
+			return &mountError{
+				op:     "mount",
+				source: device,
+				target: target,
+				flags:  oflags,
+				data:   data,
+				err:    err,
+			}
 		}
 	}
 
 	if flags&ptypes != 0 {
 		// Change the propagation type.
 		if err := unix.Mount("", target, "", flags&pflags, ""); err != nil {
+			return &mountError{
+				op:     "remount",
+				target: target,
+				flags:  flags & pflags,
+				err:    err,
+			}
 			return err
 		}
 	}
 
 	if oflags&broflags == broflags {
 		// Remount the bind to apply read only.
-		return unix.Mount("", target, "", oflags|unix.MS_REMOUNT, "")
+		if err := unix.Mount("", target, "", oflags|unix.MS_REMOUNT, ""); err != nil {
+			return &mountError{
+				op:     "remount-ro",
+				target: target,
+				flags:  oflags | unix.MS_REMOUNT,
+				err:    err,
+			}
+
+		}
 	}
 
 	return nil
 }
-
-func unmount(target string, flag int) error {
-	return unix.Unmount(target, flag)
-}

components/engine/pkg/mount/sharedsubtree_linux_test.go

@@ -7,6 +7,7 @@ import (
 	"path"
 	"testing"
 
+	"github.com/pkg/errors"
 	"golang.org/x/sys/unix"
 )
 
@@ -326,7 +327,7 @@ func TestSubtreeUnbindable(t *testing.T) {
 	}()
 
 	// then attempt to mount it to target. It should fail
-	if err := Mount(sourceDir, targetDir, "none", "bind,rw"); err != nil && err != unix.EINVAL {
+	if err := Mount(sourceDir, targetDir, "none", "bind,rw"); err != nil && errors.Cause(err) != unix.EINVAL {
 		t.Fatal(err)
 	} else if err == nil {
 		t.Fatalf("%q should not have been bindable", sourceDir)

components/engine/pkg/mount/unmount_unix.go

@@ -0,0 +1,22 @@
+// +build !windows
+
+package mount // import "github.com/docker/docker/pkg/mount"
+
+import "golang.org/x/sys/unix"
+
+func unmount(target string, flags int) error {
+	err := unix.Unmount(target, flags)
+	if err == nil || err == unix.EINVAL {
+		// Ignore "not mounted" error here. Note the same error
+		// can be returned if flags are invalid, so this code
+		// assumes that the flags value is always correct.
+		return nil
+	}
+
+	return &mountError{
+		op:     "umount",
+		target: target,
+		flags:  uintptr(flags),
+		err:    err,
+	}
+}

components/engine/plugin/manager_linux.go

@@ -61,7 +61,7 @@ func (pm *Manager) enable(p *v2.Plugin, c *controller, force bool) error {
 	if err := pm.executor.Create(p.GetID(), *spec, stdout, stderr); err != nil {
 		if p.PluginObj.Config.PropagatedMount != "" {
 			if err := mount.Unmount(propRoot); err != nil {
-				logrus.Warnf("Could not unmount %s: %v", propRoot, err)
+				logrus.WithField("plugin", p.Name()).WithError(err).Warn("Failed to unmount vplugin propagated mount root")
 			}
 		}
 		return errors.WithStack(err)

components/engine/volume/local/local.go

@@ -344,7 +344,7 @@ func (v *localVolume) unmount() error {
 	if v.opts != nil {
 		if err := mount.Unmount(v.path); err != nil {
 			if mounted, mErr := mount.Mounted(v.path); mounted || mErr != nil {
-				return errdefs.System(errors.Wrapf(err, "error while unmounting volume path '%s'", v.path))
+				return errdefs.System(err)
 			}
 		}
 		v.active.mounted = false

components/engine/volume/local/local_unix.go

@@ -86,7 +86,7 @@ func (v *localVolume) mount() error {
 		}
 	}
 	err := mount.Mount(v.opts.MountDevice, v.path, v.opts.MountType, mountOpts)
-	return errors.Wrapf(err, "error while mounting volume with options: %s", v.opts)
+	return errors.Wrap(err, "failed to mount local volume")
 }
 
 func (v *localVolume) CreatedAt() (time.Time, error) {