Make descriptor lock per pod and release it per pod

This change is needed to avoid unmapVolumeFunc for one pod blocked,
when the lock for the same volume is taken for another pod.

pkg/volume/testing/testing.go

@@ -1178,16 +1178,16 @@ func (fv *FakeVolumePathHandler) AttachFileDevice(path string) (string, error) {
 	return "", nil
 }
 
+func (fv *FakeVolumePathHandler) DetachFileDevice(path string) error {
+	// nil is success, else error
+	return nil
+}
+
 func (fv *FakeVolumePathHandler) GetLoopDevice(path string) (string, error) {
 	// nil is success, else error
 	return "/dev/loop1", nil
 }
 
-func (fv *FakeVolumePathHandler) RemoveLoopDevice(device string) error {
-	// nil is success, else error
-	return nil
-}
-
 // FindEmptyDirectoryUsageOnTmpfs finds the expected usage of an empty directory existing on
 // a tmpfs filesystem on this system.
 func FindEmptyDirectoryUsageOnTmpfs() (*resource.Quantity, error) {

pkg/volume/util/operationexecutor/operation_generator.go

@@ -1096,7 +1096,7 @@ func (og *operationGenerator) GenerateMapVolumeFunc(
 		// that the block device is silently removed and attached another device with same name.
 		// Container runtime can't handler this problem. To avoid unexpected condition fd lock
 		// for the block device is required.
-		_, err = og.blkUtil.AttachFileDevice(devicePath)
+		_, err = og.blkUtil.AttachFileDevice(filepath.Join(globalMapPath, string(volumeToMount.Pod.UID)))
 		if err != nil {
 			return volumeToMount.GenerateError("MapVolume.AttachFileDevice failed", err)
 		}
@@ -1207,6 +1207,14 @@ func (og *operationGenerator) GenerateUnmapVolumeFunc(
 	}
 
 	unmapVolumeFunc := func() (error, error) {
+		globalUnmapPath := volumeToUnmount.DeviceMountPath
+
+		// Release file descriptor lock.
+		err := og.blkUtil.DetachFileDevice(filepath.Join(globalUnmapPath, string(volumeToUnmount.PodUID)))
+		if err != nil {
+			return volumeToUnmount.GenerateError("UnmapVolume.UnmapDevice Detaching descriptor lock failed", err)
+		}
+
 		// Try to unmap volumeName symlink under pod device map path dir
 		// pods/{podUid}/volumeDevices/{escapeQualifiedPluginName}/{volumeName}
 		podDeviceUnmapPath, volName := blockVolumeUnmapper.GetPodDeviceMapPath()
@@ -1217,7 +1225,6 @@ func (og *operationGenerator) GenerateUnmapVolumeFunc(
 		}
 		// Try to unmap podUID symlink under global map path dir
 		// plugins/kubernetes.io/{PluginName}/volumeDevices/{volumePluginDependentPath}/{podUID}
-		globalUnmapPath := volumeToUnmount.DeviceMountPath
 		unmapDeviceErr = og.blkUtil.UnmapDevice(globalUnmapPath, string(volumeToUnmount.PodUID), true /* bindMount */)
 		if unmapDeviceErr != nil {
 			// On failure, return error. Caller will log and retry.
@@ -1315,27 +1322,6 @@ func (og *operationGenerator) GenerateUnmapDeviceFunc(
 			return deviceToDetach.GenerateError("UnmapDevice failed", err)
 		}
 
-		// The block volume is not referenced from Pods. Release file descriptor lock.
-		// This should be done before calling TearDownDevice, because some plugins that do local detach
-		// in TearDownDevice will fail in detaching device due to the refcnt on the loopback device.
-		klog.V(4).Infof("UnmapDevice: deviceToDetach.DevicePath: %v", deviceToDetach.DevicePath)
-		loopPath, err := og.blkUtil.GetLoopDevice(deviceToDetach.DevicePath)
-		if err != nil {
-			if err.Error() == volumepathhandler.ErrDeviceNotFound {
-				klog.Warningf(deviceToDetach.GenerateMsgDetailed("UnmapDevice: Couldn't find loopback device which takes file descriptor lock", fmt.Sprintf("device path: %q", deviceToDetach.DevicePath)))
-			} else {
-				errInfo := "UnmapDevice.GetLoopDevice failed to get loopback device, " + fmt.Sprintf("device path: %q", deviceToDetach.DevicePath)
-				return deviceToDetach.GenerateError(errInfo, err)
-			}
-		} else {
-			if len(loopPath) != 0 {
-				err = og.blkUtil.RemoveLoopDevice(loopPath)
-				if err != nil {
-					return deviceToDetach.GenerateError("UnmapDevice.RemoveLoopDevice failed", err)
-				}
-			}
-		}
-
 		// Execute tear down device
 		unmapErr := blockVolumeUnmapper.TearDownDevice(globalMapPath, deviceToDetach.DevicePath)
 		if unmapErr != nil {

pkg/volume/util/volumepathhandler/volume_path_handler.go

@@ -55,10 +55,11 @@ type BlockVolumePathHandler interface {
 	// AttachFileDevice takes a path to a regular file and makes it available as an
 	// attached block device.
 	AttachFileDevice(path string) (string, error)
+	// DetachFileDevice takes a path to the attached block device and
+	// detach it from block device.
+	DetachFileDevice(path string) error
 	// GetLoopDevice returns the full path to the loop device associated with the given path.
 	GetLoopDevice(path string) (string, error)
-	// RemoveLoopDevice removes specified loopback device
-	RemoveLoopDevice(device string) error
 }
 
 // NewBlockVolumePathHandler returns a new instance of BlockVolumeHandler.

pkg/volume/util/volumepathhandler/volume_path_handler_linux.go

@@ -50,6 +50,27 @@ func (v VolumePathHandler) AttachFileDevice(path string) (string, error) {
 	return blockDevicePath, nil
 }
 
+// DetachFileDevice takes a path to the attached block device and
+// detach it from block device.
+func (v VolumePathHandler) DetachFileDevice(path string) error {
+	loopPath, err := v.GetLoopDevice(path)
+	if err != nil {
+		if err.Error() == ErrDeviceNotFound {
+			klog.Warningf("DetachFileDevice: Couldn't find loopback device which takes file descriptor lock. device path: %q", path)
+		} else {
+			return err
+		}
+	} else {
+		if len(loopPath) != 0 {
+			err = removeLoopDevice(loopPath)
+			if err != nil {
+				return err
+			}
+		}
+	}
+	return nil
+}
+
 // GetLoopDevice returns the full path to the loop device associated with the given path.
 func (v VolumePathHandler) GetLoopDevice(path string) (string, error) {
 	_, err := os.Stat(path)
@@ -88,8 +109,8 @@ func makeLoopDevice(path string) (string, error) {
 	return strings.TrimSpace(string(out)), nil
 }
 
-// RemoveLoopDevice removes specified loopback device
-func (v VolumePathHandler) RemoveLoopDevice(device string) error {
+// removeLoopDevice removes specified loopback device
+func removeLoopDevice(device string) error {
 	args := []string{"-d", device}
 	cmd := exec.Command(losetupPath, args...)
 	out, err := cmd.CombinedOutput()

pkg/volume/util/volumepathhandler/volume_path_handler_unsupported.go

@@ -30,16 +30,17 @@ func (v VolumePathHandler) AttachFileDevice(path string) (string, error) {
 	return "", fmt.Errorf("AttachFileDevice not supported for this build.")
 }
 
+// DetachFileDevice takes a path to the attached block device and
+// detach it from block device.
+func (v VolumePathHandler) DetachFileDevice(path string) error {
+	return fmt.Errorf("DetachFileDevice not supported for this build.")
+}
+
 // GetLoopDevice returns the full path to the loop device associated with the given path.
 func (v VolumePathHandler) GetLoopDevice(path string) (string, error) {
 	return "", fmt.Errorf("GetLoopDevice not supported for this build.")
 }
 
-// RemoveLoopDevice removes specified loopback device
-func (v VolumePathHandler) RemoveLoopDevice(device string) error {
-	return fmt.Errorf("RemoveLoopDevice not supported for this build.")
-}
-
 // FindGlobalMapPathUUIDFromPod finds {pod uuid} bind mount under globalMapPath
 // corresponding to map path symlink, and then return global map path with pod uuid.
 func (v VolumePathHandler) FindGlobalMapPathUUIDFromPod(pluginDir, mapPath string, podUID types.UID) (string, error) {