On Tue, Sep 06, 2022 at 12:19:38PM +0200, Peter Krempa wrote:
On Tue, Aug 23, 2022 at 18:32:23 +0200, Pavel Hrdina wrote:
In order to save some CPU cycles we will collect all the necessary data to delete external snapshot before we even start. They will be later used by code that deletes the snapshots and updates metadata when needed.
With external snapshots we need data that libvirt gets from running QEMU process so if the VM is not running we need to start paused QEMU process for the snapshot deletion and kill at afterwards.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 144 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 142 insertions(+), 2 deletions(-)
First a few general notes for all upcoming patches.
1) All of the non-trivial functions dealing with snapshots should have documentation explaing what they do. The snapshot code, as you certainly know, is very complex and in many cases un-obvious. Having them will save us (or anybody else wanting to maintain the code) some headaches in the future.
2) I'd also suggest that you add a knowledge base internals article outlining how snapshot deletion is supposed to work. In case there are some user-visible caveats, we also have an user-focused article on snapshots ( kbase/snapshots.rst ).
3) I'll try to assess the code also from the point of view of changes needed to implement reversion of snapshots and the intricacies that might bring for deletion.
diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index da9b4c30f0..dade5dcea4 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -2232,6 +2232,120 @@ qemuSnapshotRevert(virDomainObj *vm, }
+typedef struct { + virDomainMomentObj *parentSnap; + virDomainSnapshotDiskDef *snapDisk; + virDomainDiskDef *domDisk; + virDomainDiskDef *parentDomDisk; + virStorageSource *diskSrc; + virStorageSource *parentDiskSrc; + virStorageSource *prevDiskSrc; + qemuBlockJobData *job; +} qemuSnapshotDeleteExternalData; + + +static virDomainMomentObj* +qemuSnapshotFindParentSnapForDisk(virDomainMomentObj *snap, + virDomainSnapshotDiskDef *snapDisk) +{ + virDomainMomentObj *parentSnap = snap->parent; + + while (parentSnap) { + ssize_t i; + virDomainSnapshotDef *parentSnapdef = virDomainSnapshotObjGetDef(parentSnap); + + if (!parentSnapdef) + break; + + for (i = 0; i < parentSnapdef->ndisks; i++) { + virDomainSnapshotDiskDef *parentSnapDisk = &(parentSnapdef->disks[i]); + + if (parentSnapDisk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_NO && + STREQ(snapDisk->name, parentSnapDisk->name)) { + return parentSnap; + } + } + + parentSnap = parentSnap->parent; + } + + return NULL; +} + + +static GPtrArray* +qemuSnapshotDeleteExternalPrepare(virDomainObj *vm, + virDomainMomentObj *snap) +{ + ssize_t i; + virDomainSnapshotDef *snapdef = virDomainSnapshotObjGetDef(snap); + g_autoptr(GPtrArray) ret = g_ptr_array_new_full(0, g_free);
Is there any specific requirement to access these randomly? (specifically why GPtrArray is used instead of e.g. a G(S)List?
There is no need to access these randomly but G(S)List doesn't work that nicely with g_autoptr and dynamically allocated elements. For this case we would have to use g_list_free_full().
+ for (i = 0; i < snapdef->ndisks; i++) { + g_autofree qemuSnapshotDeleteExternalData *data = NULL; + virDomainSnapshotDiskDef *snapDisk = &(snapdef->disks[i]); + + if (snapDisk->snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_NO) + continue;
[...]
typedef struct _virQEMUMomentReparent virQEMUMomentReparent; struct _virQEMUMomentReparent { const char *dir; @@ -2504,9 +2618,9 @@ qemuSnapshotDeleteValidate(virDomainMomentObj *snap, }
if (virDomainSnapshotIsExternal(snap) && - !(flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY)) { + (flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("deletion of external disk snapshots not supported")); + _("deletion of external disk snapshot with internal children disk snapshots not supported"));
This seems a bit unrelated to this patch.
return -1; }
@@ -2523,6 +2637,8 @@ qemuSnapshotDelete(virDomainObj *vm, int ret = -1; virDomainMomentObj *snap = NULL; bool metadata_only = !!(flags & VIR_DOMAIN_SNAPSHOT_DELETE_METADATA_ONLY); + bool stop_qemu = false; + g_autoptr(GPtrArray) externalData = NULL;
virCheckFlags(VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN | VIR_DOMAIN_SNAPSHOT_DELETE_METADATA_ONLY | @@ -2537,6 +2653,25 @@ qemuSnapshotDelete(virDomainObj *vm, if (!metadata_only) { if (qemuSnapshotDeleteValidate(snap, flags) < 0) goto endjob; + + if (virDomainSnapshotIsExternal(snap) && + !(flags & (VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN | + VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY))) { + if (!virDomainObjIsActive(vm)) { + if (qemuProcessStart(NULL, driver, vm, NULL, VIR_ASYNC_JOB_NONE, + NULL, -1, NULL, NULL, + VIR_NETDEV_VPORT_PROFILE_OP_CREATE, + VIR_QEMU_PROCESS_START_PAUSED) < 0) {
Note that a user might attempt to call a 'virsh resume' here. This might be a problem especially if you went the route of asynchronously handling the deletion.
+ goto endjob; + } + + stop_qemu = true; + } + + externalData = qemuSnapshotDeleteExternalPrepare(vm, snap); + if (!externalData) + goto endjob;
Preferrably this operation will be done before starting qemu to do the actual deletion. I understand that it has to be done after, to have correct pointers after the copy of the definition needed for the startup.
In this instance it probably is still better to do the checks first, throw them away if qemu startup is needed and re-do them after, as the startup of qemu is a waaay more expensive operation.
Yeah I don't like this ordering as well, I'll check what parts of the qemuSnapshotDeleteExternalPrepare() can be done without qemu running.
+ } }
if (flags & (VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN | @@ -2547,6 +2682,11 @@ qemuSnapshotDelete(virDomainObj *vm, }
endjob: + if (stop_qemu) { + qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_SHUTDOWN, + VIR_ASYNC_JOB_NONE, 0);
So definitely user interaction of the VM needs to be forbidden during this API because it would be bad if the VM gets terminated here.
Good point, will look into it. Pavel