libparted: avoid race in informing the kernel of partition table changes

[Jim Meyering]

Here are two patches.
The first is just renaming and clean-up.
The meaty 2nd one has the following long commit log.

--------------------------------------------------------------

libparted: avoid race in informing the kernel of partition table changes

When sync'ing a partition table change using the latest
code, sometimes we'd get an unwarranted failure like this:

    Warning: Partition(s) 1 on /dev/sdd have been written, but we
    have been unable to inform the kernel of the change, probably because
    it/they are in use.  As a result, the old partition(s) will remain in
    use.  You should reboot now before making further changes.

To be precise, when running the partition-resizing root-only test
in a loop:

    for i in $(seq 240); do make -C tests check VERBOSE=yes \
    TESTS=t3000-resize-fs.sh >& log.$i && printf . || echo $i $?; done

I would typically see about 50% of them fail on a Fedora 13 system.
It was obvious that this was due to a race condition when I found that
modifying that tests' parted...resize invocation to go via strace changed
the timing enough to make the test pass every time.

The fix is to retry the partition-removal step upon any EBUSY failure,
currently for up to 1 second (retrying up to 100 times, sleeping 10ms
after each failure).

* libparted/arch/linux.c (_disk_sync_part_table): Allocate "ok" using
calloc, now that its initial values matter.
Retry each removal upon EBUSY-failure.
* bootstrap.conf (gnulib_modules): Use gnulib's usleep module.



>From 9acc5869985c8616066af4825f52fb6c474f63dd Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Fri, 30 Apr 2010 12:01:08 +0200
Subject: [PATCH 1/2] libparted: variable renaming, minor "goto" reorg

* libparted/arch/linux.c (_disk_sync_part_table): Rename local array:
s/rets/ok/, for readability.
Use only a single label, "cleanup:", rather than two: free_rets
and free_errnums.
---
 libparted/arch/linux.c |   27 +++++++++++++--------------
 1 files changed, 13 insertions(+), 14 deletions(-)

diff --git a/libparted/arch/linux.c b/libparted/arch/linux.c
index d8f3393..8116c76 100644
--- a/libparted/arch/linux.c
+++ b/libparted/arch/linux.c
@@ -2452,29 +2452,29 @@ _disk_sync_part_table (PedDisk* disk)
         if (lpn < 0)
                 return 0;
         int ret = 0;
-        int *rets = ped_malloc(sizeof(int) * lpn);
-        if (!rets)
+        int *ok = ped_malloc(sizeof(int) * lpn);
+        if (!ok)
                 return 0;
         int *errnums = ped_malloc(sizeof(int) * lpn);
         if (!errnums)
