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

Avoid fork on mount for overlay2 in common case #25824

Merged
merged 1 commit into from
Aug 22, 2016

Conversation

dmcgowan
Copy link
Member

@dmcgowan dmcgowan commented Aug 18, 2016

In the common case where the user is using /var/lib/docker and
an image with less than 60 layers, forking is not needed. Calculate
whether absolute paths can be used and avoid forking to mount in
those cases.

I do not have any statistics of what percentage of images
are less than 60 layers deep, but my guess is a vast majority and
even larger majority when accounting for images which are run most often.

Mount Benchmarks

Overlay2 (pre-change) vs Overlay2 (updated)

benchmark                         old ns/op     new ns/op     delta
BenchmarkGetSingleBaseMount-8     7744409       231444        -97.01%
BenchmarkGet20BaseMount-8         8324308       307961        -96.30%
BenchmarkGet50BaseMount-8         8482378       420499        -95.04%
BenchmarkGet100BaseMount-8        8252170       7976995       -3.33%

Overlay vs Overlay2 (updated)

benchmark                         old ns/op     new ns/op     delta
BenchmarkGetSingleBaseMount-8     339539        231444        -31.84%
BenchmarkGet20BaseMount-8         249545        307961        +23.41%
BenchmarkGet50BaseMount-8         200676        420499        +109.54%
BenchmarkGet100BaseMount-8        252296        7976995       +3061.76%

AUFS vs Overlay2 (updated)

benchmark                         old ns/op     new ns/op     delta
BenchmarkGetSingleBaseMount-8     305157        231444        -24.16%
BenchmarkGet20BaseMount-8         1830297       307961        -83.17%
BenchmarkGet50BaseMount-8         10263958      420499        -95.90%
BenchmarkGet100BaseMount-8        36276899      7976995       -78.01%

AUFS vs Overlay2 (pre-change)

benchmark                         old ns/op     new ns/op     delta
BenchmarkGetSingleBaseMount-8     305157        7744409       +2437.84%
BenchmarkGet20BaseMount-8         1830297       8324308       +354.81%
BenchmarkGet50BaseMount-8         10263958      8482378       -17.36%
BenchmarkGet100BaseMount-8        36276899      8252170       -77.25%

Note overlay will always mount faster than overlay2 for a larger number of lower directories since it can only have one lower directory (which it achieves at the expense of duplicating files). These are the first benchmarks I have done with aufs vs overlay2 so I included the benchmark comparison with the pre-changed overlay2.

Benchmarks done with https://github.com/dmcgowan/dsdbench

// name and path separator must be accounted for. The absolute
// directories for work and diff are also accounted for along with a
// buffer for other mount labels.
if (len(d.home)+idLength+len(linkDir)+2)*len(splitLowers)+len(dir)*2+512 < pageSize {
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to make this calculation more precise? I think even calculating the whole opts even if it is not used would not be noticeably slow(good thing you have benchmarks to prove me wrong). If not then please add a comment that 512 is a buffer for the contants and label, I was initially confused by that.

Copy link
Member Author

Choose a reason for hiding this comment

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

I had considered just calculating the opts using absolute paths optimistically, I think this might make even more sense considering I found the threshold to be over 60 layers.

@dmcgowan dmcgowan force-pushed the overlay2-make-faster branch from c9bb1cb to d4016a3 Compare August 18, 2016 18:53
@dmcgowan
Copy link
Member Author

Updated to optimistically create the mount data with absolute paths and only use relative paths when the mount data is larger than a page. Tested to make sure that the extra buffer is not needed when the mount data is fully computed (see dmcgowan/dsdbench@071a526#diff-c16a9df1a4a40f7564a79598949a5199R136).

mountTarget = path.Join(id, "merged")
}

if len(mountData) > pageSize {
Copy link
Member

Choose a reason for hiding this comment

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

I think it will be much more readable if this branch is moved to L430. i.e.,

mountData = label.FormatMountLabel(opts, mountLabel)
if len(mountData) > pageSize {
...
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Agree, updated

In the common case where the user is using /var/lib/docker and
an image with less than 60 layers, forking is not needed. Calculate
whether absolute paths can be used and avoid forking to mount in
those cases.

Signed-off-by: Derek McGowan <derek@mcgstyle.net> (github: dmcgowan)
@dmcgowan dmcgowan force-pushed the overlay2-make-faster branch from d4016a3 to c13a985 Compare August 22, 2016 18:43
@tonistiigi
Copy link
Member

LGTM

1 similar comment
@LK4D4
Copy link
Contributor

LK4D4 commented Aug 22, 2016

LGTM

@LK4D4 LK4D4 merged commit 2e1a594 into moby:master Aug 22, 2016
@thaJeztah thaJeztah added this to the 1.13.0 milestone Sep 18, 2016
liusdu pushed a commit to liusdu/moby that referenced this pull request Oct 30, 2017
In the common case where the user is using /var/lib/docker and
an image with less than 60 layers, forking is not needed. Calculate
whether absolute paths can be used and avoid forking to mount in
those cases.

cherry-pick from: moby#25824

Signed-off-by: Derek McGowan <derek@mcgstyle.net> (github: dmcgowan)
Signed-off-by: Lei Jitang <leijitang@huawei.com>
(cherry picked from commit c13a985)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants