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

On 11/01/2012 10:22 AM, Peter Krempa wrote: s/do // 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. Hmm, I'm thinking I still got this slightly wrong: if memory is specified, the we are guaranteed that the guest will pause before the disk side of things, even if we support transaction. The only time we can get away without pausing the guest is for disk-only and transaction support. But as written, this would end up pausing if memory and _LIVE are specified but we lack transaction, which undoes the point of _LIVE. So we need a !memory conjunct in the second half of ||. More on why you can't mix _QUIESCE with _LIVE external memory snapshots: we don't control when the guest will finally converge and pause, so we can't trigger the guest agent to quiesce at the right time. We don't want to quiesce up front, as that defeats the purpose of a live snapshot, if the guest can't do any I/O for the entire live migration-to-file. So here, we should error out if _QUIESCE and _LIVE are both given; or even stronger - error out if both _QUIESCE and external memory are requested (it is always easier to relax later if someone can prove it would be useful, than it is to provide it now and wish we hadn't). On the bright side, this forbidden combination is no real loss: if you are snapshotting memory, then you don't need to worry about inconsistent disk state, since you have the memory that goes along with any in-flight I/O at the time of the snapshot - quiescing only makes sense if you are going to save disk state without in-flight I/O to go along with it. Looking at the code, we already forbid _QUIESCE if _DISK_ONLY is not present; and re-reading my table in commit 4201a7ea, all that remains is to reject the combination of DISK_ONLY and a memory snapshot request. Likewise, _LIVE and _REDEFINE are incompatible. Hmm, I see that src/libvirt.c already filters out some impossible flag combinations, so maybe my proposals below should be moved. 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. diff --git i/src/qemu/qemu_driver.c w/src/qemu/qemu_driver.c index d5cfdcc..35c8670 100644 --- i/src/qemu/qemu_driver.c +++ w/src/qemu/qemu_driver.c @@ -11498,14 +11498,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