-                goto free_rets;
+                goto cleanup;
         int i;

         for (i = 1; i <= lpn; i++) {
-                rets[i - 1] = _blkpg_remove_partition (disk, i);
+                ok[i - 1] = _blkpg_remove_partition (disk, i);
                 errnums[i - 1] = errno;
         }

         for (i = 1; i <= lpn; i++) {
                 const PedPartition *part = ped_disk_get_partition (disk, i);
                 if (part) {
-                        if (!rets[i - 1] && errnums[i - 1] == EBUSY) {
+                        if (!ok[i - 1] && errnums[i - 1] == EBUSY) {
                                 struct hd_geometry geom;
                                 unsigned long long length = 0;
                                 /* get start and length of existing partition 
*/
                                 char *dev_name = _device_get_part_path 
(disk->dev, i);
                                 if (!dev_name)
-                                        goto free_errnums;
+                                        goto cleanup;
                                 int fd = open (dev_name, O_RDONLY);
                                 if (fd == -1
                                    || ioctl (fd, HDIO_GETGEO, &geom)
@@ -2487,14 +2487,14 @@ _disk_sync_part_table (PedDisk* disk)
                                         if (fd != -1)
                                                 close (fd);
                                         free (dev_name);
-                                        goto free_errnums;
+                                        goto cleanup;
                                 }
                                 free (dev_name);
                                 length /= disk->dev->sector_size;
                                 close (fd);
                                 if (geom.start == part->geom.start
                                    && length == part->geom.length)
-                                        rets[i - 1] = 1;
+                                        ok[i - 1] = 1;
                                 /* If the new partition is unchanged and the
                                   existing one was not removed because it was
                                   in use, then reset the error flag and do not
@@ -2509,7 +2509,7 @@ _disk_sync_part_table (PedDisk* disk)
                                         PED_EXCEPTION_RETRY_CANCEL,
                                         _("Failed to add partition %d (%s)"),
                                         i, strerror (errno));
-                                goto free_errnums;
+                                goto cleanup;
                         }
                 }
         }
@@ -2517,12 +2517,12 @@ _disk_sync_part_table (PedDisk* disk)
         char *bad_part_list = NULL;
         /* now warn about any errors */
         for (i = 1; i <= lpn; i++) {
-               if (rets[i - 1] || errnums[i - 1] == ENXIO)
+               if (ok[i - 1] || errnums[i - 1] == ENXIO)
                        continue;
                if (bad_part_list == NULL) {
                          bad_part_list = malloc (lpn * 5);
                          if (!bad_part_list)
-                                 goto free_errnums;
+                                 goto cleanup;
                          bad_part_list[0] = 0;
                }
                sprintf (bad_part_list + strlen (bad_part_list), "%d, ", i);
@@ -2543,10 +2543,9 @@ _disk_sync_part_table (PedDisk* disk)
                         ret = 1;
                free (bad_part_list);
         }
- free_errnums:
+ cleanup:
         free (errnums);
- free_rets:
-        free (rets);
+        free (ok);
         return ret;
 }

--
1.7.1.328.g9993c


>From 0f850220b3f26bb969a1a7ff78dc550691a89566 Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Fri, 30 Apr 2010 12:28:16 +0200
Subject: [PATCH 2/2] libparted: avoid race in informing the kernel of partition 
table changes

When sync'ing a partition table change using the latest
code, sometimes we'd get an unwarranted failure like this:

    Warning: Partition(s) 1 on /dev/sdd have been written, but we
    have been unable to inform the kernel of the change, probably because
    it/they are in use.  As a result, the old partition(s) will remain in
    use.  You should reboot now before making further changes.

To be precise, when running the partition-resizing root-only test
in a loop:

    for i in $(seq 240); do make -C tests check VERBOSE=yes \
    TESTS=t3000-resize-fs.sh >& log.$i && printf . || echo $i $?; done

I would typically see about 50% of them fail on a Fedora 13 system.
It was obvious that this was due to a race condition when I found that
modifying that tests' parted...resize invocation to go via strace changed
the timing enough to make the test pass every time.

The fix is to retry the partition-removal step upon any EBUSY failure,
currently for up to 1 second (retrying up to 100 times, sleeping 10ms
after each failure).

* libparted/arch/linux.c (_disk_sync_part_table): Allocate "ok" using
calloc, now that its initial values matter.
Retry each removal upon EBUSY-failure.
* bootstrap.conf (gnulib_modules): Use gnulib's usleep module.
---
 bootstrap.conf         |    1 +
 libparted/arch/linux.c |   28 +++++++++++++++++++++++-----
 2 files changed, 24 insertions(+), 5 deletions(-)

diff --git a/bootstrap.conf b/bootstrap.conf
index 6c9287d..4ca51a7 100644
--- a/bootstrap.conf
+++ b/bootstrap.conf
@@ -58,6 +58,7 @@ gnulib_modules="
        unlink
        update-copyright
        useless-if-before-free
+       usleep
        vc-list-files
        version-etc-fsf
        warnings
diff --git a/libparted/arch/linux.c b/libparted/arch/linux.c
index 8116c76..73a8e6f 100644
--- a/libparted/arch/linux.c
+++ b/libparted/arch/linux.c
@@ -2452,17 +2452,35 @@ _disk_sync_part_table (PedDisk* disk)
         if (lpn < 0)
                 return 0;
         int ret = 0;
-        int *ok = ped_malloc(sizeof(int) * lpn);
+        int *ok = calloc (lpn, sizeof *ok);
         if (!ok)
                 return 0;
         int *errnums = ped_malloc(sizeof(int) * lpn);
         if (!errnums)
                 goto cleanup;
-        int i;

-        for (i = 1; i <= lpn; i++) {
-                ok[i - 1] = _blkpg_remove_partition (disk, i);
-                errnums[i - 1] = errno;
+        /* Attempt to remove each and every partition, retrying for
+           up to max_sleep_seconds upon any failure due to EBUSY. */
+        unsigned int sleep_microseconds = 10000;
+        unsigned int max_sleep_seconds = 1;
+        unsigned int n_sleep = (max_sleep_seconds
+                                * 1000000 / sleep_microseconds);
+        int i;
+        for (i = 0; i < n_sleep; i++) {
+           if (i)
+               usleep (sleep_microseconds);
+            bool busy = false;
+            int j;
+            for (j = 0; j < lpn; j++) {
+                if (!ok[j]) {
+                    ok[j] = _blkpg_remove_partition (disk, j + 1);
+                    errnums[j] = errno;
+                    if (!ok[j] && errnums[j] == EBUSY)
+                        busy = true;
+                }
+            }
+            if (!busy)
+                break;
         }

         for (i = 1; i <= lpn; i++) {
--
1.7.1.328.g9993c