From dd2108766017c13a19bdbfd1a56cd1358580e0bb Mon Sep 17 00:00:00 2001 From: Brian Goff Date: Wed, 2 Aug 2017 21:29:43 -0400 Subject: [PATCH] Optimizations for recurrsive unmount When a recursive unmount fails, don't bother parsing the mount table to check if what we expected to be a mountpoint is still mounted. `EINVAL` is returned when you try to unmount something that is not a mountpoint, the other cases of `EINVAL` would not apply here unless everything is just wrong. Parsing the mount table over and over is relatively expensive, especially in the code path that it's in. Signed-off-by: Brian Goff --- pkg/mount/mount.go | 34 ++++++++++++++++++++++++---------- 1 file changed, 24 insertions(+), 10 deletions(-) diff --git a/pkg/mount/mount.go b/pkg/mount/mount.go index ee5833c49d..0d02a575f6 100644 --- a/pkg/mount/mount.go +++ b/pkg/mount/mount.go @@ -4,6 +4,8 @@ import ( "sort" "strings" + "syscall" + "github.com/sirupsen/logrus" ) @@ -77,18 +79,30 @@ func RecursiveUnmount(target string) error { continue } logrus.Debugf("Trying to unmount %s", m.Mountpoint) - err = Unmount(m.Mountpoint) - if err != nil && i == len(mounts)-1 { - if mounted, err := Mounted(m.Mountpoint); err != nil || mounted { - return err + 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 } - // Ignore errors for submounts and continue trying to unmount others - // The final unmount should fail if there ane any submounts remaining - } else if err != nil { - logrus.Errorf("Failed to unmount %s: %v", m.Mountpoint, err) - } else if err == nil { - logrus.Debugf("Unmounted %s", m.Mountpoint) + if i == len(mounts)-1 { + if mounted, e := Mounted(m.Mountpoint); e != nil || mounted { + return err + } + continue + } + // 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) } return nil }