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

Ensure that a device mapper task is referenced until task is complete #33376

Merged
merged 1 commit into from May 24, 2017

Conversation

Projects
None yet
6 participants
@nhorman

nhorman commented May 24, 2017

DeviceMapper tasks in go use SetFinalizer to clean up C construct
counterparts in the C LVM library. While thats well and good, it relies
heavily on the exact interpretation of when the golang garbage collector
determines that an object is unreachable is subject to reclaimation.
While common sense would assert that for stack variables (which these DM
tasks always are), are unreachable when the stack frame in which they
are declared returns, thats not the case. According to this:

https://golang.org/pkg/runtime/#SetFinalizer

The garbage collector decides that, if a function calls into a
systemcall (which task.run() always will in LVM), and there are no
subsequent references to the task variable within that stack frame, then
it can be reclaimed. Those conditions are met in several devmapper.go
routines, and if the garbage collector runs in the middle of a
deviceMapper operation, then the task can be destroyed while the
operation is in progress, leading to crashes, failed operations and
other unpredictable behavior.

The fix is to use the KeepAlive interface:

https://golang.org/pkg/runtime/#KeepAlive

The KeepAlive method is effectively an empy reference that fools the
garbage collector into thinking that a variable is still reachable. By
adding a call to KeepAlive in the task.run() method, we can ensure that
the garbage collector won't reclaim a task object until its execution
within the deviceMapper C library is complete.

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

- What I did

- How I did it

- How to verify it

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

Neil Horman Neil Horman
Ensure that a device mapper task is referenced until task is complete
DeviceMapper tasks in go use SetFinalizer to clean up C construct
counterparts in the C LVM library.  While thats well and good, it relies
heavily on the exact interpretation of when the golang garbage collector
determines that an object is unreachable is subject to reclaimation.
While common sense would assert that for stack variables (which these DM
tasks always are), are unreachable when the stack frame in which they
are declared returns, thats not the case.  According to this:

https://golang.org/pkg/runtime/#SetFinalizer

The garbage collector decides that, if a function calls into a
systemcall (which task.run() always will in LVM), and there are no
subsequent references to the task variable within that stack frame, then
it can be reclaimed.  Those conditions are met in several devmapper.go
routines, and if the garbage collector runs in the middle of a
deviceMapper operation, then the task can be destroyed while the
operation is in progress, leading to crashes, failed operations and
other unpredictable behavior.

The fix is to use the KeepAlive interface:

https://golang.org/pkg/runtime/#KeepAlive

The KeepAlive method is effectively an empy reference that fools the
garbage collector into thinking that a variable is still reachable.  By
adding a call to KeepAlive in the task.run() method, we can ensure that
the garbage collector won't reclaim a task object until its execution
within the deviceMapper C library is complete.

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

This comment has been minimized.

Show comment
Hide comment
@nhorman

nhorman May 24, 2017

sorry, forgot to fill out the rest of the information. This pull is based of code review that I preformed while fixing issue #33050. I noticed that the use of SetFinalizer was in voliation of the early reclamation examples provided by the runtime documentation for golang, and the addition of the use of KeepAlive just made sense, as it fit exactly with the provided examples.

nhorman commented May 24, 2017

sorry, forgot to fill out the rest of the information. This pull is based of code review that I preformed while fixing issue #33050. I noticed that the use of SetFinalizer was in voliation of the early reclamation examples provided by the runtime documentation for golang, and the addition of the use of KeepAlive just made sense, as it fit exactly with the provided examples.

@cpuguy83 cpuguy83 added this to the 17.06.0 milestone May 24, 2017

@rhvgoyal

This comment has been minimized.

Show comment
Hide comment
@rhvgoyal

rhvgoyal May 24, 2017

Contributor

@nhorman thanks for the analysis. This makes sense to me. We ran into a crash which very well could happen due to this.

LGTM

Contributor

rhvgoyal commented May 24, 2017

@nhorman thanks for the analysis. This makes sense to me. We ran into a crash which very well could happen due to this.

LGTM

@runcom

This comment has been minimized.

Show comment
Hide comment
@runcom

runcom May 24, 2017

Member

LGTM

Member

runcom commented May 24, 2017

LGTM

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 May 24, 2017

Contributor

It seems like this would mainly be an issue on go 1.8? I've added to the 17.06 milestone.

Contributor

cpuguy83 commented May 24, 2017

It seems like this would mainly be an issue on go 1.8? I've added to the 17.06 milestone.

@rhvgoyal

This comment has been minimized.

Show comment
Hide comment
@rhvgoyal

rhvgoyal May 24, 2017

Contributor

@cpuguy83 Don't know. Is this behavior new to 1.8 and older version don't have it?

Contributor

rhvgoyal commented May 24, 2017

@cpuguy83 Don't know. Is this behavior new to 1.8 and older version don't have it?

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83
Contributor

