Lessen mount lock contention in layer store by kolyshkin · Pull Request #39135 · moby/moby

kolyshkin · 2019-04-24T17:56:55Z

While testing massive parallel container creation and removal, we see there's a lock contention in layer store that affects container creation performance (in other words, many threads are waiting for the lock). With this patches, container creation time (wall clock time) is about 2x to 3x less than before (measured with both aufs and overlay2 graph drivers).

The (global, per-layerstore) lock in question is held for the duration of

CreateRWLayer()
GetRWLayer()
ReleaseRWLayer()
CreateRWLayerByGraphID()

The lock contention observed mostly between two instances of CreateRWLayer()

Here's a stupid script I am using to test the performance before and after:

#!/bin/bash

NUMCTS=1000
PARALLEL=100

create() {
	# -v 'vol_{}:/vol'
	time ( seq $NUMCTS | parallel -j$PARALLEL docker create alpine top > /dev/null )
	echo "^^^ CREATE TIME ^^^"
}

remove() {
	time ( docker ps -qa | shuf | tail -n $NUMCTS | parallel -j$PARALLEL docker rm -f '{}' > /dev/null )
	echo "^^^ REMOVE TIME ^^^"
}

# precreate $NUMCTS containers
create
echo

# create another $NUMCTS while removing $NUMCTS containers
create &
remove &
wait

# remove the rest of it
remove

Results are:

Container creation time (the first timing print by the script) is about 40s before, about 20s after this patchset.
Concurrent container creation/removal time (the second and the third timing printed by the script) are about 70s before, 40s after this patchset.
Container removal times (last timing printed by the script) stay the same at about 30s.

GordonTheTurtle · 2019-04-24T17:56:58Z

Please sign your commits following these rules:
https://github.com/moby/moby/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "layer-mount-lock" [email protected]:kolyshkin/moby.git somewhere
$ cd somewhere
$ git rebase -i HEAD~842358683792
editor opens
change each 'pick' to 'edit'
save the file and quit
$ git commit --amend -s --no-edit
$ git rebase --continue # and repeat the amend for each commit
$ git push -f

Amending updates the existing PR. You DO NOT need to open a new one.

kolyshkin · 2019-04-24T18:02:38Z

My concern is locking after this patches might not be adequate; perhaps we need to add a per-id (rather than global) lock (using pkg/locker) on top of it (but I have yet to look into this).

kolyshkin · 2019-04-24T18:32:29Z

perhaps we need to add a per-id (rather than global) lock (using pkg/locker) on top of it

