Fixes Issue # 23418: Race condition between device deferred removal a…

…nd resume device.

Signed-off-by: Shishir Mahajan <[email protected]>

daemon/graphdriver/devmapper/deviceset.go

@@ -529,7 +529,7 @@ func (devices *DeviceSet) activateDeviceIfNeeded(info *devInfo, ignoreDeleted bo
 
 	// Make sure deferred removal on device is canceled, if one was
 	// scheduled.
-	if err := devices.cancelDeferredRemoval(info); err != nil {
+	if _, err := devices.cancelDeferredRemoval(info); err != nil {
 		return fmt.Errorf("devmapper: Device Deferred Removal Cancellation Failed: %s", err)
 	}
 
@@ -842,6 +842,41 @@ func (devices *DeviceSet) createRegisterDevice(hash string) (*devInfo, error) {
 	return info, nil
 }
 
+func (devices *DeviceSet) takeSnapshot(hash string, baseInfo *devInfo) error {
+	var doSuspend bool
+
+	if err := devices.poolHasFreeSpace(); err != nil {
+		return err
+	}
+
+	cancelledRemoval, err := devices.cancelDeferredRemoval(baseInfo)
+	if err != nil {
+		return err
+	}
+
+	// if deferred removal was cancelled, we can safely assume the device is active.
+	if cancelledRemoval {
+		doSuspend = true
+		defer devices.deactivateDevice(baseInfo)
+	} else {
+		devinfo, _ := devicemapper.GetInfo(baseInfo.Name())
+		doSuspend = devinfo != nil && devinfo.Exists != 0
+	}
+
+	if doSuspend {
+		if err := devicemapper.SuspendDevice(baseInfo.Name()); err != nil {
+			return err
+		}
+		defer devicemapper.ResumeDevice(baseInfo.Name())
+	}
+
+	if err := devices.createRegisterSnapDevice(hash, baseInfo); err != nil {
+		return err
+	}
+
+	return nil
+}
+
 func (devices *DeviceSet) createRegisterSnapDevice(hash string, baseInfo *devInfo) error {
 	if err := devices.poolHasFreeSpace(); err != nil {
 		return err
@@ -859,7 +894,7 @@ func (devices *DeviceSet) createRegisterSnapDevice(hash string, baseInfo *devInf
 	}
 
 	for {
-		if err := devicemapper.CreateSnapDevice(devices.getPoolDevName(), deviceID, baseInfo.Name(), baseInfo.DeviceID); err != nil {
+		if err := devicemapper.CreateSnapDeviceRaw(devices.getPoolDevName(), deviceID, baseInfo.Name(), baseInfo.DeviceID); err != nil {
 			if devicemapper.DeviceIDExists(err) {
 				// Device ID already exists. This should not
 				// happen. Now we have a mechanism to find
@@ -1856,7 +1891,7 @@ func (devices *DeviceSet) AddDevice(hash, baseHash string) error {
 		return fmt.Errorf("devmapper: device %s already exists. Deleted=%v", hash, info.Deleted)
 	}
 
-	if err := devices.createRegisterSnapDevice(hash, baseInfo); err != nil {
+	if err := devices.takeSnapshot(hash, baseInfo); err != nil {
 		return err
 	}
 
@@ -2073,9 +2108,11 @@ func (devices *DeviceSet) removeDevice(devname string) error {
 	return err
 }
 
-func (devices *DeviceSet) cancelDeferredRemoval(info *devInfo) error {
+func (devices *DeviceSet) cancelDeferredRemoval(info *devInfo) (bool, error) {
+	var cancelledRemoval bool
+
 	if !devices.deferredRemove {
-		return nil
+		return cancelledRemoval, nil
 	}
 
 	logrus.Debugf("devmapper: cancelDeferredRemoval START(%s)", info.Name())
@@ -2084,23 +2121,24 @@ func (devices *DeviceSet) cancelDeferredRemoval(info *devInfo) error {
 	devinfo, err := devicemapper.GetInfoWithDeferred(info.Name())
 
 	if devinfo != nil && devinfo.DeferredRemove == 0 {
-		return nil
+		return cancelledRemoval, nil
 	}
 
 	// Cancel deferred remove
 	for i := 0; i < 100; i++ {
 		err = devicemapper.CancelDeferredRemove(info.Name())
 		if err == nil {
+			cancelledRemoval = true
 			break
 		}
 
 		if err == devicemapper.ErrEnxio {
 			// Device is probably already gone. Return success.
-			return nil
+			return cancelledRemoval, nil
 		}
 
 		if err != devicemapper.ErrBusy {
-			return err
+			return cancelledRemoval, err
 		}
 
 		// If we see EBUSY it may be a transient error,
@@ -2109,7 +2147,7 @@ func (devices *DeviceSet) cancelDeferredRemoval(info *devInfo) error {
 		time.Sleep(100 * time.Millisecond)
 		devices.Lock()
 	}
-	return err
+	return cancelledRemoval, err
 }
 
 // Shutdown shuts down the device by unmounting the root.

pkg/devicemapper/devmapper.go

@@ -750,51 +750,49 @@ func activateDevice(poolName string, name string, deviceID int, size uint64, ext
 	return nil
 }
 
-// CreateSnapDevice creates a snapshot based on the device identified by the baseName and baseDeviceId,
-func CreateSnapDevice(poolName string, deviceID int, baseName string, baseDeviceID int) error {
-	devinfo, _ := GetInfo(baseName)
-	doSuspend := devinfo != nil && devinfo.Exists != 0
-
-	if doSuspend {
-		if err := SuspendDevice(baseName); err != nil {
-			return err
-		}
-	}
-
+// CreateSnapDeviceRaw creates a snapshot device. Caller is assumed to resume and suspend the device.
+func CreateSnapDeviceRaw(poolName string, deviceID int, baseName string, baseDeviceID int) error {
 	task, err := TaskCreateNamed(deviceTargetMsg, poolName)
 	if task == nil {
-		if doSuspend {
-			ResumeDevice(baseName)
-		}
 		return err
 	}
 
 	if err := task.setSector(0); err != nil {
-		if doSuspend {
-			ResumeDevice(baseName)
-		}
 		return fmt.Errorf("devicemapper: Can't set sector %s", err)
 	}
 
 	if err := task.setMessage(fmt.Sprintf("create_snap %d %d", deviceID, baseDeviceID)); err != nil {
-		if doSuspend {
-			ResumeDevice(baseName)
-		}
 		return fmt.Errorf("devicemapper: Can't set message %s", err)
 	}
 
 	dmSawExist = false // reset before the task is run
 	if err := task.run(); err != nil {
-		if doSuspend {
-			ResumeDevice(baseName)
-		}
 		// Caller wants to know about ErrDeviceIDExists so that it can try with a different device id.
 		if dmSawExist {
 			return ErrDeviceIDExists
 		}
-
 		return fmt.Errorf("devicemapper: Error running deviceCreate (createSnapDevice) %s", err)
+	}
+
+	return nil
+}
+
+// CreateSnapDevice creates a snapshot based on the device identified by the baseName and baseDeviceId,
+func CreateSnapDevice(poolName string, deviceID int, baseName string, baseDeviceID int) error {
+	devinfo, _ := GetInfo(baseName)
+	doSuspend := devinfo != nil && devinfo.Exists != 0
 
+	if doSuspend {
+		if err := SuspendDevice(baseName); err != nil {
+			return err
+		}
+	}
+
+	if err := CreateSnapDeviceRaw(poolName, deviceID, baseName, baseDeviceID); err != nil {
+		if doSuspend {
+			ResumeDevice(baseName)
+		}
+		return err
 	}
 
 	if doSuspend {