os.Unmount: do not consult mountinfo, drop flags

1. Currently, Unmount() call takes a burden to parse the whole nine yards
of /proc/self/mountinfo to figure out whether the given mount point is
mounted or not (and returns an error in case parsing fails somehow).

Instead, let's just call umount() and ignore EINVAL, which results
in the same behavior, but much better performance.

This also introduces a slight change: in case target does not exist,
the appropriate error (ENOENT) is returned -- document that.

2. As Unmount() is always used with MNT_DETACH flag, let's drop the
flags argument. This way, the only reason of EINVAL returned from
umount(2) can only be "target is not mounted".

3. While at it, remove the 'containerdmount' alias from the package.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

pkg/os/os.go

@@ -22,9 +22,8 @@ import (
 	"os"
 	"path/filepath"
 
-	containerdmount "github.com/containerd/containerd/mount"
+	"github.com/containerd/containerd/mount"
 	"github.com/containerd/fifo"
-	"github.com/docker/docker/pkg/mount"
 	"github.com/docker/docker/pkg/symlink"
 	"golang.org/x/net/context"
 	"golang.org/x/sys/unix"
@@ -42,8 +41,8 @@ type OS interface {
 	CopyFile(src, dest string, perm os.FileMode) error
 	WriteFile(filename string, data []byte, perm os.FileMode) error
 	Mount(source string, target string, fstype string, flags uintptr, data string) error
-	Unmount(target string, flags int) error
+	Unmount(target string) error
-	LookupMount(path string) (containerdmount.Info, error)
+	LookupMount(path string) (mount.Info, error)
 }
 
 // RealOS is used to dispatch the real system level operations.
@@ -115,20 +114,23 @@ func (RealOS) Mount(source string, target string, fstype string, flags uintptr,
 }
 
 // Unmount will call Unmount to unmount the file.
-func (RealOS) Unmount(target string, flags int) error {
+func (RealOS) Unmount(target string) error {
-	return Unmount(target, flags)
+	return Unmount(target)
 }
 
 // LookupMount gets mount info of a given path.
-func (RealOS) LookupMount(path string) (containerdmount.Info, error) {
+func (RealOS) LookupMount(path string) (mount.Info, error) {
-	return containerdmount.Lookup(path)
+	return mount.Lookup(path)
 }
 
-// Unmount will call unix.Unmount to unmount the file. The function doesn't
+// Unmount unmounts the target. It does not return an error in case the target is not mounted.
-// return error if target is not mounted.
+// In case the target does not exist, the appropriate error is returned.
-func Unmount(target string, flags int) error {
+func Unmount(target string) error {
-	if mounted, err := mount.Mounted(target); err != nil || !mounted {
+	err := unix.Unmount(target, unix.MNT_DETACH)
-		return err
+	if err == unix.EINVAL {
+		// ignore "not mounted" error
+		err = nil
 	}
-	return unix.Unmount(target, flags)
+
+	return err
 }

pkg/os/testing/fake_os.go

@@ -49,7 +49,7 @@ type FakeOS struct {
 	CopyFileFn             func(string, string, os.FileMode) error
 	WriteFileFn            func(string, []byte, os.FileMode) error
 	MountFn                func(source string, target string, fstype string, flags uintptr, data string) error
-	UnmountFn              func(target string, flags int) error
+	UnmountFn              func(target string) error
 	LookupMountFn          func(path string) (containerdmount.Info, error)
 	calls                  []CalledDetail
 	errors                 map[string]error
@@ -230,14 +230,14 @@ func (f *FakeOS) Mount(source string, target string, fstype string, flags uintpt
 }
 
 // Unmount is a fake call that invokes UnmountFn or just return nil.
-func (f *FakeOS) Unmount(target string, flags int) error {
+func (f *FakeOS) Unmount(target string) error {
-	f.appendCalls("Unmount", target, flags)
+	f.appendCalls("Unmount", target)
 	if err := f.getError("Unmount"); err != nil {
 		return err
 	}
 
 	if f.UnmountFn != nil {
-		return f.UnmountFn(target, flags)
+		return f.UnmountFn(target)
 	}
 	return nil
 }

pkg/server/sandbox_run.go

@@ -494,16 +494,14 @@ func parseDNSOptions(servers, searches, options []string) (string, error) {
 }
 
 // unmountSandboxFiles unmount some sandbox files, we rely on the removal of sandbox root directory to
-// remove these files. Unmount should *NOT* return error when:
+// remove these files. Unmount should *NOT* return error if the mount point is already unmounted.
-//  1) The mount point is already unmounted.
-//  2) The mount point doesn't exist.
 func (c *criService) unmountSandboxFiles(id string, config *runtime.PodSandboxConfig) error {
 	if config.GetLinux().GetSecurityContext().GetNamespaceOptions().GetIpc() != runtime.NamespaceMode_NODE {
 		path, err := c.os.FollowSymlinkInScope(c.getSandboxDevShm(id), "/")
 		if err != nil {
 			return errors.Wrap(err, "failed to follow symlink")
 		}
-		if err := c.os.Unmount(path, unix.MNT_DETACH); err != nil && !os.IsNotExist(err) {
+		if err := c.os.Unmount(path); err != nil && !os.IsNotExist(err) {
 			return errors.Wrapf(err, "failed to unmount %q", path)
 		}
 	}

pkg/store/sandbox/netns.go

@@ -23,7 +23,6 @@ import (
 	cnins "github.com/containernetworking/plugins/pkg/ns"
 	"github.com/docker/docker/pkg/symlink"
 	"github.com/pkg/errors"
-	"golang.org/x/sys/unix"
 
 	osinterface "github.com/containerd/cri/pkg/os"
 )
@@ -93,7 +92,7 @@ func (n *NetNS) Remove() error {
 		if err != nil {
 			return errors.Wrap(err, "failed to follow symlink")
 		}
-		if err := osinterface.Unmount(path, unix.MNT_DETACH); err != nil && !os.IsNotExist(err) {
+		if err := osinterface.Unmount(path); err != nil && !os.IsNotExist(err) {
 			return errors.Wrap(err, "failed to umount netns")
 		}
 		if err := os.RemoveAll(path); err != nil {