[libvirt] [PATCHv3 02/16] blockjob: wire up qemu async virDomainBlockJobAbort

[Laine Stump]

On 04/06/2012 02:29 PM, Eric Blake wrote:
> From: Adam Litke <agl at us.ibm.com>
>
> Without the VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC flag, libvirt will internally poll
> using qemu's "query-block-jobs" API and will not return until the operation has
> been completed.  API users are advised that this operation is unbounded and
> further interaction with the domain during this period may block.  Future
> patches may refactor things to allow other queries in parallel with this
> polling.  Unfortunately, there's no good way to tell if qemu will emit the
> new event, so this implementation always polls to deal with older qemu.
>
> Signed-off-by: Adam Litke <agl at us.ibm.com>
> Cc: Stefan Hajnoczi <stefanha at gmail.com>
> Signed-off-by: Eric Blake <eblake at redhat.com>
> ---
>  src/qemu/qemu_driver.c |   55 +++++++++++++++++++++++++++++++++++++++++------
>  1 files changed, 48 insertions(+), 7 deletions(-)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index d9e35be..5f0eb05 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -11599,7 +11599,7 @@ cleanup:
>  static int
>  qemuDomainBlockJobImpl(virDomainPtr dom, const char *path, const char *base,
>                         unsigned long bandwidth, virDomainBlockJobInfoPtr info,
> -                       int mode)
> +                       int mode, unsigned int flags)
>  {
>      struct qemud_driver *driver = dom->conn->privateData;
>      virDomainObjPtr vm = NULL;
> @@ -11641,6 +11641,45 @@ qemuDomainBlockJobImpl(virDomainPtr dom, const char *path, const char *base,
>      ret = qemuMonitorBlockJob(priv->mon, device, base, bandwidth, info, mode);
>      qemuDomainObjExitMonitorWithDriver(driver, vm);


Do I understand correctly that qemu's abort was always asynchronous, and
prior to this, an abort call to libvirt would erroneously return
immediately? (I'm still trying to understand the code...)


>
> +    /* Qemu provides asynchronous block job cancellation, but without
> +     * the VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC flag libvirt guarantees a
> +     * synchronous operation.  Provide this behavior by waiting here,
> +     * so we don't get confused by newly scheduled block jobs.
> +     */
> +    if (ret == 0 && mode == BLOCK_JOB_ABORT &&
> +        !(flags & VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC)) {
> +        ret = 1;


Why is ret set to 1? It will be assigned the return value of
qemuMonitorBlockJob before it's ever used for anything, so this seems
unnecessary.


> +        while (1) {
> +            /* Poll every 50ms */
> +            struct timespec ts = { .tv_sec = 0,
> +                                   .tv_nsec = 50 * 1000 * 1000ull };


This timespec could just as well be static const, couldn't it? No big
deal one way or the other, though.


> +            virDomainBlockJobInfo dummy;
> +
> +            qemuDomainObjEnterMonitorWithDriver(driver, vm);
> +            ret = qemuMonitorBlockJob(priv->mon, device, NULL, 0, &dummy,
> +                                      BLOCK_JOB_INFO);
> +            qemuDomainObjExitMonitorWithDriver(driver, vm);
> +
> +            if (ret <= 0)
> +                break;
> +
> +            virDomainObjUnlock(vm);
> +            qemuDriverUnlock(driver);
> +
> +            nanosleep(&ts, NULL);
> +
> +            qemuDriverLock(driver);
> +            virDomainObjLock(vm);
> +
> +            if (!virDomainObjIsActive(vm)) {
> +                qemuReportError(VIR_ERR_OPERATION_INVALID, "%s",
> +                                _("domain is not running"));
> +                ret = -1;
> +                break;
> +            }
> +        }
> +    }
> +
>  endjob:
>      if (qemuDomainObjEndJob(driver, vm) == 0) {
>          vm = NULL;
> @@ -11658,8 +11697,9 @@ cleanup:
>  static int
>  qemuDomainBlockJobAbort(virDomainPtr dom, const char *path, unsigned int flags)
>  {
> -    virCheckFlags(0, -1);
> -    return qemuDomainBlockJobImpl(dom, path, NULL, 0, NULL, BLOCK_JOB_ABORT);
> +    virCheckFlags(VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC, -1);
> +    return qemuDomainBlockJobImpl(dom, path, NULL, 0, NULL, BLOCK_JOB_ABORT,
> +                                  flags);
>  }
>
>  static int
> @@ -11667,7 +11707,8 @@ qemuDomainGetBlockJobInfo(virDomainPtr dom, const char *path,
>                             virDomainBlockJobInfoPtr info, unsigned int flags)
>  {
>      virCheckFlags(0, -1);
> -    return qemuDomainBlockJobImpl(dom, path, NULL, 0, info, BLOCK_JOB_INFO);
> +    return qemuDomainBlockJobImpl(dom, path, NULL, 0, info, BLOCK_JOB_INFO,
> +                                  flags);
>  }
>
>  static int
> @@ -11676,7 +11717,7 @@ qemuDomainBlockJobSetSpeed(virDomainPtr dom, const char *path,
>  {
>      virCheckFlags(0, -1);
>      return qemuDomainBlockJobImpl(dom, path, NULL, bandwidth, NULL,
> -                                  BLOCK_JOB_SPEED);
> +                                  BLOCK_JOB_SPEED, flags);
>  }
>
>  static int
> @@ -11687,10 +11728,10 @@ qemuDomainBlockRebase(virDomainPtr dom, const char *path, const char *base,
>
>      virCheckFlags(0, -1);
>      ret = qemuDomainBlockJobImpl(dom, path, base, bandwidth, NULL,
> -                                 BLOCK_JOB_PULL);
> +                                 BLOCK_JOB_PULL, flags);
>      if (ret == 0 && bandwidth != 0)
>          ret = qemuDomainBlockJobImpl(dom, path, NULL, bandwidth, NULL,
> -                                     BLOCK_JOB_SPEED);
> +                                     BLOCK_JOB_SPEED, flags);
>      return ret;
>  }
>


ACK once the items above are addressed (either by explaining, changing
code, or telling me why I'm wrong)