On Wed, Apr 11, 2012 at 05:40:34PM -0600, Eric Blake wrote:

RHEL 6.2 was released with an early version of block jobs, which only worked on the qed file format, where the commands were spelled with underscore (contrary to QMP style), and where 'block_job_cancel' was synchronous and did not trigger an event.

qemu 1.1 has fixed these short-comings [1][2]: the commands now work on multiple file types, are spelled with dash, and 'block-job-cancel' is asynchronous and emits an event upon conclusion.

[1]qemu commit 370521a1d6f5537ea7271c119f3fbb7b0fa57063 [2]https://lists.gnu.org/archive/html/qemu-devel/2012-04/msg01248.html

This patch recognizes the new spellings, and fixes virDomainBlockRebase to give a graceful error when talking to a too-old qemu on a partial rebase attempt.

* src/qemu/qemu_capabilities.h (QEMU_CAPS_BLOCKJOB_SYNC) (QEMU_CAPS_BLOCKJOB_ASYNC): New bits. * src/qemu/qemu_capabilities.c (qemuCaps): Name them. * src/qemu/qemu_monitor_json.c (qemuMonitorJSONCheckCommands): Set them. (qemuMonitorJSONBlockJob): Manage both command names. (qemuMonitorJSONDiskSnapshot): Minor formatting fix. * src/qemu/qemu_monitor.h (qemuMonitorBlockJob): Alter signature. * src/qemu/qemu_monitor_json.h (qemuMonitorJSONBlockJob): Likewise. * src/qemu/qemu_monitor.c (qemuMonitorBlockJob): Pass through capability bit. * src/qemu/qemu_driver.c (qemuDomainBlockJobImpl): Update callers. --- src/qemu/qemu_capabilities.c | 3 ++ src/qemu/qemu_capabilities.h | 2 + src/qemu/qemu_driver.c | 22 ++++++++++++++---- src/qemu/qemu_monitor.c | 11 +++++--- src/qemu/qemu_monitor.h | 5 ++- src/qemu/qemu_monitor_json.c | 51 +++++++++++++++++++++++------------------- src/qemu/qemu_monitor_json.h | 4 ++- 7 files changed, 63 insertions(+), 35 deletions(-)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 0e09d6d..fd4ca4d 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -156,6 +156,9 @@ VIR_ENUM_IMPL(qemuCaps, QEMU_CAPS_LAST, "scsi-disk.channel", "scsi-block", "transaction", + + "block-job-sync", /* 90 */ + "block-job-async", );

struct qemu_feature_flags { diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index 78cdbe0..6550d59 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -124,6 +124,8 @@ enum qemuCapsFlags { QEMU_CAPS_SCSI_DISK_CHANNEL = 87, /* Is scsi-disk.channel available? */ QEMU_CAPS_SCSI_BLOCK = 88, /* -device scsi-block */ QEMU_CAPS_TRANSACTION = 89, /* transaction monitor command */ + QEMU_CAPS_BLOCKJOB_SYNC = 90, /* RHEL 6.2 block_job_cancel */ + QEMU_CAPS_BLOCKJOB_ASYNC = 91, /* qemu 1.1 block-job-cancel */

QEMU_CAPS_LAST, /* this must always be the last item */ }; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index e18e72d..f939a31 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -11607,6 +11607,7 @@ qemuDomainBlockJobImpl(virDomainPtr dom, const char *path, const char *base, char uuidstr[VIR_UUID_STRING_BUFLEN]; char *device = NULL; int ret = -1; + bool async = false;

qemuDriverLock(driver); virUUIDFormat(dom->uuid, uuidstr); @@ -11616,6 +11617,19 @@ qemuDomainBlockJobImpl(virDomainPtr dom, const char *path, const char *base, _("no domain with matching uuid '%s'"), uuidstr); goto cleanup; } + priv = vm->privateData; + if (qemuCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKJOB_ASYNC)) { + async = true; + } else if (!qemuCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKJOB_SYNC)) { + qemuReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("block jobs not supported with this QEMU binary")); + goto cleanup; + } else if (base) { + qemuReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("partial block pull not supported with this " + "QEMU binary")); + goto cleanup; + }

device = qemuDiskPathToAlias(vm, path, NULL); if (!device) { @@ -11632,13 +11646,11 @@ qemuDomainBlockJobImpl(virDomainPtr dom, const char *path, const char *base, }

qemuDomainObjEnterMonitorWithDriver(driver, vm); - priv = vm->privateData; - /* XXX - add a qemu capability check, since only qemu 1.1 or newer - * supports the base argument. - * XXX - libvirt should really be tracking the backing file chain + /* XXX - libvirt should really be tracking the backing file chain * itself, and validating that base is on the chain, rather than * relying on qemu to do this. */ - ret = qemuMonitorBlockJob(priv->mon, device, base, bandwidth, info, mode); + ret = qemuMonitorBlockJob(priv->mon, device, base, bandwidth, info, mode, + async); qemuDomainObjExitMonitorWithDriver(driver, vm);

endjob: diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index e1a8d4c..36f3832 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -2773,15 +2773,18 @@ int qemuMonitorBlockJob(qemuMonitorPtr mon, const char *base, unsigned long bandwidth, virDomainBlockJobInfoPtr info, - int mode) + int mode, + bool async) { int ret = -1;

- VIR_DEBUG("mon=%p, device=%s, base=%s, bandwidth=%lu, info=%p, mode=%o", - mon, device, NULLSTR(base), bandwidth, info, mode); + VIR_DEBUG("mon=%p, device=%s, base=%s, bandwidth=%lu, info=%p, mode=%o, " + "async=%d", mon, device, NULLSTR(base), bandwidth, info, mode, + async);

if (mon->json) - ret = qemuMonitorJSONBlockJob(mon, device, base, bandwidth, info, mode); + ret = qemuMonitorJSONBlockJob(mon, device, base, bandwidth, info, mode, + async); else qemuReportError(VIR_ERR_INVALID_ARG, "%s", _("block jobs require JSON monitor")); diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index b480966..dc02964 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -538,8 +538,9 @@ int qemuMonitorBlockJob(qemuMonitorPtr mon, const char *back, unsigned long bandwidth, virDomainBlockJobInfoPtr info, - int mode) - ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(5); + int mode, + bool async) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);

Hum, gcc wasn't complaining on an "ATTRIBUTE_NONNULL(5)" for something which wasn't a pointer ?

int qemuMonitorOpenGraphics(qemuMonitorPtr mon, const char *protocol, diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index d09d779..d4d2c3e 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -939,12 +939,14 @@ qemuMonitorJSONCheckCommands(qemuMonitorPtr mon,

if (STREQ(name, "human-monitor-command")) *json_hmp = 1; - - if (STREQ(name, "system_wakeup")) + else if (STREQ(name, "system_wakeup")) qemuCapsSet(qemuCaps, QEMU_CAPS_WAKEUP); - - if (STREQ(name, "transaction")) + else if (STREQ(name, "transaction")) qemuCapsSet(qemuCaps, QEMU_CAPS_TRANSACTION); + else if (STREQ(name, "block_job_cancel")) + qemuCapsSet(qemuCaps, QEMU_CAPS_BLOCKJOB_SYNC); + else if (STREQ(name, "block-job-cancel")) + qemuCapsSet(qemuCaps, QEMU_CAPS_BLOCKJOB_ASYNC); }

Hum ... seems to me that we always set bits QEMU_CAPS_BLOCKJOB_SYNC and QEMU_CAPS_BLOCKJOB_ASYNC together, so do you envision cases where one was set and not the other ? If not why not merge them for the sake of one less bit to manage ?

ret = 0; @@ -3114,7 +3116,7 @@ qemuMonitorJSONDiskSnapshot(qemuMonitorPtr mon, virJSONValuePtr actions, const char *device, const char *file, const char *format, bool reuse) { - int ret; + int ret = -1; virJSONValuePtr cmd; virJSONValuePtr reply = NULL;

@@ -3132,14 +3134,13 @@ qemuMonitorJSONDiskSnapshot(qemuMonitorPtr mon, virJSONValuePtr actions, if (actions) { if (virJSONValueArrayAppend(actions, cmd) < 0) { virReportOOMError(); - ret = -1; } else { ret = 0; cmd = NULL; } } else { if ((ret = qemuMonitorJSONCommand(mon, cmd, &reply)) < 0) - goto cleanup; + goto cleanup;

if (qemuMonitorJSONHasError(reply, "CommandNotFound") && qemuMonitorCheckHMP(mon, "snapshot_blkdev")) { @@ -3388,39 +3389,43 @@ qemuMonitorJSONBlockJob(qemuMonitorPtr mon, const char *base, unsigned long bandwidth, virDomainBlockJobInfoPtr info, - int mode) + int mode, + bool async) { int ret = -1; virJSONValuePtr cmd = NULL; virJSONValuePtr reply = NULL; const char *cmd_name = NULL;

- if (base && mode != BLOCK_JOB_PULL) { + if (base && (mode != BLOCK_JOB_PULL || !async)) { qemuReportError(VIR_ERR_INTERNAL_ERROR, - _("only block pull supports base: %s"), base); + _("only modern block pull supports base: %s"), base); return -1; }

- if (mode == BLOCK_JOB_ABORT) { - cmd_name = "block_job_cancel"; + switch ((BLOCK_JOB_CMD) mode) { + case BLOCK_JOB_ABORT: + cmd_name = async ? "block-job-cancel" : "block_job_cancel"; cmd = qemuMonitorJSONMakeCommand(cmd_name, "s:device", device, NULL); - } else if (mode == BLOCK_JOB_INFO) { + break; + case BLOCK_JOB_INFO: cmd_name = "query-block-jobs"; cmd = qemuMonitorJSONMakeCommand(cmd_name, NULL); - } else if (mode == BLOCK_JOB_SPEED) { - cmd_name = "block_job_set_speed"; + break; + case BLOCK_JOB_SPEED: + cmd_name = async ? "block-job-set-speed" : "block_job_set_speed"; cmd = qemuMonitorJSONMakeCommand(cmd_name, "s:device", device, "U:value", bandwidth * 1024ULL * 1024ULL, NULL); - } else if (mode == BLOCK_JOB_PULL) { - cmd_name = "block_stream"; - if (base) - cmd = qemuMonitorJSONMakeCommand(cmd_name, "s:device", - device, "s:base", base, NULL); - else - cmd = qemuMonitorJSONMakeCommand(cmd_name, "s:device", - device, NULL); + break; + case BLOCK_JOB_PULL: + cmd_name = async ? "block-stream" : "block_stream"; + cmd = qemuMonitorJSONMakeCommand(cmd_name, + "s:device", device, + base ? "s:base" : NULL, base, + NULL); + break; }

if (!cmd) diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index a0f67aa..aacbb5f 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -253,7 +253,9 @@ int qemuMonitorJSONBlockJob(qemuMonitorPtr mon, const char *base, unsigned long bandwidth, virDomainBlockJobInfoPtr info, - int mode); + int mode, + bool async) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);

int qemuMonitorJSONSetLink(qemuMonitorPtr mon, const char *name,

Minor point about the extra bit, to me that's fine, ACK 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/

On Wed, Apr 11, 2012 at 08:17:11PM -0600, Eric Blake wrote:

On 04/11/2012 07:09 PM, Daniel Veillard wrote:

...

On Wed, Apr 11, 2012 at 05:40:34PM -0600, Eric Blake wrote:

...

RHEL 6.2 was released with an early version of block jobs, which only worked on the qed file format, where the commands were spelled with underscore (contrary to QMP style), and where 'block_job_cancel' was synchronous and did not trigger an event.

...

...

+++ b/src/qemu/qemu_monitor.h @@ -538,8 +538,9 @@ int qemuMonitorBlockJob(qemuMonitorPtr mon, const char *back, unsigned long bandwidth, virDomainBlockJobInfoPtr info, - int mode) - ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(5); + int mode, + bool async) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);

Hum, gcc wasn't complaining on an "ATTRIBUTE_NONNULL(5)" for something which wasn't a pointer ?

Context. There are lines omitted between the @@ marker and the changed lines; param 5 is virDomainBlockJobInfoPtr. In fact, I introduced a bug in commit 10ec36e2 for adding ATTRIBUTE_NONNULL(5) in the first place, since we already have existing callers that pass NULL.

...

...

+ else if (STREQ(name, "block_job_cancel")) + qemuCapsSet(qemuCaps, QEMU_CAPS_BLOCKJOB_SYNC); + else if (STREQ(name, "block-job-cancel")) + qemuCapsSet(qemuCaps, QEMU_CAPS_BLOCKJOB_ASYNC); }

Hum ... seems to me that we always set bits QEMU_CAPS_BLOCKJOB_SYNC and QEMU_CAPS_BLOCKJOB_ASYNC together, so do you envision cases where one was set and not the other ? If not why not merge them for the sake of one less bit to manage ?

On the contrary, you will set at most one of the two, and never both. That is, there is no qemu image that supports both 'block_job_cancel' and 'block-job-cancel' simultaneously, so we need the two bits to tell the two styles apart.

gahh, I misread, I see my mistake now :-) thanks ! 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/

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/

On Wed, Apr 11, 2012 at 05:40:36PM -0600, Eric Blake wrote:

Probably in the noise, but this will let us scale more efficiently as we recognize ever more qemu events.

s/ever/even/

* src/qemu/qemu_monitor_json.c (eventHandlers): Sort. (eventSearch): New helper function. (qemuMonitorJSONIOProcessEvent): Optimize event lookup. --- src/qemu/qemu_monitor_json.c | 54 ++++++++++++++++++++++++------------------ 1 files changed, 31 insertions(+), 23 deletions(-)

diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index d4d2c3e..f7d9ef2 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -66,36 +66,45 @@ static void qemuMonitorJSONHandleTrayChange(qemuMonitorPtr mon, virJSONValuePtr static void qemuMonitorJSONHandlePMWakeup(qemuMonitorPtr mon, virJSONValuePtr data); static void qemuMonitorJSONHandlePMSuspend(qemuMonitorPtr mon, virJSONValuePtr data);

-static struct { +typedef struct { const char *type; void (*handler)(qemuMonitorPtr mon, virJSONValuePtr data); -} eventHandlers[] = { - { "SHUTDOWN", qemuMonitorJSONHandleShutdown, }, - { "RESET", qemuMonitorJSONHandleReset, }, - { "POWERDOWN", qemuMonitorJSONHandlePowerdown, }, - { "STOP", qemuMonitorJSONHandleStop, }, - { "RTC_CHANGE", qemuMonitorJSONHandleRTCChange, }, - { "WATCHDOG", qemuMonitorJSONHandleWatchdog, }, +} qemuEventHandler; +static qemuEventHandler eventHandlers[] = { { "BLOCK_IO_ERROR", qemuMonitorJSONHandleIOError, }, - { "VNC_CONNECTED", qemuMonitorJSONHandleVNCConnect, }, - { "VNC_INITIALIZED", qemuMonitorJSONHandleVNCInitialize, }, - { "VNC_DISCONNECTED", qemuMonitorJSONHandleVNCDisconnect, }, { "BLOCK_JOB_COMPLETED", qemuMonitorJSONHandleBlockJob, }, + { "DEVICE_TRAY_MOVED", qemuMonitorJSONHandleTrayChange, }, + { "POWERDOWN", qemuMonitorJSONHandlePowerdown, }, + { "RESET", qemuMonitorJSONHandleReset, }, + { "RTC_CHANGE", qemuMonitorJSONHandleRTCChange, }, + { "SHUTDOWN", qemuMonitorJSONHandleShutdown, }, { "SPICE_CONNECTED", qemuMonitorJSONHandleSPICEConnect, }, - { "SPICE_INITIALIZED", qemuMonitorJSONHandleSPICEInitialize, }, { "SPICE_DISCONNECTED", qemuMonitorJSONHandleSPICEDisconnect, }, - { "DEVICE_TRAY_MOVED", qemuMonitorJSONHandleTrayChange, }, - { "WAKEUP", qemuMonitorJSONHandlePMWakeup, }, + { "SPICE_INITIALIZED", qemuMonitorJSONHandleSPICEInitialize, }, + { "STOP", qemuMonitorJSONHandleStop, }, { "SUSPEND", qemuMonitorJSONHandlePMSuspend, }, + { "VNC_CONNECTED", qemuMonitorJSONHandleVNCConnect, }, + { "VNC_DISCONNECTED", qemuMonitorJSONHandleVNCDisconnect, }, + { "VNC_INITIALIZED", qemuMonitorJSONHandleVNCInitialize, }, + { "WAKEUP", qemuMonitorJSONHandlePMWakeup, }, + { "WATCHDOG", qemuMonitorJSONHandleWatchdog, }, + /* We use bsearch, so keep this list sorted. */ };

+static int +qemuMonitorEventCompare(const void *key, const void *elt) +{ + const char *type = key; + const qemuEventHandler *handler = elt; + return strcmp(type, handler->type); +}

static int qemuMonitorJSONIOProcessEvent(qemuMonitorPtr mon, virJSONValuePtr obj) { const char *type; - int i; + qemuEventHandler *handler; VIR_DEBUG("mon=%p obj=%p", mon, obj);

type = virJSONValueObjectGetString(obj, "event"); @@ -105,14 +114,13 @@ qemuMonitorJSONIOProcessEvent(qemuMonitorPtr mon, return -1; }

- for (i = 0 ; i < ARRAY_CARDINALITY(eventHandlers) ; i++) { - if (STREQ(eventHandlers[i].type, type)) { - virJSONValuePtr data = virJSONValueObjectGet(obj, "data"); - VIR_DEBUG("handle %s handler=%p data=%p", type, - eventHandlers[i].handler, data); - (eventHandlers[i].handler)(mon, data); - break; - } + handler = bsearch(type, eventHandlers, ARRAY_CARDINALITY(eventHandlers), + sizeof(eventHandlers[0]), qemuMonitorEventCompare); + if (handler) { + virJSONValuePtr data = virJSONValueObjectGet(obj, "data"); + VIR_DEBUG("handle %s handler=%p data=%p", type, + handler->handler, data); + (handler->handler)(mon, data); } return 0; }

Okay, this should speed up the lookups somehow. I also appreciate separating the type and the array variable declaration, ACK 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/

On Wed, Apr 11, 2012 at 05:40:37PM -0600, Eric Blake wrote:

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. For older qemu, we synthesize the cancelation event, since qemu won't generate it.

Signed-off-by: Adam Litke <agl@us.ibm.com> Cc: Stefan Hajnoczi <stefanha@gmail.com> Signed-off-by: Eric Blake <eblake@redhat.com> --- src/qemu/qemu_driver.c | 75 ++++++++++++++++++++++++++++++++++++----- src/qemu/qemu_monitor_json.c | 45 +++++++++++++++++++++---- 2 files changed, 103 insertions(+), 17 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index f939a31..bb6089b 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; @@ -11608,6 +11608,9 @@ qemuDomainBlockJobImpl(virDomainPtr dom, const char *path, const char *base, char *device = NULL; int ret = -1; bool async = false; + virDomainEventPtr event = NULL; + int idx; + virDomainDiskDefPtr disk;

qemuDriverLock(driver); virUUIDFormat(dom->uuid, uuidstr); @@ -11631,10 +11634,10 @@ qemuDomainBlockJobImpl(virDomainPtr dom, const char *path, const char *base, goto cleanup; }

- device = qemuDiskPathToAlias(vm, path, NULL); - if (!device) { + device = qemuDiskPathToAlias(vm, path, &idx); + if (!device) goto cleanup; - } + disk = vm->def->disks[idx];

if (qemuDomainObjBeginJobWithDriver(driver, vm, QEMU_JOB_MODIFY) < 0) goto cleanup; @@ -11652,6 +11655,54 @@ qemuDomainBlockJobImpl(virDomainPtr dom, const char *path, const char *base, ret = qemuMonitorBlockJob(priv->mon, device, base, bandwidth, info, mode, async); qemuDomainObjExitMonitorWithDriver(driver, vm); + if (ret < 0) + goto endjob; + + /* With synchronous block cancel, we must synthesize an event, and + * we silently ignore the ABORT_ASYNC flag. With asynchronous + * block cancel, the event will come from qemu, but without the + * ABORT_ASYNC flag, we must block to guarantee synchronous + * operation. We do the waiting while still holding the VM job, + * to prevent newly scheduled block jobs from confusing us. + */ + if (mode == BLOCK_JOB_ABORT) { + if (!async) { + int type = VIR_DOMAIN_BLOCK_JOB_TYPE_PULL; + int status = VIR_DOMAIN_BLOCK_JOB_CANCELED; + event = virDomainEventBlockJobNewFromObj(vm, disk->src, type, + status); + } else if (!(flags & VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC)) { + while (1) { + /* Poll every 50ms */ + static struct timespec ts = { .tv_sec = 0, + .tv_nsec = 50 * 1000 * 1000ull }; + virDomainBlockJobInfo dummy; + + qemuDomainObjEnterMonitorWithDriver(driver, vm); + ret = qemuMonitorBlockJob(priv->mon, device, NULL, 0, &dummy, + BLOCK_JOB_INFO, async); + qemuDomainObjExitMonitorWithDriver(driver, vm); + + if (ret <= 0) + break; + + virDomainObjUnlock(vm); + qemuDriverUnlock(driver); + + nanosleep(&ts, NULL);

Okay, I was wondering how to justify the arbitrary 50ms, we use the same value on migration for potential user input. It's slightly over the human perception delay, but for those kind of operation that sounds fine...

+ 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) { @@ -11663,6 +11714,8 @@ cleanup: VIR_FREE(device); if (vm) virDomainObjUnlock(vm); + if (event) + qemuDomainEventQueue(driver, event); qemuDriverUnlock(driver); return ret; } @@ -11670,8 +11723,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 @@ -11679,7 +11733,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 @@ -11688,7 +11743,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 @@ -11699,10 +11754,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; }

diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index f7d9ef2..242889c 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);

typedef struct { const char *type; @@ -72,7 +73,8 @@ typedef struct { } qemuEventHandler; static qemuEventHandler eventHandlers[] = { { "BLOCK_IO_ERROR", qemuMonitorJSONHandleIOError, }, - { "BLOCK_JOB_COMPLETED", qemuMonitorJSONHandleBlockJob, }, + { "BLOCK_JOB_CANCELLED", qemuMonitorJSONHandleBlockJobCanceled, }, + { "BLOCK_JOB_COMPLETED", qemuMonitorJSONHandleBlockJobCompleted, }, { "DEVICE_TRAY_MOVED", qemuMonitorJSONHandleTrayChange, }, { "POWERDOWN", qemuMonitorJSONHandlePowerdown, }, { "RESET", qemuMonitorJSONHandleReset, }, @@ -762,13 +764,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"); @@ -793,11 +797,22 @@ 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 ((virConnectDomainEventBlockJobStatus) event) { + case VIR_DOMAIN_BLOCK_JOB_COMPLETED: + /* Make sure the whole device has been processed */ + if (offset != len) + event = VIR_DOMAIN_BLOCK_JOB_FAILED; + break; + case VIR_DOMAIN_BLOCK_JOB_CANCELED: + break; + case VIR_DOMAIN_BLOCK_JOB_FAILED: + case VIR_DOMAIN_BLOCK_JOB_LAST: + VIR_DEBUG("should not get here"); + break; + }

out: - qemuMonitorEmitBlockJob(mon, device, type, status); + qemuMonitorEmitBlockJob(mon, device, type, event); }

static void @@ -840,6 +855,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,

ACK, I was wondering how we could test for those events but since this requires an actual qemu-kvm process we really can't implement it as part of the existing python event test program. 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/