From a450a575a672b90f02a4b1d3d9300ce1d70a6311 Mon Sep 17 00:00:00 2001
From: Kir Kolyshkin <kolyshkin@gmail.com>
Date: Fri, 1 Dec 2017 12:36:18 -0800
Subject: [PATCH] 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>
---
 daemon/graphdriver/zfs/zfs.go | 37 ++++++++++++++++++++++-------------
 1 file changed, 23 insertions(+), 14 deletions(-)

diff --git a/daemon/graphdriver/zfs/zfs.go b/daemon/graphdriver/zfs/zfs.go
index 51d31bcfc5..8b3f78d829 100644
--- a/daemon/graphdriver/zfs/zfs.go
+++ b/daemon/graphdriver/zfs/zfs.go
@@ -181,9 +181,10 @@ func (d *Driver) String() string {
 	return "zfs"
 }
 
-// Cleanup is used to implement graphdriver.ProtoDriver. There is no cleanup required for this driver.
+// Cleanup is called on daemon shutdown. It unmounts the bind mount
+// created by mount.MakePrivate() in Init().
 func (d *Driver) Cleanup() error {
-	return nil
+	return mount.Unmount(d.options.mountPath)
 }
 
 // Status returns information about the ZFS filesystem. It returns a two dimensional array of information
@@ -357,11 +358,24 @@ func (d *Driver) Remove(id string) error {
 }
 
 // Get returns the mountpoint for the given id after creating the target directories if necessary.
-func (d *Driver) Get(id, mountLabel string) (containerfs.ContainerFS, error) {
+func (d *Driver) Get(id, mountLabel string) (_ containerfs.ContainerFS, retErr error) {
 	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 {
+				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) {
+					logrus.Debugf("Failed to remove %s: %v", id, rmErr)
+				}
+
+			}
+		}
+	}()
 
 	filesystem := d.zfsPath(id)
 	options := label.FormatMountLabel("", mountLabel)
@@ -369,25 +383,20 @@ func (d *Driver) Get(id, mountLabel string) (containerfs.ContainerFS, error) {
 
 	rootUID, rootGID, err := idtools.GetRootUIDGID(d.uidMaps, d.gidMaps)
 	if err != nil {
-		d.ctr.Decrement(mountpoint)
 		return nil, err
 	}
 	// Create the target directories if they don't exist
 	if err := idtools.MkdirAllAndChown(mountpoint, 0755, idtools.IDPair{rootUID, rootGID}); err != nil {
-		d.ctr.Decrement(mountpoint)
 		return nil, err
 	}
 
 	if err := mount.Mount(filesystem, mountpoint, "zfs", options); err != nil {
-		d.ctr.Decrement(mountpoint)
 		return nil, fmt.Errorf("error creating zfs mount of %s to %s: %v", filesystem, mountpoint, err)
 	}
 
 	// this could be our first mount after creation of the filesystem, and the root dir may still have root
 	// permissions instead of the remapped root uid:gid (if user namespaces are enabled):
 	if err := os.Chown(mountpoint, rootUID, rootGID); err != nil {
-		mount.Unmount(mountpoint)
-		d.ctr.Decrement(mountpoint)
 		return nil, fmt.Errorf("error modifying zfs mountpoint (%s) directory ownership: %v", mountpoint, err)
 	}
 
@@ -400,16 +409,16 @@ func (d *Driver) Put(id string) error {
 	if count := d.ctr.Decrement(mountpoint); count > 0 {
 		return nil
 	}
-	mounted, err := graphdriver.Mounted(graphdriver.FsMagicZfs, mountpoint)
-	if err != nil || !mounted {
-		return err
-	}
 
 	logrus.Debugf(`[zfs] unmount("%s")`, mountpoint)
 
-	if err := mount.Unmount(mountpoint); err != nil {
-		return fmt.Errorf("error unmounting to %s: %v", mountpoint, err)
+	if err := unix.Unmount(mountpoint, unix.MNT_DETACH); err != nil {
+		logrus.Warnf("Failed to unmount %s mount %s: %v", id, mountpoint, err)
 	}
+	if err := unix.Rmdir(mountpoint); err != nil && !os.IsNotExist(err) {
+		logrus.Debugf("Failed to remove %s mount point %s: %v", id, mountpoint, err)
+	}
+
 	return nil
 }