Retry dm device removal if busy

PATCH New Peter Rajnoha 5 participants 6 messages Sep 13, 2011 – Sep 14, 2011

Share & tools

Peter Rajnoha

author

2011-09-13 14:31 UTC #0 lore raw

PATCH 1/1 Retry dm device removal if busy

If a dm device is being opened in parallel while we're trying to remove it, we'll end up with an error that the device is busy. This is a legitimate error, but with udev in play and asynchronous events generated as a result of using the WATCH udev rule, we can get into a situation where such failure is very unpleasant.

Let's try the removal a few times then. Though this is not a complete solution to the problem, let's use this until we have one.

Peter

---
 libdm/ioctl/libdm-iface.c |   15 +++++++++++++++
 1 files changed, 15 insertions(+), 0 deletions(-)

libdm/ioctl/libdm-iface.c +15 -0

index 816f1e6..e4a3a1f 100644
@@ -1539,11 +1539,14 @@ static const char *_sanitise_message(char *message)
 	return sanitised_message;
 }
 
+#define DM_REMOVE_IOCTL_RETRIES 5
+
 static struct dm_ioctl *_do_dm_ioctl(struct dm_task *dmt, unsigned command,
 				     unsigned repeat_count)
 {
 	struct dm_ioctl *dmi;
 	int ioctl_with_uevent;
+	int retries = DM_REMOVE_IOCTL_RETRIES;
 
 	dmi = _flatten(dmt, repeat_count);
 	if (!dmi) {

@@ -1627,11 +1630,23 @@ static struct dm_ioctl *_do_dm_ioctl(struct dm_task *dmt, unsigned command,
 		  dmt->sector, _sanitise_message(dmt->message),
 		  dmi->data_size);
 #ifdef DM_IOCTLS
+repeat_dm_ioctl:
 	if (ioctl(_control_fd, command, dmi) < 0) {
 		if (errno == ENXIO && ((dmt->type == DM_DEVICE_INFO) ||
 				       (dmt->type == DM_DEVICE_MKNODES) ||
 				       (dmt->type == DM_DEVICE_STATUS)))
 			dmi->flags &= ~DM_EXISTS_FLAG;	/* FIXME */
+		/*
+		 * FIXME: This is a workaround for asynchronous events generated
+		 *        as a result of using the WATCH udev rule with which we
+		 *        have no way of synchronizing. Processing such events in
+		 *        parallel causes devices to be open.
+		 */
+		else if (errno == EBUSY && (dmt->type == DM_DEVICE_REMOVE) && retries--) {
+			log_debug("device-mapper: device is busy, retrying removal");
+			sleep(1);
+			goto repeat_dm_ioctl;
+		}
 		else {
 			if (_log_suppress)
 				log_verbose("device-mapper: %s ioctl "

Milan Broz

2011-09-13 14:55 UTC #1 lore raw

PATCH 1/1 Retry dm device removal if busy

Quoted 10 lines · Peter Rajnoha

If a dm device is being opened in parallel while we're trying to remove it, we'll end up with an error that the device is busy. This is a legitimate error, but with udev in play and asynchronous events generated as a result of using the WATCH udev rule, we can get into a situation where such failure is very unpleasant.
Let's try the removal a few times then. Though this is not a complete solution to the problem, let's use this until we have one.

Before anyone start to complain:

we spent incredible amount of time with fixing the unfixable,

I really prefer make it to work despite it is not 100% correct. It is ugly but it will allow us to fix another things and not waste time here for now.

This approach is already used in cryptsetup, mdadm and perhaps other similar packages.

ACK for me (in principle), just with different time (see below).

(please keep this as internal workaround, no lvm.conf bloat etc. Users are already very very confused by trillion switches.)

libdm/ioctl/libdm-iface.c static const char *_sanitise_message(char *message) · line 1542

Quoted 9 lines

libdm/ioctl/libdm-iface.c +1 -0

index 816f1e6..e4a3a1f 100644
@@ -1539,11 +1539,14 @@ static const char *_sanitise_message(char *message)
 	return sanitised_message;
 }
 
+#define DM_REMOVE_IOCTL_RETRIES 5

I would use the same as array stop for mdadm : 25 x usleep(200000).

#define DM_REMOVE_IOCTL_RETRIES 25

maybe lower that to 10 ?

Quoted 19 lines

libdm/ioctl/libdm-iface.c +3 -0

+
 static struct dm_ioctl *_do_dm_ioctl(struct dm_task *dmt, unsigned command,
 				     unsigned repeat_count)
 {
 	struct dm_ioctl *dmi;
 	int ioctl_with_uevent;
+	int retries = DM_REMOVE_IOCTL_RETRIES;
 
 	dmi = _flatten(dmt, repeat_count);
 	if (!dmi) {

@@ -1627,11 +1630,23 @@ static struct dm_ioctl *_do_dm_ioctl(struct dm_task *dmt, unsigned command,
 		  dmt->sector, _sanitise_message(dmt->message),
 		  dmi->data_size);
 #ifdef DM_IOCTLS
+repeat_dm_ioctl:
 	if (ioctl(_control_fd, command, dmi) < 0) {
 		if (errno == ENXIO && ((dmt->type == DM_DEVICE_INFO) ||
 				       (dmt->type == DM_DEVICE_MKNODES) ||
 				       (dmt->type == DM_DEVICE_STATUS)))

libdm/ioctl/libdm-iface.c static struct dm_ioctl *_do_dm_ioctl(struct dm_task *dmt, unsigned command, · line 1647

Quoted 10 lines

libdm/ioctl/libdm-iface.c +9 -0

 			dmi->flags &= ~DM_EXISTS_FLAG;	/* FIXME */
+		/*
+		 * FIXME: This is a workaround for asynchronous events generated
+		 *        as a result of using the WATCH udev rule with which we
+		 *        have no way of synchronizing. Processing such events in
+		 *        parallel causes devices to be open.
+		 */
+		else if (errno == EBUSY && (dmt->type == DM_DEVICE_REMOVE) && retries--) {
+			log_debug("device-mapper: device is busy, retrying removal");
+			sleep(1);

usleep(200000);

It would be good to not retry if we are sure that it is not transient use of device. Not sure if it is possible here though.

libdm/ioctl/libdm-iface.c static struct dm_ioctl *_do_dm_ioctl(struct dm_task *dmt, unsigned command, · line 1652

Quoted 5 lines

libdm/ioctl/libdm-iface.c +2 -0

+			goto repeat_dm_ioctl;
+		}
 		else {
 			if (_log_suppress)
 				log_verbose("device-mapper: %s ioctl "

Milan

Joe Thornber

2011-09-13 15:01 UTC #2 lore raw

PATCH 1/1 Retry dm device removal if busy

Quoted 10 lines · Milan Broz

Before anyone start to complain:
we spent incredible amount of time with fixing the unfixable,
I really prefer make it to work despite it is not 100% correct. It is ugly but it will allow us to fix another things and not waste time here for now.
This approach is already used in cryptsetup, mdadm and perhaps other similar packages.

I've resorted to doing exactly this in the thin test suite.

- Joe

Jonathan Brassow

2011-09-13 18:29 UTC #3 lore raw

PATCH 1/1 Retry dm device removal if busy

In reply to Peter Rajnoha

Yes please!

All to often we come across this issue. There are a number of bugs that are likely fixed by doing this change - even if it isn't academically perfect.

brassow

Quoted 7 lines · Peter Rajnoha

If a dm device is being opened in parallel while we're trying to remove it, we'll end up with an error that the device is busy. This is a legitimate error, but with udev in play and asynchronous events generated as a result of using the WATCH udev rule, we can get into a situation where such failure is very unpleasant.

Quoted 5 lines

Let's try the removal a few times then. Though this is not a complete solution to the problem, let's use this until we have one.
Peter

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/lvm-devel/attachments/20110913/f1201b63/attachment.htm>

Zdenek Kabelac

2011-09-14 12:14 UTC #4 lore raw

PATCH 1/1 Retry dm device removal if busy

Dne 13.9.2011 20:29, Jonathan Brassow napsal(a):

Quoted 4 lines

Yes please!
All to often we come across this issue. There are a number of bugs that are likely fixed by doing this change - even if it isn't academically perfect.

Workaround is for 'external' tools cooperating with lvm. lvm itself should work well without this hack.

(Thus I believe Petr should add lvm.conf option - and for buildbot this hack should be disabled - so the internal bugs are clearly visible.)

Zdenek

Quoted 16 lines

brassow
Quoted 12 lines · Peter Rajnoha
If a dm device is being opened in parallel while we're trying to remove it, we'll end up with an error that the device is busy. This is a legitimate error, but with udev in play and asynchronous events generated as a result of using the WATCH udev rule, we can get into a situation where such failure is very unpleasant.
Let's try the removal a few times then. Though this is not a complete solution to the problem, let's use this until we have one.
Peter

Milan Broz

2011-09-14 12:26 UTC #5 lore raw

PATCH 1/1 Retry dm device removal if busy

Quoted 2 lines · Zdenek Kabelac

(Thus I believe Petr should add lvm.conf option - and for buildbot this hack should be disabled - so the internal bugs are clearly visible.)

Builbot is just another user, I do not think we should test it without flag. What it will prove? That udisks is still using watch? it does, really :-)

I have to always manually remove udev rule (breaking other things), it is not dedicated machine.

Milan