Re: [libvirt] [PATCH 5/5] 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 annoying; we can alter the speed as part of the same job that started the block pull for less likelihood of hitting the speed failure 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 bb6089b..e31f407 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; @@ -11750,15 +11753,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 dc02964..f3cdcdd 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 242889c..26e41ac 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -3452,6 +3452,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", @@ -3473,22 +3474,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)