From daeab40b453418a65715be064aa3fb7a487a259a Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Thu, 26 Apr 2018 13:06:34 -0700 Subject: [PATCH] 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 --- pkg/os/os.go | 30 ++++++++++++++++-------------- pkg/os/testing/fake_os.go | 8 ++++---- pkg/server/sandbox_run.go | 6 ++---- pkg/store/sandbox/netns.go | 3 +-- 4 files changed, 23 insertions(+), 24 deletions(-) diff --git a/pkg/os/os.go b/pkg/os/os.go index f0e7a3271..30252cbc2 100644 --- a/pkg/os/os.go +++ b/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 - LookupMount(path string) (containerdmount.Info, error) + Unmount(target string) 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 { - return Unmount(target, flags) +func (RealOS) Unmount(target string) error { + return Unmount(target) } // LookupMount gets mount info of a given path. -func (RealOS) LookupMount(path string) (containerdmount.Info, error) { - return containerdmount.Lookup(path) +func (RealOS) LookupMount(path string) (mount.Info, error) { + return mount.Lookup(path) } -// Unmount will call unix.Unmount to unmount the file. The function doesn't -// return error if target is not mounted. -func Unmount(target string, flags int) error { - if mounted, err := mount.Mounted(target); err != nil || !mounted { - return err +// Unmount unmounts the target. It does not return an error in case the target is not mounted. +// In case the target does not exist, the appropriate error is returned. +func Unmount(target string) error { + err := unix.Unmount(target, unix.MNT_DETACH) + if err == unix.EINVAL { + // ignore "not mounted" error + err = nil } - return unix.Unmount(target, flags) + + return err } diff --git a/pkg/os/testing/fake_os.go b/pkg/os/testing/fake_os.go index f33cbbd4f..aaa29fa0d 100644 --- a/pkg/os/testing/fake_os.go +++ b/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 { - f.appendCalls("Unmount", target, flags) +func (f *FakeOS) Unmount(target string) error { + 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 } diff --git a/pkg/server/sandbox_run.go b/pkg/server/sandbox_run.go index cfa085835..309f69ead 100644 --- a/pkg/server/sandbox_run.go +++ b/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: -// 1) The mount point is already unmounted. -// 2) The mount point doesn't exist. +// remove these files. Unmount should *NOT* return error if the mount point is already unmounted. 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) } } diff --git a/pkg/store/sandbox/netns.go b/pkg/store/sandbox/netns.go index 5d56d9222..a96eb6aad 100644 --- a/pkg/store/sandbox/netns.go +++ b/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 {