We need a capability bit to gracefully error out if some of the additions in future patches can't be implemented by the running qemu. * src/qemu/qemu_capabilities.h (QEMU_CAPS_TRANSACTION): New cap. * src/qemu/qemu_capabilities.c (qemuCaps): Name it. * src/qemu/qemu_monitor_json.c (qemuMonitorJSONCheckCommands): Set it. --- I haven't yet decided if I need to check for 'block-mirror' as an independent capability, but that can be a later patch. src/qemu/qemu_capabilities.c | 1 + src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_monitor_json.c | 3 +++ 3 files changed, 5 insertions(+), 0 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index ace5011..0e09d6d 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -155,6 +155,7 @@ VIR_ENUM_IMPL(qemuCaps, QEMU_CAPS_LAST, "system_wakeup", "scsi-disk.channel", "scsi-block", + "transaction", ); struct qemu_feature_flags { diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index 62b4270..78cdbe0 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -123,6 +123,7 @@ enum qemuCapsFlags { QEMU_CAPS_WAKEUP = 86, /* system_wakeup monitor command */ QEMU_CAPS_SCSI_DISK_CHANNEL = 87, /* Is scsi-disk.channel available? */ QEMU_CAPS_SCSI_BLOCK = 88, /* -device scsi-block */ + QEMU_CAPS_TRANSACTION = 89, /* transaction monitor command */ QEMU_CAPS_LAST, /* this must always be the last item */ }; diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index ce68e69..4dd6924 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -884,6 +884,9 @@ qemuMonitorJSONCheckCommands(qemuMonitorPtr mon, if (STREQ(name, "system_wakeup")) qemuCapsSet(qemuCaps, QEMU_CAPS_WAKEUP); + + if (STREQ(name, "transaction")) + qemuCapsSet(qemuCaps, QEMU_CAPS_TRANSACTION); } ret = 0; -- 1.7.7.6

Right now, it is appallingly easy to cause qemu disk snapshots to alter a domain then fail; for example, by requesting a two-disk snapshot where the second disk name resides on read-only storage. In this failure scenario, libvirt reports failure, but modifies the live domain XML in-place to record that the first disk snapshot was taken; and places a difficult burden on the management app to grab the XML and reparse it to see which disks, if any, were altered by the partial snapshot. This patch adds a new flag where implementations can request that the hypervisor make snapshots atomically; either no changes to XML occur, or all disks were altered as a group. If you request the flag, you either get outright failure up front, or you take advantage of hypervisor abilities to make an atomic snapshot. Of course, drivers should prefer the atomic means even without the flag explicitly requested. There's no way to make snapshots 100% bulletproof - even if the hypervisor does it perfectly atomic, we could run out of memory during the followup tasks of updating our in-memory XML, and report a failure. However, these sorts of catastrophic failures are rare and unlikely, and it is still nicer to know that either all snapshots happened or none of them, as that is an easier state to recover from. * include/libvirt/libvirt.h.in (VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC): New flag. * src/libvirt.c (virDomainSnapshotCreateXML): Document it. * tools/virsh.c (cmdSnapshotCreate, cmdSnapshotCreateAs): Expose it. * tools/virsh.pod (snapshot-create, snapshot-create-as): Document it. --- include/libvirt/libvirt.h.in | 2 ++ src/libvirt.c | 21 ++++++++++++++------- tools/virsh.c | 6 ++++++ tools/virsh.pod | 16 ++++++++++++++-- 4 files changed, 36 insertions(+), 9 deletions(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 7d41642..4566580 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -3289,6 +3289,8 @@ typedef enum { quiesce all mounted file systems within the domain */ + VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC = (1 << 7), /* atomically avoid + partial changes */ } virDomainSnapshotCreateFlags; /* Take a snapshot of the current VM state */ diff --git a/src/libvirt.c b/src/libvirt.c index 7f8d42c..48d08c4 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -17119,14 +17119,21 @@ virDomainSnapshotGetConnect(virDomainSnapshotPtr snapshot) * @flags includes VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT, then existing * destination files are instead truncated and reused. * - * Returns an (opaque) virDomainSnapshotPtr on success, NULL on failure. * Be aware that although libvirt prefers to report errors up front with - * no other effect, there are certain types of failures where a failure - * can occur even though the guest configuration was changed (for - * example, if a disk snapshot request over two disks only fails on the - * second disk, leaving the first disk altered); so after getting a NULL - * return, it can be wise to use virDomainGetXMLDesc() to determine if - * any partial changes occurred. + * no other effect, some hypervisors have certain types of failures where + * the overall command can easily fail even though the guest configuration + * was partially altered (for example, if a disk snapshot request for two + * disks fails on the second disk, but the first disk alteration cannot be + * rolled back). If this API call fails, it is therefore normally + * necessary to follow up with virDomainGetXMLDesc() and check each disk + * to determine if any partial changes occurred. However, if @flags + * contains VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC, then libvirt guarantees + * that this command will not alter any disks unless the entire set of + * changes can be done atomically, making failure recovery simpler (note + * that it is still possible to fail after disks have changed, but only + * in the much rarer cases of running out of memory or disk space). + * + * Returns an (opaque) virDomainSnapshotPtr on success, NULL on failure. */ virDomainSnapshotPtr virDomainSnapshotCreateXML(virDomainPtr domain, diff --git a/tools/virsh.c b/tools/virsh.c index e48c56a..980e206 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -15715,6 +15715,7 @@ static const vshCmdOptDef opts_snapshot_create[] = { {"disk-only", VSH_OT_BOOL, 0, N_("capture disk state but not vm state")}, {"reuse-external", VSH_OT_BOOL, 0, N_("reuse any existing external files")}, {"quiesce", VSH_OT_BOOL, 0, N_("quiesce guest's file systems")}, + {"atomic", VSH_OT_BOOL, 0, N_("require atomic operation")}, {NULL, 0, 0, NULL} }; @@ -15741,6 +15742,8 @@ cmdSnapshotCreate(vshControl *ctl, const vshCmd *cmd) flags |= VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT; if (vshCommandOptBool(cmd, "quiesce")) flags |= VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE; + if (vshCommandOptBool(cmd, "atomic")) + flags |= VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC; if (!vshConnectionUsability(ctl, ctl->conn)) goto cleanup; @@ -15851,6 +15854,7 @@ static const vshCmdOptDef opts_snapshot_create_as[] = { {"disk-only", VSH_OT_BOOL, 0, N_("capture disk state but not vm state")}, {"reuse-external", VSH_OT_BOOL, 0, N_("reuse any existing external files")}, {"quiesce", VSH_OT_BOOL, 0, N_("quiesce guest's file systems")}, + {"atomic", VSH_OT_BOOL, 0, N_("require atomic operation")}, {"diskspec", VSH_OT_ARGV, 0, N_("disk attributes: disk[,snapshot=type][,driver=type][,file=name]")}, {NULL, 0, 0, NULL} @@ -15878,6 +15882,8 @@ cmdSnapshotCreateAs(vshControl *ctl, const vshCmd *cmd) flags |= VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT; if (vshCommandOptBool(cmd, "quiesce")) flags |= VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE; + if (vshCommandOptBool(cmd, "atomic")) + flags |= VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC; if (!vshConnectionUsability(ctl, ctl->conn)) goto cleanup; diff --git a/tools/virsh.pod b/tools/virsh.pod index ecf6aaa..232bf3c 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -2270,7 +2270,7 @@ used to represent properties of snapshots. =item B<snapshot-create> I<domain> [I<xmlfile>] {[I<--redefine> [I<--current>]] | [I<--no-metadata>] [I<--halt>] [I<--disk-only>] [I<--reuse-external>] -[I<--quiesce>]} +[I<--quiesce>] [I<--atomic>]} Create a snapshot for domain I<domain> with the properties specified in I<xmlfile>. Normally, the only properties settable for a domain snapshot @@ -2317,6 +2317,12 @@ to freeze and unfreeze domain's mounted file systems. However, if domain has no guest agent, snapshot creation will fail. Currently, this requires I<--disk-only> to be passed as well. +If I<--atomic> is specified, libvirt will guarantee that the snapshot +either succeeds, or fails with no changes; not all hypervisors support +this. If this flag is not specified, then some hypervisors may fail +after partially performing the action, and B<dumpxml> must be used to +see whether any partial changes occurred. + Existence of snapshot metadata will prevent attempts to B<undefine> a persistent domain. However, for transient domains, snapshot metadata is silently lost when the domain quits running (whether @@ -2324,7 +2330,7 @@ by command such as B<destroy> or by internal guest action). =item B<snapshot-create-as> I<domain> {[I<--print-xml>] | [I<--no-metadata>] [I<--halt>] [I<--reuse-existing>]} [I<name>] -[I<description>] [I<--disk-only> [I<--quiesce>] +[I<description>] [I<--disk-only> [I<--quiesce>] [I<--atomic>] [[I<--diskspec>] B<diskspec>]...] Create a snapshot for domain I<domain> with the given <name> and @@ -2364,6 +2370,12 @@ treat the snapshot as current, and cannot revert to the snapshot unless B<snapshot-create> is later used to teach libvirt about the metadata again). This flag is incompatible with I<--print-xml>. +If I<--atomic> is specified, libvirt will guarantee that the snapshot +either succeeds, or fails with no changes; not all hypervisors support +this. If this flag is not specified, then some hypervisors may fail +after partially performing the action, and B<dumpxml> must be used to +see whether any partial changes occurred. + =item B<snapshot-current> I<domain> {[I<--name>] | [I<--security-info>] | [I<snapshotname>]} -- 1.7.7.6

Taking an external snapshot of just one disk is atomic, without having to pause and resume the VM. This also paves the way for later patches to interact with the new qemu 'transaction' monitor command. The various scenarios when requesting atomic are: online, 1 disk, old qemu - safe, allowed by this patch online, more than 1 disk, old qemu - failure, this patch offline snapshot - safe, once a future patch implements offline disk snapshot online, 1 or more disks, new qemu - safe, once future patch uses transaction * src/qemu/qemu_driver.c (qemuDomainSnapshotCreateXML): Support new flag for single-disk setups. (qemuDomainSnapshotDiskPrepare): Check for atomic here. (qemuDomainSnapshotCreateDiskActive): Skip pausing the VM when atomic supported. --- src/qemu/qemu_driver.c | 40 +++++++++++++++++++++++++++------------- 1 files changed, 27 insertions(+), 13 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 13ca92f..a46ce10 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -9712,13 +9712,16 @@ endjob: static int qemuDomainSnapshotDiskPrepare(virDomainObjPtr vm, virDomainSnapshotDefPtr def, - bool allow_reuse) + unsigned int *flags) { int ret = -1; int i; bool found = false; bool active = virDomainObjIsActive(vm); struct stat st; + bool allow_reuse = (*flags & VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT) != 0; + bool atomic = (*flags & VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC) != 0; + int external = 0; for (i = 0; i < def->ndisks; i++) { virDomainSnapshotDiskDefPtr disk = &def->disks[i]; @@ -9773,6 +9776,7 @@ qemuDomainSnapshotDiskPrepare(virDomainObjPtr vm, virDomainSnapshotDefPtr def, goto cleanup; } found = true; + external++; break; case VIR_DOMAIN_DISK_SNAPSHOT_NO: @@ -9792,6 +9796,13 @@ qemuDomainSnapshotDiskPrepare(virDomainObjPtr vm, virDomainSnapshotDefPtr def, "selected for snapshot")); goto cleanup; } + if (atomic && external > 1) { + qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("atomic snapshot of multiple disks is unsupported")); + goto cleanup; + } else if (external == 1) { + *flags |= VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC; + } ret = 0; @@ -9920,6 +9931,7 @@ qemuDomainSnapshotCreateDiskActive(virConnectPtr conn, int i; bool persist = false; int thaw = 0; /* 1 if freeze succeeded, -1 if freeze failed */ + bool atomic = (flags & VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC) != 0; if (qemuDomainObjBeginJobWithDriver(driver, vm, QEMU_JOB_MODIFY) < 0) return -1; @@ -9944,14 +9956,14 @@ qemuDomainSnapshotCreateDiskActive(virConnectPtr conn, } } - if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_RUNNING) { - /* In qemu, snapshot_blkdev on a single disk will pause cpus, - * but this confuses libvirt since notifications are not given - * when qemu resumes. And for multiple disks, libvirt must - * pause externally to get all snapshots to be at the same - * point in time. For simplicitly, we always pause ourselves - * rather than relying on qemu doing pause. - */ + /* 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 + * 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 (!atomic && virDomainObjGetState(vm, NULL) == VIR_DOMAIN_RUNNING) { if (qemuProcessStopCPUs(driver, vm, VIR_DOMAIN_PAUSED_SAVE, QEMU_ASYNC_JOB_NONE) < 0) goto cleanup; @@ -10069,7 +10081,8 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, VIR_DOMAIN_SNAPSHOT_CREATE_HALT | VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY | VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT | - VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE, NULL); + VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE | + VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC, NULL); if ((flags & VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE) && !(flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY)) { @@ -10211,14 +10224,15 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, goto cleanup; if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY) { - bool allow_reuse; + qemuDomainObjPrivatePtr priv = vm->privateData; - allow_reuse = (flags & VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT) != 0; if (virDomainSnapshotAlignDisks(def, VIR_DOMAIN_DISK_SNAPSHOT_EXTERNAL, false) < 0) goto cleanup; - if (qemuDomainSnapshotDiskPrepare(vm, def, allow_reuse) < 0) + if (qemuCapsGet(priv->qemuCaps, QEMU_CAPS_TRANSACTION)) + flags |= VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC; + if (qemuDomainSnapshotDiskPrepare(vm, def, &flags) < 0) goto cleanup; def->state = VIR_DOMAIN_DISK_SNAPSHOT; } else { -- 1.7.7.6

Taking an external snapshot of just one disk is atomic, without having to pause and resume the VM. This also paves the way for later patches to interact with the new qemu 'transaction' monitor command. The various scenarios when requesting atomic are: online, 1 disk, old qemu - safe, allowed by this patch online, more than 1 disk, old qemu - failure, this patch offline snapshot - safe, once a future patch implements offline disk snapshot online, 1 or more disks, new qemu - safe, once future patch uses transaction Taking an online system checkpoint snapshot is atomic, since it is done via a single 'savevm' monitor command. Taking an offline system checkpoint snapshot currently uses multiple qemu-img invocations with no provision for cleanup of partial failure, so for now we mark it unsupported. * src/qemu/qemu_driver.c (qemuDomainSnapshotCreateXML): Support new flag for single-disk setups. (qemuDomainSnapshotDiskPrepare): Check for atomic here. (qemuDomainSnapshotCreateDiskActive): Skip pausing the VM when atomic supported. (qemuDomainSnapshotIsAllowed): Use bool instead of int. --- Patches 1/3 and 2/3 unchanged. v2: consider interactions of atomic with non-disk-snapshots src/qemu/qemu_driver.c | 56 +++++++++++++++++++++++++++++++++-------------- 1 files changed, 39 insertions(+), 17 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 85f8cf7..9e62738 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -9535,7 +9535,8 @@ cleanup: return ret; } -static int qemuDomainSnapshotIsAllowed(virDomainObjPtr vm) +static bool +qemuDomainSnapshotIsAllowed(virDomainObjPtr vm) { int i; @@ -9550,11 +9551,11 @@ static int qemuDomainSnapshotIsAllowed(virDomainObjPtr vm) qemuReportError(VIR_ERR_OPERATION_INVALID, _("Disk '%s' does not support snapshotting"), vm->def->disks[i]->src); - return 0; + return false; } } - return 1; + return true; } static int @@ -9712,13 +9713,17 @@ endjob: static int qemuDomainSnapshotDiskPrepare(virDomainObjPtr vm, virDomainSnapshotDefPtr def, - bool allow_reuse) + unsigned int *flags) { int ret = -1; int i; bool found = false; bool active = virDomainObjIsActive(vm); struct stat st; + bool allow_reuse = (*flags & VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT) != 0; + bool atomic = (*flags & VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC) != 0; + int external = 0; + qemuDomainObjPrivatePtr priv = vm->privateData; for (i = 0; i < def->ndisks; i++) { virDomainSnapshotDiskDefPtr disk = &def->disks[i]; @@ -9773,6 +9778,7 @@ qemuDomainSnapshotDiskPrepare(virDomainObjPtr vm, virDomainSnapshotDefPtr def, goto cleanup; } found = true; + external++; break; case VIR_DOMAIN_DISK_SNAPSHOT_NO: @@ -9792,6 +9798,17 @@ qemuDomainSnapshotDiskPrepare(virDomainObjPtr vm, virDomainSnapshotDefPtr def, "selected for snapshot")); goto cleanup; } + if (active) { + if (external == 1 || + qemuCapsGet(priv->qemuCaps, QEMU_CAPS_TRANSACTION)) { + *flags |= VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC; + } else if (atomic && external > 1) { + qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("atomic live snapshot of multiple disks " + "is unsupported")); + goto cleanup; + } + } ret = 0; @@ -9920,6 +9937,7 @@ qemuDomainSnapshotCreateDiskActive(virConnectPtr conn, int i; bool persist = false; int thaw = 0; /* 1 if freeze succeeded, -1 if freeze failed */ + bool atomic = (flags & VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC) != 0; if (qemuDomainObjBeginJobWithDriver(driver, vm, QEMU_JOB_MODIFY) < 0) return -1; @@ -9944,14 +9962,14 @@ qemuDomainSnapshotCreateDiskActive(virConnectPtr conn, } } - if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_RUNNING) { - /* In qemu, snapshot_blkdev on a single disk will pause cpus, - * but this confuses libvirt since notifications are not given - * when qemu resumes. And for multiple disks, libvirt must - * pause externally to get all snapshots to be at the same - * point in time. For simplicitly, we always pause ourselves - * rather than relying on qemu doing pause. - */ + /* 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 + * 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 (!atomic && virDomainObjGetState(vm, NULL) == VIR_DOMAIN_RUNNING) { if (qemuProcessStopCPUs(driver, vm, VIR_DOMAIN_PAUSED_SAVE, QEMU_ASYNC_JOB_NONE) < 0) goto cleanup; @@ -10070,7 +10088,8 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, VIR_DOMAIN_SNAPSHOT_CREATE_HALT | VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY | VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT | - VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE, NULL); + VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE | + VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC, NULL); if ((flags & VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE) && !(flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY)) { @@ -10212,14 +10231,11 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, goto cleanup; if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY) { - bool allow_reuse; - - allow_reuse = (flags & VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT) != 0; if (virDomainSnapshotAlignDisks(def, VIR_DOMAIN_DISK_SNAPSHOT_EXTERNAL, false) < 0) goto cleanup; - if (qemuDomainSnapshotDiskPrepare(vm, def, allow_reuse) < 0) + if (qemuDomainSnapshotDiskPrepare(vm, def, &flags) < 0) goto cleanup; def->state = VIR_DOMAIN_DISK_SNAPSHOT; } else { @@ -10279,6 +10295,12 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, &vm, snap, flags) < 0) goto cleanup; } else if (!virDomainObjIsActive(vm)) { + if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC) { + qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("atomic snapshots of inactive domains not " + "implemented yet")); + goto cleanup; + } if (qemuDomainSnapshotCreateInactive(driver, vm, snap) < 0) goto cleanup; } else { -- 1.7.7.6