discover/devmapper: Retry dm-device remove if busy
diff mbox series

Message ID 20181204023450.28142-1-sam@mendozajonas.com
State Accepted
Headers show
Series
  • discover/devmapper: Retry dm-device remove if busy
Related show

Commit Message

Samuel Mendoza-Jonas Dec. 4, 2018, 2:34 a.m. UTC
Buildroot's libdm is not built with --enable-udev_sync, so device-mapper
actions are not able to sync or wait for udev events.
(see 185676316, "discover/devmapper: Disable libdm udev sync support")

This can cause an issue when tearing down a snapshot in
devmapper_destroy_snapshot() which performs a DM_DEVICE_REMOVE task
against the snapshot, origin, and base devices one after the other. In
some cases if the interval between these actions is too short the action
can fail as the preceding device hasn't disappeared yet and the device
being removed is still busy.

Since we don't yet have a way to tell exactly when the device is ready,
pause for a short time and retry the action, letting
devmapper_destroy_snapshot() continue and, for example, letting
mount_device() fall back to the physical device.

Signed-off-by: Samuel Mendoza-Jonas <sam@mendozajonas.com>
---
In the future we'll enable udev_sync in Buildroot but this should help
us fallback properly on existing systems. Using usleep() isn't perfect,
however it's similar to what existing lvm tools & libraries do in the
same situation, and allows us to fall back to the physical device
quickly.

 discover/devmapper.c | 24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

Comments

Samuel Mendoza-Jonas Dec. 13, 2018, 12:51 a.m. UTC | #1
On Tue, 2018-12-04 at 13:34 +1100, Samuel Mendoza-Jonas wrote:
> Buildroot's libdm is not built with --enable-udev_sync, so device-mapper
> actions are not able to sync or wait for udev events.
> (see 185676316, "discover/devmapper: Disable libdm udev sync support")
> 
> This can cause an issue when tearing down a snapshot in
> devmapper_destroy_snapshot() which performs a DM_DEVICE_REMOVE task
> against the snapshot, origin, and base devices one after the other. In
> some cases if the interval between these actions is too short the action
> can fail as the preceding device hasn't disappeared yet and the device
> being removed is still busy.
> 
> Since we don't yet have a way to tell exactly when the device is ready,
> pause for a short time and retry the action, letting
> devmapper_destroy_snapshot() continue and, for example, letting
> mount_device() fall back to the physical device.
> 
> Signed-off-by: Samuel Mendoza-Jonas <sam@mendozajonas.com>

Merged as 89cde533

> ---
> In the future we'll enable udev_sync in Buildroot but this should help
> us fallback properly on existing systems. Using usleep() isn't perfect,
> however it's similar to what existing lvm tools & libraries do in the
> same situation, and allows us to fall back to the physical device
> quickly.
> 
>  discover/devmapper.c | 24 +++++++++++++++++++-----
>  1 file changed, 19 insertions(+), 5 deletions(-)
> 
> diff --git a/discover/devmapper.c b/discover/devmapper.c
> index f7407b77..0a4d320e 100644
> --- a/discover/devmapper.c
> +++ b/discover/devmapper.c
> @@ -457,24 +457,32 @@ static int destroy_device(const char *dm_name)
>  {
>  	struct dm_task *task;
>  	uint32_t cookie;
> -	int rc = -1;
> +	int rc, retries = 0;
>  
> +retry:
>  	task = dm_task_create(DM_DEVICE_REMOVE);
>  	if (!task) {
> -		pb_log_fn("could not create dm_task\n");
> +		pb_debug_fn("could not create dm_task\n");
>  		return -1;
>  	}
>  
>  	if (!dm_task_set_name(task, dm_name)) {
> -		pb_log("No dm device named '%s'\n", dm_name);
> +		rc = dm_task_get_errno(task);
> +		pb_debug_fn("Couldn't set name for %s, %s (%d)\n",
> +				dm_name, strerror(rc), rc);
>  		goto out;
>  	}
>  
> -	if (!set_cookie(task, &cookie))
> +	if (!set_cookie(task, &cookie)) {
> +		rc = dm_task_get_errno(task);
> +		pb_debug_fn("set_cookie failed, %s (%d)\n", strerror(rc), rc);
>  		goto out;
> +	}
>  
>  	if (!dm_task_run(task)) {
> -		pb_log("Unable to remove device '%s'\n", dm_name);
> +		rc = dm_task_get_errno(task);
> +		pb_debug_fn("Unable to remove device '%s', %s (%d)\n",
> +				dm_name, strerror(rc), rc);
>  		goto out;
>  	}
>  
> @@ -485,6 +493,12 @@ static int destroy_device(const char *dm_name)
>  
>  out:
>  	dm_task_destroy(task);
> +	if (rc == EBUSY && retries < 5) {
> +		pb_log_fn("Device busy, retry %d..\n", retries);
> +		usleep(100000);
> +		retries++;
> +		goto retry;
> +	}
>  	return rc;
>  }
>

Patch
diff mbox series

diff --git a/discover/devmapper.c b/discover/devmapper.c
index f7407b77..0a4d320e 100644
--- a/discover/devmapper.c
+++ b/discover/devmapper.c
@@ -457,24 +457,32 @@  static int destroy_device(const char *dm_name)
 {
 	struct dm_task *task;
 	uint32_t cookie;
-	int rc = -1;
+	int rc, retries = 0;
 
+retry:
 	task = dm_task_create(DM_DEVICE_REMOVE);
 	if (!task) {
-		pb_log_fn("could not create dm_task\n");
+		pb_debug_fn("could not create dm_task\n");
 		return -1;
 	}
 
 	if (!dm_task_set_name(task, dm_name)) {
-		pb_log("No dm device named '%s'\n", dm_name);
+		rc = dm_task_get_errno(task);
+		pb_debug_fn("Couldn't set name for %s, %s (%d)\n",
+				dm_name, strerror(rc), rc);
 		goto out;
 	}
 
-	if (!set_cookie(task, &cookie))
+	if (!set_cookie(task, &cookie)) {
+		rc = dm_task_get_errno(task);
+		pb_debug_fn("set_cookie failed, %s (%d)\n", strerror(rc), rc);
 		goto out;
+	}
 
 	if (!dm_task_run(task)) {
-		pb_log("Unable to remove device '%s'\n", dm_name);
+		rc = dm_task_get_errno(task);
+		pb_debug_fn("Unable to remove device '%s', %s (%d)\n",
+				dm_name, strerror(rc), rc);
 		goto out;
 	}
 
@@ -485,6 +493,12 @@  static int destroy_device(const char *dm_name)
 
 out:
 	dm_task_destroy(task);
+	if (rc == EBUSY && retries < 5) {
+		pb_log_fn("Device busy, retry %d..\n", retries);
+		usleep(100000);
+		retries++;
+		goto retry;
+	}
 	return rc;
 }