libvirt supports taking external disk snapshots on a running VM,
using qemu's "blockdev-snapshot" command.
qemu also supports "blockdev-snapshot-internal-sync" to do the
same for internal snapshots.
This commit wraps this (old) qemu capability to allow future libvirt
users to take internal disk snapshots on a running VM.
This will only work for disk types which support internal snapshots,
and thus we require the disk type to be part of a white list of known
types. For this commit, the list of supported formats is empty.
An upcoming commit will allow RBD disks to use this new capability.
Signed-off-by: Or Ozeri <oro@il.ibm.com>
---
src/qemu/qemu_monitor.c | 9 +++
src/qemu/qemu_monitor.h | 5 ++
src/qemu/qemu_monitor_json.c | 14 ++++
src/qemu/qemu_monitor_json.h | 5 ++
src/qemu/qemu_snapshot.c | 72 ++++++++++++-------
.../disk_snapshot.xml | 2 +-
.../disk_snapshot.xml | 2 +-
.../disk_snapshot_redefine.xml | 2 +-
tests/qemumonitorjsontest.c | 1 +
9 files changed, 82 insertions(+), 30 deletions(-)
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index 38f89167e0..f6dab34243 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -4225,6 +4225,15 @@ qemuMonitorTransactionSnapshotBlockdev(virJSONValue *actions,
}
+int
+qemuMonitorTransactionInternalSnapshotBlockdev(virJSONValue *actions,
+ const char *device,
+ const char *name)
+{
+ return qemuMonitorJSONTransactionInternalSnapshotBlockdev(actions, device, name);
+}
+
+
int
qemuMonitorTransactionBackup(virJSONValue *actions,
const char *device,
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index 2d16214ba2..1bfd1ccbc2 100644
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -1411,6 +1411,11 @@ qemuMonitorTransactionSnapshotBlockdev(virJSONValue *actions,
const char *node,
const char *overlay);
+int
+qemuMonitorTransactionInternalSnapshotBlockdev(virJSONValue *actions,
+ const char *device,
+ const char *name);
+
typedef enum {
QEMU_MONITOR_TRANSACTION_BACKUP_SYNC_MODE_NONE = 0,
QEMU_MONITOR_TRANSACTION_BACKUP_SYNC_MODE_INCREMENTAL,
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index db99017555..002a6caa52 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -8307,6 +8307,20 @@ qemuMonitorJSONTransactionSnapshotBlockdev(virJSONValue *actions,
NULL);
}
+
+int
+qemuMonitorJSONTransactionInternalSnapshotBlockdev(virJSONValue *actions,
+ const char *device,
+ const char *name)
+{
+ return qemuMonitorJSONTransactionAdd(actions,
+ "blockdev-snapshot-internal-sync",
+ "s:device", device,
+ "s:name", name,
+ NULL);
+}
+
+
VIR_ENUM_DECL(qemuMonitorTransactionBackupSyncMode);
VIR_ENUM_IMPL(qemuMonitorTransactionBackupSyncMode,
QEMU_MONITOR_TRANSACTION_BACKUP_SYNC_MODE_LAST,
diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h
index 6f376cf9b7..313004f327 100644
--- a/src/qemu/qemu_monitor_json.h
+++ b/src/qemu/qemu_monitor_json.h
@@ -779,6 +779,11 @@ qemuMonitorJSONTransactionSnapshotBlockdev(virJSONValue *actions,
const char *node,
const char *overlay);
+int
+qemuMonitorJSONTransactionInternalSnapshotBlockdev(virJSONValue *actions,
+ const char *device,
+ const char *name);
+
int
qemuMonitorJSONTransactionBackup(virJSONValue *actions,
const char *device,
diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
index c1855b3028..e82352ba7d 100644
--- a/src/qemu/qemu_snapshot.c
+++ b/src/qemu/qemu_snapshot.c
@@ -736,7 +736,7 @@ qemuSnapshotPrepare(virDomainObj *vm,
}
/* disk snapshot requires at least one disk */
- if (def->state == VIR_DOMAIN_SNAPSHOT_DISK_SNAPSHOT && !external) {
+ if (def->state == VIR_DOMAIN_SNAPSHOT_DISK_SNAPSHOT && !external && !found_internal) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("disk-only snapshots require at least "
"one disk to be selected for snapshot"));
@@ -852,6 +852,7 @@ qemuSnapshotDiskCleanup(qemuSnapshotDiskData *data,
struct _qemuSnapshotDiskContext {
qemuSnapshotDiskData *dd;
size_t ndd;
+ bool has_internal;
virJSONValue *actions;
@@ -1070,17 +1071,17 @@ qemuSnapshotDiskPrepareOne(qemuSnapshotDiskContext *snapctxt,
/**
- * qemuSnapshotDiskPrepareActiveExternal:
+ * qemuSnapshotDiskPrepareActive:
*
* Collects and prepares a list of structures that hold information about disks
* that are selected for the snapshot.
*/
static qemuSnapshotDiskContext *
-qemuSnapshotDiskPrepareActiveExternal(virDomainObj *vm,
- virDomainMomentObj *snap,
- bool reuse,
- GHashTable *blockNamedNodeData,
- virDomainAsyncJob asyncJob)
+qemuSnapshotDiskPrepareActive(virDomainObj *vm,
+ virDomainMomentObj *snap,
+ bool reuse,
+ GHashTable *blockNamedNodeData,
+ virDomainAsyncJob asyncJob)
{
g_autoptr(qemuSnapshotDiskContext) snapctxt = NULL;
size_t i;
@@ -1089,16 +1090,33 @@ qemuSnapshotDiskPrepareActiveExternal(virDomainObj *vm,
snapctxt = qemuSnapshotDiskContextNew(snapdef->ndisks, vm, asyncJob);
for (i = 0; i < snapdef->ndisks; i++) {
- if (snapdef->disks[i].snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL)
- continue;
+ switch (snapdef->disks[i].snapshot) {
+ case VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL: {
+ if (qemuSnapshotDiskPrepareOne(snapctxt,
+ vm->def->disks[i],
+ snapdef->disks + i,
+ blockNamedNodeData,
+ reuse,
+ true) < 0)
+ return NULL;
+ break;
+ }
- if (qemuSnapshotDiskPrepareOne(snapctxt,
- vm->def->disks[i],
- snapdef->disks + i,
- blockNamedNodeData,
- reuse,
- true) < 0)
- return NULL;
+ case VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL: {
+ snapctxt->has_internal = true;
+ if (qemuMonitorTransactionInternalSnapshotBlockdev(snapctxt->actions,
+ vm->def->disks[i]->src->nodeformat,
+ snapdef->disks[i].snapshot_name) < 0)
+ return NULL;
+ break;
+ }
+
+ case VIR_DOMAIN_SNAPSHOT_LOCATION_DEFAULT:
+ case VIR_DOMAIN_SNAPSHOT_LOCATION_NO:
+ case VIR_DOMAIN_SNAPSHOT_LOCATION_MANUAL:
+ case VIR_DOMAIN_SNAPSHOT_LOCATION_LAST:
+ continue;
+ }
}
return g_steal_pointer(&snapctxt);
@@ -1182,7 +1200,7 @@ qemuSnapshotDiskCreate(qemuSnapshotDiskContext *snapctxt)
int rc;
/* check whether there's anything to do */
- if (snapctxt->ndd == 0)
+ if (snapctxt->ndd == 0 && !snapctxt->has_internal)
return 0;
if (qemuDomainObjEnterMonitorAsync(snapctxt->vm, snapctxt->asyncJob) < 0)
@@ -1215,11 +1233,11 @@ qemuSnapshotDiskCreate(qemuSnapshotDiskContext *snapctxt)
/* The domain is expected to be locked and active. */
static int
-qemuSnapshotCreateActiveExternalDisks(virDomainObj *vm,
- virDomainMomentObj *snap,
- GHashTable *blockNamedNodeData,
- unsigned int flags,
- virDomainAsyncJob asyncJob)
+qemuSnapshotCreateActiveDisks(virDomainObj *vm,
+ virDomainMomentObj *snap,
+ GHashTable *blockNamedNodeData,
+ unsigned int flags,
+ virDomainAsyncJob asyncJob)
{
bool reuse = (flags & VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT) != 0;
g_autoptr(qemuSnapshotDiskContext) snapctxt = NULL;
@@ -1229,8 +1247,8 @@ qemuSnapshotCreateActiveExternalDisks(virDomainObj *vm,
/* prepare a list of objects to use in the vm definition so that we don't
* have to roll back later */
- if (!(snapctxt = qemuSnapshotDiskPrepareActiveExternal(vm, snap, reuse,
- blockNamedNodeData, asyncJob)))
+ if (!(snapctxt = qemuSnapshotDiskPrepareActive(vm, snap, reuse,
+ blockNamedNodeData, asyncJob)))
return -1;
if (qemuSnapshotDiskCreate(snapctxt) < 0)
@@ -1370,9 +1388,9 @@ qemuSnapshotCreateActiveExternal(virQEMUDriver *driver,
/* the domain is now paused if a memory snapshot was requested */
- if ((ret = qemuSnapshotCreateActiveExternalDisks(vm, snap,
- blockNamedNodeData, flags,
- VIR_ASYNC_JOB_SNAPSHOT)) < 0)
+ if ((ret = qemuSnapshotCreateActiveDisks(vm, snap,
+ blockNamedNodeData, flags,
+ VIR_ASYNC_JOB_SNAPSHOT)) < 0)
goto cleanup;
/* the snapshot is complete now */
diff --git a/tests/qemudomainsnapshotxml2xmlin/disk_snapshot.xml b/tests/qemudomainsnapshotxml2xmlin/disk_snapshot.xml
index cf5ea0814e..87b6251a7f 100644
--- a/tests/qemudomainsnapshotxml2xmlin/disk_snapshot.xml
+++ b/tests/qemudomainsnapshotxml2xmlin/disk_snapshot.xml
@@ -4,7 +4,7 @@
<disks>
<disk name='/dev/HostVG/QEMUGuest1'/>
<disk name='hdb' snapshot='no'/>
- <disk name='hdc' snapshot='internal'/>
+ <disk name='hdc' snapshot='internal' snapshotName='snap1'/>
<disk name='hdd' snapshot='external'>
<source/>
<driver type='qed'/>
diff --git a/tests/qemudomainsnapshotxml2xmlout/disk_snapshot.xml b/tests/qemudomainsnapshotxml2xmlout/disk_snapshot.xml
index 76c543d25c..6cf93183d5 100644
--- a/tests/qemudomainsnapshotxml2xmlout/disk_snapshot.xml
+++ b/tests/qemudomainsnapshotxml2xmlout/disk_snapshot.xml
@@ -5,7 +5,7 @@
<disks>
<disk name='/dev/HostVG/QEMUGuest1'/>
<disk name='hdb' snapshot='no'/>
- <disk name='hdc' snapshot='internal'/>
+ <disk name='hdc' snapshot='internal' snapshotName='snap1'/>
<disk name='hdd' snapshot='external' type='file'>
<driver type='qed'/>
</disk>
diff --git a/tests/qemudomainsnapshotxml2xmlout/disk_snapshot_redefine.xml b/tests/qemudomainsnapshotxml2xmlout/disk_snapshot_redefine.xml
index 24b41ba7c5..f574793edf 100644
--- a/tests/qemudomainsnapshotxml2xmlout/disk_snapshot_redefine.xml
+++ b/tests/qemudomainsnapshotxml2xmlout/disk_snapshot_redefine.xml
@@ -10,7 +10,7 @@
<disks>
<disk name='hda' snapshot='no'/>
<disk name='hdb' snapshot='no'/>
- <disk name='hdc' snapshot='internal'/>
+ <disk name='hdc' snapshot='internal' snapshotName='snap1'/>
<disk name='hdd' snapshot='external' type='file'>
<driver type='qed'/>
<source file='/path/to/generated4'/>
diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c
index 1db1f2b949..1269c74e43 100644
--- a/tests/qemumonitorjsontest.c
+++ b/tests/qemumonitorjsontest.c
@@ -2587,6 +2587,7 @@ testQemuMonitorJSONTransaction(const void *opaque)
qemuMonitorTransactionBitmapDisable(actions, "node4", "bitmap4") < 0 ||
qemuMonitorTransactionBitmapMerge(actions, "node5", "bitmap5", &mergebitmaps) < 0 ||
qemuMonitorTransactionSnapshotBlockdev(actions, "node7", "overlay7") < 0 ||
+ qemuMonitorTransactionInternalSnapshotBlockdev(actions, "device1", "snapshot1") < 0 ||
qemuMonitorTransactionBackup(actions, "dev8", "job8", "target8", "bitmap8",
QEMU_MONITOR_TRANSACTION_BACKUP_SYNC_MODE_NONE) < 0 ||
qemuMonitorTransactionBackup(actions, "dev9", "job9", "target9", "bitmap9",
--
2.25.1On Wed, Feb 15, 2023 at 05:28:19 -0600, Or Ozeri wrote:
> libvirt supports taking external disk snapshots on a running VM,
> using qemu's "blockdev-snapshot" command.
> qemu also supports "blockdev-snapshot-internal-sync" to do the
> same for internal snapshots.
> This commit wraps this (old) qemu capability to allow future libvirt
> users to take internal disk snapshots on a running VM.
> This will only work for disk types which support internal snapshots,
> and thus we require the disk type to be part of a white list of known
> types. For this commit, the list of supported formats is empty.
> An upcoming commit will allow RBD disks to use this new capability.
>
> Signed-off-by: Or Ozeri <oro@il.ibm.com>
> ---
[...]
There's too much going on in this commit:
- adds new monitor API and its tests
- renames function for preparing snapshot data
- adds the code to do internal snapshot into it
- modifies checks to report error
- modifies the external memory snapshot code to work with the new
snapshot
Split out at least the rename and monitor addition into two separate
patches.
Similarly the bit which shuffles around and prepares the snapshot data
can be done as a separate commit.
> diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
> index c1855b3028..e82352ba7d 100644
> --- a/src/qemu/qemu_snapshot.c
> +++ b/src/qemu/qemu_snapshot.c
> @@ -736,7 +736,7 @@ qemuSnapshotPrepare(virDomainObj *vm,
[...]
> @@ -1229,8 +1247,8 @@ qemuSnapshotCreateActiveExternalDisks(virDomainObj *vm,
>
> /* prepare a list of objects to use in the vm definition so that we don't
> * have to roll back later */
> - if (!(snapctxt = qemuSnapshotDiskPrepareActiveExternal(vm, snap, reuse,
> - blockNamedNodeData, asyncJob)))
> + if (!(snapctxt = qemuSnapshotDiskPrepareActive(vm, snap, reuse,
> + blockNamedNodeData, asyncJob)))
> return -1;
>
> if (qemuSnapshotDiskCreate(snapctxt) < 0)
[...]
> @@ -1370,9 +1388,9 @@ qemuSnapshotCreateActiveExternal(virQEMUDriver *driver,
>
> /* the domain is now paused if a memory snapshot was requested */
>
> - if ((ret = qemuSnapshotCreateActiveExternalDisks(vm, snap,
> - blockNamedNodeData, flags,
> - VIR_ASYNC_JOB_SNAPSHOT)) < 0)
> + if ((ret = qemuSnapshotCreateActiveDisks(vm, snap,
> + blockNamedNodeData, flags,
> + VIR_ASYNC_JOB_SNAPSHOT)) < 0)
This modification is done to a function named
`qemuSnapshotCreateActiveExternal` but clearly enabled creation of
internal snapshots. You'll need to address that one too.
Since your patches add intermingling (at least) partial of snapshots the
code will need to be split up separately.
1) check that the old-style full internal snapshot is requested and deal
with that as a special case
2) separate qemuSnapshotCreateActiveExternal into:
- 2.1) - the bit that creates the external memory snapshot
- 2.2) - the bit that creates the external disk snapshot
- 2.3) - modify the bit that creates external disk snapshot to also
do the internal disk snapshot for rbd
- 2.4) - call them in proper sequence. I suspect you also want to do
a external memory snapshot including the internal RBD
snapshot as s transaction.
Good bit is that the oldschool internal snapshot is a completely
separate case and can't be intermingled here.
But the existing code must be modified to honour the change of semantics
you are introducing.
> -----Original Message----- > From: Peter Krempa <pkrempa@redhat.com> > Sent: Monday, 20 February 2023 16:00 > To: Or Ozeri <ORO@il.ibm.com> > Cc: libvir-list@redhat.com; Danny Harnik <DANNYH@il.ibm.com> > Subject: [EXTERNAL] Re: [PATCH v2 3/6] qemu: Add internal support for > active disk internal snapshots > > This modification is done to a function named > `qemuSnapshotCreateActiveExternal` but clearly enabled creation of internal > snapshots. You'll need to address that one too. By address, you mean rename? It treats both external and disks-only (which can be internal / external / mixed) So maybe qemuSnapshotCreateActiveExternalOrDisksOnly? I can't think of a neat name for it :\ > > Since your patches add intermingling (at least) partial of snapshots the code > will need to be split up separately. > > 1) check that the old-style full internal snapshot is requested and deal with > that as a special case This check already exists, so I guess no change is required? > 2) separate qemuSnapshotCreateActiveExternal into: > - 2.1) - the bit that creates the external memory snapshot > - 2.2) - the bit that creates the external disk snapshot You mean separate each of the bits mentioned above to a new function? The bit that creates the memory snapshot also pauses the VM, and unpause after all is done (including the disks snapshot). I'm guessing you do not want to include this as part of the 2.1 mentioned above? > - 2.3) - modify the bit that creates external disk snapshot to also > do the internal disk snapshot for rbd > - 2.4) - call them in proper sequence. I suspect you also want to do > a external memory snapshot including the internal RBD > snapshot as s transaction. Actually no, there is no current use-case for that (that I know of). > > Good bit is that the oldschool internal snapshot is a completely separate case > and can't be intermingled here. > > But the existing code must be modified to honour the change of semantics > you are introducing.
© 2016 - 2026 Red Hat, Inc.