[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 2/5] blockjob: add API for async virDomainBlockJobAbort



On Wed, Apr 11, 2012 at 05:40:35PM -0600, Eric Blake wrote:
> From: Adam Litke <agl us ibm com>
> 
> Block job cancellation can take a while.  Now that upstream qemu 1.1
> has asynchronous block cancellation, we want to expose that to the user.
> Therefore, the following updates are made to the virDomainBlockJob API:
> 
> A new block job event type VIR_DOMAIN_BLOCK_JOB_CANCELED is managed by
> libvirt.  Regardless of the flags used with virDomainBlockJobAbort, this
> event will be raised: 1. when using synchronous block_job_cancel (the
> event will be synthesized by libvirt), and 2. whenever it is received
> from qemu (via asynchronous block-job-cancel).  Note that the event
> may be detected by libvirt even before the virDomainBlockJobAbort
> completes (always true when it is synthesized, but also possible if
> cancellation was fast).
> 
> A new extension flag VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC is added to the
> virDomainBlockJobAbort API.  When enabled, this function will allow
> (but not require) asynchronous operation (ie, it returns as soon as
> possible, which might be before the job has actually been canceled).
> When the API is used in this mode, it is the responsibility of the
> caller to wait for a VIR_DOMAIN_BLOCK_JOB_CANCELED event or poll via
> the virDomainGetBlockJobInfo API to check the cancellation status.
> 
> This patch also exposes the new flag through virsh, and makes virsh
> slightly easier to use (--async implies --abort, and lack of any options
> implies --info), although it leaves the qemu implementation for later
> patches.
> 
> Signed-off-by: Adam Litke <agl us ibm com>
> Cc: Stefan Hajnoczi <stefanha gmail com>
> Signed-off-by: Eric Blake <eblake redhat com>
> ---
>  include/libvirt/libvirt.h.in |   10 ++++++++
>  src/libvirt.c                |   13 ++++++++++-
>  tools/virsh.c                |   51 ++++++++++++++++++++++++++---------------
>  tools/virsh.pod              |   35 ++++++++++++++++++++--------
>  4 files changed, 79 insertions(+), 30 deletions(-)
> 
> diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
> index 499dcd4..97ad99d 100644
> --- a/include/libvirt/libvirt.h.in
> +++ b/include/libvirt/libvirt.h.in
> @@ -1946,6 +1946,15 @@ typedef enum {
>  #endif
>  } virDomainBlockJobType;
> 
> +/**
> + * virDomainBlockJobAbortFlags:
> + *
> + * VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC: Request only, do not wait for completion
> + */
> +typedef enum {
> +    VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC = 1 << 0,
> +} virDomainBlockJobAbortFlags;
> +
>  /* An iterator for monitoring block job operations */
>  typedef unsigned long long virDomainBlockJobCursor;
> 
> @@ -3617,6 +3626,7 @@ typedef void (*virConnectDomainEventGraphicsCallback)(virConnectPtr conn,
>  typedef enum {
>      VIR_DOMAIN_BLOCK_JOB_COMPLETED = 0,
>      VIR_DOMAIN_BLOCK_JOB_FAILED = 1,
> +    VIR_DOMAIN_BLOCK_JOB_CANCELED = 2,
> 
>  #ifdef VIR_ENUM_SENTINELS
>      VIR_DOMAIN_BLOCK_JOB_LAST
> diff --git a/src/libvirt.c b/src/libvirt.c
> index 16d1fd5..93ec817 100644
> --- a/src/libvirt.c
> +++ b/src/libvirt.c
> @@ -17902,7 +17902,7 @@ error:
>   * virDomainBlockJobAbort:
>   * @dom: pointer to domain object
>   * @disk: path to the block device, or device shorthand
> - * @flags: extra flags; not used yet, so callers should always pass 0
> + * @flags: bitwise-OR of virDomainBlockJobAbortFlags
>   *
>   * Cancel the active block job on the given disk.
>   *
> @@ -17913,6 +17913,17 @@ error:
>   * can be found by calling virDomainGetXMLDesc() and inspecting
>   * elements within //domain/devices/disk.
>   *
> + * By default, this function performs a synchronous operation and the caller
> + * may assume that the operation has completed when 0 is returned.  However,
> + * BlockJob operations may take a long time to cancel, and during this time
> + * further domain interactions may be unresponsive.  To avoid this problem,
> + * pass VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC in the @flags argument to enable
> + * asynchronous behavior, returning as soon as possible.  When the job has
> + * been canceled, a BlockJob event will be emitted, with status
> + * VIR_DOMAIN_BLOCK_JOB_CANCELED (even if the ABORT_ASYNC flag was not
> + * used); it is also possible to poll virDomainBlockJobInfo() to see if
> + * the job cancellation is still pending.
> + *
>   * Returns -1 in case of failure, 0 when successful.
>   */
>  int virDomainBlockJobAbort(virDomainPtr dom, const char *disk,
> diff --git a/tools/virsh.c b/tools/virsh.c
> index 44514c4..8ef57e0 100644
> --- a/tools/virsh.c
> +++ b/tools/virsh.c
> @@ -7525,6 +7525,7 @@ blockJobImpl(vshControl *ctl, const vshCmd *cmd,
>      const char *name, *path;
>      unsigned long bandwidth = 0;
>      int ret = -1;
> +    unsigned int flags = 0;
> 
>      if (!vshConnectionUsability(ctl, ctl->conn))
>          goto cleanup;
> @@ -7541,7 +7542,9 @@ blockJobImpl(vshControl *ctl, const vshCmd *cmd,
>      }
> 
>      if (mode == VSH_CMD_BLOCK_JOB_ABORT) {
> -        ret = virDomainBlockJobAbort(dom, path, 0);
> +        if (vshCommandOptBool(cmd, "async"))
> +            flags |= VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC;
> +        ret = virDomainBlockJobAbort(dom, path, flags);
>      } else if (mode == VSH_CMD_BLOCK_JOB_INFO) {
>          ret = virDomainGetBlockJobInfo(dom, path, info, 0);
>      } else if (mode == VSH_CMD_BLOCK_JOB_SPEED) {
> @@ -7589,20 +7592,25 @@ cmdBlockPull(vshControl *ctl, const vshCmd *cmd)
>  }
> 
>  /*
> - * "blockjobinfo" command
> + * "blockjob" command
>   */
>  static const vshCmdInfo info_block_job[] = {
> -    {"help", N_("Manage active block operations.")},
> -    {"desc", N_("Manage active block operations.")},
> +    {"help", N_("Manage active block operations")},
> +    {"desc", N_("Query, adjust speed, or cancel active block operations.")},
>      {NULL, NULL}
>  };
> 
>  static const vshCmdOptDef opts_block_job[] = {
>      {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")},
>      {"path", VSH_OT_DATA, VSH_OFLAG_REQ, N_("Fully-qualified path of disk")},
> -    {"abort", VSH_OT_BOOL, VSH_OFLAG_NONE, N_("Abort the active job on the specified disk")},
> -    {"info", VSH_OT_BOOL, VSH_OFLAG_NONE, N_("Get active job information for the specified disk")},
> -    {"bandwidth", VSH_OT_DATA, VSH_OFLAG_NONE, N_("Set the Bandwidth limit in MB/s")},
> +    {"abort", VSH_OT_BOOL, VSH_OFLAG_NONE,
> +     N_("Abort the active job on the specified disk")},
> +    {"async", VSH_OT_BOOL, VSH_OFLAG_NONE,
> +     N_("don't wait for --abort to complete")},
> +    {"info", VSH_OT_BOOL, VSH_OFLAG_NONE,
> +     N_("Get active job information for the specified disk")},
> +    {"bandwidth", VSH_OT_DATA, VSH_OFLAG_NONE,
> +     N_("Set the Bandwidth limit in MB/s")},
>      {NULL, 0, 0, NULL}
>  };
> 
> @@ -7613,19 +7621,24 @@ cmdBlockJob(vshControl *ctl, const vshCmd *cmd)
>      virDomainBlockJobInfo info;
>      const char *type;
>      int ret;
> +    bool abortMode = (vshCommandOptBool(cmd, "abort") ||
> +                      vshCommandOptBool(cmd, "async"));
> +    bool infoMode = vshCommandOptBool(cmd, "info");
> +    bool bandwidth = vshCommandOptBool(cmd, "bandwidth");
> 
> -    if (vshCommandOptBool (cmd, "abort")) {
> -        mode = VSH_CMD_BLOCK_JOB_ABORT;
> -    } else if (vshCommandOptBool (cmd, "info")) {
> -        mode = VSH_CMD_BLOCK_JOB_INFO;
> -    } else if (vshCommandOptBool (cmd, "bandwidth")) {
> -        mode = VSH_CMD_BLOCK_JOB_SPEED;
> -    } else {
> +    if (abortMode + infoMode + bandwidth > 1) {
>          vshError(ctl, "%s",
> -                 _("One of --abort, --info, or --bandwidth is required"));
> +                 _("conflict between --abort, --info, and --bandwidth modes"));
>          return false;
>      }
> 
> +    if (abortMode)
> +        mode = VSH_CMD_BLOCK_JOB_ABORT;
> +    else if (bandwidth)
> +        mode = VSH_CMD_BLOCK_JOB_SPEED;
> +    else
> +        mode = VSH_CMD_BLOCK_JOB_INFO;
> +
>      ret = blockJobImpl(ctl, cmd, &info, mode);
>      if (ret < 0)
>          return false;
> @@ -7634,13 +7647,13 @@ cmdBlockJob(vshControl *ctl, const vshCmd *cmd)
>          return true;
> 
>      if (info.type == VIR_DOMAIN_BLOCK_JOB_TYPE_PULL)
> -        type = "Block Pull";
> +        type = _("Block Pull");
>      else
> -        type = "Unknown job";
> +        type = _("Unknown job");
> 
>      print_job_progress(type, info.end - info.cur, info.end);
>      if (info.bandwidth != 0)
> -        vshPrint(ctl, "    Bandwidth limit: %lu MB/s\n", info.bandwidth);
> +        vshPrint(ctl, _("    Bandwidth limit: %lu MB/s\n"), info.bandwidth);
>      return true;
>  }
> 
> @@ -17115,8 +17128,8 @@ static const vshCmdDef domManagementCmds[] = {
>      {"autostart", cmdAutostart, opts_autostart, info_autostart, 0},
>      {"blkdeviotune", cmdBlkdeviotune, opts_blkdeviotune, info_blkdeviotune, 0},
>      {"blkiotune", cmdBlkiotune, opts_blkiotune, info_blkiotune, 0},
> -    {"blockpull", cmdBlockPull, opts_block_pull, info_block_pull, 0},
>      {"blockjob", cmdBlockJob, opts_block_job, info_block_job, 0},
> +    {"blockpull", cmdBlockPull, opts_block_pull, info_block_pull, 0},
>      {"blockresize", cmdBlockResize, opts_block_resize, info_block_resize, 0},
>      {"change-media", cmdChangeMedia, opts_change_media, info_change_media, 0},
>  #ifndef WIN32
> diff --git a/tools/virsh.pod b/tools/virsh.pod
> index c6e0ff3..5f3d9b1 100644
> --- a/tools/virsh.pod
> +++ b/tools/virsh.pod
> @@ -650,7 +650,10 @@ pulled, the disk no longer depends on that portion of the backing chain.
>  It pulls data for the entire disk in the background, the process of the
>  operation can be checked with B<blockjob>.
> 
> -I<path> specifies fully-qualified path of the disk.
> +I<path> specifies fully-qualified path of the disk; it corresponds
> +to a unique target name (<target dev='name'/>) or source file (<source
> +file='name'/>) for one of the disk devices attached to I<domain> (see
> +also B<domblklist> for listing these names).
>  I<bandwidth> specifies copying bandwidth limit in Mbps.
> 
>  =item B<blkdeviotune> I<domain> I<device>
> @@ -687,13 +690,21 @@ Both I<--live> and I<--current> flags may be given, but I<--current> is
>  exclusive. If no flag is specified, behavior is different depending
>  on hypervisor.
> 
> -=item B<blockjob> I<domain> I<path> [I<--abort>] [I<--info>] [I<bandwidth>]
> +=item B<blockjob> I<domain> I<path> { [I<--abort>] [I<--async>] |
> +[I<--info>] | [I<bandwidth>] }
> 
> -Manage active block operations.
> +Manage active block operations.  There are three modes: I<--info>,
> +I<bandwidth>, and I<--abort>; I<--info> is default except that I<--async>
> +implies I<--abort>.
> +
> +I<path> specifies fully-qualified path of the disk; it corresponds
> +to a unique target name (<target dev='name'/>) or source file (<source
> +file='name'/>) for one of the disk devices attached to I<domain> (see
> +also B<domblklist> for listing these names).
> 
> -I<path> specifies fully-qualified path of the disk.
>  If I<--abort> is specified, the active job on the specified disk will
> -be aborted.
> +be aborted.  If I<--async> is also specified, this command will return
> +immediately, rather than waiting for the cancelation to complete.
>  If I<--info> is specified, the active job information on the specified
>  disk will be printed.
>  I<bandwidth> can be used to set bandwidth limit for the active job.
> @@ -701,11 +712,15 @@ I<bandwidth> can be used to set bandwidth limit for the active job.
>  =item B<blockresize> I<domain> I<path> I<size>
> 
>  Resize a block device of domain while the domain is running, I<path>
> -specifies the absolute path of the block device, I<size> is a scaled
> -integer (see B<NOTES> above) which defaults to KiB (blocks of 1024 bytes)
> -if there is no suffix.  You must use a suffix of "B" to get bytes (note
> -that for historical reasons, this differs from B<vol-resize> which
> -defaults to bytes without a suffix).
> +specifies the absolute path of the block device; it corresponds
> +to a unique target name (<target dev='name'/>) or source file (<source
> +file='name'/>) for one of the disk devices attached to I<domain> (see
> +also B<domblklist> for listing these names).
> +
> +I<size> is a scaled integer (see B<NOTES> above) which defaults to KiB
> +(blocks of 1024 bytes) if there is no suffix.  You must use a suffix of
> +"B" to get bytes (note that for historical reasons, this differs from
> +B<vol-resize> which defaults to bytes without a suffix).
> 
>  =item B<dominfo> I<domain-id>

  ACK, API change is good, and noted improvements on virsh docs, info
  pages and string localization !

Daniel

-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel veillard com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]