On 11/02/2012 04:28 PM, Eric Blake wrote:
> On 11/01/2012 10:22 AM, Peter Krempa wrote:
>> This patch adds support to take external system checkpoints.
>>
>> The functionality is layered on top of the previous disk-only snapshot
>> code. When the checkpoint is requested the domain memory is saved to the
>> memory image file using migration to file. (The user may specify to
>> do take the memory image while the guest is live with the
>
> s/do //
>
>> VIR_DOMAIN_SNAPSHOT_CREATE_LIVE flag.)
>>
>> The memory save image shares format with the image created by
>> virDomainSave() API.
>> ---
> Ouch. You left the quiesce logic in qemuDomainSnapshotCreateDiskActive,
> but changed the caller so that this point can be reached after the
> domain has been already paused. We already reject _QUIESCE without
> _DISK_ONLY, so we know there is no memory to worry about, but the
> quiesce must occur before pausing things.
>
> That means you need to move the quiesce and thaw logic out of
> CreateDiskActive and into CreateActiveExternal.
>
>
> I'm playing with squashing this in, but I also need to fix the quiesce
> outside pause issue before I can give ACK, if you can beat me to it.
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@xxxxxxxxxx +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list