[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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 redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]