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.

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 847afa6d1377d8e76dfe6dcbbcd45cb004d9e896..d37b990c8dc8ff9a1cd52072509f272340018341 100644 (file)
--- 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 dc02964fd2cd17d20202b6b5728e104ee7970c6a..f3cdcddb43d314488bb6d099c468ccdf2eacc35c 100644 (file)
--- 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 0b7660508c537e34eac96ed19a68643c3c07589e..eb58f138045a636fab9e7164be166babbbca9ea8 100644 (file)
--- 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)