devicemapper: Don't mount in Create()

We used to mount in Create() to be able to create a few files that
needs to be in each device. However, this mount is problematic for
selinux, as we need to set the mount label at mount-time, and it
is not known at the time of Create().

This change just moves the file creation to first Get() call and
drops the mount from Create(). Additionally, this lets us remove
some complexities we had to avoid an extra unmount+mount cycle.

Docker-DCO-1.1-Signed-off-by: Alexander Larsson <alexl@redhat.com> (github: alexlarsson)

daemon/graphdriver/devmapper/deviceset.go

@@ -35,12 +35,6 @@ type DevInfo struct {
 
 	mountCount int    `json:"-"`
 	mountPath  string `json:"-"`
-	// A floating mount means one reference is not owned and
-	// will be stolen by the next mount. This allows us to
-	// avoid unmounting directly after creation before the
-	// first get (since we need to mount to set up the device
-	// a bit first).
-	floating bool `json:"-"`
 
 	// The global DeviceSet lock guarantees that we serialize all
 	// the calls to libdevmapper (which is not threadsafe), but we
@@ -94,14 +88,6 @@ type DevStatus struct {
 	HighestMappedSector uint64
 }
 
-type UnmountMode int
-
-const (
-	UnmountRegular UnmountMode = iota
-	UnmountFloat
-	UnmountSink
-)
-
 func getDevName(name string) string {
 	return "/dev/mapper/" + name
 }
@@ -876,12 +862,7 @@ func (devices *DeviceSet) MountDevice(hash, path string, mountLabel string) erro
 			return fmt.Errorf("Trying to mount devmapper device in multple places (%s, %s)", info.mountPath, path)
 		}
 
-		if info.floating {
-			// Steal floating ref
-			info.floating = false
-		} else {
-			info.mountCount++
-		}
+		info.mountCount++
 		return nil
 	}
 
@@ -903,13 +884,12 @@ func (devices *DeviceSet) MountDevice(hash, path string, mountLabel string) erro
 
 	info.mountCount = 1
 	info.mountPath = path
-	info.floating = false
 
 	return devices.setInitialized(info)
 }
 
