devmapper cleanup improvements #36307
Conversation
There was a problem hiding this comment.
right; patch updated
There was a problem hiding this comment.
The only errors returned are from os.Open() (other than ENOENT) or Readdir(), meaning it failed to scan through the entries in the directory (and unmount and deactivate those). Given that this is a shutdown procedure, called when the daemon is exiting, I think in this situation it makes sense to proceed with the rest of the code in Shutdown() (i.e. deactivate the base device and the pool).
There was a problem hiding this comment.
seems this function shouldn't return an error then if desired behavior is to always ignore them.
There was a problem hiding this comment.
Makes total sense, I overlooked it. Please see the updated commit
There was a problem hiding this comment.
errors.Wrapf(umountErr, "error unmounting %s", d.home)
There was a problem hiding this comment.
how is that better?
There was a problem hiding this comment.
errors are supposed to be lowercase for future wrapping in the caller
There was a problem hiding this comment.
I thought you were asking to change using Wrap and string concatenation to Wrapf and %s format, and overlooked the lowercase part.
Patch updated
7f1a2b2 to
9c110d8
Compare
|
SGTM |
There was a problem hiding this comment.
My only problem with this is, if there are many containers we will be making lots of potentially unneeded system calls.
I think it's still better to check the mount table to see if it's a mountpoint before attempting an unmount.
There was a problem hiding this comment.
Reading the /proc/self/mountinfo also involves system calls, at least three: open(), read(), close(). More importantly, it also involves the kernel going through internal structures (possibly with some locking) to get various bits of information about all the mounts, which is definitely a costly operation -- especially in the case of many containers, so the mountinfo will have many entries.
Having said that, we only perform the above thing once and when we repeat a relatively quick (albeit linear) search for the string in a relatively short array. It might be faster that way, but since we now remove the mount point upon unmounting, the chance that a given directory is a mount point is close to 100%.
There was a problem hiding this comment.
Oh good point, there shouldn't be many dirs here.
|
I ran some (very non-scientific) benchmarks on my laptop, here are the results:
Notes:
Observations:
Conclusions:
Testing code: package main
import (
"bufio"
"os"
"sort"
"strings"
"testing"
"golang.org/x/sys/unix"
)
func BenchmarkUmount(b *testing.B) {
for i := 0; i < b.N; i++ {
_ = unix.Unmount("nonexistent/path", unix.MNT_DETACH)
}
}
func BenchmarkUmountCheckLinear(b *testing.B) {
const mnt = "nonexistent/path"
mounts, err := parseMountinfo()
if err != nil {
b.Fatal(err)
}
for i := 0; i < b.N; i++ {
for _, m := range mounts {
if m == mnt {
unix.Unmount(mnt, unix.MNT_DETACH)
break
}
}
}
}
func BenchmarkUmountCheckOptimized(b *testing.B) {
const mnt = "nonexistent/path"
mounts, err := parseMountinfo()
if err != nil {
b.Fatal(err)
}
sort.Strings(mounts)
for i := 0; i < b.N; i++ {
pos := sort.SearchStrings(mounts, mnt)
if pos < len(mounts) && mounts[pos] == mnt {
unix.Unmount(mnt, unix.MNT_DETACH)
}
}
}
func parseMountinfo() (mounts []string, err error) {
f, err := os.Open("/proc/self/mountinfo")
if err != nil {
return mounts, err
}
s := bufio.NewScanner(f)
for s.Scan() {
if err := s.Err(); err != nil {
return mounts, err
}
text := s.Text()
fields := strings.SplitN(text, " ", 6)
mounts = append(mounts, fields[4])
}
f.Close()
return mounts, nil
}
func BenchmarkReadMountinfo(b *testing.B) {
for i := 0; i < b.N; i++ {
_, err := parseMountinfo()
if err != nil {
b.Fatal(err)
}
}
} |
Codecov Report
@@ Coverage Diff @@
## master #36307 +/- ##
=========================================
Coverage ? 34.64%
=========================================
Files ? 613
Lines ? 45387
Branches ? 0
=========================================
Hits ? 15723
Misses ? 27601
Partials ? 2063 |
|
@kolyshkin looks like this needs a rebase @tonistiigi LGTY? |
1. Make sure it's clear the error is from unmount. 2. Simplify the code a bit to make it more readable. [v2: use errors.Wrap] [v3: use errors.Wrapf] [v4: lowercase the error message] Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Move the "unmount and deactivate" code into a separate method, and optimize it a bit: 1. Do not use filepath.Walk() as there's no requirement to recursively go into every directory under home/mnt; a list of directories in mnt is sufficient. With filepath.Walk(), in case some container will fail to unmount, it'll go through the whole container filesystem which is excessive and useless. 2. Do not use GetMounts() and check if a directory is mounted; just unmount it and ignore "not mounted" error. Note the same error is returned in case of wrong flags set, but as flags are hardcoded we can safely ignore such case. While at it, promote "can't unmount" log level from debug to warning. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
|
@tonistiigi PTAL |
devmapper.shutdown: optimize
Move the "unmount and deactivate" code into a separate method, and optimize it a bit:
Do not use
filepath.Walk()as there's no requirement to recursively go into every directory under home/mnt; a list of directories in mnt is sufficient. Withfilepath.Walk(), in case some container failed to unmount, it'll go through the whole container filesystem which is excessive and useless.Do not use
GetMounts()and check if a directory is mounted; just unmount it and ignore "not mounted" error. Note the same error is returned in case of wrong flags set, but as flags are hardcoded we can safely ignore such cases.While at it, promote "can't unmount" message log level from debug to warning.
devmapper cleanup: improve error msg
Make sure it's clear the error is from unmount.
Simplify the code a bit to make it more readable.