added (not sure if it's needed though)

kolyshkin · 2019-04-24T18:34:41Z

@dmcgowan PTAL

dmcgowan · 2019-04-24T18:57:03Z

A finer grain lock is needed in this case. The mounted layer operates on a reference map so there does require locking to prevent alterations on that object in parallel. It maybe possible to just add that lock to *mountedLayer, assuming the global lock has already synchronized creation of the same names.

kolyshkin · 2019-05-02T20:26:36Z

A finer grain lock is needed in this case. The mounted layer operates on a reference map so there does require locking to prevent alterations on that object in parallel. It maybe possible to just add that lock to *mountedLayer, assuming the global lock has already synchronized creation of the same names.

@xinfengliu if you have time to address the above comments? I'm currently away for DockerCon and don't have time :(

xinfengliu · 2019-05-03T02:33:35Z

@kolyshkin , yes mountedLayer need a lock. Sorry I'm on China public holidays (May 1~3) now , I will spend some time on it but I'm not sure I can make it by the end of this week.

kolyshkin · 2019-05-03T22:36:30Z

@xinfengliu no problem, I think I fixed it.

@dmcgowan PTAL at the last commit (I decided to not use RWMutex for the sake of simplicity). Do we need same for roLayer.references map?

Add a mutex to protect concurrent access to mountedLayer.references map. Signed-off-by: Kir Kolyshkin <[email protected]>

dmcgowan · 2019-05-06T18:47:59Z

assuming the global lock has already synchronized creation of the same names.

I don't see this part being done. Possibly setting a placeholder in the map and clear it if there is an error. In that case a second call for the same name would get the conflicting error. I agree locking for the entire save is unnecessary just to prevent simultaneous creation of the same name.

codecov · 2019-05-07T02:10:42Z

Codecov Report

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

@@            Coverage Diff            @@
##             master   #39135   +/-   ##
=========================================
  Coverage          ?   37.02%           
=========================================
  Files             ?      612           
  Lines             ?    45448           
  Branches          ?        0           
=========================================
  Hits              ?    16829           
  Misses            ?    26333           
  Partials          ?     2286

kolyshkin · 2019-05-07T02:26:26Z

assuming the global lock has already synchronized creation of the same names.

I don't see this part being done. Possibly setting a placeholder in the map and clear it if there is an error. In that case a second call for the same name would get the conflicting error. I agree locking for the entire save is unnecessary just to prevent simultaneous creation of the same name.

@dmcgowan thanks for review! Sorry I missed that part entirely; I think I know what you mean now, please see the updated commit. I had to introduce the third state of ls.mount[name] when it's not empty but the value is nil, meaning the name being created. All the other places treat nil as non-existent, except for CreateRWLayer() which treats it as a name conflict. I'm not in love with this approach as we can have potential nil dereferences but it looks cleaner than yet another lock or map (such as map of names being created).

Oh, if you can think of any test cases that can be created, I'm happy to implement those.

layer/layer_store.go

kolyshkin · 2019-05-09T17:47:01Z

Some changes based on @dmcgowan and my own review:

added locking to loadMount() (just for consistency/uniformity, techinically it's not required in there);
added missing Unlock() to an error path in CreateRWLayer();
simplified return statements in CreateRWLayer as we use named return values;
simplified map element presence check by dropping ok assignment and check (if m, ok := l.mounts[name]; ok && m != nil --> if m := l.mounts[name]; m != nil).

Goroutine stack analisys shown some lock contention while doing massively (100 instances of `docker rm`) parallel image removal, with many goroutines waiting for the mountL mutex. Optimize it. With this commit, the above operation is about 3x faster, with no noticeable change to container creation times (tested on aufs and overlay2). kolyshkin@: - squashed commits - added description - protected CreateRWLayer against name collisions by temporary assiging nil to ls.mounts[name], and treating nil as "non-existent" in all the other functions. Signed-off-by: Kir Kolyshkin <[email protected]>

dmcgowan

LGTM

kolyshkin · 2019-05-13T18:36:10Z

A comment from @tonistiigi :

have some concerns about #39135. eg.in https://github.com/moby/moby/pull/39135/files#diff-5137c87d28c11ce0691d2f12e124409eR587 deleteRef, hasRef + remove is not atomic anymore. so if you call GetRWLayer after hasReference you get a rwlayer but the storage gets deleted in another goroutine

is to be addressed in a followup (update: #39209)

Fixes CVE-2018-15664: API endpoints behind the 'docker cp' command are vulnerable to a symlink-exchange attack with Directory Traversal, giving attackers arbitrary read-write access to the host filesystem with root privileges, because daemon/archive.go does not do archive operations on a frozen filesystem (or from within a chroot). And includes additional post-18.09.6 fixes: Builder - Fixed a panic error when building dockerfiles that contain only comments. moby/moby#38487 - Added a workaround for GCR authentication issue. moby/moby#38246 - Builder-next: Fixed a bug in the GCR token cache implementation workaround. moby/moby#39183 Runtime - Added performance optimizations in aufs and layer store that helps in massively parallel container creation and removal. moby/moby#39107, moby/moby#39135 - daemon: fixed a mirrors validation issue. moby/moby#38991 - Docker no longer supports sorting UID and GID ranges in ID maps. moby/moby#39288 Logging - Added a fix that now allows large log lines for logger plugins. moby/moby#39038 Signed-off-by: Peter Korsgaard <[email protected]> Signed-off-by: Arnout Vandecappelle (Essensium/Mind) <[email protected]>

Fixes CVE-2018-15664: API endpoints behind the 'docker cp' command are vulnerable to a symlink-exchange attack with Directory Traversal, giving attackers arbitrary read-write access to the host filesystem with root privileges, because daemon/archive.go does not do archive operations on a frozen filesystem (or from within a chroot). And includes additional post-18.09.6 fixes: Builder - Fixed a panic error when building dockerfiles that contain only comments. moby/moby#38487 - Added a workaround for GCR authentication issue. moby/moby#38246 - Builder-next: Fixed a bug in the GCR token cache implementation workaround. moby/moby#39183 Runtime - Added performance optimizations in aufs and layer store that helps in massively parallel container creation and removal. moby/moby#39107, moby/moby#39135 - daemon: fixed a mirrors validation issue. moby/moby#38991 - Docker no longer supports sorting UID and GID ranges in ID maps. moby/moby#39288 Logging - Added a fix that now allows large log lines for logger plugins. moby/moby#39038 Signed-off-by: Peter Korsgaard <[email protected]> Signed-off-by: Arnout Vandecappelle (Essensium/Mind) <[email protected]> (cherry picked from commit 13cf6f0) Signed-off-by: Peter Korsgaard <[email protected]>

Fixes CVE-2018-15664: API endpoints behind the 'docker cp' command are vulnerable to a symlink-exchange attack with Directory Traversal, giving attackers arbitrary read-write access to the host filesystem with root privileges, because daemon/archive.go does not do archive operations on a frozen filesystem (or from within a chroot). And includes additional post-18.09.6 fixes: Builder - Fixed a panic error when building dockerfiles that contain only comments. moby/moby#38487 - Added a workaround for GCR authentication issue. moby/moby#38246 - Builder-next: Fixed a bug in the GCR token cache implementation workaround. moby/moby#39183 Runtime - Added performance optimizations in aufs and layer store that helps in massively parallel container creation and removal. moby/moby#39107, moby/moby#39135 - daemon: fixed a mirrors validation issue. moby/moby#38991 - Docker no longer supports sorting UID and GID ranges in ID maps. moby/moby#39288 Logging - Added a fix that now allows large log lines for logger plugins. moby/moby#39038 Signed-off-by: Peter Korsgaard <[email protected]> Signed-off-by: Arnout Vandecappelle (Essensium/Mind) <[email protected]> (cherry picked from commit cdbb3ce) Signed-off-by: Peter Korsgaard <[email protected]>

Fixes CVE-2018-15664: API endpoints behind the 'docker cp' command are vulnerable to a symlink-exchange attack with Directory Traversal, giving attackers arbitrary read-write access to the host filesystem with root privileges, because daemon/archive.go does not do archive operations on a frozen filesystem (or from within a chroot). And includes additional post-18.09.6 fixes: Builder - Fixed a panic error when building dockerfiles that contain only comments. moby/moby#38487 - Added a workaround for GCR authentication issue. moby/moby#38246 - Builder-next: Fixed a bug in the GCR token cache implementation workaround. moby/moby#39183 Runtime - Added performance optimizations in aufs and layer store that helps in massively parallel container creation and removal. moby/moby#39107, moby/moby#39135 - daemon: fixed a mirrors validation issue. moby/moby#38991 - Docker no longer supports sorting UID and GID ranges in ID maps. moby/moby#39288 Logging - Added a fix that now allows large log lines for logger plugins. moby/moby#39038 Signed-off-by: Peter Korsgaard <[email protected]> Signed-off-by: Arnout Vandecappelle (Essensium/Mind) <[email protected]> (cherry picked from commit 13cf6f0) Signed-off-by: Peter Korsgaard <[email protected]>

Fixes CVE-2018-15664: API endpoints behind the 'docker cp' command are vulnerable to a symlink-exchange attack with Directory Traversal, giving attackers arbitrary read-write access to the host filesystem with root privileges, because daemon/archive.go does not do archive operations on a frozen filesystem (or from within a chroot). And includes additional post-18.09.6 fixes: Builder - Fixed a panic error when building dockerfiles that contain only comments. moby/moby#38487 - Added a workaround for GCR authentication issue. moby/moby#38246 - Builder-next: Fixed a bug in the GCR token cache implementation workaround. moby/moby#39183 Runtime - Added performance optimizations in aufs and layer store that helps in massively parallel container creation and removal. moby/moby#39107, moby/moby#39135 - daemon: fixed a mirrors validation issue. moby/moby#38991 - Docker no longer supports sorting UID and GID ranges in ID maps. moby/moby#39288 Logging - Added a fix that now allows large log lines for logger plugins. moby/moby#39038 Signed-off-by: Peter Korsgaard <[email protected]> Signed-off-by: Arnout Vandecappelle (Essensium/Mind) <[email protected]> (cherry picked from commit cdbb3ce) Signed-off-by: Peter Korsgaard <[email protected]>

GordonTheTurtle added dco/no Automatically set by a bot when one of the commits lacks proper signature status/0-triage labels Apr 24, 2019

kolyshkin changed the title ~~Lessen mount lock contention in layer store~~ [WIP] Lessen mount lock contention in layer store Apr 24, 2019

kolyshkin force-pushed the layer-mount-lock branch from 1a695ce to 1aba6e5 Compare May 3, 2019 17:03

kolyshkin mentioned this pull request May 3, 2019

aufs optimizations #39107

Merged

kolyshkin requested a review from tianon as a code owner May 4, 2019 00:31

kolyshkin force-pushed the layer-mount-lock branch from e07fb1f to 1aba6e5 Compare May 4, 2019 00:32

layer: protect mountedLayer.references

f73b5cb

Add a mutex to protect concurrent access to mountedLayer.references map. Signed-off-by: Kir Kolyshkin <[email protected]>

kolyshkin force-pushed the layer-mount-lock branch from 1aba6e5 to fb499c6 Compare May 6, 2019 17:56

GordonTheTurtle removed the dco/no Automatically set by a bot when one of the commits lacks proper signature label May 6, 2019

kolyshkin force-pushed the layer-mount-lock branch from fb499c6 to d4c5ddf Compare May 6, 2019 17:59

kolyshkin force-pushed the layer-mount-lock branch from d4c5ddf to 3a0eaeb Compare May 7, 2019 02:10

kolyshkin force-pushed the layer-mount-lock branch from 3a0eaeb to f52aa5c Compare May 7, 2019 02:11

GordonTheTurtle assigned yongtang May 9, 2019

kolyshkin commented May 9, 2019

View reviewed changes

layer/layer_store.go Outdated Show resolved Hide resolved

kolyshkin force-pushed the layer-mount-lock branch from f52aa5c to f0c95a2 Compare May 9, 2019 17:44

kolyshkin force-pushed the layer-mount-lock branch from f0c95a2 to 05250a4 Compare May 9, 2019 18:05

dmcgowan approved these changes May 9, 2019

View reviewed changes

kolyshkin changed the title ~~[WIP] Lessen mount lock contention in layer store~~ Lessen mount lock contention in layer store May 9, 2019

kolyshkin merged commit a4d6993 into moby:master May 9, 2019

thaJeztah added area/daemon Core Engine area/storage Image Storage area/storage/aufs area/storage/overlay and removed area/daemon Core Engine labels May 13, 2019

kolyshkin mentioned this pull request May 13, 2019

layer: fix same rw layer races #39209

Merged

thaJeztah added the process/cherry-pick label May 25, 2019

This was referenced May 25, 2019

[19.03 backport ENGCORE-831] backport layer store optimizations docker-archive/engine#247

Merged

[18.09 backport ENGCORE-830] layer store optimizations docker-archive/engine#248

Merged

thaJeztah added process/cherry-picked status/4-merge and removed process/cherry-pick status/0-triage labels Jun 5, 2019

Levovar mentioned this pull request Jul 31, 2019

Docker daemon crashes (again) with "fatal error: concurrent map read and map write" #39643

Closed

kolyshkin mentioned this pull request Jul 31, 2019

projectquota: protect concurrent map access (ENGCORE-920) #39644

Merged

thaJeztah added this to the 20.03.0 milestone Apr 2, 2020

Conversation

kolyshkin commented Apr 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GordonTheTurtle commented Apr 24, 2019

Uh oh!

kolyshkin commented Apr 24, 2019

Uh oh!

kolyshkin commented Apr 24, 2019

Uh oh!

kolyshkin commented Apr 24, 2019

Uh oh!

dmcgowan commented Apr 24, 2019

Uh oh!

kolyshkin commented May 2, 2019

Uh oh!

xinfengliu commented May 3, 2019

Uh oh!

kolyshkin commented May 3, 2019

Uh oh!

dmcgowan commented May 6, 2019

Uh oh!

codecov bot commented May 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

kolyshkin commented May 7, 2019

Uh oh!

Uh oh!

kolyshkin commented May 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dmcgowan left a comment

Choose a reason for hiding this comment

Uh oh!

kolyshkin commented May 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

kolyshkin commented Apr 24, 2019 •

edited

Loading

codecov bot commented May 7, 2019 •

edited

Loading

kolyshkin commented May 9, 2019 •

edited

Loading

kolyshkin commented May 13, 2019 •

edited

Loading