From 6f78462c1a513ee1471e53aebf5b117df5fa7220 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Wed, 24 Oct 2018 17:29:03 -0700 Subject: [PATCH 01/12] UnmountIpcMount: simplify As standard mount.Unmount does what we need, let's use it. In addition, this adds ignoring "not mounted" condition, which was previously implemented (see PR#33329, commit cfa2591d3f26) via a very expensive call to mount.Mounted(). Signed-off-by: Kir Kolyshkin (cherry picked from commit 77bc327e24a60791fe7e87980faf704cf7273cf9) Upstream-commit: 893b24b80db170279d5c9532ed508a81c328de5e Component: engine --- components/engine/container/container_unix.go | 10 ++++------ components/engine/container/container_windows.go | 2 +- components/engine/daemon/container_operations_unix.go | 4 ---- .../engine/daemon/container_operations_windows.go | 4 ---- components/engine/daemon/start.go | 2 +- 5 files changed, 6 insertions(+), 16 deletions(-) diff --git a/components/engine/container/container_unix.go b/components/engine/container/container_unix.go index 1a184555ee..7ee0a7e7da 100644 --- a/components/engine/container/container_unix.go +++ b/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 errors.Wrapf(err, "umount %s", shmPath) } return nil } diff --git a/components/engine/container/container_windows.go b/components/engine/container/container_windows.go index b5bdb5bc34..090db12c20 100644 --- a/components/engine/container/container_windows.go +++ b/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 } diff --git a/components/engine/daemon/container_operations_unix.go b/components/engine/daemon/container_operations_unix.go index c0aab72342..03b01b8061 100644 --- a/components/engine/daemon/container_operations_unix.go +++ b/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()] diff --git a/components/engine/daemon/container_operations_windows.go b/components/engine/daemon/container_operations_windows.go index 349d3a1566..10bfd53d6e 100644 --- a/components/engine/daemon/container_operations_windows.go +++ b/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 diff --git a/components/engine/daemon/start.go b/components/engine/daemon/start.go index e2265a4fae..e2416efe5e 100644 --- a/components/engine/daemon/start.go +++ b/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) } From 51371cb25220f530414d6c2b5aca4d34e35da7ef Mon Sep 17 00:00:00 2001 From: Omri Shiv Date: Fri, 30 Nov 2018 12:58:10 -0800 Subject: [PATCH 02/12] fix typo Signed-off-by: Omri Shiv (cherry picked from commit fe1083d4622658e9e5bf7319d0f94873019fced2) Upstream-commit: b941f081523cce7defafd4457f380442e10fa345 Component: engine --- components/engine/container/container_unix.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/components/engine/container/container_unix.go b/components/engine/container/container_unix.go index 7ee0a7e7da..592a6dc375 100644 --- a/components/engine/container/container_unix.go +++ b/components/engine/container/container_unix.go @@ -381,7 +381,7 @@ 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.Warnf("%s unmountVolumes: Failed to do lazy umount for volume '%s': %v", container.ID, mountPath, err) } } return container.UnmountVolumes(volumeEventLog) From e213748382c523e9c6a42dd7f4a2d968629a9d82 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Thu, 25 Oct 2018 10:44:28 -0700 Subject: [PATCH 03/12] pkg/mount: refactor Unmount() It has been pointed out that we're ignoring EINVAL from umount(2) everywhere, so let's move it to a lower-level function. Also, its implementation should be the same for any UNIX incarnation, so let's consolidate it. Signed-off-by: Kir Kolyshkin (cherry picked from commit 90be078fe59a8cfeff2bcc5dc2f34a00309837b6) Upstream-commit: 47c51447e1b6dacf92b40574f6f929958ca9d621 Component: engine --- components/engine/pkg/mount/mount.go | 27 ++++--------------- .../engine/pkg/mount/mounter_freebsd.go | 6 ----- components/engine/pkg/mount/mounter_linux.go | 4 --- components/engine/pkg/mount/unmount_unix.go | 17 ++++++++++++ 4 files changed, 22 insertions(+), 32 deletions(-) create mode 100644 components/engine/pkg/mount/unmount_unix.go diff --git a/components/engine/pkg/mount/mount.go b/components/engine/pkg/mount/mount.go index 874aff6545..2d79bc390d 100644 --- a/components/engine/pkg/mount/mount.go +++ b/components/engine/pkg/mount/mount.go @@ -3,7 +3,6 @@ package mount // import "github.com/docker/docker/pkg/mount" import ( "sort" "strings" - "syscall" "github.com/sirupsen/logrus" ) @@ -89,12 +88,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 +108,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) diff --git a/components/engine/pkg/mount/mounter_freebsd.go b/components/engine/pkg/mount/mounter_freebsd.go index b6ab83a230..8a46eaf454 100644 --- a/components/engine/pkg/mount/mounter_freebsd.go +++ b/components/engine/pkg/mount/mounter_freebsd.go @@ -14,8 +14,6 @@ import ( "fmt" "strings" "unsafe" - - "golang.org/x/sys/unix" ) func allocateIOVecs(options []string) []C.struct_iovec { @@ -54,7 +52,3 @@ func mount(device, target, mType string, flag uintptr, data string) error { } return nil } - -func unmount(target string, flag int) error { - return unix.Unmount(target, flag) -} diff --git a/components/engine/pkg/mount/mounter_linux.go b/components/engine/pkg/mount/mounter_linux.go index 631daf10a5..2c189de0ae 100644 --- a/components/engine/pkg/mount/mounter_linux.go +++ b/components/engine/pkg/mount/mounter_linux.go @@ -51,7 +51,3 @@ func mount(device, target, mType string, flags uintptr, data string) error { return nil } - -func unmount(target string, flag int) error { - return unix.Unmount(target, flag) -} diff --git a/components/engine/pkg/mount/unmount_unix.go b/components/engine/pkg/mount/unmount_unix.go new file mode 100644 index 0000000000..d027e3086d --- /dev/null +++ b/components/engine/pkg/mount/unmount_unix.go @@ -0,0 +1,17 @@ +// +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 == 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. + err = nil + } + + return err +} From c81448bb56c0083877ec45fffe2fc4d196bbde3a Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Mon, 22 Oct 2018 18:30:34 -0700 Subject: [PATCH 04/12] pkg/mount: wrap mount/umount errors The errors returned from Mount and Unmount functions are raw syscall.Errno errors (like EPERM or EINVAL), which provides no context about what has happened and why. Similar to os.PathError type, introduce mount.Error type with some context. The error messages will now look like this: > mount /tmp/mount-tests/source:/tmp/mount-tests/target, flags: 0x1001: operation not permitted or > mount tmpfs:/tmp/mount-test-source-516297835: operation not permitted Before this patch, it was just > operation not permitted [v2: add Cause()] [v3: rename MountError to Error, document Cause()] [v4: fixes; audited all users] [v5: make Error type private; changes after @cpuguy83 reviews] Signed-off-by: Kir Kolyshkin (cherry picked from commit 65331369617e89ce54cc9be080dba70f3a883d1c) Signed-off-by: Kir Kolyshkin Upstream-commit: 7f1c6bf5a745c8faeba695d3556dff4c4ff5f473 Component: engine --- components/engine/container/container_unix.go | 5 +-- .../engine/daemon/graphdriver/btrfs/btrfs.go | 2 +- .../daemon/graphdriver/devmapper/deviceset.go | 4 +-- .../daemon/graphdriver/devmapper/driver.go | 7 +--- .../engine/daemon/graphdriver/zfs/zfs.go | 3 +- components/engine/pkg/mount/mount.go | 35 +++++++++++++++++++ .../engine/pkg/mount/mounter_freebsd.go | 11 ++++-- components/engine/pkg/mount/mounter_linux.go | 25 +++++++++++-- .../pkg/mount/sharedsubtree_linux_test.go | 3 +- components/engine/pkg/mount/unmount_unix.go | 11 ++++-- components/engine/plugin/manager_linux.go | 2 +- components/engine/volume/local/local.go | 2 +- components/engine/volume/local/local_unix.go | 2 +- 13 files changed, 88 insertions(+), 24 deletions(-) diff --git a/components/engine/container/container_unix.go b/components/engine/container/container_unix.go index 592a6dc375..6d402be3a0 100644 --- a/components/engine/container/container_unix.go +++ b/components/engine/container/container_unix.go @@ -191,7 +191,7 @@ func (container *Container) UnmountIpcMount() error { return nil } if err = mount.Unmount(shmPath); err != nil && !os.IsNotExist(err) { - return errors.Wrapf(err, "umount %s", shmPath) + return err } return nil } @@ -381,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 for volume '%s': %v", container.ID, mountPath, err) + logrus.WithError(err).WithField("container", container.ID). + Warn("Unable to unmount") } } return container.UnmountVolumes(volumeEventLog) diff --git a/components/engine/daemon/graphdriver/btrfs/btrfs.go b/components/engine/daemon/graphdriver/btrfs/btrfs.go index 7ce7edef36..fcaedc6eab 100644 --- a/components/engine/daemon/graphdriver/btrfs/btrfs.go +++ b/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 diff --git a/components/engine/daemon/graphdriver/devmapper/deviceset.go b/components/engine/daemon/graphdriver/devmapper/deviceset.go index 5dc01d71d9..c41b50c119 100644 --- a/components/engine/daemon/graphdriver/devmapper/deviceset.go +++ b/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 != "" { diff --git a/components/engine/daemon/graphdriver/devmapper/driver.go b/components/engine/daemon/graphdriver/devmapper/driver.go index 899b1f8670..a42a03ba90 100644 --- a/components/engine/daemon/graphdriver/devmapper/driver.go +++ b/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 diff --git a/components/engine/daemon/graphdriver/zfs/zfs.go b/components/engine/daemon/graphdriver/zfs/zfs.go index 8a798778d2..c83446cf8f 100644 --- a/components/engine/daemon/graphdriver/zfs/zfs.go +++ b/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 diff --git a/components/engine/pkg/mount/mount.go b/components/engine/pkg/mount/mount.go index 2d79bc390d..4afd63c427 100644 --- a/components/engine/pkg/mount/mount.go +++ b/components/engine/pkg/mount/mount.go @@ -2,11 +2,46 @@ package mount // import "github.com/docker/docker/pkg/mount" import ( "sort" + "strconv" "strings" "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 diff --git a/components/engine/pkg/mount/mounter_freebsd.go b/components/engine/pkg/mount/mounter_freebsd.go index 8a46eaf454..09ad360608 100644 --- a/components/engine/pkg/mount/mounter_freebsd.go +++ b/components/engine/pkg/mount/mounter_freebsd.go @@ -11,8 +11,8 @@ package mount // import "github.com/docker/docker/pkg/mount" import "C" import ( - "fmt" "strings" + "syscall" "unsafe" ) @@ -47,8 +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 } diff --git a/components/engine/pkg/mount/mounter_linux.go b/components/engine/pkg/mount/mounter_linux.go index 2c189de0ae..48837adde5 100644 --- a/components/engine/pkg/mount/mounter_linux.go +++ b/components/engine/pkg/mount/mounter_linux.go @@ -33,20 +33,41 @@ 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 diff --git a/components/engine/pkg/mount/sharedsubtree_linux_test.go b/components/engine/pkg/mount/sharedsubtree_linux_test.go index 019514491f..7a37f66098 100644 --- a/components/engine/pkg/mount/sharedsubtree_linux_test.go +++ b/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) diff --git a/components/engine/pkg/mount/unmount_unix.go b/components/engine/pkg/mount/unmount_unix.go index d027e3086d..4be4276851 100644 --- a/components/engine/pkg/mount/unmount_unix.go +++ b/components/engine/pkg/mount/unmount_unix.go @@ -6,12 +6,17 @@ import "golang.org/x/sys/unix" func unmount(target string, flags int) error { err := unix.Unmount(target, flags) - if err == unix.EINVAL { + 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. - err = nil + return nil } - return err + return &mountError{ + op: "umount", + target: target, + flags: uintptr(flags), + err: err, + } } diff --git a/components/engine/plugin/manager_linux.go b/components/engine/plugin/manager_linux.go index df1fe5b73d..86ada8d02f 100644 --- a/components/engine/plugin/manager_linux.go +++ b/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) diff --git a/components/engine/volume/local/local.go b/components/engine/volume/local/local.go index 7190de9ed6..d3119cb2ff 100644 --- a/components/engine/volume/local/local.go +++ b/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 diff --git a/components/engine/volume/local/local_unix.go b/components/engine/volume/local/local_unix.go index b1c68b931b..5ee2ed894b 100644 --- a/components/engine/volume/local/local_unix.go +++ b/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) { From e03614a1565da2853447e0e954eb3203c4b6d337 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Wed, 24 Oct 2018 18:45:27 -0700 Subject: [PATCH 05/12] aufs: get rid of mount() The function is not needed as it's just a shallow wrapper around unix.Mount(). Signed-off-by: Kir Kolyshkin (cherry picked from commit 2f98b5f51fb0a21e1bd930c0e660c4a7c4918aa5) Upstream-commit: 023b63a0f276b79277c1c961d8c98d4d5b39525d Component: engine --- components/engine/daemon/graphdriver/aufs/aufs.go | 10 +++++----- .../engine/daemon/graphdriver/aufs/mount_linux.go | 7 ------- .../daemon/graphdriver/aufs/mount_unsupported.go | 12 ------------ 3 files changed, 5 insertions(+), 24 deletions(-) delete mode 100644 components/engine/daemon/graphdriver/aufs/mount_linux.go delete mode 100644 components/engine/daemon/graphdriver/aufs/mount_unsupported.go diff --git a/components/engine/daemon/graphdriver/aufs/aufs.go b/components/engine/daemon/graphdriver/aufs/aufs.go index 114aa9a615..11ca939904 100644 --- a/components/engine/daemon/graphdriver/aufs/aufs.go +++ b/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" @@ -598,7 +598,7 @@ func (a *Driver) Cleanup() error { logger.Debugf("error unmounting %s: %s", m, err) } } - return mountpk.RecursiveUnmount(a.root) + return mount.RecursiveUnmount(a.root) } func (a *Driver) aufsMount(ro []string, rw, target, mountLabel string) (err error) { @@ -632,14 +632,14 @@ 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 { + if err = unix.Mount("none", target, "aufs", 0, data); err != nil { 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 { + if err = unix.Mount("none", target, "aufs", unix.MS_REMOUNT, data); err != nil { return } } @@ -666,7 +666,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 diff --git a/components/engine/daemon/graphdriver/aufs/mount_linux.go b/components/engine/daemon/graphdriver/aufs/mount_linux.go deleted file mode 100644 index 8d5ad8f32d..0000000000 --- a/components/engine/daemon/graphdriver/aufs/mount_linux.go +++ /dev/null @@ -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) -} diff --git a/components/engine/daemon/graphdriver/aufs/mount_unsupported.go b/components/engine/daemon/graphdriver/aufs/mount_unsupported.go deleted file mode 100644 index cf7f58c29e..0000000000 --- a/components/engine/daemon/graphdriver/aufs/mount_unsupported.go +++ /dev/null @@ -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") -} From a829a04518b9898fbfb655fd0f443e50fc7b4ec1 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Wed, 17 Apr 2019 19:29:32 -0700 Subject: [PATCH 06/12] aufs: remove extra locking Both mount and unmount calls are already protected by fine-grained (per id) locks in Get()/Put() introduced in commit fc1cf1911bb ("Add more locking to storage drivers"), so there's no point in having a global lock in mount/unmount. The only place from which unmount is called without any locking is Cleanup() -- this is to be addressed in the next patch. This reverts commit 824c24e6802ad3ed7e26b4f16e5ae81869b98185. Signed-off-by: Kir Kolyshkin (cherry picked from commit f93750b2c4d5f6144f0790ffa89291da3c097b80) Upstream-commit: 701939112efc71a0c867a626f0a56df9c56030db Component: engine --- components/engine/daemon/graphdriver/aufs/aufs.go | 7 ------- 1 file changed, 7 deletions(-) diff --git a/components/engine/daemon/graphdriver/aufs/aufs.go b/components/engine/daemon/graphdriver/aufs/aufs.go index 11ca939904..6c929fff35 100644 --- a/components/engine/daemon/graphdriver/aufs/aufs.go +++ b/components/engine/daemon/graphdriver/aufs/aufs.go @@ -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 @@ -547,9 +546,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 +560,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 } From 4fdc82012a1a9de2557737e5757b357c59187fe2 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Wed, 17 Apr 2019 19:52:36 -0700 Subject: [PATCH 07/12] aufs: use mount.Unmount 1. Use mount.Unmount() which ignores EINVAL ("not mounted") error, and provides better error diagnostics (so we don't have to explicitly add target to error messages). 2. Since we're ignoring "not mounted" error, we can call multiple unmounts without any locking -- but since "auplink flush" is still involved and can produce an error in logs, let's keep the check for fs being mounted (it's just a statfs so should be fast). 2. While at it, improve the "can't unmount" error message in Put(). Signed-off-by: Kir Kolyshkin (cherry picked from commit 4beee98026feabe4f4f0468215b8fd9b56f90d5e) Upstream-commit: 5b68d00abc4cad8b707a9a01dde9a420ca6eb995 Component: engine --- components/engine/daemon/graphdriver/aufs/aufs.go | 8 ++++---- components/engine/daemon/graphdriver/aufs/mount.go | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/components/engine/daemon/graphdriver/aufs/aufs.go b/components/engine/daemon/graphdriver/aufs/aufs.go index 6c929fff35..38df774225 100644 --- a/components/engine/daemon/graphdriver/aufs/aufs.go +++ b/components/engine/daemon/graphdriver/aufs/aufs.go @@ -326,11 +326,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++ @@ -436,7 +436,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 } diff --git a/components/engine/daemon/graphdriver/aufs/mount.go b/components/engine/daemon/graphdriver/aufs/mount.go index 9f5510380c..4e85cf235d 100644 --- a/components/engine/daemon/graphdriver/aufs/mount.go +++ b/components/engine/daemon/graphdriver/aufs/mount.go @@ -5,7 +5,7 @@ package aufs // import "github.com/docker/docker/daemon/graphdriver/aufs" import ( "os/exec" - "golang.org/x/sys/unix" + "github.com/docker/docker/pkg/mount" ) // Unmount the target specified. @@ -13,5 +13,5 @@ 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) } - return unix.Unmount(target, 0) + return mount.Unmount(target) } From 5db6e4ad9bff09b428483f785423c62b27a9666c Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Thu, 18 Apr 2019 15:55:55 -0700 Subject: [PATCH 08/12] aufs: aufsMount: better errors for unix.Mount() Signed-off-by: Kir Kolyshkin (cherry picked from commit 5873768dbe3be2733874b8cf68cb492817f81a94) Upstream-commit: 4ab3020e8df18b5c12c668df6ef3e21b648ce04b Component: engine --- components/engine/daemon/graphdriver/aufs/aufs.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/components/engine/daemon/graphdriver/aufs/aufs.go b/components/engine/daemon/graphdriver/aufs/aufs.go index 38df774225..5a95ef42e0 100644 --- a/components/engine/daemon/graphdriver/aufs/aufs.go +++ b/components/engine/daemon/graphdriver/aufs/aufs.go @@ -626,6 +626,7 @@ func (a *Driver) aufsMount(ro []string, rw, target, mountLabel string) (err erro } data := label.FormatMountLabel(fmt.Sprintf("%s,%s", string(b[:bp]), opts), mountLabel) if err = unix.Mount("none", target, "aufs", 0, data); err != nil { + err = errors.Wrap(err, "mount target="+target+" data="+data) return } @@ -633,6 +634,7 @@ func (a *Driver) aufsMount(ro []string, rw, target, mountLabel string) (err erro layer := fmt.Sprintf(":%s=ro+wh", ro[index]) data := label.FormatMountLabel(fmt.Sprintf("append%s", layer), mountLabel) if err = unix.Mount("none", target, "aufs", unix.MS_REMOUNT, data); err != nil { + err = errors.Wrap(err, "mount target="+target+" flags=MS_REMOUNT data="+data) return } } From 5ada897229ec476206c06b1ef976d9b39d550260 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Mon, 22 Apr 2019 18:34:24 +0000 Subject: [PATCH 09/12] aufs: add lock around mount Apparently there is some kind of race in aufs kernel module code, which leads to the errors like: [98221.158606] aufs au_xino_create2:186:dockerd[25801]: aufs.xino create err -17 [98221.162128] aufs au_xino_set:1229:dockerd[25801]: I/O Error, failed creating xino(-17). [98362.239085] aufs au_xino_create2:186:dockerd[6348]: aufs.xino create err -17 [98362.243860] aufs au_xino_set:1229:dockerd[6348]: I/O Error, failed creating xino(-17). [98373.775380] aufs au_xino_create:767:dockerd[27435]: open /dev/shm/aufs.xino(-17) [98389.015640] aufs au_xino_create2:186:dockerd[26753]: aufs.xino create err -17 [98389.018776] aufs au_xino_set:1229:dockerd[26753]: I/O Error, failed creating xino(-17). [98424.117584] aufs au_xino_create:767:dockerd[27105]: open /dev/shm/aufs.xino(-17) So, we have to have a lock around mount syscall. While at it, don't call the whole Unmount() on an error path, as it leads to bogus error from auplink flush. Signed-off-by: Kir Kolyshkin (cherry picked from commit 5cd62852fa199f272b542d828c8c5d1db427ea53) Upstream-commit: bb4b9fe29eba57ed71d8105ec0e5dacc8e72129e Component: engine --- components/engine/daemon/graphdriver/aufs/aufs.go | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/components/engine/daemon/graphdriver/aufs/aufs.go b/components/engine/daemon/graphdriver/aufs/aufs.go index 5a95ef42e0..fd2cf58063 100644 --- a/components/engine/daemon/graphdriver/aufs/aufs.go +++ b/components/engine/daemon/graphdriver/aufs/aufs.go @@ -80,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. @@ -597,7 +598,7 @@ func (a *Driver) Cleanup() error { func (a *Driver) aufsMount(ro []string, rw, target, mountLabel string) (err error) { defer func() { if err != nil { - Unmount(target) + mount.Unmount(target) } }() @@ -625,7 +626,10 @@ 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 = unix.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 } @@ -633,7 +637,10 @@ func (a *Driver) aufsMount(ro []string, rw, target, mountLabel string) (err erro for ; index < len(ro); index++ { layer := fmt.Sprintf(":%s=ro+wh", ro[index]) data := label.FormatMountLabel(fmt.Sprintf("append%s", layer), mountLabel) - if err = unix.Mount("none", target, "aufs", unix.MS_REMOUNT, data); err != nil { + 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 } From 3b8f3b67f29aa2b3f697dbba8a9f038eebf6d7f7 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 23 Apr 2019 02:18:17 +0000 Subject: [PATCH 10/12] aufs: optimize lots of layers case In case there are a big number of layers, so that mount data won't fit into a single memory page (4096 bytes on most platforms, which is good enough for about 40 layers, depending on how long graphdriver root path is), we supply additional layers with O_REMOUNT, as described in aufs documentation. Problem is, the current implementation does that one layer at a time (i.e. there is one mount syscall per each additional layer). Optimize the code to supply as many layers as we can fit in one page (basically reusing the same code as for the original mount). Note, per aufs docs, "[a]t remount-time, the options are interpreted in the given order, e.g. left to right" so we should be good. Tested on an image with ~100 layers. Before (35 syscalls): > [pid 22756] 1556919088.686955 mount("none", "/mnt/volume_sfo2_09/docker-aufs/aufs/mnt/a86f8c9dd0ec2486293119c20b0ec026e19bbc4d51332c554f7cf05d777c9866", "aufs", 0, "br:/mnt/volume_sfo2_09/docker-au"...) = 0 <0.000504> > [pid 22756] 1556919088.687643 mount("none", "/mnt/volume_sfo2_09/docker-aufs/aufs/mnt/a86f8c9dd0ec2486293119c20b0ec026e19bbc4d51332c554f7cf05d777c9866", 0xc000c451b0, MS_REMOUNT, "append:/mnt/volume_sfo2_09/docke"...) = 0 <0.000105> > [pid 22756] 1556919088.687851 mount("none", "/mnt/volume_sfo2_09/docker-aufs/aufs/mnt/a86f8c9dd0ec2486293119c20b0ec026e19bbc4d51332c554f7cf05d777c9866", 0xc000c451ba, MS_REMOUNT, "append:/mnt/volume_sfo2_09/docke"...) = 0 <0.000098> > ..... (~30 lines skipped for clarity) > [pid 22756] 1556919088.696182 mount("none", "/mnt/volume_sfo2_09/docker-aufs/aufs/mnt/a86f8c9dd0ec2486293119c20b0ec026e19bbc4d51332c554f7cf05d777c9866", 0xc000c45310, MS_REMOUNT, "append:/mnt/volume_sfo2_09/docke"...) = 0 <0.000266> After (2 syscalls): > [pid 24352] 1556919361.799889 mount("none", "/mnt/volume_sfo2_09/docker-aufs/aufs/mnt/8e7ba189e347a834e99eea4ed568f95b86cec809c227516afdc7c70286ff9a20", "aufs", 0, "br:/mnt/volume_sfo2_09/docker-au"...) = 0 <0.001717> > [pid 24352] 1556919361.801761 mount("none", "/mnt/volume_sfo2_09/docker-aufs/aufs/mnt/8e7ba189e347a834e99eea4ed568f95b86cec809c227516afdc7c70286ff9a20", 0xc000dbecb0, MS_REMOUNT, "append:/mnt/volume_sfo2_09/docke"...) = 0 <0.001358> Signed-off-by: Kir Kolyshkin (cherry picked from commit d58c434bffef76e48bff75ede290937874488dd3) Upstream-commit: 75488521735325e4564e66d9cf9c2b1bc1c2c64b Component: engine --- components/engine/daemon/graphdriver/aufs/aufs.go | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/components/engine/daemon/graphdriver/aufs/aufs.go b/components/engine/daemon/graphdriver/aufs/aufs.go index fd2cf58063..9a0a6207f0 100644 --- a/components/engine/daemon/graphdriver/aufs/aufs.go +++ b/components/engine/daemon/graphdriver/aufs/aufs.go @@ -634,9 +634,16 @@ func (a *Driver) aufsMount(ro []string, rw, target, mountLabel string) (err erro return } - for ; index < len(ro); index++ { - layer := fmt.Sprintf(":%s=ro+wh", ro[index]) - data := label.FormatMountLabel(fmt.Sprintf("append%s", layer), mountLabel) + 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() From c70089ebb32fa6c6755f151dd54498c9f0ac5dc0 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Wed, 17 Apr 2019 19:56:48 -0700 Subject: [PATCH 11/12] aufs.Cleanup: optimize Do not use filepath.Walk() as there's no requirement to recursively go into every directory under mnt -- a (non-recursive) 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 both excessive and useless. This is similar to commit f1a459229724f5e8e440b49f ("devmapper.shutdown: optimize") While at it, raise the priority of "unmount error" message from debug to a warning. Note we don't have to explicitly add `m` as unmount error (from pkg/mount) will have it. Signed-off-by: Kir Kolyshkin (cherry picked from commit 8fda12c6078ed5c86be0822a7a980c6fbc9220bf) Upstream-commit: b85d4a4f09badf40201ef7fcbd5b930de216190d Component: engine --- .../engine/daemon/graphdriver/aufs/aufs.go | 25 ++++++++----------- 1 file changed, 11 insertions(+), 14 deletions(-) diff --git a/components/engine/daemon/graphdriver/aufs/aufs.go b/components/engine/daemon/graphdriver/aufs/aufs.go index 9a0a6207f0..bbd19a82b0 100644 --- a/components/engine/daemon/graphdriver/aufs/aufs.go +++ b/components/engine/daemon/graphdriver/aufs/aufs.go @@ -573,23 +573,20 @@ 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 mount.RecursiveUnmount(a.root) From 00419438192534eb6f5aa8ae458a225a536c46e4 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Fri, 10 May 2019 17:19:49 -0700 Subject: [PATCH 12/12] aufs: retry auplink flush Running a bundled aufs benchmark sometimes results in this warning: > WARN[0001] Couldn't run auplink before unmount /tmp/aufs-tests/aufs/mnt/XXXXX error="exit status 22" storage-driver=aufs If we take a look at what aulink utility produces on stderr, we'll see: > auplink:proc_mnt.c:96: /tmp/aufs-tests/aufs/mnt/XXXXX: Invalid argument and auplink exits with exit code of 22 (EINVAL). Looking into auplink source code, what happens is it tries to find a record in /proc/self/mounts corresponding to the mount point (by using setmntent()/getmntent_r() glibc functions), and it fails. Some manual testing, as well as runtime testing with lots of printf added on mount/unmount, as well as calls to check the superblock fs magic on mount point (as in graphdriver.Mounted(graphdriver.FsMagicAufs, target) confirmed that this record is in fact there, but sometimes auplink can't find it. I was also able to reproduce the same error (inability to find a mount in /proc/self/mounts that should definitely be there) using a small C program, mocking what `auplink` does: ```c #include #include #include #include #include int main(int argc, char **argv) { FILE *fp; struct mntent m, *p; char a[4096]; char buf[4096 + 1024]; int found =0, lines = 0; if (argc != 2) { fprintf(stderr, "Usage: %s \n", argv[0]); exit(1); } fp = setmntent("/proc/self/mounts", "r"); if (!fp) { err(1, "setmntent"); } setvbuf(fp, a, _IOLBF, sizeof(a)); while ((p = getmntent_r(fp, &m, buf, sizeof(buf)))) { lines++; if (!strcmp(p->mnt_dir, argv[1])) { found++; } } printf("found %d entries for %s (%d lines seen)\n", found, argv[1], lines); return !found; } ``` I have also wrote a few other C proggies -- one that reads /proc/self/mounts directly, one that reads /proc/self/mountinfo instead. They are also prone to the same occasional error. It is not perfectly clear why this happens, but so far my best theory is when a lot of mounts/unmounts happen in parallel with reading contents of /proc/self/mounts, sometimes the kernel fails to provide continuity (i.e. it skips some part of file or mixes it up in some other way). In other words, this is a kernel bug (which is probably hard to fix unless some other interface to get a mount entry is added). Now, there is no real fix, and a workaround I was able to come up with is to retry when we got EINVAL. It usually works on the second attempt, although I've once seen it took two attempts to go through. Signed-off-by: Kir Kolyshkin (cherry picked from commit ae431b10a9508e2bf3b1782e9d435855e3e13977) Upstream-commit: c303e63ca6c3d25d29b8992451898734fa6e4e7c Component: engine --- .../engine/daemon/graphdriver/aufs/mount.go | 28 +++++++++++++++++-- 1 file changed, 26 insertions(+), 2 deletions(-) diff --git a/components/engine/daemon/graphdriver/aufs/mount.go b/components/engine/daemon/graphdriver/aufs/mount.go index 4e85cf235d..fc20a5eca6 100644 --- a/components/engine/daemon/graphdriver/aufs/mount.go +++ b/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" "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 mount.Unmount(target) }