-func (devices *DeviceSet) UnmountDevice(hash string, mode UnmountMode) error {
-	utils.Debugf("[devmapper] UnmountDevice(hash=%s, mode=%d)", hash, mode)
+func (devices *DeviceSet) UnmountDevice(hash string) error {
+	utils.Debugf("[devmapper] UnmountDevice(hash=%s)", hash)
 	defer utils.Debugf("[devmapper] UnmountDevice END")
 
 	info, err := devices.lookupDevice(hash)
@@ -923,24 +903,6 @@ func (devices *DeviceSet) UnmountDevice(hash string, mode UnmountMode) error {
 	devices.Lock()
 	defer devices.Unlock()
 
-	if mode == UnmountFloat {
-		if info.floating {
-			return fmt.Errorf("UnmountDevice: can't float floating reference %s\n", hash)
-		}
-
-		// Leave this reference floating
-		info.floating = true
-		return nil
-	}
-
-	if mode == UnmountSink {
-		if !info.floating {
-			// Someone already sunk this
-			return nil
-		}
-		// Otherwise, treat this as a regular unmount
-	}
-
 	if info.mountCount == 0 {
 		return fmt.Errorf("UnmountDevice: device not-mounted id %s\n", hash)
 	}

daemon/graphdriver/devmapper/driver.go

@@ -64,26 +64,6 @@ func (d *Driver) Create(id, parent string, mountLabel string) error {
 	if err := d.DeviceSet.AddDevice(id, parent); err != nil {
 		return err
 	}
-	mp := path.Join(d.home, "mnt", id)
-	if err := d.mount(id, mp); err != nil {
-		return err
-	}
-
-	if err := osMkdirAll(path.Join(mp, "rootfs"), 0755); err != nil && !osIsExist(err) {
-		return err
-	}
-
-	// Create an "id" file with the container/image id in it to help reconscruct this in case
-	// of later problems
-	if err := ioutil.WriteFile(path.Join(mp, "id"), []byte(id), 0600); err != nil {
-		return err
-	}
-
-	// We float this reference so that the next Get call can
-	// steal it, so we don't have to unmount
-	if err := d.DeviceSet.UnmountDevice(id, UnmountFloat); err != nil {
-		return err
-	}
 
 	return nil
 }
@@ -96,10 +76,6 @@ func (d *Driver) Remove(id string) error {
 		return nil
 	}
 
-	// Sink the float from create in case no Get() call was made
-	if err := d.DeviceSet.UnmountDevice(id, UnmountSink); err != nil {
-		return err
-	}
 	// This assumes the device has been properly Get/Put:ed and thus is unmounted
 	if err := d.DeviceSet.DeleteDevice(id); err != nil {
 		return err
@@ -115,28 +91,42 @@ func (d *Driver) Remove(id string) error {
 
 func (d *Driver) Get(id string) (string, error) {
 	mp := path.Join(d.home, "mnt", id)
-	if err := d.mount(id, mp); err != nil {
+
+	// Create the target directories if they don't exist
+	if err := osMkdirAll(mp, 0755); err != nil && !osIsExist(err) {
 		return "", err
 	}
 
-	return path.Join(mp, "rootfs"), nil
-}
+	// Mount the device
+	if err := d.DeviceSet.MountDevice(id, mp, ""); err != nil {
+		return "", err
+	}
 
-func (d *Driver) Put(id string) {
-	if err := d.DeviceSet.UnmountDevice(id, UnmountRegular); err != nil {
-		utils.Errorf("Warning: error unmounting device %s: %s\n", id, err)
+	rootFs := path.Join(mp, "rootfs")
+	if err := osMkdirAll(rootFs, 0755); err != nil && !osIsExist(err) {
+		d.DeviceSet.UnmountDevice(id)
+		return "", err
 	}
+
+	idFile := path.Join(mp, "id")
+	if _, err := osStat(idFile); err != nil && osIsNotExist(err) {
+		// Create an "id" file with the container/image id in it to help reconscruct this in case
+		// of later problems
+		if err := ioutil.WriteFile(idFile, []byte(id), 0600); err != nil {
+			d.DeviceSet.UnmountDevice(id)
+			return "", err
+		}
+	}
+
+	return rootFs, nil
 }
 
-func (d *Driver) mount(id, mountPoint string) error {
-	// Create the target directories if they don't exist
-	if err := osMkdirAll(mountPoint, 0755); err != nil && !osIsExist(err) {
-		return err
+func (d *Driver) Put(id string) {
+	if err := d.DeviceSet.UnmountDevice(id); err != nil {
+		utils.Errorf("Warning: error unmounting device %s: %s\n", id, err)
 	}
-	// Mount the device
-	return d.DeviceSet.MountDevice(id, mountPoint, "")
 }
 
 func (d *Driver) Exists(id string) bool {
-	return d.Devices[id] != nil
+	return d.DeviceSet.HasDevice(id)
 }

daemon/graphdriver/devmapper/driver_test.go

@@ -500,15 +500,10 @@ func TestDriverCreate(t *testing.T) {
 		calls.Assert(t,
 			"DmTaskCreate",
 			"DmTaskGetInfo",
-			"sysMount",
 			"DmTaskRun",
-			"DmTaskSetTarget",
 			"DmTaskSetSector",
-			"DmTaskSetCookie",
-			"DmUdevWait",
 			"DmTaskSetName",
 			"DmTaskSetMessage",
-			"DmTaskSetAddNode",
 		)
 
 	}()
@@ -619,15 +614,10 @@ func TestDriverRemove(t *testing.T) {
 		calls.Assert(t,
 			"DmTaskCreate",
 			"DmTaskGetInfo",
-			"sysMount",
 			"DmTaskRun",
-			"DmTaskSetTarget",
 			"DmTaskSetSector",
-			"DmTaskSetCookie",
-			"DmUdevWait",
 			"DmTaskSetName",
 			"DmTaskSetMessage",
-			"DmTaskSetAddNode",
 		)
 
 		Mounted = func(mnt string) (bool, error) {
@@ -650,7 +640,6 @@ func TestDriverRemove(t *testing.T) {
 			"DmTaskSetTarget",
 			"DmTaskSetAddNode",
 			"DmUdevWait",
-			"sysUnmount",
 		)
 	}()
 	runtime.GC()