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

devmapper: ensure that UdevWait is called after calls to setCookie #33732

Merged
merged 1 commit into from Jun 19, 2017

Conversation

Projects
None yet
7 participants
@nhorman

nhorman commented Jun 19, 2017

Recent changes to devmapper broke the implicit requirement that UdevWait be
called after every call to task.setCookie. Failure to do so results in leaks of
semaphores in the LVM code, eventually leading to semaphore exhaustion.
Previously this was handled by calling UdevWait in a ubiquitous defer function,
but that had its own problems with ordering between execution of the wait and
other uses of the semaphore. So now we delay returning until after the UdevWait
completes, once we've set the cookie value.

Problem can be observed by running moby with an lvm2 storage backend for extended periods of time. When enough cookies have leaked, then they problem will present.

Signed-off-by: Neil Horman nhorman@tuxdriver.com

@nhorman

This comment has been minimized.

Show comment
Hide comment
@nhorman

nhorman Jun 19, 2017

@rhvgoyal as requested, tagging you to review this

nhorman commented Jun 19, 2017

@rhvgoyal as requested, tagging you to review this

@rhvgoyal

This comment has been minimized.

Show comment
Hide comment
@rhvgoyal

rhvgoyal Jun 19, 2017

Contributor

@nhorman How about going back to defer UdevWait(). That seems to be much simpler code.

IIRC, UdevWait() inline was redundant once we started doing allocation from heap?

Contributor

rhvgoyal commented Jun 19, 2017

@nhorman How about going back to defer UdevWait(). That seems to be much simpler code.

IIRC, UdevWait() inline was redundant once we started doing allocation from heap?

Neil Horman
devmapper: ensure that UdevWait is called after calls to setCookie
Recent changes to devmapper broke the implicit requirement that UdevWait be
called after every call to task.setCookie.  Failure to do so results in leaks of
semaphores in the LVM code, eventually leading to semaphore exhaustion.
Previously this was handled by calling UdevWait in a ubiquitous defer function.
While there was initially some concern with deferring the UdevWait function
would cause some amount of race possibiliy, the fact that we never return the
cookie value or any value used to find it, makes that possibility seem unlikely,
so lets go back to that method

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
@rhvgoyal

This comment has been minimized.

Show comment
Hide comment
@rhvgoyal
Contributor

rhvgoyal commented Jun 19, 2017

@rhvgoyal

This comment has been minimized.

Show comment
Hide comment
@rhvgoyal

rhvgoyal Jun 19, 2017

Contributor

LGTM

Contributor

rhvgoyal commented Jun 19, 2017

LGTM

@rhvgoyal

This comment has been minimized.

Show comment
Hide comment
@rhvgoyal

rhvgoyal Jun 19, 2017

Contributor

I did basic testing and it seems to fix cookies leaking issue when a busy device is being removed.

Contributor

rhvgoyal commented Jun 19, 2017

I did basic testing and it seems to fix cookies leaking issue when a busy device is being removed.

@cpuguy83

LGTM

@cpuguy83 cpuguy83 merged commit 7e3d0a5 into moby:master Jun 19, 2017

6 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 35126 has succeeded
Details
janky Jenkins build Docker-PRs 43732 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 4102 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 15040 has succeeded
Details
z Jenkins build Docker-PRs-s390x 3836 has succeeded
Details
@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Jun 19, 2017

Member

@rhvgoyal @nhorman IIUC, this is the PR that introduced this issue? #33119

Member

thaJeztah commented Jun 19, 2017

@rhvgoyal @nhorman IIUC, this is the PR that introduced this issue? #33119

@rhvgoyal

This comment has been minimized.

Show comment
Hide comment
@rhvgoyal

rhvgoyal Jun 19, 2017

Contributor

@thaJeztah Right. Last patch in that patch series introduced the issue.

Contributor

rhvgoyal commented Jun 19, 2017

@thaJeztah Right. Last patch in that patch series introduced the issue.

@fitz123

This comment has been minimized.

Show comment
Hide comment
@fitz123

fitz123 Oct 4, 2017

Thank you! Finally it's released! Horaay

fitz123 commented Oct 4, 2017

Thank you! Finally it's released! Horaay

@thaJeztah thaJeztah referenced this pull request Oct 30, 2017

Closed

Devicemapper: Can't set cookie dm_task_set_cookie failed #85

2 of 3 tasks complete
@leecade

This comment has been minimized.

Show comment
Hide comment
@leecade

leecade commented Nov 21, 2017

cool

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment