From a9d3495e67990315ca476dad5369835b43a086db Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Wed, 11 Apr 2012 16:28:35 -0600 Subject: [PATCH] blockjob: allow for fast-finishing job In my testing, I was able to provoke an odd block pull failure: $ virsh blockpull dom vda --bandwidth 10000 error: Requested operation is not valid: No active operation on device: drive-virtio-disk0 merely by using gdb to artifically wait to do the block job set speed until after the pull had already finished. But in reality, that should be a success, since the pull finished before we had a chance to set speed. Furthermore, using a double job lock is not only annoying, but a bug in itself - if you do parallel virDomainBlockRebase, and hit the race window just right, the first call grabs the VM job to start a fast block job, then the second call grabs the VM job to start a long-running job with unspecified speed, then the first call finally regrabs the VM job and sets the speed, which ends up running the second job under the speed from the first call. By consolidating things into a single job, we avoid opening that race, as well as reduce the time between starting the job and changing the speed, for less likelihood of the speed change happening after block job completion in the first place. * src/qemu/qemu_monitor.h (BLOCK_JOB_CMD): Add new mode. * src/qemu/qemu_driver.c (qemuDomainBlockRebase): Move secondary job call... (qemuDomainBlockJobImpl): ...here, for fewer locks. * src/qemu/qemu_monitor_json.c (qemuMonitorJSONBlockJob): Change return value on new internal mode. --- src/qemu/qemu_driver.c | 13 +++++-------- src/qemu/qemu_monitor.h | 3 ++- src/qemu/qemu_monitor_json.c | 31 ++++++++++++++++++++----------- 3 files changed, 27 insertions(+), 20 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 847afa6d13..d37b990c8d 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -11654,6 +11654,9 @@ qemuDomainBlockJobImpl(virDomainPtr dom, const char *path, const char *base, * relying on qemu to do this. */ ret = qemuMonitorBlockJob(priv->mon, device, base, bandwidth, info, mode, async); + if (ret == 0 && mode == BLOCK_JOB_PULL && bandwidth) + ret = qemuMonitorBlockJob(priv->mon, device, NULL, bandwidth, NULL, + BLOCK_JOB_SPEED_INTERNAL, async); qemuDomainObjExitMonitorWithDriver(driver, vm); if (ret < 0) goto endjob; @@ -11749,15 +11752,9 @@ static int qemuDomainBlockRebase(virDomainPtr dom, const char *path, const char *base, unsigned long bandwidth, unsigned int flags) { - int ret; - virCheckFlags(0, -1); - ret = qemuDomainBlockJobImpl(dom, path, base, bandwidth, NULL, - BLOCK_JOB_PULL, flags); - if (ret == 0 && bandwidth != 0) - ret = qemuDomainBlockJobImpl(dom, path, NULL, bandwidth, NULL, - BLOCK_JOB_SPEED, flags); - return ret; + return qemuDomainBlockJobImpl(dom, path, base, bandwidth, NULL, + BLOCK_JOB_PULL, flags); } static int diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index dc02964fd2..f3cdcddb43 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -530,7 +530,8 @@ typedef enum { BLOCK_JOB_ABORT = 0, BLOCK_JOB_INFO = 1, BLOCK_JOB_SPEED = 2, - BLOCK_JOB_PULL = 3, + BLOCK_JOB_SPEED_INTERNAL = 3, + BLOCK_JOB_PULL = 4, } BLOCK_JOB_CMD; int qemuMonitorBlockJob(qemuMonitorPtr mon, diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 0b7660508c..eb58f13804 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -3453,6 +3453,7 @@ qemuMonitorJSONBlockJob(qemuMonitorPtr mon, cmd = qemuMonitorJSONMakeCommand(cmd_name, NULL); break; case BLOCK_JOB_SPEED: + case BLOCK_JOB_SPEED_INTERNAL: cmd_name = async ? "block-job-set-speed" : "block_job_set_speed"; cmd = qemuMonitorJSONMakeCommand(cmd_name, "s:device", device, "U:value", @@ -3474,22 +3475,30 @@ qemuMonitorJSONBlockJob(qemuMonitorPtr mon, ret = qemuMonitorJSONCommand(mon, cmd, &reply); if (ret == 0 && virJSONValueObjectHasKey(reply, "error")) { - if (qemuMonitorJSONHasError(reply, "DeviceNotActive")) - qemuReportError(VIR_ERR_OPERATION_INVALID, - _("No active operation on device: %s"), device); - else if (qemuMonitorJSONHasError(reply, "DeviceInUse")) + ret = -1; + if (qemuMonitorJSONHasError(reply, "DeviceNotActive")) { + /* If a job completes before we get a chance to set the + * speed, we don't want to fail the original command. */ + if (mode == BLOCK_JOB_SPEED_INTERNAL) + ret = 0; + else + qemuReportError(VIR_ERR_OPERATION_INVALID, + _("No active operation on device: %s"), + device); + } else if (qemuMonitorJSONHasError(reply, "DeviceInUse")){ qemuReportError(VIR_ERR_OPERATION_FAILED, - _("Device %s in use"), device); - else if (qemuMonitorJSONHasError(reply, "NotSupported")) + _("Device %s in use"), device); + } else if (qemuMonitorJSONHasError(reply, "NotSupported")) { qemuReportError(VIR_ERR_OPERATION_INVALID, - _("Operation is not supported for device: %s"), device); - else if (qemuMonitorJSONHasError(reply, "CommandNotFound")) + _("Operation is not supported for device: %s"), + device); + } else if (qemuMonitorJSONHasError(reply, "CommandNotFound")) { qemuReportError(VIR_ERR_OPERATION_INVALID, - _("Command '%s' is not found"), cmd_name); - else + _("Command '%s' is not found"), cmd_name); + } else { qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Unexpected error")); - ret = -1; + } } if (ret == 0 && mode == BLOCK_JOB_INFO) -- GitLab