-
Notifications
You must be signed in to change notification settings - Fork 46
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,7 +5,9 @@ package mount | |
|
||
import ( | ||
"fmt" | ||
"path" | ||
"sort" | ||
"strings" | ||
|
||
"github.com/moby/sys/mountinfo" | ||
"golang.org/x/sys/unix" | ||
|
@@ -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, | ||
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 For example, if we have the following directory tree (mounts are marked with
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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 { | ||
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems that you need to unmount the target itself here. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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).