[PATCH 1/2] Revert "daemon: Run udev_settle after pwrite-device finishes."
From: "Richard W.M. Jones" <rjones@redhat.com> This reverts commit a9c8123c72db47bcab8dd738e8d5256a9ae87f11. --- daemon/file.c | 18 +++--------------- daemon/parted.c | 3 +-- 2 files changed, 4 insertions(+), 17 deletions(-) diff --git a/daemon/file.c b/daemon/file.c index 057e15d..91746e0 100644 --- a/daemon/file.c +++ b/daemon/file.c @@ -525,7 +525,7 @@ do_pread_device (const char *device, int count, int64_t offset, size_t *size_r) static int pwrite_fd (int fd, const char *content, size_t size, int64_t offset, - const char *display_path, int settle) + const char *display_path) { ssize_t r; @@ -541,18 +541,6 @@ pwrite_fd (int fd, const char *content, size_t size, int64_t offset, return -1; } - /* When you call close on any block device, udev kicks off a rule - * which runs blkid to reexamine the device. We need to wait for - * this rule to finish running since it holds the device open and - * can cause other operations to fail, notably BLKRRPART. 'settle' - * flag is only set on block devices. - * - * XXX We should be smarter about when we do this or should get rid - * of the udev rules since we don't use blkid in cached mode. - */ - if (settle) - udev_settle (); - return r; } @@ -575,7 +563,7 @@ do_pwrite (const char *path, const char *content, size_t size, int64_t offset) return -1; } - return pwrite_fd (fd, content, size, offset, path, 0); + return pwrite_fd (fd, content, size, offset, path); } int @@ -593,7 +581,7 @@ do_pwrite_device (const char *device, const char *content, size_t size, return -1; } - return pwrite_fd (fd, content, size, offset, device, 1); + return pwrite_fd (fd, content, size, offset, device); } /* This runs the 'file' command. */ diff --git a/daemon/parted.c b/daemon/parted.c index 64a7d1d..16f0843 100644 --- a/daemon/parted.c +++ b/daemon/parted.c @@ -34,8 +34,7 @@ * COMMAND_FLAG_FOLD_STDOUT_ON_STDERR flag. * * parted occasionally fails to do ioctl(BLKRRPART) on the device, - * probably because udev monitors all 'close' on block devices - * and runs 'blkid' which opens and examines the device. We attempt + * apparently because of some internal race in the code. We attempt * to detect and recover from this error if we can. */ static int -- 1.7.6
From: "Richard W.M. Jones" <rjones@redhat.com> See comments in the code for details. This is an alternate fix to commit a9c8123c72db47bcab8dd738e8d5256a9ae87f11. --- daemon/blockdev.c | 14 +++++ daemon/parted.c | 153 +++++++++++++++++++++++++++++++++++------------------ 2 files changed, 115 insertions(+), 52 deletions(-) diff --git a/daemon/blockdev.c b/daemon/blockdev.c index d056abe..a7fd2cb 100644 --- a/daemon/blockdev.c +++ b/daemon/blockdev.c @@ -46,6 +46,20 @@ call_blockdev (const char *device, const char *switc, int extraarg, int prints) }; char buf[64]; + /* When you call close on any block device, udev kicks off a rule + * which runs blkid to reexamine the device. We need to wait for + * this rule to finish running (from a previous operation) since it + * holds the device open and can cause other operations to fail, + * notably BLKRRPART. + * + * This is particularly a problem where we have just written to a + * device (eg. zeroing it) and immediately call blockdev --rereadpt. + * + * Therefore, wait for udev to finish all outstanding events before + * performing any blockdev command. + */ + udev_settle (); + if (extraarg > 0) { snprintf (buf, sizeof buf, "%d", extraarg); argv[2] = buf; diff --git a/daemon/parted.c b/daemon/parted.c index 16f0843..2610042 100644 --- a/daemon/parted.c +++ b/daemon/parted.c @@ -33,45 +33,14 @@ * Parted 1.9 sends error messages to stdout, hence use of the * COMMAND_FLAG_FOLD_STDOUT_ON_STDERR flag. * - * parted occasionally fails to do ioctl(BLKRRPART) on the device, - * apparently because of some internal race in the code. We attempt - * to detect and recover from this error if we can. + * There is a reason why we call udev_settle both before and after + * each command. When you call close on any block device, udev kicks + * off a rule which runs blkid to reexamine the device. We need to + * wait for this rule to finish running (from a previous operation) + * since it holds the device open. Since parted also closes the block + * device, it can cause udev to run again, hence the call to + * udev_settle afterwards. */ -static int -recover_blkrrpart (const char *device, const char *err) -{ - int r; - - if (!strstr (err, - "Error informing the kernel about modifications to partition")) - return -1; - - r = command (NULL, NULL, "blockdev", "--rereadpt", device, NULL); - if (r == -1) - return -1; - - udev_settle (); - - return 0; -} - -#define RUN_PARTED(error,device,...) \ - do { \ - int r; \ - char *err; \ - \ - r = commandf (NULL, &err, COMMAND_FLAG_FOLD_STDOUT_ON_STDERR, \ - "parted", "-s", "--", (device), __VA_ARGS__); \ - if (r == -1) { \ - if (recover_blkrrpart ((device), err) == -1) { \ - reply_with_error ("%s: parted: %s: %s", __func__, (device), err); \ - free (err); \ - error; \ - } \ - } \ - \ - free (err); \ - } while (0) static const char * check_parttype (const char *parttype) @@ -101,13 +70,25 @@ check_parttype (const char *parttype) int do_part_init (const char *device, const char *parttype) { + int r; + char *err; + parttype = check_parttype (parttype); if (!parttype) { reply_with_error ("unknown partition type: common choices are \"gpt\" and \"msdos\""); return -1; } - RUN_PARTED (return -1, device, "mklabel", parttype, NULL); + udev_settle (); + + r = commandf (NULL, &err, COMMAND_FLAG_FOLD_STDOUT_ON_STDERR, + "parted", "-s", "--", device, "mklabel", parttype, NULL); + if (r == -1) { + reply_with_error ("parted: %s: %s", device, err); + free (err); + return -1; + } + free (err); udev_settle (); @@ -118,6 +99,8 @@ int do_part_add (const char *device, const char *prlogex, int64_t startsect, int64_t endsect) { + int r; + char *err; char startstr[32]; char endstr[32]; @@ -146,12 +129,22 @@ do_part_add (const char *device, const char *prlogex, snprintf (startstr, sizeof startstr, "%" PRIi64 "s", startsect); snprintf (endstr, sizeof endstr, "%" PRIi64 "s", endsect); + udev_settle (); + /* XXX Bug: If the partition table type (which we don't know in this * function) is GPT, then this parted command sets the _partition * name_ to prlogex, eg. "primary". I would essentially describe * this as a bug in the parted mkpart command. */ - RUN_PARTED (return -1, device, "mkpart", prlogex, startstr, endstr, NULL); + r = commandf (NULL, &err, COMMAND_FLAG_FOLD_STDOUT_ON_STDERR, + "parted", "-s", "--", + device, "mkpart", prlogex, startstr, endstr, NULL); + if (r == -1) { + reply_with_error ("parted: %s: %s", device, err); + free (err); + return -1; + } + free (err); udev_settle (); @@ -161,6 +154,9 @@ do_part_add (const char *device, const char *prlogex, int do_part_del (const char *device, int partnum) { + int r; + char *err; + if (partnum <= 0) { reply_with_error ("partition number must be >= 1"); return -1; @@ -169,15 +165,28 @@ do_part_del (const char *device, int partnum) char partnum_str[16]; snprintf (partnum_str, sizeof partnum_str, "%d", partnum); - RUN_PARTED (return -1, device, "rm", partnum_str, NULL); + udev_settle (); + + r = commandf (NULL, &err, COMMAND_FLAG_FOLD_STDOUT_ON_STDERR, + "parted", "-s", "--", device, "rm", partnum_str, NULL); + if (r == -1) { + reply_with_error ("parted: %s: %s", device, err); + free (err); + return -1; + } + free (err); udev_settle (); + return 0; } int do_part_disk (const char *device, const char *parttype) { + int r; + char *err; + parttype = check_parttype (parttype); if (!parttype) { reply_with_error ("unknown partition type: common choices are \"gpt\" and \"msdos\""); @@ -195,12 +204,21 @@ do_part_disk (const char *device, const char *parttype) const char *startstr = "128s"; const char *endstr = "-128s"; - RUN_PARTED (return -1, - device, - "mklabel", parttype, - /* See comment about about the parted mkpart command. */ - "mkpart", STREQ (parttype, "gpt") ? "p1" : "primary", - startstr, endstr, NULL); + udev_settle (); + + r = commandf (NULL, &err, COMMAND_FLAG_FOLD_STDOUT_ON_STDERR, + "parted", "-s", "--", + device, + "mklabel", parttype, + /* See comment about about the parted mkpart command. */ + "mkpart", STREQ (parttype, "gpt") ? "p1" : "primary", + startstr, endstr, NULL); + if (r == -1) { + reply_with_error ("parted: %s: %s", device, err); + free (err); + return -1; + } + free (err); udev_settle (); @@ -210,6 +228,9 @@ do_part_disk (const char *device, const char *parttype) int do_part_set_bootable (const char *device, int partnum, int bootable) { + int r; + char *err; + if (partnum <= 0) { reply_with_error ("partition number must be >= 1"); return -1; @@ -219,8 +240,17 @@ do_part_set_bootable (const char *device, int partnum, int bootable) snprintf (partstr, sizeof partstr, "%d", partnum); - RUN_PARTED (return -1, - device, "set", partstr, "boot", bootable ? "on" : "off", NULL); + udev_settle (); + + r = commandf (NULL, &err, COMMAND_FLAG_FOLD_STDOUT_ON_STDERR, + "parted", "-s", "--", + device, "set", partstr, "boot", bootable ? "on" : "off", NULL); + if (r == -1) { + reply_with_error ("parted: %s: %s", device, err); + free (err); + return -1; + } + free (err); udev_settle (); @@ -230,6 +260,9 @@ do_part_set_bootable (const char *device, int partnum, int bootable) int do_part_set_name (const char *device, int partnum, const char *name) { + int r; + char *err; + if (partnum <= 0) { reply_with_error ("partition number must be >= 1"); return -1; @@ -239,7 +272,16 @@ do_part_set_name (const char *device, int partnum, const char *name) snprintf (partstr, sizeof partstr, "%d", partnum); - RUN_PARTED (return -1, device, "name", partstr, name, NULL); + udev_settle (); + + r = commandf (NULL, &err, COMMAND_FLAG_FOLD_STDOUT_ON_STDERR, + "parted", "-s", "--", device, "name", partstr, name, NULL); + if (r == -1) { + reply_with_error ("parted: %s: %s", device, err); + free (err); + return -1; + } + free (err); udev_settle (); @@ -686,6 +728,8 @@ do_part_get_mbr_id (const char *device, int partnum) char *out, *err; int r; + udev_settle (); + r = command (&out, &err, "sfdisk", "--print-id", device, partnum_str, NULL); if (r == -1) { reply_with_error ("sfdisk --print-id: %s", err); @@ -693,9 +737,10 @@ do_part_get_mbr_id (const char *device, int partnum) free (err); return -1; } - free (err); + udev_settle (); + /* It's printed in hex ... */ int id; if (sscanf (out, "%x", &id) != 1) { @@ -725,6 +770,8 @@ do_part_set_mbr_id (const char *device, int partnum, int idbyte) char *err; int r; + udev_settle (); + r = command (NULL, &err, "sfdisk", "--change-id", device, partnum_str, idbyte_str, NULL); if (r == -1) { @@ -732,7 +779,9 @@ do_part_set_mbr_id (const char *device, int partnum, int idbyte) free (err); return -1; } - free (err); + + udev_settle (); + return 0; } -- 1.7.6
Ignore this repeated patch ... git send-email confused me and I thought it hadn't sent the patch out first time! Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones virt-df lists disk usage of guests without needing to install any software inside the virtual machine. Supports Linux and Windows. http://et.redhat.com/~rjones/virt-df/
participants (1)
-
Richard W.M. Jones