Optimizations for recursive unmount#34379
Conversation
adc2d8d to
9bfb553
Compare
|
there's a recursive |
| import ( | ||
| "sort" | ||
| "strings" | ||
|
|
pkg/mount/mount.go
Outdated
| // 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. | ||
| if err := Unmount(m.Mountpoint); err != nil && err != syscall.EINVAL && i == len(mounts)-1 { |
There was a problem hiding this comment.
Perhaps break this up a bit, e.g.;
if err := Unmount(m.Mountpoint); (err == nil || err == syscall.EINVAL) {
continue
}
if i == len(mounts)-1 {
if mounted, err := Mounted(m.Mountpoint); err != nil || mounted {
return err
}
}The i == len(mounts)-1 may need a comment / explanation perhaps
|
ping @cpuguy83 😇 |
9bfb553 to
08d4dcd
Compare
|
Updated |
pkg/mount/mount.go
Outdated
There was a problem hiding this comment.
We're returning the original error here, correct?
There was a problem hiding this comment.
Alright; wasn't sure if we wanted the error returned by Mounted() as well
There was a problem hiding this comment.
Not really because the Mounted call is just a sanity check and the original error is the real interest. We also can't wrap two errors but the low-level system error is what we really want to propagate.
pkg/mount/mount.go
Outdated
There was a problem hiding this comment.
In view of 069fdc8 and similar commits, shouldn't we use sys/unix instead of syscall here and below?
There was a problem hiding this comment.
The main reason I chose syscall is because this is called from both Windows and Linux, both of which have EINVAL defined in syscall, but unix. is not compiled on Windows (and vice-versa).
We ultimately have the go1 guarantee on this and being it's not functionality which could be buggy but rather an error value I think we are fine here.
pkg/mount/mount.go
Outdated
There was a problem hiding this comment.
Is this a check for the top (i.e. final) mount? It seems it's wrong, as we are iterating through all mounts, not just the one provided by a user and its submounts.
In other words, I believe the correct check here has to be m.Mountpoint == target.
There was a problem hiding this comment.
Well, the path passed in may not even be a mount. The reason it's the final mount is the list is sorted in reverse-lexicographic order.
pkg/mount/mount.go
Outdated
There was a problem hiding this comment.
Perhaps it make sense to raise level to warning (as this message will be a good hint about why the final umount failed), as well as saying this is a submount. Something like
logrus.WithError(err).Warnf("Failed ti umount submount %s", m.Mountpoint)
pkg/mount/mount.go
Outdated
There was a problem hiding this comment.
In here, Unmount() calls Mounted(), which is expensive, and making optimization of removing another Mounted() on an error path kinda moot.
Perhaps unmount() should be used here instead, as it does not call Mounted().
|
ping @cpuguy83 |
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 <cpuguy83@gmail.com>
08d4dcd to
dd21087
Compare
|
Updated |
|
Not directly related to this PR, but I wonder why mount.Unmount() checks the mount table? Would be easier just to call umount() and ignore EINVAL. I traced the code back to original commit adding this flow: @crosbymichael being the original author of the above, do you think we can drop the check? |
|
Ping |
|
LGTM |
1 similar comment
|
LGTM |
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.
EINVALisreturned when you try to unmount something that is not a mountpoint, the
other cases of
EINVALwould not apply here unless everything is justwrong. Parsing the mount table over and over is relatively expensive,
especially in the code path that it's in.