devmapper: Move refcounting to DeviceSet

We already have some kind of refcounting in DeviceSet, this fleshes
it out to allow it to completely subsume the refcounting in
devmapper.Driver.

This allows us to drop the double refcounting, and the locking inside
devmapper.Driver. This, in particular the locking simplification will
make it easier in the future to parallelize the device mapper.

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

graphdriver/devmapper/deviceset.go

@@ -29,6 +29,15 @@ type DevInfo struct {
 	TransactionId uint64     `json:"transaction_id"`
 	Initialized   bool       `json:"initialized"`
 	devices       *DeviceSet `json:"-"`
+
+	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:"-"`
 }
 
 type MetaData struct {
@@ -43,7 +52,6 @@ type DeviceSet struct {
 	TransactionId    uint64
 	NewTransactionId uint64
 	nextFreeDevice   int
-	activeMounts     map[string]int
 }
 
 type DiskUsage struct {
@@ -69,6 +77,14 @@ type DevStatus struct {
 	HighestMappedSector uint64
 }
 
+type UnmountMode int
+
+const (
+	UnmountRegular UnmountMode = iota
+	UnmountFloat
+	UnmountSink
+)
+
 func getDevName(name string) string {
 	return "/dev/mapper/" + name
 }
@@ -733,13 +749,12 @@ func (devices *DeviceSet) Shutdown() error {
 	utils.Debugf("[devmapper] Shutting down DeviceSet: %s", devices.root)
 	defer utils.Debugf("[deviceset %s] shutdown END", devices.devicePrefix)
 
-	for path, count := range devices.activeMounts {
+	for _, info := range devices.Devices {
-		for i := count; i > 0; i-- {
+		if info.mountCount > 0 {
-			if err := sysUnmount(path, 0); err != nil {
+			if err := sysUnmount(info.mountPath, 0); err != nil {
-				utils.Debugf("Shutdown unmounting %s, error: %s\n", path, err)
+				utils.Debugf("Shutdown unmounting %s, error: %s\n", info.mountPath, err)
 			}
 		}
-		delete(devices.activeMounts, path)
 	}
 
 	for _, d := range devices.Devices {
@@ -761,22 +776,32 @@ func (devices *DeviceSet) Shutdown() error {
 	return nil
 }
 
-func (devices *DeviceSet) MountDevice(hash, path string, readOnly bool) error {
+func (devices *DeviceSet) MountDevice(hash, path string) error {
 	devices.Lock()
 	defer devices.Unlock()
 
+	info := devices.Devices[hash]
+
+	if info.mountCount > 0 {
+		if path != info.mountPath {
+			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++
+		}
+		return nil
+	}
+
 	if err := devices.activateDeviceIfNeeded(hash); err != nil {
 		return fmt.Errorf("Error activating devmapper device for '%s': %s", hash, err)
 	}
 
-	info := devices.Devices[hash]
-
 	var flags uintptr = sysMsMgcVal
 
-	if readOnly {
-		flags = flags | sysMsRdOnly
-	}
-
 	err := sysMount(info.DevName(), path, "ext4", flags, "discard")
 	if err != nil && err == sysEInval {
 		err = sysMount(info.DevName(), path, "ext4", flags, "")
@@ -785,20 +810,50 @@ func (devices *DeviceSet) MountDevice(hash, path string, readOnly bool) error {
 		return fmt.Errorf("Error mounting '%s' on '%s': %s", info.DevName(), path, err)
 	}
 
-	count := devices.activeMounts[path]
+	info.mountCount = 1
-	devices.activeMounts[path] = count + 1
+	info.mountPath = path
+	info.floating = false
 
 	return devices.setInitialized(hash)
 }
 
-func (devices *DeviceSet) UnmountDevice(hash, path string, deactivate bool) error {
+func (devices *DeviceSet) UnmountDevice(hash string, mode UnmountMode) error {
-	utils.Debugf("[devmapper] UnmountDevice(hash=%s path=%s)", hash, path)
+	utils.Debugf("[devmapper] UnmountDevice(hash=%s, mode=%d)", hash, mode)
 	defer utils.Debugf("[devmapper] UnmountDevice END")
 	devices.Lock()
 	defer devices.Unlock()
 
-	utils.Debugf("[devmapper] Unmount(%s)", path)
+	info := devices.Devices[hash]
-	if err := sysUnmount(path, 0); err != nil {
+
+	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)
+	}
+
+	info.mountCount--
+	if info.mountCount > 0 {
+		return nil
+	}
+
+	utils.Debugf("[devmapper] Unmount(%s)", info.mountPath)
+	if err := sysUnmount(info.mountPath, 0); err != nil {
 		utils.Debugf("\n--->Err: %s\n", err)
 		return err
 	}
@@ -809,15 +864,9 @@ func (devices *DeviceSet) UnmountDevice(hash, path string, deactivate bool) erro
 		return err
 	}
 
-	if count := devices.activeMounts[path]; count > 1 {
+	devices.deactivateDevice(hash)
-		devices.activeMounts[path] = count - 1
-	} else {
-		delete(devices.activeMounts, path)
-	}
 
-	if deactivate {
+	info.mountPath = ""
-		devices.deactivateDevice(hash)
-	}
 
 	return nil
 }
@@ -960,9 +1009,8 @@ func NewDeviceSet(root string, doInit bool) (*DeviceSet, error) {
 	SetDevDir("/dev")
 
 	devices := &DeviceSet{
-		root:         root,
+		root:     root,
-		MetaData:     MetaData{Devices: make(map[string]*DevInfo)},
+		MetaData: MetaData{Devices: make(map[string]*DevInfo)},
-		activeMounts: make(map[string]int),
 	}
 
 	if err := devices.initDevmapper(doInit); err != nil {

graphdriver/devmapper/driver.go

@@ -8,7 +8,6 @@ import (
 	"github.com/dotcloud/docker/utils"
 	"io/ioutil"
 	"path"
-	"sync"
 )
 
 func init() {
@@ -22,9 +21,7 @@ func init() {
 
 type Driver struct {
 	*DeviceSet
-	home       string
+	home string
-	sync.Mutex // Protects concurrent modification to active
-	active     map[string]int
 }
 
 var Init = func(home string) (graphdriver.Driver, error) {
@@ -35,7 +32,6 @@ var Init = func(home string) (graphdriver.Driver, error) {
 	d := &Driver{
 		DeviceSet: deviceSet,
 		home:      home,
-		active:    make(map[string]int),
 	}
 	return d, nil
 }
@@ -83,55 +79,36 @@ func (d *Driver) Create(id, parent string) error {
 		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
 }
 
 func (d *Driver) Remove(id string) error {
-	// Protect the d.active from concurrent access
+	// Sink the float from create in case no Get() call was made
-	d.Lock()
+	if err := d.DeviceSet.UnmountDevice(id, UnmountSink); err != nil {
-	defer d.Unlock()
-
-	if d.active[id] != 0 {
-		utils.Errorf("Warning: removing active id %s\n", id)
-	}
-
-	mp := path.Join(d.home, "mnt", id)
-	if err := d.unmount(id, mp); err != nil {
 		return err
 	}
+	// This assumes the device has been properly Get/Put:ed and thus is unmounted
 	return d.DeviceSet.DeleteDevice(id)
 }
 
 func (d *Driver) Get(id string) (string, error) {
-	// Protect the d.active from concurrent access
-	d.Lock()
-	defer d.Unlock()
-
-	count := d.active[id]
-
 	mp := path.Join(d.home, "mnt", id)
-	if count == 0 {
+	if err := d.mount(id, mp); err != nil {
-		if err := d.mount(id, mp); err != nil {
+		return "", err
-			return "", err
-		}
 	}
 
-	d.active[id] = count + 1
-
 	return path.Join(mp, "rootfs"), nil
 }
 
 func (d *Driver) Put(id string) {
-	// Protect the d.active from concurrent access
+	if err := d.DeviceSet.UnmountDevice(id, UnmountRegular); err != nil {
-	d.Lock()
+		utils.Errorf("Warning: error unmounting device %s: %s\n", id, err)
-	defer d.Unlock()
-
-	if count := d.active[id]; count > 1 {
-		d.active[id] = count - 1
-	} else {
-		mp := path.Join(d.home, "mnt", id)
-		d.unmount(id, mp)
-		delete(d.active, id)
 	}
 }
 
@@ -140,25 +117,8 @@ func (d *Driver) mount(id, mountPoint string) error {
 	if err := osMkdirAll(mountPoint, 0755); err != nil && !osIsExist(err) {
 		return err
 	}
-	// If mountpoint is already mounted, do nothing
-	if mounted, err := Mounted(mountPoint); err != nil {
-		return fmt.Errorf("Error checking mountpoint: %s", err)
-	} else if mounted {
-		return nil
-	}
 	// Mount the device
-	return d.DeviceSet.MountDevice(id, mountPoint, false)
+	return d.DeviceSet.MountDevice(id, mountPoint)
-}
-
-func (d *Driver) unmount(id, mountPoint string) error {
-	// If mountpoint is not mounted, do nothing
-	if mounted, err := Mounted(mountPoint); err != nil {
-		return fmt.Errorf("Error checking mountpoint: %s", err)
-	} else if !mounted {
-		return nil
-	}
-	// Unmount the device
-	return d.DeviceSet.UnmountDevice(id, mountPoint, true)
 }
 
 func (d *Driver) Exists(id string) bool {

graphdriver/devmapper/driver_test.go

@@ -495,7 +495,6 @@ func TestDriverCreate(t *testing.T) {
 			"DmTaskCreate",
 			"DmTaskGetInfo",
 			"sysMount",
-			"Mounted",
 			"DmTaskRun",
 			"DmTaskSetTarget",
 			"DmTaskSetSector",
@@ -614,7 +613,6 @@ func TestDriverRemove(t *testing.T) {
 			"DmTaskCreate",
 			"DmTaskGetInfo",
 			"sysMount",
-			"Mounted",
 			"DmTaskRun",
 			"DmTaskSetTarget",
 			"DmTaskSetSector",
@@ -645,7 +643,6 @@ func TestDriverRemove(t *testing.T) {
 			"DmTaskSetTarget",
 			"DmTaskSetAddNode",
 			"DmUdevWait",
-			"Mounted",
 			"sysUnmount",
 		)
 	}()