[libvirt] [PATCH 04/20] snapshot: merge pre-snapshot checks
Osier Yang
jyang at redhat.com
Wed Oct 24 05:18:34 UTC 2012
On 2012年10月23日 23:12, Peter Krempa wrote:
> From: Eric Blake<eblake at redhat.com>
>
> Both system checkpoint snapshots and disk snapshots were iterating
> over all disks, doing a final sanity check before doing any work.
> But since future patches will allow offline snapshots to be either
> external or internal, it makes sense to share the pass over all
> disks, and then relax restrictions in that pass as new modes are
> implemented. Future patches can then handle external disks when
> the domain is offline, then handle offline --disk-snapshot, and
> finally, combine with migration to file to gain a complete external
> system checkpoint snapshot of an active domain without using 'savevm'.
>
> * src/qemu/qemu_driver.c (qemuDomainSnapshotDiskPrepare)
> (qemuDomainSnapshotIsAllowed): Merge...
> (qemuDomainSnapshotPrepare): ...into one function.
> (qemuDomainSnapshotCreateXML): Update caller.
> ---
> src/qemu/qemu_driver.c | 92 +++++++++++++++++++-------------------------------
> 1 file changed, 34 insertions(+), 58 deletions(-)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index cbabd62..df02783 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -10516,34 +10516,6 @@ cleanup:
> }
>
>
> -static bool
> -qemuDomainSnapshotIsAllowed(virDomainObjPtr vm)
> -{
> - int i;
> -
> - /* FIXME: we need to figure out what else here might succeed; in
> - * particular, if it's a raw device but on LVM, we could probably make
> - * that succeed as well
> - */
> - for (i = 0; i< vm->def->ndisks; i++) {
> - virDomainDiskDefPtr disk = vm->def->disks[i];
> - if (disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK&&
> - (disk->protocol == VIR_DOMAIN_DISK_PROTOCOL_SHEEPDOG ||
> - disk->protocol == VIR_DOMAIN_DISK_PROTOCOL_RBD))
> - continue;
> -
> - if ((disk->device == VIR_DOMAIN_DISK_DEVICE_LUN) ||
> - (disk->device == VIR_DOMAIN_DISK_DEVICE_DISK&&
> - disk->format> 0&& disk->format != VIR_STORAGE_FILE_QCOW2)) {
> - virReportError(VIR_ERR_OPERATION_INVALID,
> - _("Disk '%s' does not support snapshotting"),
> - disk->src);
> - return false;
> - }
> - }
> -
> - return true;
> -}
>
> /* this function expects the driver lock to be held by the caller */
> static int
> @@ -10697,8 +10669,8 @@ endjob:
> }
>
> static int
> -qemuDomainSnapshotDiskPrepare(virDomainObjPtr vm, virDomainSnapshotDefPtr def,
> - unsigned int *flags)
> +qemuDomainSnapshotPrepare(virDomainObjPtr vm, virDomainSnapshotDefPtr def,
> + unsigned int *flags)
> {
> int ret = -1;
> int i;
> @@ -10710,7 +10682,8 @@ qemuDomainSnapshotDiskPrepare(virDomainObjPtr vm, virDomainSnapshotDefPtr def,
> int external = 0;
> qemuDomainObjPrivatePtr priv = vm->privateData;
>
> - if (reuse&& !qemuCapsGet(priv->caps, QEMU_CAPS_TRANSACTION)) {
> + if (def->state == VIR_DOMAIN_DISK_SNAPSHOT&&
> + reuse&& !qemuCapsGet(priv->caps, QEMU_CAPS_TRANSACTION)) {
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> _("reuse is not supported with this QEMU binary"));
> goto cleanup;
> @@ -10718,15 +10691,16 @@ qemuDomainSnapshotDiskPrepare(virDomainObjPtr vm, virDomainSnapshotDefPtr def,
>
> for (i = 0; i< def->ndisks; i++) {
> virDomainSnapshotDiskDefPtr disk =&def->disks[i];
> + virDomainDiskDefPtr dom_disk = vm->def->disks[i];
>
> switch (disk->snapshot) {
> case VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL:
> - if (active) {
> - virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> - _("active qemu domains require external disk "
> - "snapshots; disk %s requested internal"),
> - disk->name);
> - goto cleanup;
> + if (def->state != VIR_DOMAIN_DISK_SNAPSHOT&&
> + dom_disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK&&
> + (dom_disk->protocol == VIR_DOMAIN_DISK_PROTOCOL_SHEEPDOG ||
> + dom_disk->protocol == VIR_DOMAIN_DISK_PROTOCOL_RBD)) {
> + found = true;
> + break;
> }
> if (vm->def->disks[i]->format> 0&&
> vm->def->disks[i]->format != VIR_STORAGE_FILE_QCOW2) {
> @@ -10738,10 +10712,25 @@ qemuDomainSnapshotDiskPrepare(virDomainObjPtr vm, virDomainSnapshotDefPtr def,
> vm->def->disks[i]->format));
> goto cleanup;
> }
> + if (def->state == VIR_DOMAIN_DISK_SNAPSHOT&& active) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("active qemu domains require external disk "
> + "snapshots; disk %s requested internal"),
> + disk->name);
> + goto cleanup;
> + }
> found = true;
> break;
>
> case VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL:
> + if (def->state != VIR_DOMAIN_DISK_SNAPSHOT) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("system checkpoint external snapshot for "
> + "disk %s not implemented yet"),
> + disk->name);
> + goto cleanup;
> + }
> +
> if (!disk->format) {
> disk->format = VIR_STORAGE_FILE_QCOW2;
> } else if (disk->format != VIR_STORAGE_FILE_QCOW2&&
> @@ -10789,11 +10778,11 @@ qemuDomainSnapshotDiskPrepare(virDomainObjPtr vm, virDomainSnapshotDefPtr def,
>
> if (!found) {
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> - _("disk snapshots require at least one disk to be "
> + _("snapshots require at least one disk to be "
> "selected for snapshot"));
> goto cleanup;
> }
> - if (active) {
> + if (def->state != VIR_DOMAIN_DISK_SNAPSHOT&& active) {
> if (external == 1 ||
> qemuCapsGet(priv->caps, QEMU_CAPS_TRANSACTION)) {
> *flags |= VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC;
> @@ -11004,7 +10993,7 @@ qemuDomainSnapshotCreateDiskActive(virConnectPtr conn,
> /* 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 qemuDomainSnapshotDiskPrepare, if
> + * 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.
> */
> @@ -11030,7 +11019,7 @@ qemuDomainSnapshotCreateDiskActive(virConnectPtr conn,
>
> /* No way to roll back if first disk succeeds but later disks
> * fail, unless we have transaction support.
> - * Based on earlier qemuDomainSnapshotDiskPrepare, all
> + * Based on earlier qemuDomainSnapshotPrepare, all
> * disks in this list are now either SNAPSHOT_NO, or
> * SNAPSHOT_EXTERNAL with a valid file name and qcow2 format. */
> qemuDomainObjEnterMonitorWithDriver(driver, vm);
> @@ -11334,31 +11323,18 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
> }
> align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL;
> align_match = false;
> - if (virDomainSnapshotAlignDisks(def, align_location,
> - align_match)< 0 ||
> - qemuDomainSnapshotDiskPrepare(vm, def,&flags)< 0)
> - goto cleanup;
> def->state = VIR_DOMAIN_DISK_SNAPSHOT;
> def->memory = VIR_DOMAIN_SNAPSHOT_LOCATION_NONE;
> } else {
> - /* In a perfect world, we would allow qemu to tell us this.
> - * The problem is that qemu only does this check
> - * device-by-device; so if you had a domain that booted from a
> - * large qcow2 device, but had a secondary raw device
> - * attached, you wouldn't find out that you can't snapshot
> - * your guest until *after* it had spent the time to snapshot
> - * the boot device. This is probably a bug in qemu, but we'll
> - * work around it here for now.
> - */
> - if (!qemuDomainSnapshotIsAllowed(vm) ||
> - virDomainSnapshotAlignDisks(def, align_location,
> - align_match)< 0)
> - goto cleanup;
> def->state = virDomainObjGetState(vm, NULL);
> def->memory = (def->state == VIR_DOMAIN_SHUTOFF ?
> VIR_DOMAIN_SNAPSHOT_LOCATION_NONE :
> VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL);
> }
> + if (virDomainSnapshotAlignDisks(def, align_location,
> + align_match)< 0 ||
> + qemuDomainSnapshotPrepare(vm, def,&flags)< 0)
> + goto cleanup;
> }
>
> if (snap)
ACK
More information about the libvir-list
mailing list