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

zfs: fix ebusy on umount etc #35674

Merged
merged 1 commit into from Dec 28, 2017

Conversation

Projects
None yet
9 participants
@kolyshkin
Contributor

kolyshkin commented Dec 1, 2017

This commit is a set of fixes and improvement for zfs graph driver,
in particular:

  1. Remove mount point after umount in Get() error path, as well
    as in Put() (with MNT_DETACH flag). This should solve "ebusy
    on umount" error reported in #35642.

  2. Fix/improve error handling in Get() -- do not try to umount
    if refcount > 0

  3. Simplifies unmount in Get(). Specifically, remove call to
    graphdriver.Mounted() (which checks if fs is mounted using
    statfs() and check for fs type) and mount.Unmount() (which
    parses /proc/self/mountinfo). Calling unix.Unmount() is
    simple and sufficient.

  4. Add unmounting of driver's home to Cleanup().

Signed-off-by: Kir Kolyshkin kolyshkin@gmail.com

- How to verify it

  • There is a test case described in #35642

- Description for the changelog

  • zfs: fix busy error on container stop
zfs: fix ebusy on umount
This commit is a set of fixes and improvement for zfs graph driver,
in particular:

1. Remove mount point after umount in `Get()` error path, as well
   as in `Put()` (with `MNT_DETACH` flag). This should solve "failed
   to remove root filesystem for <ID> .... dataset is busy" error
   reported in Moby issue 35642.

To reproduce the issue:

   - start dockerd with zfs
   - docker run -d --name c1 --rm busybox top
   - docker run -d --name c2 --rm busybox top
   - docker stop c1
   - docker rm c1

Output when the bug is present:
```
Error response from daemon: driver "zfs" failed to remove root
filesystem for XXX : exit status 1: "/sbin/zfs zfs destroy -r
scratch/docker/YYY" => cannot destroy 'scratch/docker/YYY':
dataset is busy
```

Output when the bug is fixed:
```
Error: No such container: c1
```
(as the container has been successfully autoremoved on stop)

2. Fix/improve error handling in `Get()` -- do not try to umount
   if `refcount` > 0

3. Simplifies unmount in `Get()`. Specifically, remove call to
   `graphdriver.Mounted()` (which checks if fs is mounted using
   `statfs()` and check for fs type) and `mount.Unmount()` (which
   parses `/proc/self/mountinfo`). Calling `unix.Unmount()` is
   simple and sufficient.

4. Add unmounting of driver's home to `Cleanup()`.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
mountpoint := d.mountPath(id)
if count := d.ctr.Increment(mountpoint); count > 1 {
return containerfs.NewLocalContainerFS(mountpoint), nil
}
defer func() {
if retErr != nil {
if c := d.ctr.Decrement(mountpoint); c <= 0 {

This comment has been minimized.

@stevvooe

stevvooe Dec 2, 2017

Contributor

Are there any locks here to prevent another process from increment at the same time this decrements?

@stevvooe

stevvooe Dec 2, 2017

Contributor

Are there any locks here to prevent another process from increment at the same time this decrements?

This comment has been minimized.

@kolyshkin

kolyshkin Dec 4, 2017

Contributor

Yes, d.ctr is of graphdriver.RefCounter type which does inc/dec under a mutex (see https://github.com/moby/moby/blob/master/daemon/graphdriver/counter.go)

@kolyshkin

kolyshkin Dec 4, 2017

Contributor

Yes, d.ctr is of graphdriver.RefCounter type which does inc/dec under a mutex (see https://github.com/moby/moby/blob/master/daemon/graphdriver/counter.go)

@WyseNynja

This comment has been minimized.

Show comment
Hide comment
@WyseNynja

WyseNynja Dec 3, 2017

Do you think this PR will fix my issue with ntp or is that different? zfsonlinux/zfs#1810 (comment)

WyseNynja commented Dec 3, 2017

Do you think this PR will fix my issue with ntp or is that different? zfsonlinux/zfs#1810 (comment)

@kolyshkin

This comment has been minimized.

Show comment
Hide comment
@kolyshkin

kolyshkin Dec 4, 2017

Contributor

Do you think this PR will fix my issue with ntp or is that different? zfsonlinux/zfs#1810 (comment)

@WyseNynja yes, it might (if the kernel you use is sufficiently recent). Update: yes, it appears to be fixing it.

Contributor

kolyshkin commented Dec 4, 2017

Do you think this PR will fix my issue with ntp or is that different? zfsonlinux/zfs#1810 (comment)

@WyseNynja yes, it might (if the kernel you use is sufficiently recent). Update: yes, it appears to be fixing it.

@vieux

This comment has been minimized.

Show comment
Hide comment
@vieux

vieux Dec 5, 2017

Collaborator

SGTM

Collaborator

vieux commented Dec 5, 2017

SGTM

if mntErr := unix.Unmount(mountpoint, 0); mntErr != nil {
logrus.Errorf("Error unmounting %v: %v", mountpoint, mntErr)
}
if rmErr := unix.Rmdir(mountpoint); rmErr != nil && !os.IsNotExist(rmErr) {

This comment has been minimized.

@mlaventure

mlaventure Dec 7, 2017

Contributor

maybe don't rmdir if the unmount failed?

@mlaventure

mlaventure Dec 7, 2017

Contributor

maybe don't rmdir if the unmount failed?

This comment has been minimized.

@kolyshkin

kolyshkin Dec 11, 2017

Contributor

unmount may failure for a handful of reasons, not just EBUSY, and Rmdir is harmless (i.e. it won't remove any data, just an empty directory), so it's slightly more robust to try it anyway.

@kolyshkin

kolyshkin Dec 11, 2017

Contributor

unmount may failure for a handful of reasons, not just EBUSY, and Rmdir is harmless (i.e. it won't remove any data, just an empty directory), so it's slightly more robust to try it anyway.

This comment has been minimized.

@mlaventure

mlaventure Dec 12, 2017

Contributor

@kolyshkin my bad, I read it as a RemoveAll

@mlaventure

mlaventure Dec 12, 2017

Contributor

@kolyshkin my bad, I read it as a RemoveAll

@kolyshkin

This comment has been minimized.

Show comment
Hide comment
@kolyshkin

kolyshkin Dec 14, 2017

Contributor

I believe I have addressed all the review comments.

Contributor

kolyshkin commented Dec 14, 2017

I believe I have addressed all the review comments.

@tonistiigi

This comment has been minimized.

Show comment
Hide comment
@tonistiigi

tonistiigi Dec 28, 2017

Member

LGTM

Member

tonistiigi commented Dec 28, 2017

LGTM

@tonistiigi tonistiigi merged commit daded8d into moby:master Dec 28, 2017

6 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 38138 has succeeded
Details
janky Jenkins build Docker-PRs 46856 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 7260 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 18405 has succeeded
Details
z Jenkins build Docker-PRs-s390x 7079 has succeeded
Details
@WyseNynja

This comment has been minimized.

Show comment
Hide comment
@WyseNynja

WyseNynja Dec 29, 2017

Will this be in the next release? If not, what version should I wait for to get this fix?

WyseNynja commented Dec 29, 2017

Will this be in the next release? If not, what version should I wait for to get this fix?

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Dec 29, 2017

Member

This will be included in the Docker 18.01 release

Member

thaJeztah commented Dec 29, 2017

This will be included in the Docker 18.01 release

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