devmapper cleanup improvements by kolyshkin · Pull Request #36307 · moby/moby

kolyshkin · 2018-02-14T00:27:22Z

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. With filepath.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.

tonistiigi

cc @rhvgoyal

tonistiigi · 2018-02-14T18:18:54Z

daemon/graphdriver/devmapper/driver.go

nit: errors.Wrapf

right; patch updated

tonistiigi · 2018-02-14T18:22:15Z

daemon/graphdriver/devmapper/deviceset.go

why is this error ignored?

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).

seems this function shouldn't return an error then if desired behavior is to always ignore them.

Makes total sense, I overlooked it. Please see the updated commit

tonistiigi · 2018-02-14T19:01:15Z

daemon/graphdriver/devmapper/driver.go

errors.Wrapf(umountErr, "error unmounting %s", d.home)

how is that better?

errors are supposed to be lowercase for future wrapping in the caller

I thought you were asking to change using Wrap and string concatenation to Wrapf and %s format, and overlooked the lowercase part.

Patch updated

tonistiigi · 2018-02-16T18:49:53Z

SGTM

cpuguy83 · 2018-02-16T19:06:36Z

daemon/graphdriver/devmapper/deviceset.go

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.

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%.

Oh good point, there shouldn't be many dirs here.

kolyshkin · 2018-02-19T22:30:25Z

I ran some (very non-scientific) benchmarks on my laptop, here are the results:

`wc -l /proc/self/mountinfo`	`unix.Unmount()`	parse mountinfo	linear search in mountinfo list	quick search in mountinfo list
35	153	38855	43	35
76	153	150407	93	43
148	153	256400	167	47

Notes:

The times in columns 2...5 are in nanoseconds.
For column 3, a very simple/fast parser is used (see the code below).
For the last two columns, only the time to search within a list is measured, i.e. preparing the list is not included.
About 30 dummy containers had to be started to achieve number 150 in the first column.

Observations:

Parsing /proc/self/mountinfo takes 10³ to 10⁴ times more than calling unix.Umount(). Note I am using an extremely trivial parser implementation; the one that is used currently is much slower.
The code we currently use (linear search in mountinfo table) becomes slower than calling unix.Unmount() when the size of /proc/self/mountinfo grows to about 150 entries. This is not taking into account the time to parse /proc/self/mountinfo.
Calling umount() appears to be O(1) or close to it.
Reading /proc/self/mountinfo and linear search in the list is O(n).

Conclusions:

It only makes sense to parse /proc/self/mountinfo if it would help to avoid calling umount more than 10³... 10⁴ times.
Calling umount straight ahead is the only approach that scales well.

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 · 2018-02-19T23:58:49Z

Codecov Report

❗ No coverage uploaded for pull request base (master@8830ef8). Click here to learn what that means.
The diff coverage is 33.33%.

@@            Coverage Diff            @@
##             master   #36307   +/-   ##
=========================================
  Coverage          ?   34.64%           
=========================================
  Files             ?      613           
  Lines             ?    45387           
  Branches          ?        0           
=========================================
  Hits              ?    15723           
  Misses            ?    27601           
  Partials          ?     2063

cpuguy83

LGTM

thaJeztah · 2018-03-05T12:09:58Z

@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>

kolyshkin · 2018-03-07T20:30:54Z

@tonistiigi PTAL

GordonTheTurtle added the status/0-triage label Feb 14, 2018

kolyshkin force-pushed the dm-misc branch from a0ad421 to e28d854 Compare February 14, 2018 00:29

vdemeester added status/2-code-review and removed status/0-triage labels Feb 14, 2018

vdemeester requested review from cpuguy83 and tonistiigi February 14, 2018 07:24

tonistiigi reviewed Feb 14, 2018

View reviewed changes

kolyshkin force-pushed the dm-misc branch from e28d854 to dd1f6ca Compare February 14, 2018 18:57

tonistiigi reviewed Feb 14, 2018

View reviewed changes

kolyshkin force-pushed the dm-misc branch 2 times, most recently from 7f1a2b2 to 9c110d8 Compare February 14, 2018 23:39

thaJeztah added the area/storage/devicemapper label Feb 15, 2018

cpuguy83 reviewed Feb 16, 2018

View reviewed changes

kolyshkin force-pushed the dm-misc branch from 9c110d8 to 2742208 Compare February 17, 2018 03:40

cpuguy83 approved these changes Feb 20, 2018

View reviewed changes

kolyshkin mentioned this pull request Feb 22, 2018

pkg/mount improvements #36091

Merged

GordonTheTurtle assigned runcom Mar 1, 2018

kolyshkin added 2 commits March 5, 2018 10:08

devmapper cleanup: improve error msg

9d00aed

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>

kolyshkin force-pushed the dm-misc branch from 2742208 to f1a4592 Compare March 5, 2018 18:10

kolyshkin mentioned this pull request Mar 7, 2018

Use os.ReadDir instead of filepath.Walk in netns cleanup #36511

Open

tonistiigi approved these changes Mar 7, 2018

View reviewed changes

tonistiigi merged commit bc7424b into moby:master Mar 7, 2018

kolyshkin mentioned this pull request Sep 15, 2020

Devmapper fixes and cleanups containers/storage#719

Merged

Conversation

kolyshkin commented Feb 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

devmapper.shutdown: optimize

devmapper cleanup: improve error msg

Uh oh!

tonistiigi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tonistiigi commented Feb 16, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kolyshkin commented Feb 19, 2018

Uh oh!

codecov bot commented Feb 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

cpuguy83 left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Mar 5, 2018

Uh oh!

kolyshkin commented Mar 7, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

kolyshkin commented Feb 14, 2018 •

edited

Loading

codecov bot commented Feb 19, 2018 •

edited

Loading