parted and libdevmapper's udev_sync feature

[Peter Rajnoha]

Hi,

(resending here to make it public)

I've noticed that parted does not use the udev synchronisation
feature ("udev_sync") when dealing with DM mappings. It would be
fine to add this to prevent any future udev synchronisation
problems (e.g. trying to remove a mapping that is still processed
by udev rules).

Also, when using udev_sync, you'll have your /dev/mapper nodes
managed by udev (but you can turn it off with a dedicated flag
if you want).

The basic interface for udev_sync is:

  dm_udev_set_sync_support - enables/disables the udev_sync feature

  dm_task_set_cookie - sets the udev synchronisation cookie (opens
                       a semaphore waiting for completion of udev
                       processing in udev rules)

  dm_udev_wait - waits for the notification from udev rules that the
                 processing is done and resumes the process, closes
                 the semaphore (dm_udev_wait needs to be called on
                 any error path as well, so the semaphore won't stay
                 in the system)

This should be applied for any DM_DEVICE_CREATE (with table provided!),
DM_DEVICE_RENAME, DM_DEVICE_RESUME and DM_DEVICE_REMOVE ioctl.

So something like this:

 - somewhere at program init:
     dm_udev_set_sync_support(1);

 - then for each ioctl:
     uint32_t cookie = 0;
     ...
     dm_task_set_cookie(task, &cookie, udev_flags);
     dm_task_run(task);
     dm_udev_wait(cookie);
     ...

Where "udev_flags" in dm_task_set_cookie can be set by using the
flags defined in libdevmapper.h - DM_UDEV_*_FLAG.

Thing is that you need recent enough libdevmapper for including
this (libdevmapper v1.02.39 and higher). But maybe we could even
support old libdevmapper (e.g. like crypsetup does - by defining
simple dummy functions) so we can avoid setting a tight dependency
on libdevmapper version... Also, we could possibly make it
configurable to switch the DM udev synchronisation on demand in
libparted? It's up to you. Any comments are welcome. The patch
below is just an example - the change would be quite
straightforward...

(please, add me to cc in any replies, I'm not subscribed to the list)

Thanks

Peter
---
 libparted/arch/linux.c |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/libparted/arch/linux.c b/libparted/arch/linux.c
index b1f7dc9..0a02394 100644
--- a/libparted/arch/linux.c
+++ b/libparted/arch/linux.c
@@ -2582,15 +2582,20 @@ _dm_remove_map_name(char *name)
 {
         struct dm_task  *task = NULL;
         int             rc;
+        uint32_t        cookie = 0;
 
         task = dm_task_create(DM_DEVICE_REMOVE);
         if (!task)
                 return 1;
 
         dm_task_set_name (task, name);
+        if (!dm_task_set_cookie(task, &cookie, 0))
+                goto bad;
 
         rc = dm_task_run(task);
+        dm_udev_wait(cookie);
         dm_task_update_nodes();
+bad:
         dm_task_destroy(task);
         if (!rc)
                 return 1;
@@ -2701,6 +2706,7 @@ _dm_add_partition (PedDisk* disk, PedPartition* part)
         const char*     dev_name = NULL;
         char*           params = NULL;
         LinuxSpecific*  arch_specific = LINUX_SPECIFIC (disk->dev);
+        uint32_t        cookie = 0;
 
         if (!_has_partitions(disk))
                 return 0;
@@ -2737,14 +2743,21 @@ _dm_add_partition (PedDisk* disk, PedPartition* part)
         dm_task_set_name (task, vol_name);
         dm_task_add_target (task, 0, part->geom.length,
                 "linear", params);
+        if (!dm_task_set_cookie(task, &cookie, 0))
+                goto err;
         if (dm_task_run (task)) {
                 //printf("0 %ld linear %s\n", part->geom.length, params);
+                dm_udev_wait(cookie);
                 dm_task_update_nodes();
                 dm_task_destroy(task);
                 free(params);
                 free(vol_name);
                 return 1;
         } else {
+                /* Though dm_udev_wait will not wait for any udev processing
+                 * here on the error path, we still need to call it to clean
+                 * up the synchronisation entity! */
+                dm_udev_wait(cookie);
                 _dm_remove_map_name(vol_name);
         }
 err: