On 07/29/14 06:20, Eric Blake wrote: Seems reasonable to store the state of the final step here. I'd go with _NONE here and ... ... as this describes the ready state of the mirroring operation I'd go with virDomainDiskMirrorState and rename those fields accordingly. Without that it's not clear what the enum describes. Otherwise looks good. ACK if you change the enum name and default value name. Peter

We were not directly saving the domain XML to file after starting or finishing a blockcopy. Without the startup write, a libvirtd restart in the middle of a copy job would forget that the job was underway. Then at pivot, we were indirectly writing new XML in reaction to events that occur as we stop and restart the guest CPUs. But there was a race: since pivot is an async action, it is possible that libvirtd is restarted before the pivot completes, so if XML changes during the event, that change was not written. The original blockcopy code cleared out the <mirror> element prior to restarting the CPUs, but this is also a race, observed if a user does an async pivot and a dumpxml before the event occurs. Furthermore, this race will interfere with active commit in a future patch, because that code will rely on the <mirror> element at the time of the qemu event to determine whether to inform the user of a normal commit or an active commit. Fix things by saving state any time we modify live XML, while delaying XML disk modifications until after the event completes. We still need a to teach libvirtd restarts to examine all existing <mirror> elements to see if the job completed in the meantime (that is, if libvirtd misses the event, the updated state still needs to be updated in live XML), but that will be a later patch, in part because we also need to to start taking advantage of newer qemu's ability to keep the job around after completion rather than the current usage where the job disappears both on error and on success. * src/qemu/qemu_driver.c (qemuDomainBlockCopy): Track XML change on disk. (qemuDomainBlockJobImpl, qemuDomainBlockPivot): Move job-end XML rewrites... * src/qemu/qemu_process.c (qemuProcessHandleBlockJob): ...here. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- src/qemu/qemu_driver.c | 121 ++++++++++++++++++++++++++++++------------------ src/qemu/qemu_process.c | 53 ++++++++++++++++----- 2 files changed, 117 insertions(+), 57 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index acc9ef0..180333c 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -14911,38 +14911,43 @@ qemuDomainBlockPivot(virConnectPtr conn, virSecurityManagerSetDiskLabel(driver->securityManager, vm->def, disk) < 0)) goto cleanup; - /* Attempt the pivot. */ + /* Attempt the pivot. Record the attempt now, to prevent duplicate + * attempts; but the actual disk change will be made when emitting + * the event. + * XXX On libvirtd restarts, if we missed the qemu event, we need + * to double check what state qemu is in. + * XXX We should be using qemu's rerror flag to make sure the job + * remains alive until we know it's final state. + * XXX If the abort command is synchronous but the qemu event says + * that pivot failed, we need to reflect that failure into the + * overall return value. */ + disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_PIVOT; qemuDomainObjEnterMonitor(driver, vm); ret = qemuMonitorDrivePivot(priv->mon, device, disk->mirror->path, format); qemuDomainObjExitMonitor(driver, vm); - if (ret == 0) { - /* XXX We want to revoke security labels and disk lease, as - * well as audit that revocation, before dropping the original - * source. But it gets tricky if both source and mirror share - * common backing files (we want to only revoke the non-shared - * portion of the chain, and is made more difficult by the - * fact that we aren't tracking the full chain ourselves; so - * for now, we leak the access to the original. */ - virStorageSourceFree(oldsrc); - oldsrc = NULL; - } else { + if (ret < 0) { /* On failure, qemu abandons the mirror, and reverts back to * the source disk (RHEL 6.3 has a bug where the revert could * cause catastrophic failure in qemu, but we don't need to * worry about it here as it is not an upstream qemu problem. */ /* XXX should we be parsing the exact qemu error, or calling * 'query-block', to see what state we really got left in - * before killing the mirroring job? And just as on the - * success case, there's security labeling to worry about. */ + * before killing the mirroring job? + * XXX We want to revoke security labels and disk lease, as + * well as audit that revocation, before dropping the original + * source. But it gets tricky if both source and mirror share + * common backing files (we want to only revoke the non-shared + * portion of the chain); so for now, we leak the access to + * the original. */ virStorageSourceFree(disk->mirror); + disk->mirror = NULL; + disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_DEFAULT; } - - disk->mirror = NULL; - disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_DEFAULT; + if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) + ret = -1; cleanup: - /* revert to original disk def on failure */ if (oldsrc) disk->src = oldsrc; @@ -14985,6 +14990,8 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm, unsigned int baseIndex = 0; char *basePath = NULL; char *backingPath = NULL; + virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + bool save = false; if (!virDomainObjIsActive(vm)) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", @@ -15038,19 +15045,30 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm, disk->dst); goto endjob; } - if (mode == BLOCK_JOB_ABORT && - (flags & VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT) && - !(async && disk->mirror)) { - virReportError(VIR_ERR_OPERATION_INVALID, - _("pivot of disk '%s' requires an active copy job"), - disk->dst); - goto endjob; - } + if (mode == BLOCK_JOB_ABORT) { + if ((flags & VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT) && + !(async && disk->mirror)) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("pivot of disk '%s' requires an active copy job"), + disk->dst); + goto endjob; + } + if (disk->mirrorState != VIR_DOMAIN_DISK_MIRROR_DEFAULT && + disk->mirrorState != VIR_DOMAIN_DISK_MIRROR_READY) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("another job on disk '%s' is still being ended"), + disk->dst); + goto endjob; + } - if (disk->mirror && mode == BLOCK_JOB_ABORT && - (flags & VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT)) { - ret = qemuDomainBlockPivot(conn, driver, vm, device, disk); - goto waitjob; + if (disk->mirror && (flags & VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT)) { + ret = qemuDomainBlockPivot(conn, driver, vm, device, disk); + goto waitjob; + } + if (disk->mirror) { + disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_ABORT; + save = true; + } } if (base && @@ -15089,36 +15107,40 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm, ret = qemuMonitorBlockJob(priv->mon, device, basePath, backingPath, bandwidth, info, mode, async); qemuDomainObjExitMonitor(driver, vm); - if (ret < 0) + if (ret < 0) { + if (mode == BLOCK_JOB_ABORT && disk->mirror) + disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_DEFAULT; goto endjob; + } /* Snoop block copy operations, so future cancel operations can * avoid checking if pivot is safe. */ if (mode == BLOCK_JOB_INFO && ret == 1 && disk->mirror && - info->cur == info->end && info->type == VIR_DOMAIN_BLOCK_JOB_TYPE_COPY) + info->cur == info->end && !disk->mirrorState) { disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_READY; - - /* A successful block job cancelation stops any mirroring. */ - if (mode == BLOCK_JOB_ABORT && disk->mirror) { - /* XXX We should also revoke security labels and disk lease on - * the mirror, and audit that fact, before dropping things. */ - virStorageSourceFree(disk->mirror); - disk->mirror = NULL; - disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_DEFAULT; + save = true; } waitjob: + /* If we have made changes to XML due to a copy job, make a best + * effort to save it now. But we can ignore failure, since there + * will be further changes when the event marks completion. */ + if (save) + ignore_value(virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm)); + /* 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. */ + * block cancel, the event will come from qemu and will update the + * XML as appropriate, 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) { /* Older qemu that lacked async reporting also lacked - * active commit, so we can hardcode the event to pull. - * We have to generate two variants of the event. */ + * blockcopy and active commit, so we can hardcode the + * event to pull, and we know the XML doesn't need + * updating. We have to generate two event variants. */ int type = VIR_DOMAIN_BLOCK_JOB_TYPE_PULL; int status = VIR_DOMAIN_BLOCK_JOB_CANCELED; event = virDomainEventBlockJobNewFromObj(vm, disk->src->path, type, @@ -15126,6 +15148,8 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm, event2 = virDomainEventBlockJob2NewFromObj(vm, disk->dst, type, status); } else if (!(flags & VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC)) { + /* XXX If the event reports failure, we should reflect + * that back into the return status of this API call. */ while (1) { /* Poll every 50ms */ static struct timespec ts = { .tv_sec = 0, @@ -15163,6 +15187,7 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm, } cleanup: + virObjectUnref(cfg); VIR_FREE(basePath); VIR_FREE(backingPath); VIR_FREE(device); @@ -15394,6 +15419,10 @@ qemuDomainBlockCopy(virDomainObjPtr vm, disk->mirror = mirror; mirror = NULL; + if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) + VIR_WARN("Unable to save status on vm %s after state change", + vm->def->name); + endjob: if (need_unlink && unlink(dest)) VIR_WARN("unable to unlink just-created %s", dest); diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 73ad27d..e770a77 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -1016,6 +1016,8 @@ qemuProcessHandleBlockJob(qemuMonitorPtr mon ATTRIBUTE_UNUSED, virObjectEventPtr event2 = NULL; const char *path; virDomainDiskDefPtr disk; + virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + bool save = false; virObjectLock(vm); disk = qemuProcessFindDomainDiskByAlias(vm, diskAlias); @@ -1027,29 +1029,58 @@ qemuProcessHandleBlockJob(qemuMonitorPtr mon ATTRIBUTE_UNUSED, event = virDomainEventBlockJobNewFromObj(vm, path, type, status); event2 = virDomainEventBlockJob2NewFromObj(vm, disk->dst, type, status); - /* XXX If we completed a block pull or commit, then recompute - * the cached backing chain to match. Better would be storing - * the chain ourselves rather than reprobing, but this - * requires modifying domain_conf and our XML to fully track - * the chain across libvirtd restarts. For that matter, if - * qemu gains support for committing the active layer, we have - * to update disk->src. */ - if ((type == VIR_DOMAIN_BLOCK_JOB_TYPE_PULL || - type == VIR_DOMAIN_BLOCK_JOB_TYPE_COMMIT) && - status == VIR_DOMAIN_BLOCK_JOB_COMPLETED) + + /* If we completed a block pull or commit, then update the XML + * to match. */ + if (status == VIR_DOMAIN_BLOCK_JOB_COMPLETED) { + bool has_mirror = !!disk->mirror; + + if (disk->mirrorState == VIR_DOMAIN_DISK_MIRROR_PIVOT) { + /* XXX We want to revoke security labels and disk + * lease, as well as audit that revocation, before + * dropping the original source. But it gets tricky + * if both source and mirror share common backing + * files (we want to only revoke the non-shared + * portion of the chain); so for now, we leak the + * access to the original. */ + virStorageSourceFree(disk->src); + disk->src = disk->mirror; + disk->mirror = NULL; + } + if (has_mirror) { + virStorageSourceFree(disk->mirror); + disk->mirror = NULL; + save = disk->mirrorState != VIR_DOMAIN_DISK_MIRROR_DEFAULT; + disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_DEFAULT; + } + /* Recompute the cached backing chain to match our + * updates. Better would be storing the chain ourselves + * rather than reprobing, but we haven't quite completed + * that conversion to use our XML tracking. */ qemuDomainDetermineDiskChain(driver, vm, disk, true); - if (disk->mirror && type == VIR_DOMAIN_BLOCK_JOB_TYPE_COPY) { + } + + if (disk->mirror && + type == VIR_DOMAIN_BLOCK_JOB_TYPE_COPY) { if (status == VIR_DOMAIN_BLOCK_JOB_READY) { disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_READY; + save = true; } else if (status == VIR_DOMAIN_BLOCK_JOB_FAILED) { virStorageSourceFree(disk->mirror); disk->mirror = NULL; disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_DEFAULT; + save = true; } } } + if (save) { + if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) + VIR_WARN("Unable to save status on vm %s after block job", + vm->def->name); + } virObjectUnlock(vm); + virObjectUnref(cfg); if (event) qemuDomainEventQueue(driver, event); -- 1.9.3

On 07/29/2014 05:40 AM, Peter Krempa wrote: Yeah, I struggled on that for a while. Basically, once a job completes, we have to clean up disk->mirror whether it was abort or pivot; but if it was pivot, we also have to adjust the source to be the former mirror contents. But I agree that a minor tweak to the flow makes it easier to read. Also correct. After rebasing on top of 1/4, here's what I squashed in to improve the control flow, before pushing 1 and 2. I'll wait on 3 and 4 for any other opinions on whether active commit deserves to be in rc2. diff --git i/src/qemu/qemu_process.c w/src/qemu/qemu_process.c index 1340c8d..48da843 100644 --- i/src/qemu/qemu_process.c +++ w/src/qemu/qemu_process.c @@ -1033,8 +1033,6 @@ qemuProcessHandleBlockJob(qemuMonitorPtr mon ATTRIBUTE_UNUSED, /* If we completed a block pull or commit, then update the XML * to match. */ if (status == VIR_DOMAIN_BLOCK_JOB_COMPLETED) { - bool has_mirror = !!disk->mirror; - if (disk->mirrorState == VIR_DOMAIN_DISK_MIRROR_STATE_PIVOT) { /* XXX We want to revoke security labels and disk * lease, as well as audit that revocation, before @@ -1045,22 +1043,19 @@ qemuProcessHandleBlockJob(qemuMonitorPtr mon ATTRIBUTE_UNUSED, * access to the original. */ virStorageSourceFree(disk->src); disk->src = disk->mirror; - disk->mirror = NULL; - } - if (has_mirror) { + } else { virStorageSourceFree(disk->mirror); - disk->mirror = NULL; - save = disk->mirrorState != VIR_DOMAIN_DISK_MIRROR_STATE_NONE; - disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_NONE; } + /* Recompute the cached backing chain to match our * updates. Better would be storing the chain ourselves * rather than reprobing, but we haven't quite completed * that conversion to use our XML tracking. */ + disk->mirror = NULL; + save = disk->mirrorState != VIR_DOMAIN_DISK_MIRROR_STATE_NONE; + disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_NONE; qemuDomainDetermineDiskChain(driver, vm, disk, true); - } - - if (disk->mirror && type == VIR_DOMAIN_BLOCK_JOB_TYPE_COPY) { + } else if (disk->mirror && type == VIR_DOMAIN_BLOCK_JOB_TYPE_COPY) { if (status == VIR_DOMAIN_BLOCK_JOB_READY) { disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_READY; save = true; -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

A future patch is going to wire up qemu active block commit jobs; but as they have similar events and are canceled/pivoted in the same way as block copy jobs, it is easiest to track all bookkeeping for the commit job by reusing the <mirror> element. This patch adds domain XML to track which job was responsible for creating a mirroring situation, and adds a job='copy' attribute to all existing uses of <mirror>. Along the way, it also massages the qemu monitor backend to read the new field in order to generate the correct type of libvirt job (even though it requires a future patch to actually cause a qemu event that can be reported as an active commit). It also prepares to update persistent XML to match changes made to live XML when a copy completes. * docs/schemas/domaincommon.rng: Enhance schema. * docs/formatdomain.html.in: Document it. * src/conf/domain_conf.h (_virDomainDiskDef): Add a field. * src/conf/domain_conf.c (virDomainBlockJobType): String conversion. (virDomainDiskDefParseXML): Parse job type. (virDomainDiskDefFormat): Output job type. * src/qemu/qemu_process.c (qemuProcessHandleBlockJob): Distinguish active from regular commit. * src/qemu/qemu_driver.c (qemuDomainBlockCopy): Set job type. (qemuDomainBlockPivot, qemuDomainBlockJobImpl): Clean up job type on completion. * tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-mirror-old.xml: Update tests. * tests/qemuxml2argvdata/qemuxml2argv-disk-mirror.xml: Likewise. * tests/qemuxml2argvdata/qemuxml2argv-disk-active-commit.xml: New file. * tests/qemuxml2xmltest.c (mymain): Drive new test. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- docs/formatdomain.html.in | 15 +++++---- docs/schemas/domaincommon.rng | 13 ++++++++ src/conf/domain_conf.c | 33 ++++++++++++++++++- src/conf/domain_conf.h | 1 + src/qemu/qemu_driver.c | 2 ++ src/qemu/qemu_process.c | 37 +++++++++++++++++++++- .../qemuxml2argv-disk-active-commit.xml | 37 ++++++++++++++++++++++ .../qemuxml2argvdata/qemuxml2argv-disk-mirror.xml | 6 ++-- .../qemuxml2xmlout-disk-mirror-old.xml | 4 +-- tests/qemuxml2xmltest.c | 1 + 10 files changed, 136 insertions(+), 13 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-active-commit.xml diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 4cb4289..0e7fad2 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -1898,16 +1898,19 @@ </dd> <dt><code>mirror</code></dt> <dd> - This element is present if the hypervisor has started a block - copy operation (via the <code>virDomainBlockRebase</code> - API), where the mirror location in the <code>source</code> - sub-element will eventually have the same contents as the - source, and with the file format in the + This element is present if the hypervisor has started a + long-running block job operation, where the mirror location in + the <code>source</code> sub-element will eventually have the + same contents as the source, and with the file format in the sub-element <code>format</code> (which might differ from the format of the source). The details of the <code>source</code> sub-element are determined by the <code>type</code> attribute of the mirror, similar to what is done for the - overall <code>disk</code> device element. The + overall <code>disk</code> device element. The <code>job</code> + attribute mentions which API started the operation ("copy" for + the <code>virDomainBlockRebase</code> API, or "active-commit" + for the <code>virDomainBlockCommit</code> + API), <span class="since">since 1.2.7</span>. The attribute <code>ready</code>, if present, tracks progress of the job: <code>yes</code> if the disk is known to be ready to pivot, or, <span class="since">since diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 389c8c9..3911a9f 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -4265,6 +4265,13 @@ </attribute> </optional> <optional> + <attribute name='job'> + <choice> + <value>copy</value> + </choice> + </attribute> + </optional> + <optional> <interleave> <ref name='diskSourceFile'/> <optional> @@ -4274,6 +4281,12 @@ </optional> </group> <group> <!-- preferred format --> + <attribute name='job'> + <choice> + <value>copy</value> + <value>active-commit</value> + </choice> + </attribute> <interleave> <ref name="diskSource"/> <optional> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 1c8f8cf..1a76ce7 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -753,6 +753,12 @@ VIR_ENUM_IMPL(virDomainDiskMirror, VIR_DOMAIN_DISK_MIRROR_LAST, "abort", "pivot") +/* Internal mapping: subset of block job types that can be present in + * <mirror> XML (remaining types are not two-phase). */ +VIR_ENUM_DECL(virDomainBlockJob) +VIR_ENUM_IMPL(virDomainBlockJob, VIR_DOMAIN_BLOCK_JOB_TYPE_LAST, + "", "", "copy", "", "active-commit") + #define VIR_DOMAIN_XML_WRITE_FLAGS VIR_DOMAIN_XML_SECURE #define VIR_DOMAIN_XML_READ_FLAGS VIR_DOMAIN_XML_INACTIVE @@ -5437,10 +5443,26 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, xmlStrEqual(cur->name, BAD_CAST "mirror") && !(flags & VIR_DOMAIN_XML_INACTIVE)) { char *ready; + char *blockJob; if (VIR_ALLOC(def->mirror) < 0) goto error; + blockJob = virXMLPropString(cur, "job"); + if (blockJob) { + def->mirrorJob = virDomainBlockJobTypeFromString(blockJob); + if (def->mirrorJob <= 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown mirror job type '%s'"), + blockJob); + VIR_FREE(blockJob); + goto error; + } + VIR_FREE(blockJob); + } else { + def->mirrorJob = VIR_DOMAIN_BLOCK_JOB_TYPE_COPY; + } + mirrorType = virXMLPropString(cur, "type"); if (mirrorType) { def->mirror->type = virStorageTypeFromString(mirrorType); @@ -5463,6 +5485,12 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, _("mirror requires file name")); goto error; } + if (def->mirrorJob != VIR_DOMAIN_BLOCK_JOB_TYPE_COPY) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("mirror without type only supported " + "by copy job")); + goto error; + } mirrorFormat = virXMLPropString(cur, "format"); } if (mirrorFormat) { @@ -15282,10 +15310,13 @@ virDomainDiskDefFormat(virBufferPtr buf, formatStr = virStorageFileFormatTypeToString(def->mirror->format); virBufferAsprintf(buf, "<mirror type='%s'", virStorageTypeToString(def->mirror->type)); - if (def->mirror->type == VIR_STORAGE_TYPE_FILE) { + if (def->mirror->type == VIR_STORAGE_TYPE_FILE && + def->mirrorJob == VIR_DOMAIN_BLOCK_JOB_TYPE_COPY) { virBufferEscapeString(buf, " file='%s'", def->mirror->path); virBufferEscapeString(buf, " format='%s'", formatStr); } + virBufferEscapeString(buf, " job='%s'", + virDomainBlockJobTypeToString(def->mirrorJob)); if (def->mirrorState) { const char *mirror; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index bc8966d..00ad137 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -632,6 +632,7 @@ struct _virDomainDiskDef { virStorageSourcePtr mirror; int mirrorState; /* enum virDomainDiskMirror */ + int mirrorJob; /* virDomainBlockJobType */ struct { unsigned int cylinders; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 180333c..683eaf3 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -14943,6 +14943,7 @@ qemuDomainBlockPivot(virConnectPtr conn, virStorageSourceFree(disk->mirror); disk->mirror = NULL; disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_DEFAULT; + disk->mirrorJob = VIR_DOMAIN_BLOCK_JOB_TYPE_UNKNOWN; } if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) ret = -1; @@ -15418,6 +15419,7 @@ qemuDomainBlockCopy(virDomainObjPtr vm, need_unlink = false; disk->mirror = mirror; mirror = NULL; + disk->mirrorJob = VIR_DOMAIN_BLOCK_JOB_TYPE_COPY; if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) VIR_WARN("Unable to save status on vm %s after state change", diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index e770a77..4ea5493 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -1017,6 +1017,7 @@ qemuProcessHandleBlockJob(qemuMonitorPtr mon ATTRIBUTE_UNUSED, const char *path; virDomainDiskDefPtr disk; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + virDomainDiskDefPtr persistDisk = NULL; bool save = false; virObjectLock(vm); @@ -1025,6 +1026,9 @@ qemuProcessHandleBlockJob(qemuMonitorPtr mon ATTRIBUTE_UNUSED, if (disk) { /* Have to generate two variants of the event for old vs. new * client callbacks */ + if (type == VIR_DOMAIN_BLOCK_JOB_TYPE_COMMIT && + disk->mirrorJob == VIR_DOMAIN_BLOCK_JOB_TYPE_ACTIVE_COMMIT) + type = disk->mirrorJob; path = virDomainDiskGetSource(disk); event = virDomainEventBlockJobNewFromObj(vm, path, type, status); event2 = virDomainEventBlockJob2NewFromObj(vm, disk->dst, type, @@ -1036,6 +1040,31 @@ qemuProcessHandleBlockJob(qemuMonitorPtr mon ATTRIBUTE_UNUSED, bool has_mirror = !!disk->mirror; if (disk->mirrorState == VIR_DOMAIN_DISK_MIRROR_PIVOT) { + if (vm->newDef) { + int indx = virDomainDiskIndexByName(vm->newDef, disk->dst, + false); + virStorageSourcePtr copy = NULL; + + if (indx >= 0) { + persistDisk = vm->newDef->disks[indx]; + copy = virStorageSourceCopy(disk->mirror, false); + if (virStorageSourceInitChainElement(copy, + persistDisk->src, + false) < 0) { + VIR_WARN("Unable to update persistent definition " + "on vm %s after block job", + vm->def->name); + virStorageSourceFree(copy); + copy = NULL; + persistDisk = NULL; + } + } + if (copy) { + virStorageSourceFree(persistDisk->src); + persistDisk->src = copy; + } + } + /* XXX We want to revoke security labels and disk * lease, as well as audit that revocation, before * dropping the original source. But it gets tricky @@ -1061,7 +1090,8 @@ qemuProcessHandleBlockJob(qemuMonitorPtr mon ATTRIBUTE_UNUSED, } if (disk->mirror && - type == VIR_DOMAIN_BLOCK_JOB_TYPE_COPY) { + (type == VIR_DOMAIN_BLOCK_JOB_TYPE_COPY || + type == VIR_DOMAIN_BLOCK_JOB_TYPE_ACTIVE_COMMIT)) { if (status == VIR_DOMAIN_BLOCK_JOB_READY) { disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_READY; save = true; @@ -1069,6 +1099,7 @@ qemuProcessHandleBlockJob(qemuMonitorPtr mon ATTRIBUTE_UNUSED, virStorageSourceFree(disk->mirror); disk->mirror = NULL; disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_DEFAULT; + disk->mirrorJob = VIR_DOMAIN_BLOCK_JOB_TYPE_UNKNOWN; save = true; } } @@ -1078,6 +1109,10 @@ qemuProcessHandleBlockJob(qemuMonitorPtr mon ATTRIBUTE_UNUSED, if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) VIR_WARN("Unable to save status on vm %s after block job", vm->def->name); + if (persistDisk && virDomainSaveConfig(cfg->configDir, + vm->newDef) < 0) + VIR_WARN("Unable to update persistent definition on vm %s " + "after block job", vm->def->name); } virObjectUnlock(vm); virObjectUnref(cfg); diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-active-commit.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-active-commit.xml new file mode 100644 index 0000000..6ba5724 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-active-commit.xml @@ -0,0 +1,37 @@ +<domain type='qemu' id='1'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='file' device='disk'> + <driver name='qemu' type='qcow2'/> + <source file='/tmp/HostVG/QEMUGuest1-snap'/> + <backingStore type='block' index='1'> + <format type='raw'/> + <source dev='/dev/HostVG/QEMUGuest1'/> + <backingStore/> + </backingStore> + <mirror type='block' job='active-commit'> + <format type='raw'/> + <source dev='/dev/HostVG/QEMUGuest1'/> + </mirror> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='usb' index='0'/> + <controller type='ide' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-mirror.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-mirror.xml index fa7af31..46f2a3e 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-disk-mirror.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-mirror.xml @@ -17,7 +17,7 @@ <disk type='block' device='disk'> <source dev='/dev/HostVG/QEMUGuest1'/> <backingStore/> - <mirror type='file' file='/dev/HostVG/QEMUGuest1Copy' ready='yes'> + <mirror type='file' file='/dev/HostVG/QEMUGuest1Copy' job='copy' ready='yes'> <source file='/dev/HostVG/QEMUGuest1Copy'/> </mirror> <target dev='hda' bus='ide'/> @@ -33,7 +33,7 @@ <disk type='file' device='disk'> <source file='/tmp/data.img'/> <backingStore/> - <mirror type='file' file='/tmp/copy.img' format='qcow2'> + <mirror type='file' file='/tmp/copy.img' format='qcow2' job='copy'> <format type='qcow2'/> <source file='/tmp/copy.img'/> </mirror> @@ -42,7 +42,7 @@ <disk type='file' device='disk'> <source file='/tmp/logs.img'/> <backingStore/> - <mirror type='file' file='/tmp/logcopy.img' format='qcow2' ready='abort'> + <mirror type='file' file='/tmp/logcopy.img' format='qcow2' job='copy' ready='abort'> <format type='qcow2'/> <source file='/tmp/logcopy.img'/> </mirror> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-mirror-old.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-mirror-old.xml index 72b03c9..abec180 100644 --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-mirror-old.xml +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-mirror-old.xml @@ -17,7 +17,7 @@ <disk type='block' device='disk'> <source dev='/dev/HostVG/QEMUGuest1'/> <backingStore/> - <mirror type='file' file='/dev/HostVG/QEMUGuest1Copy' ready='yes'> + <mirror type='file' file='/dev/HostVG/QEMUGuest1Copy' job='copy' ready='yes'> <source file='/dev/HostVG/QEMUGuest1Copy'/> </mirror> <target dev='hda' bus='ide'/> @@ -33,7 +33,7 @@ <disk type='file' device='disk'> <source file='/tmp/data.img'/> <backingStore/> - <mirror type='file' file='/tmp/copy.img' format='qcow2'> + <mirror type='file' file='/tmp/copy.img' format='qcow2' job='copy'> <format type='qcow2'/> <source file='/tmp/copy.img'/> </mirror> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index cefe05b..8966b9a 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -229,6 +229,7 @@ mymain(void) DO_TEST_DIFFERENT("disk-mirror-old"); DO_TEST_FULL("disk-mirror", false, WHEN_ACTIVE); DO_TEST_FULL("disk-mirror", true, WHEN_INACTIVE); + DO_TEST_FULL("disk-active-commit", false, WHEN_ACTIVE); DO_TEST("graphics-listen-network"); DO_TEST("graphics-vnc"); DO_TEST("graphics-vnc-websocket"); -- 1.9.3

On 07/29/2014 07:03 AM, Peter Krempa wrote: Maybe, but then the next commit doesn't look so deceptively simple, and I _did_ mention it in the commit message :) It's part of the effort. The other half is actually being able to restart a blockcopy operation across qemu restarts (not available in qemu 2.1, it requires a persistent bitmap; but might be available in qemu 2.2). The reason that active commit can be done on a persistent domain is that you don't lose progress - if you spend 4 minutes getting 80% of the way, then the guest shuts down, then you restart qemu and restart the commit operation, then you only need 1 more minute for the remaining 20%. That is, aborting a commit job is not expensive. On the other hand, aborting a copy job at 80% progress, and then starting a new one, will end up copying all 100% of the file (5 more minutes, for a total of 9 instead of 5), because without a persistent bitmap, qemu loses track of how much of the copy has already been made. So for now, I'm still going to require transient domain for blockcopy. That, and I _really_ need a new API that lets you target a <disk> XML element as the copy destination (I ran out of time for 1.2.7, but 1.2.8 is looking good). Plus we need a way to expose qemu's drive-backup operation; the existing blockcopy code is crammed into virDomainBlockRebase where we don't have good exposure for pre- vs. post- snapshot; but a new virDomainBlockCopy() with an XML destination and proper flags would be better for it. For those less familiar with the difference between the two qemu commands, here's a timeline: A B C +---------------------------------------------------------- ^ ^ ^ request drive-mirror | | sync ready request complete/pivot the copy taken by drive-mirror is consistent at point C, which may be several minutes after the user actually requested the copy. So whatever the guest saw at point A may be long gone by the time the copy is actually finalized; planning for user point-in-time snapshots requires forethought to actually get a mirror up and running long before the user has a need for the snapshot. However, with that planning, the time between declaring the snapshot complete and actually having the snapshot to work with is nearly instant. A B +------------------------------------------ ^ ^ request drive-backup | backup complete the copy taken by drive-backup is not consistent until point B, but its contents match what the guest saw at point A. This is much nicer for point-in-time semantics, although it does mean that the user has to wait from the time they requested a backup until it is actually ready for them to use. I'll wait for a third-party opinion, then (maybe DV or danpb will weigh in before DV tries to cut rc2 in the next 24 hours...) -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org