Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mount: implement UnmountAll() #62

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
72 changes: 56 additions & 16 deletions mount/mount_unix.go
Expand Up @@ -5,7 +5,9 @@ package mount

import (
"fmt"
"path"
"sort"
"strings"

"github.com/moby/sys/mountinfo"
"golang.org/x/sys/unix"
Expand Down Expand Up @@ -38,35 +40,56 @@ func Unmount(target string) error {
}
}

// RecursiveUnmount unmounts the target and all mounts underneath, starting
// with the deepest mount first. The argument does not have to be a mount
// point itself.
func RecursiveUnmount(target string) error {
// Fast path, works if target is a mount point that can be unmounted.
// UnmountAll unmounts all mounts and submounts underneath parent,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// UnmountAll unmounts all mounts and submounts underneath parent,
// UnmountAll unmounts all mounts underneath parent, excluding the parent itself.
// Note that parent does not have to be a mount point.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I've removed "and submount" because from user's perspective all mounts are the same -- it is only in the code we distinguish between the top-level mounts and their sub-mounts).

func UnmountAll(parent string) error {
// Get all mounts in "parent"
mounts, err := mountinfo.GetMounts(mountinfo.PrefixFilter(parent))
if err != nil {
return err
}

// Fast path: try to unmount top-level mounts first. This works if target is
// a mount point that can be unmounted.
// On Linux, mntDetach flag ensures a recursive unmount. For other
// platforms, if there are submounts, we'll get EBUSY (and fall back
// to the slow path). NOTE we do not ignore EINVAL here as target might
// not be a mount point itself (but there can be mounts underneath).
if err := unix.Unmount(target, mntDetach); err == nil {
return nil
// to the slow path). We're not using RecursiveUnmount() here, to avoid
// repeatedly calling mountinfo.GetMounts()
Comment on lines +55 to +56
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this comment is no longer valid (as RecursiveUnmount itself is using this function).


var skipParents []string
for _, m := range mounts {
// Skip parent itself, and skip non-top-level mounts
if m.Mountpoint == parent || path.Dir(m.Mountpoint) != parent {
Comment on lines +60 to +61
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, I think this is not entirely correct optimization. Let me describe what I think would be a correct one.

We have a list of mounts, it's a tree, and we assume that if we use mntDetach flag we will only need to unmount the top ones. But the top ones are not those whose parent is parent, but those who have submounts.

For example, if we have the following directory tree (mounts are marked with [n]):

.
├── [1]
│   ├── [4]
│   └── [5]
├── 2
│   └── [6]
└── 3
    └── 7
        └── [8]

the top-level mounts are [1], [6], and [8], and we should try unmounting them first, hoping that [4] and [5] will be unmounted.

One way of doing that is to sort the tree so the shortest paths will be first, and upon successful unmount remove the children of the unmounted directory.

I am not that great at algorithms and not sure if it's possible to create the one described above, which will be faster that just iterating over a list and unmounting everything. Maybe it will be possible and even efficient if we use a real tree-like structure and do a width-first tree traversal, discarding the entire branch underneath if umount is successful.

My gut feeling though is that the "smart" approach will be slower than the "stupid" one.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One other things is, for special cases, when we know that the top mounts are right below the parent (which is what your code does in fast path), we can avoid reading mountinfo and just do something like

entries, err := os.Readdirnames(parent)
if err != nil {
   return err
}
for _, de := range entries {
   mp := filepath.Join(parent, de)
   err := unix.Unmount(mp, mntDetach)
   if err != nil && err != unix.EINVAL {
      return &os.PathError{Op: "UnmountAll", Path: mp, Err: err}
   }
}

after which we can read mountinfo and see if there's anything left.

Now, again, this optimization only makes sense if we assume that in most of the cases the mounts will be a direct descendants of the parent (which is also true for your code).

continue
}
if err := unix.Unmount(m.Mountpoint, mntDetach); err == nil {
skipParents = append(skipParents, m.Mountpoint)
}
}

// Slow path: get all submounts, sort, unmount one by one.
mounts, err := mountinfo.GetMounts(mountinfo.PrefixFilter(target))
if err != nil {
return err
// Remove all sub-mounts of paths that were successfully unmounted from the list
subMounts := mounts[:0]
for _, m := range mounts {
for _, p := range skipParents {
if m.Mountpoint == parent || m.Mountpoint == p {
// Skip parent itself, and mounts that already were unmounted
continue
}
if !strings.HasPrefix(m.Mountpoint, p) {
subMounts = append(subMounts, m)
}
}
}

// Make the deepest mount be first
sort.Slice(mounts, func(i, j int) bool {
return len(mounts[i].Mountpoint) > len(mounts[j].Mountpoint)
sort.Slice(subMounts, func(i, j int) bool {
Copy link
Collaborator

@kolyshkin kolyshkin Feb 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd add something like

if len(subMounts) == 0 {
     return nil
}

before here

return len(subMounts[i].Mountpoint) > len(subMounts[j].Mountpoint)
})

var (
suberr error
lastMount = len(mounts) - 1
)
for i, m := range mounts {
for i, m := range subMounts {
err = Unmount(m.Mountpoint)
if err != nil {
if i == lastMount {
Expand All @@ -86,3 +109,20 @@ func RecursiveUnmount(target string) error {
}
return nil
}

// RecursiveUnmount unmounts the target and all mounts underneath, starting
// with the deepest mount first. The argument does not have to be a mount
// point itself.
func RecursiveUnmount(target string) error {
// Fast path, works if target is a mount point that can be unmounted.
// On Linux, mntDetach flag ensures a recursive unmount. For other
// platforms, if there are submounts, we'll get EBUSY (and fall back
// to the slow path). NOTE we do not ignore EINVAL here as target might
// not be a mount point itself (but there can be mounts underneath).
if err := unix.Unmount(target, mntDetach); err == nil {
return nil
}

// Slow path: unmount all mounts inside target one by one.
return UnmountAll(target)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that you need to unmount the target itself here.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I am not mistaken above, this function now works incorrectly on its slow path, and thus our tests are not adequate.

Surely there needs to be a test added for UnmountAll.

}