On 04/09/2012 11:52 PM, Eric Blake wrote:
> From: Adam Litke <agl@xxxxxxxxxx>
>
> Qemu has changed the semantics of the "block_job_cancel" API. The original
> qed implementation (pretty much only backported to RHEL 6.2 qemu) was
> synchronous (ie. upon command completion, the operation was guaranteed to
> be completely stopped). With the new semantics going into qemu 1.1 for
> qcow2, a "block_job_cancel" merely requests that the operation be cancelled
> and an event is triggered once the cancellation request has been honored.
>
> To adopt the new semantics while preserving compatibility the following
> updates are made to the virDomainBlockJob API:
>
> A new block job event type VIR_DOMAIN_BLOCK_JOB_CANCELED is recognized by
> libvirt. Regardless of the flags used with virDomainBlockJobAbort, this
> event will be raised whenever it is received from qemu. This event
> indicates that a block job has been successfully cancelled. For now,
> libvirt does not try to synthesize this event if using an older qemu that
> did not generate it.
Okay, as I understand it, the only qemu binary that has a synchronous
block_job_cancel command is the version that is part of RHEL6.2. So any
compatibility code to allow for a synchronous block_job_cancel command
in qemu would only ever make a difference for someone who built from
upstream libvirt sources (or post-RHEL6.2 source rpm) to run on RHEL6.2
(and *didn't* build a post-RHEL6.2 qemu). Is that correct?
>
> A new extension flag VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC is added to the
> virDomainBlockJobAbort API. When enabled, this function will operate
> asynchronously (ie, it can return 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 flag
> is an error if it is not known if the hypervisor supports asynchronous cancel.
>
> This patch also exposes the new flag through virsh, as well as wiring
> up the new event, but leaves the new flag for a later patch.
Did you leave out a word there? It exposes the new flag through virsh,
..., but leaves [blank] the new flag for a later patch? "implementing"?
(It looks that way - the front end is there, but the backend hasn't been
changed, and the backend has been rearranged a bit, but it would still
error out if someone specified the flag, and doesn't implement the loop
for the case when they hadn't specified the flag).
Since we're stuck with the assumption of "synchronous unless otherwise
specified", and we definitely want to allow for asynchronous, I'm okay
with this, *as long as upstream qemu has also committed (in git, not
just in email) to the block job cancel command being asynchronous*. I
*think* I understood you correctly that this is the case.
>
> Signed-off-by: Adam Litke <agl@xxxxxxxxxx>
> Cc: Stefan Hajnoczi <stefanha@xxxxxxxxx>
> Signed-off-by: Eric Blake <eblake@xxxxxxxxxx>
> ---
> include/libvirt/libvirt.h.in | 10 ++++++++
> src/libvirt.c | 14 ++++++++++-
> src/qemu/qemu_monitor_json.c | 44 ++++++++++++++++++++++++++++++-----
> tools/virsh.c | 51 ++++++++++++++++++++++++++---------------
> tools/virsh.pod | 35 ++++++++++++++++++++--------
> 5 files changed, 117 insertions(+), 37 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..d44335a 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,18 @@ 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 complete, 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; this flag fails if asynchronous support is not
> + * possible. If asynchronous jobs are supported, then 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); but since events can be missed, it is also possible to use
> + * virDomainBlockJobInfo() to poll if the job is still running.
> + *
> * Returns -1 in case of failure, 0 when successful.
> */
> int virDomainBlockJobAbort(virDomainPtr dom, const char *disk,
> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index c8ed087..7c893d4 100644
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -58,13 +58,14 @@ static void qemuMonitorJSONHandleIOError(qemuMonitorPtr mon, virJSONValuePtr dat
> static void qemuMonitorJSONHandleVNCConnect(qemuMonitorPtr mon, virJSONValuePtr data);
> static void qemuMonitorJSONHandleVNCInitialize(qemuMonitorPtr mon, virJSONValuePtr data);
> static void qemuMonitorJSONHandleVNCDisconnect(qemuMonitorPtr mon, virJSONValuePtr data);
> -static void qemuMonitorJSONHandleBlockJob(qemuMonitorPtr mon, virJSONValuePtr data);
> static void qemuMonitorJSONHandleSPICEConnect(qemuMonitorPtr mon, virJSONValuePtr data);
> static void qemuMonitorJSONHandleSPICEInitialize(qemuMonitorPtr mon, virJSONValuePtr data);
> static void qemuMonitorJSONHandleSPICEDisconnect(qemuMonitorPtr mon, virJSONValuePtr data);
> static void qemuMonitorJSONHandleTrayChange(qemuMonitorPtr mon, virJSONValuePtr data);
> static void qemuMonitorJSONHandlePMWakeup(qemuMonitorPtr mon, virJSONValuePtr data);
> static void qemuMonitorJSONHandlePMSuspend(qemuMonitorPtr mon, virJSONValuePtr data);
> +static void qemuMonitorJSONHandleBlockJobCompleted(qemuMonitorPtr mon, virJSONValuePtr data);
> +static void qemuMonitorJSONHandleBlockJobCanceled(qemuMonitorPtr mon, virJSONValuePtr data);
>
> static struct {
> const char *type;
> @@ -80,13 +81,14 @@ static struct {
> { "VNC_CONNECTED", qemuMonitorJSONHandleVNCConnect, },
> { "VNC_INITIALIZED", qemuMonitorJSONHandleVNCInitialize, },
> { "VNC_DISCONNECTED", qemuMonitorJSONHandleVNCDisconnect, },
> - { "BLOCK_JOB_COMPLETED", qemuMonitorJSONHandleBlockJob, },
> { "SPICE_CONNECTED", qemuMonitorJSONHandleSPICEConnect, },
> { "SPICE_INITIALIZED", qemuMonitorJSONHandleSPICEInitialize, },
> { "SPICE_DISCONNECTED", qemuMonitorJSONHandleSPICEDisconnect, },
> { "DEVICE_TRAY_MOVED", qemuMonitorJSONHandleTrayChange, },
> { "WAKEUP", qemuMonitorJSONHandlePMWakeup, },
> { "SUSPEND", qemuMonitorJSONHandlePMSuspend, },
> + { "BLOCK_JOB_COMPLETED", qemuMonitorJSONHandleBlockJobCompleted, },
> + { "BLOCK_JOB_CANCELLED", qemuMonitorJSONHandleBlockJobCanceled, },
> };
What? This isn't in alphabetical order? :-)
>
>
> @@ -754,13 +756,15 @@ static void qemuMonitorJSONHandleSPICEDisconnect(qemuMonitorPtr mon, virJSONValu
> qemuMonitorJSONHandleGraphics(mon, data, VIR_DOMAIN_EVENT_GRAPHICS_DISCONNECT);
> }
>
> -static void qemuMonitorJSONHandleBlockJob(qemuMonitorPtr mon, virJSONValuePtr data)
> +static void
> +qemuMonitorJSONHandleBlockJobImpl(qemuMonitorPtr mon,
> + virJSONValuePtr data,
> + int event)
> {
> const char *device;
> const char *type_str;
> int type = VIR_DOMAIN_BLOCK_JOB_TYPE_UNKNOWN;
> unsigned long long offset, len;
> - int status = VIR_DOMAIN_BLOCK_JOB_FAILED;
>
> if ((device = virJSONValueObjectGetString(data, "device")) == NULL) {
> VIR_WARN("missing device in block job event");
> @@ -785,11 +789,21 @@ static void qemuMonitorJSONHandleBlockJob(qemuMonitorPtr mon, virJSONValuePtr da
> if (STREQ(type_str, "stream"))
> type = VIR_DOMAIN_BLOCK_JOB_TYPE_PULL;
>
> - if (offset != 0 && offset == len)
> - status = VIR_DOMAIN_BLOCK_JOB_COMPLETED;
> + switch (event) {
> + case VIR_DOMAIN_BLOCK_JOB_COMPLETED:
> + /* Make sure the whole device has been processed */
> + if (offset != len)
> + event = VIR_DOMAIN_BLOCK_JOB_FAILED;
Previously we also tested for offset != 0. Is that not actually necessary?
> + break;
> + case VIR_DOMAIN_BLOCK_JOB_CANCELED:
> + break;
> + case VIR_DOMAIN_BLOCK_JOB_FAILED:
> + VIR_DEBUG("should not get here");
> + break;
> + }
>
> out:
> - qemuMonitorEmitBlockJob(mon, device, type, status);
> + qemuMonitorEmitBlockJob(mon, device, type, event);
> }
>
> static void
> @@ -832,6 +846,22 @@ qemuMonitorJSONHandlePMSuspend(qemuMonitorPtr mon,
> qemuMonitorEmitPMSuspend(mon);
> }
>
> +static void
> +qemuMonitorJSONHandleBlockJobCompleted(qemuMonitorPtr mon,
> + virJSONValuePtr data)
> +{
> + qemuMonitorJSONHandleBlockJobImpl(mon, data,
> + VIR_DOMAIN_BLOCK_JOB_COMPLETED);
> +}
> +
> +static void
> +qemuMonitorJSONHandleBlockJobCanceled(qemuMonitorPtr mon,
> + virJSONValuePtr data)
> +{
> + qemuMonitorJSONHandleBlockJobImpl(mon, data,
> + VIR_DOMAIN_BLOCK_JOB_CANCELED);
> +}
> +
> int
> qemuMonitorJSONHumanCommandWithFd(qemuMonitorPtr mon,
> const char *cmd_str,
> diff --git a/tools/virsh.c b/tools/virsh.c
> index cfdd040..084d533 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"));
"modes", or "options"? I guess "options" implies they can be combined,
while "modes" implies they are mutually exclusive...
> 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 a60e667..d6ba035 100644
> --- a/tools/virsh.pod
> +++ b/tools/virsh.pod
> @@ -649,7 +649,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>
> @@ -686,13 +689,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.
> @@ -700,11 +711,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>
>
Really no surprises here. Functionally ACK, but as with the other
patches in this series, I'll leave the philosophical/strategic ACK to
others :-)
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list