On 07/29/2014 05:20 AM, Peter Krempa wrote:

On 07/29/14 06:20, Eric Blake wrote:

...

Doing a blockcopy operation across a libvirtd restart is not very robust at the moment. In particular, we are clearing the <mirror> element prior to telling qemu to finish the job; and thanks to the ability to request async completion, the user can easily regain control prior to qemu actually finishing the effort, and they should be able to poll the domain XML to see if the job is still going.

...

+typedef enum { + VIR_DOMAIN_DISK_MIRROR_DEFAULT = 0, /* No job, or job still not synced */

I'd go with _NONE here and ...

...

+ VIR_DOMAIN_DISK_MIRROR_READY, /* Job in second phase */ + VIR_DOMAIN_DISK_MIRROR_ABORT, /* Job aborted, waiting for event */ + VIR_DOMAIN_DISK_MIRROR_PIVOT, /* Job pivoted, waiting for event */ + + VIR_DOMAIN_DISK_MIRROR_LAST +} virDomainDiskMirror;

... 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.

I'm squashing this in, and pushing. diff --git i/src/conf/domain_conf.c w/src/conf/domain_conf.c index 1c8f8cf..c2a81f1 100644 --- i/src/conf/domain_conf.c +++ w/src/conf/domain_conf.c @@ -747,8 +747,8 @@ VIR_ENUM_IMPL(virDomainDiskDiscard, VIR_DOMAIN_DISK_DISCARD_LAST, "unmap", "ignore") -VIR_ENUM_IMPL(virDomainDiskMirror, VIR_DOMAIN_DISK_MIRROR_LAST, - "default", +VIR_ENUM_IMPL(virDomainDiskMirrorState, VIR_DOMAIN_DISK_MIRROR_STATE_LAST, + "none", "yes", "abort", "pivot") @@ -5488,8 +5488,8 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, } ready = virXMLPropString(cur, "ready"); if (ready) { - def->mirrorState = virDomainDiskMirrorTypeFromString(ready); - if (def->mirrorState < 0) { + if ((def->mirrorState = + virDomainDiskMirrorStateTypeFromString(ready)) < 0) { virReportError(VIR_ERR_XML_ERROR, _("unknown mirror ready state %s"), ready); @@ -15289,7 +15289,7 @@ virDomainDiskDefFormat(virBufferPtr buf, if (def->mirrorState) { const char *mirror; - mirror = virDomainDiskMirrorTypeToString(def->mirrorState); + mirror = virDomainDiskMirrorStateTypeToString(def->mirrorState); virBufferEscapeString(buf, " ready='%s'", mirror); } virBufferAddLit(buf, ">\n"); diff --git i/src/conf/domain_conf.h w/src/conf/domain_conf.h index bc8966d..2df3ae2 100644 --- i/src/conf/domain_conf.h +++ w/src/conf/domain_conf.h @@ -611,13 +611,13 @@ typedef virDomainBlockIoTuneInfo *virDomainBlockIoTuneInfoPtr; typedef enum { - VIR_DOMAIN_DISK_MIRROR_DEFAULT = 0, /* No job, or job still not synced */ - VIR_DOMAIN_DISK_MIRROR_READY, /* Job in second phase */ - VIR_DOMAIN_DISK_MIRROR_ABORT, /* Job aborted, waiting for event */ - VIR_DOMAIN_DISK_MIRROR_PIVOT, /* Job pivoted, waiting for event */ + VIR_DOMAIN_DISK_MIRROR_STATE_NONE = 0, /* No job, or job still not synced */ + VIR_DOMAIN_DISK_MIRROR_STATE_READY, /* Job in second phase */ + VIR_DOMAIN_DISK_MIRROR_STATE_ABORT, /* Job aborted, waiting for event */ + VIR_DOMAIN_DISK_MIRROR_STATE_PIVOT, /* Job pivoted, waiting for event */ - VIR_DOMAIN_DISK_MIRROR_LAST -} virDomainDiskMirror; + VIR_DOMAIN_DISK_MIRROR_STATE_LAST +} virDomainDiskMirrorState; /* Stores the virtual disk configuration */ @@ -631,7 +631,7 @@ struct _virDomainDiskDef { int removable; /* enum virTristateSwitch */ virStorageSourcePtr mirror; - int mirrorState; /* enum virDomainDiskMirror */ + int mirrorState; /* enum virDomainDiskMirrorState */ struct { unsigned int cylinders; @@ -2566,7 +2566,7 @@ VIR_ENUM_DECL(virDomainDiskIo) VIR_ENUM_DECL(virDomainDeviceSGIO) VIR_ENUM_DECL(virDomainDiskTray) VIR_ENUM_DECL(virDomainDiskDiscard) -VIR_ENUM_DECL(virDomainDiskMirror) +VIR_ENUM_DECL(virDomainDiskMirrorState) VIR_ENUM_DECL(virDomainController) VIR_ENUM_DECL(virDomainControllerModelPCI) VIR_ENUM_DECL(virDomainControllerModelSCSI) diff --git i/src/qemu/qemu_driver.c w/src/qemu/qemu_driver.c index acc9ef0..836adba 100644 --- i/src/qemu/qemu_driver.c +++ w/src/qemu/qemu_driver.c @@ -14860,10 +14860,10 @@ qemuDomainBlockPivot(virConnectPtr conn, } if (rc == 1 && info.cur == info.end && info.type == VIR_DOMAIN_BLOCK_JOB_TYPE_COPY) - disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_READY; + disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_READY; } - if (disk->mirrorState != VIR_DOMAIN_DISK_MIRROR_READY) { + if (disk->mirrorState != VIR_DOMAIN_DISK_MIRROR_STATE_READY) { virReportError(VIR_ERR_BLOCK_COPY_ACTIVE, _("disk '%s' not ready for pivot yet"), disk->dst); @@ -14939,7 +14939,7 @@ qemuDomainBlockPivot(virConnectPtr conn, } disk->mirror = NULL; - disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_DEFAULT; + disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_NONE; cleanup: /* revert to original disk def on failure */ @@ -15096,7 +15096,7 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm, * 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) - disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_READY; + disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_READY; /* A successful block job cancelation stops any mirroring. */ if (mode == BLOCK_JOB_ABORT && disk->mirror) { @@ -15104,7 +15104,7 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm, * the mirror, and audit that fact, before dropping things. */ virStorageSourceFree(disk->mirror); disk->mirror = NULL; - disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_DEFAULT; + disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_NONE; } waitjob: diff --git i/src/qemu/qemu_process.c w/src/qemu/qemu_process.c index 73ad27d..378c6d3 100644 --- i/src/qemu/qemu_process.c +++ w/src/qemu/qemu_process.c @@ -1040,11 +1040,11 @@ qemuProcessHandleBlockJob(qemuMonitorPtr mon ATTRIBUTE_UNUSED, qemuDomainDetermineDiskChain(driver, vm, disk, true); if (disk->mirror && type == VIR_DOMAIN_BLOCK_JOB_TYPE_COPY) { if (status == VIR_DOMAIN_BLOCK_JOB_READY) { - disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_READY; + disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_READY; } else if (status == VIR_DOMAIN_BLOCK_JOB_FAILED) { virStorageSourceFree(disk->mirror); disk->mirror = NULL; - disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_DEFAULT; + disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_NONE; } } } -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On 07/29/14 06:20, Eric Blake wrote:

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@redhat.com> --- 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. */

Libvirt has now the option to revert labeling on individual images, but that can be done later.

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;

The control flow is becoming less obvious in this function.

+ + 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 we were here, then

+ } + if (has_mirror) {

This condition is also true and we try to free the source of the mirror again, even if we did it above. On the other hand, this is reached even if the _ABORT job is completed, where this makes sense.

+ 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 &&

Now this really is an else section to the above as both paths above clear 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) {

Save is true if this function was reached due to a mirroring operation change ...

+ 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);

ACK, the function flow isn't obvious but it's correct from my POV Peter

On 07/28/2014 10:20 PM, Eric Blake wrote:

Fix things by saving state any time we modify live XML, while delaying XML disk modifications until after the event completes.

Except that I didn't, which means I caused a use-after-free situation which can crash libvirtd when doing blockcopy or active blockcommit:

+++ 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.

What I missed was that a couple lines before, the code did 'disk->src = disk->mirror', which means that the qemu event handler...

@@ -1027,29 +1029,58 @@ 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_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;

is promptly calling virStorageSourceFree() on the same pointer that it is about to assign into disk->src, as well as leaking the original disk->src. Fix coming up. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On 07/29/14 06:20, Eric Blake wrote:

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@redhat.com> --- 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/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;

Hmm, looks like this would be better of in the next patch but one of the tests probably wouldn't work without this.

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; + } + }

Won't this allow us to enable block copy on persistent domains?

+ /* 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);

ACK, although this patch an the next one are a bit borderline now that libvirt is frozen for the next release. Peter

On 07/29/2014 07:06 AM, Peter Krempa wrote:

On 07/29/14 06:20, Eric Blake wrote:

...

With this in place, I can (finally!) now do:

virsh blockcommit $dom vda --shallow --verbose --pivot

and watch qemu shorten the backing chain by one, followed by libvirt automatically updating the dumpxml output, effectively undoing the work of virsh snapshot-commit --no-metadata --disk-only. Commit is SOOOO much faster than blockpull, when I'm still fairly close in time to when the temporary qcow2 wrapper file was created via a snapshot operation!

* src/qemu/qemu_driver.c (qemuDomainBlockCommit): Implement live commit.

Signed-off-by: Eric Blake <eblake@redhat.com> --- src/qemu/qemu_driver.c | 36 ++++++++++++++++++++++++++++++++---- 1 file changed, 32 insertions(+), 4 deletions(-)

ACK,

didn't change from my previous commit much.

Alas, it needs to change - now that I'm more careful about using the qemu events to react to state changes, I have to be prepared for a qemu event that occurs prior to regaining lock in the main code. I'll post a v6, but here's the interdiff: diff --git i/src/qemu/qemu_driver.c w/src/qemu/qemu_driver.c index 6a649e5..c619fbb 100644 --- i/src/qemu/qemu_driver.c +++ w/src/qemu/qemu_driver.c @@ -15673,23 +15673,31 @@ qemuDomainBlockCommit(virDomainPtr dom, * if any, through to qemu, since qemu may behave differently * depending on whether the input was specified as relative or * absolute (that is, our absolute top_canon may do the wrong - * thing if the user specified a relative name). */ + * thing if the user specified a relative name). Be prepared for + * a ready event to occur while locks are dropped. */ + if (mirror) { + disk->mirror = mirror; + disk->mirrorJob = VIR_DOMAIN_BLOCK_JOB_TYPE_ACTIVE_COMMIT; + } qemuDomainObjEnterMonitor(driver, vm); ret = qemuMonitorBlockCommit(priv->mon, device, topPath, basePath, backingPath, bandwidth); qemuDomainObjExitMonitor(driver, vm); - if (ret == 0 && mirror) { - virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + if (mirror) { + if (ret == 0) { + virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); - disk->mirror = mirror; - mirror = NULL; - disk->mirrorJob = VIR_DOMAIN_BLOCK_JOB_TYPE_ACTIVE_COMMIT; - if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) - VIR_WARN("Unable to save status on vm %s after block job", - vm->def->name); - virObjectUnref(cfg); + mirror = NULL; + if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) + VIR_WARN("Unable to save status on vm %s after block job", + vm->def->name); + virObjectUnref(cfg); + } else { + disk->mirror = NULL; + disk->mirrorJob = VIR_DOMAIN_BLOCK_JOB_TYPE_UNKNOWN; + } } endjob: -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org