Re: [libvirt] [PATCHv2 10/20] snapshot: qemu: Add support for external checkpoints

On 11/02/2012 04:28 PM, Eric Blake wrote: Here's what I ended up with; I can ACK your patch plus this squashed in, although it wouldn't hurt to do another round of reviews and/or testing. diff --git i/src/qemu/qemu_driver.c w/src/qemu/qemu_driver.c index d5cfdcc..2a9e09b 100644 --- i/src/qemu/qemu_driver.c +++ w/src/qemu/qemu_driver.c @@ -11020,7 +11020,6 @@ qemuDomainSnapshotCreateDiskActive(struct qemud_driver *driver, int ret = -1; int i; bool persist = false; - int thaw = 0; /* 1 if freeze succeeded, -1 if freeze failed */ bool reuse = (flags & VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT) != 0; virCgroupPtr cgroup = NULL; @@ -11039,20 +11038,6 @@ qemuDomainSnapshotCreateDiskActive(struct qemud_driver *driver, } /* 'cgroup' is still NULL if cgroups are disabled. */ - /* If quiesce was requested, then issue a freeze command, and a - * counterpart thaw command, no matter what. The command will - * fail if the guest is paused or the guest agent is not - * running. */ - if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE) { - if (qemuDomainSnapshotFSFreeze(driver, vm) < 0) { - /* helper reported the error */ - thaw = -1; - goto cleanup; - } else { - thaw = 1; - } - } - if (qemuCapsGet(priv->caps, QEMU_CAPS_TRANSACTION)) { if (!(actions = virJSONValueNewArray())) { virReportOOMError(); @@ -11131,13 +11116,6 @@ cleanup: ret = -1; } - if (thaw != 0 && - qemuDomainSnapshotFSThaw(driver, vm, thaw > 0) < 0) { - /* helper reported the error, if it was needed */ - if (thaw > 0) - ret = -1; - } - return ret; } @@ -11157,24 +11135,43 @@ qemuDomainSnapshotCreateActiveExternal(virConnectPtr conn, bool memory = snap->def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL; bool atomic = !!(flags & VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC); bool transaction = qemuCapsGet(priv->caps, QEMU_CAPS_TRANSACTION); + int thaw = 0; /* 1 if freeze succeeded, -1 if freeze failed */ if (qemuDomainObjBeginAsyncJobWithDriver(driver, vm, QEMU_ASYNC_JOB_SNAPSHOT) < 0) goto cleanup; + /* If quiesce was requested, then issue a freeze command, and a + * counterpart thaw command, no matter what. The command will + * fail if the guest is paused or the guest agent is not + * running. */ + if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE) { + if (qemuDomainSnapshotFSFreeze(driver, vm) < 0) { + /* helper reported the error */ + thaw = -1; + goto endjob; + } else { + thaw = 1; + } + } + /* we need to resume the guest only if it was previously running */ if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_RUNNING) { resume = true; - /* For multiple disks, libvirt must pause externally to get all - * snapshots to be at the same point in time, unless qemu supports - * transactions. For a single disk, snapshot is atomic without - * requiring a pause. Thanks to qemuDomainSnapshotPrepare, if - * we got to this point, the atomic flag now says whether we need - * to pause, and a capability bit says whether to use transaction. + /* For external checkpoints (those with memory), the guest + * must pause (either by libvirt up front, or by qemu after + * _LIVE converges). For disk-only snapshots with multiple + * disks, libvirt must pause externally to get all snapshots + * to be at the same point in time, unless qemu supports + * transactions. For a single disk, snapshot is atomic + * without requiring a pause. Thanks to + * qemuDomainSnapshotPrepare, if we got to this point, the + * atomic flag now says whether we need to pause, and a + * capability bit says whether to use transaction. */ if ((memory && !(flags & VIR_DOMAIN_SNAPSHOT_CREATE_LIVE)) || - (atomic && !transaction)) { + (!memory && atomic && !transaction)) { if (qemuProcessStopCPUs(driver, vm, VIR_DOMAIN_PAUSED_SNAPSHOT, QEMU_ASYNC_JOB_SNAPSHOT) < 0) goto endjob; @@ -11230,6 +11227,7 @@ qemuDomainSnapshotCreateActiveExternal(virConnectPtr conn, * only, so this end job never drops the last reference. */ ignore_value(qemuDomainObjEndAsyncJob(driver, vm)); resume = false; + thaw = 0; vm = NULL; if (event) qemuDomainEventQueue(driver, event); @@ -11238,16 +11236,6 @@ qemuDomainSnapshotCreateActiveExternal(virConnectPtr conn, ret = 0; endjob: - if (vm && !qemuDomainObjEndAsyncJob(driver, vm)) { - /* Only possible if a transient vm quit while our locks were down, - * in which case we don't want to save snapshot metadata. - */ - *vmptr = NULL; - ret = -1; - } - -cleanup: - VIR_FREE(xml); if (resume && vm && virDomainObjIsActive(vm) && qemuProcessStartCPUs(driver, vm, conn, VIR_DOMAIN_RUNNING_UNPAUSED, @@ -11258,6 +11246,22 @@ cleanup: return -1; } + if (vm && thaw != 0 && + qemuDomainSnapshotFSThaw(driver, vm, thaw > 0) < 0) { + /* helper reported the error, if it was needed */ + if (thaw > 0) + ret = -1; + } + if (vm && !qemuDomainObjEndAsyncJob(driver, vm)) { + /* Only possible if a transient vm quit while our locks were down, + * in which case we don't want to save snapshot metadata. + */ + *vmptr = NULL; + ret = -1; + } + +cleanup: + VIR_FREE(xml); return ret; } @@ -11498,14 +11502,23 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, } /* reject the VIR_DOMAIN_SNAPSHOT_CREATE_LIVE flag where not supported */ - if ((flags & VIR_DOMAIN_SNAPSHOT_CREATE_LIVE) && + if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_LIVE && (!virDomainObjIsActive(vm) || - snap->def->memory != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL)) { + snap->def->memory != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL || + flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE)) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", _("live snapshot creation is supported only " "with external checkpoints")); goto cleanup; } + if ((snap->def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL || + snap->def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL) && + flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("disk-only snapshot creation is not compatible with " + "memory snapshot")); + goto cleanup; + } /* actually do the snapshot */ if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE) { -- Eric Blake eblake(a)redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org