cpuguy83 commented May 24, 2017

@rhvgoyal

This comment has been minimized.

Show comment
Hide comment
@rhvgoyal

rhvgoyal May 24, 2017

Contributor

@cpuguy83 thanks for the pointer. So this is new to 1.8.

I am surprised that how can newer versions can break existing programs. If this is an optimization, then there should be an option to opt-in.

Now all the programs which have been running fine with older versions, have potential to be broken?

Contributor

rhvgoyal commented May 24, 2017

@cpuguy83 thanks for the pointer. So this is new to 1.8.

I am surprised that how can newer versions can break existing programs. If this is an optimization, then there should be an option to opt-in.

Now all the programs which have been running fine with older versions, have potential to be broken?

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83
Contributor

cpuguy83 commented May 24, 2017

@cpuguy83

LGTM

@cpuguy83 cpuguy83 merged commit e4abe7c into moby:master May 24, 2017

6 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 34485 has succeeded
Details
janky Jenkins build Docker-PRs 43082 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 3471 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 14355 has succeeded
Details
z Jenkins build Docker-PRs-s390x 3193 has succeeded
Details
@nhorman

This comment has been minimized.

Show comment
Hide comment
@nhorman

nhorman May 25, 2017

FWIW, I agree that the addition of this liveness limitation added to the garbage collector in go 1.8 is quite shocking. I would have thought the golang devs would have been more conservative at this point. Thanks all for the quick review

nhorman commented May 25, 2017

FWIW, I agree that the addition of this liveness limitation added to the garbage collector in go 1.8 is quite shocking. I would have thought the golang devs would have been more conservative at this point. Thanks all for the quick review

@thaJeztah thaJeztah referenced this pull request May 30, 2017

Closed

17.06.0 RC2 tracker #2

23 of 23 tasks complete

liusdu pushed a commit to liusdu/moby that referenced this pull request Oct 30, 2017

Ensure that a device mapper task is referenced until task is complete
DeviceMapper tasks in go use SetFinalizer to clean up C construct
counterparts in the C LVM library.  While thats well and good, it relies
heavily on the exact interpretation of when the golang garbage collector
determines that an object is unreachable is subject to reclaimation.
While common sense would assert that for stack variables (which these DM
tasks always are), are unreachable when the stack frame in which they
are declared returns, thats not the case.  According to this:

https://golang.org/pkg/runtime/#SetFinalizer

The garbage collector decides that, if a function calls into a
systemcall (which task.run() always will in LVM), and there are no
subsequent references to the task variable within that stack frame, then
it can be reclaimed.  Those conditions are met in several devmapper.go
routines, and if the garbage collector runs in the middle of a
deviceMapper operation, then the task can be destroyed while the
operation is in progress, leading to crashes, failed operations and
other unpredictable behavior.

The fix is to use the KeepAlive interface:

https://golang.org/pkg/runtime/#KeepAlive

The KeepAlive method is effectively an empy reference that fools the
garbage collector into thinking that a variable is still reachable.  By
adding a call to KeepAlive in the task.run() method, we can ensure that
the garbage collector won't reclaim a task object until its execution
within the deviceMapper C library is complete.

cherry-picked from upstream moby#33376

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
Signed-off-by: Fengtu Wang <wangfengtu@huawei.com>

liusdu pushed a commit to liusdu/moby that referenced this pull request Oct 30, 2017

Merge branch 'ensure_task_complete' into 'huawei-1.11.2'
Ensure that a device mapper task is referenced until task is complete

DeviceMapper tasks in go use SetFinalizer to clean up C construct
counterparts in the C LVM library.  While thats well and good, it relies
heavily on the exact interpretation of when the golang garbage collector
determines that an object is unreachable is subject to reclaimation.
While common sense would assert that for stack variables (which these DM
tasks always are), are unreachable when the stack frame in which they
are declared returns, thats not the case.  According to this:

https://golang.org/pkg/runtime/#SetFinalizer

The garbage collector decides that, if a function calls into a
systemcall (which task.run() always will in LVM), and there are no
subsequent references to the task variable within that stack frame, then
it can be reclaimed.  Those conditions are met in several devmapper.go
routines, and if the garbage collector runs in the middle of a
deviceMapper operation, then the task can be destroyed while the
operation is in progress, leading to crashes, failed operations and
other unpredictable behavior.

The fix is to use the KeepAlive interface:

https://golang.org/pkg/runtime/#KeepAlive

The KeepAlive method is effectively an empy reference that fools the
garbage collector into thinking that a variable is still reachable.  By
adding a call to KeepAlive in the task.run() method, we can ensure that
the garbage collector won't reclaim a task object until its execution
within the deviceMapper C library is complete.

cherry-picked from upstream moby#33376

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
Signed-off-by: Fengtu Wang <wangfengtu@huawei.com>



See merge request docker/docker!601
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment