On Thu, Dec 08, 2022 at 14:30:59 +0100, 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 <phrd...@redhat.com>
> ---
> src/qemu/qemu_snapshot.c | 163 +++++++++++++++++++++++++++++++++++++++
> 1 file changed, 163 insertions(+)
>
> diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
> index 97df448363..882224b0a7 100644
> --- a/src/qemu/qemu_snapshot.c
> +++ b/src/qemu/qemu_snapshot.c
> @@ -2232,6 +2232,134 @@ qemuSnapshotRevert(virDomainObj *vm,
> }
>
>
> +typedef struct _qemuSnapshotDeleteExternalData {
> + virDomainMomentObj *parentSnap;
> + virDomainSnapshotDiskDef *snapDisk;
> + virDomainDiskDef *domDisk;
> + virDomainDiskDef *parentDomDisk;
> + virStorageSource *diskSrc;
> + virStorageSource *parentDiskSrc;
> + virStorageSource *prevDiskSrc;
> + qemuBlockJobData *job;
Please annotate which 'source' belongs to which 'disk def' and such.
You can also theoretically reorder them.
> +} qemuSnapshotDeleteExternalData;
> +
> +G_DEFINE_AUTOPTR_CLEANUP_FUNC(qemuSnapshotDeleteExternalData, g_free);
> +
> +
> +/**
> + * qemuSnapshotFindParentSnapForDisk:
> + * @snap: snapshot object that is about to be deleted
> + * @snapDisk: snapshot disk definition
> + *
> + * Find parent snapshot object that contains snapshot of the @snapDisk.
> + * It may not be the next snapshot in the snapshot tree as the disk in
> + * question may be skipped by using VIR_DOMAIN_SNAPSHOT_LOCATION_NO.
> + *
> + * Returns virDomainMomentObj* on success or NULL if there is no parent
> + * snapshot containing the @snapDisk.
> + * */
> +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 GSList*
> +qemuSnapshotDeleteExternalPrepare(virDomainObj *vm,
> + virDomainMomentObj *snap)
> +{
> + ssize_t i;
> + virDomainSnapshotDef *snapdef = virDomainSnapshotObjGetDef(snap);
> + g_autoslist(qemuSnapshotDeleteExternalData) ret = NULL;
> +
> + 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;
> +
> + data = g_new0(qemuSnapshotDeleteExternalData, 1);
> + data->snapDisk = snapDisk;
> +
> + data->domDisk = qemuDomainDiskByName(vm->def, snapDisk->name);
> + if (!data->domDisk)
> + return NULL;
> +
> + data->diskSrc = virStorageSourceChainLookup(data->domDisk->src, NULL,
> +
> data->snapDisk->src->path,
So here you are looking up based on 'src->path' which works properly
really only for file-based/local storage. For anything else it will
break.
Since you have a virStorageSource to compare to you should probably
create a helper which walks the backing chain and uses
virStorageSourceIsSameLocation internally instead virStorageSourceChainLookup
which should really be used only for user-provided data as the third
argument (for backwards compatibility with old-style API args).
> + NULL,
> &data->prevDiskSrc);
> + if (!data->diskSrc)
> + return NULL;
> +
> + if (!virStorageSourceIsSameLocation(data->diskSrc,
> data->snapDisk->src)) {
> + virReportError(VIR_ERR_OPERATION_FAILED, "%s",
> + _("VM disk source and snapshot disk source are
> not the same"));
> + return NULL;
> + }
> +
> + data->parentDomDisk = virDomainDiskByTarget(snapdef->parent.dom,
> + data->snapDisk->name);
> + if (!data->parentDomDisk) {
> + virReportError(VIR_ERR_OPERATION_FAILED,
> + _("failed to find disk '%s' in snapshot VM XML"),
> + snapDisk->name);
> + return NULL;
> + }
> +
> + if (virDomainObjIsActive(vm)) {
> + data->parentDiskSrc = data->diskSrc->backingStore;
> + if (!data->parentDiskSrc) {
Note that active definitions contain also a "terminator"
virStorageSource put into the 'backingStore' object, which is a valid
pointer. You probably want to use 'virStorageSourceIsBacking' here.
> + virReportError(VIR_ERR_OPERATION_FAILED, "%s",
> + _("failed to find parent disk source in
> backing chain"));
> + return NULL;
> + }
> +
> + if (!virStorageSourceIsSameLocation(data->parentDiskSrc,
> data->parentDomDisk->src)) {
> + virReportError(VIR_ERR_OPERATION_FAILED, "%s",
> + _("snapshot VM disk source and parent disk
> source are not the same"));
> + return NULL;
> + }
> + }
> +
> + data->parentSnap = qemuSnapshotFindParentSnapForDisk(snap,
> data->snapDisk);
> +
> + if (data->parentSnap &&
> !virDomainSnapshotIsExternal(data->parentSnap)) {
> + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
> + _("deleting external snapshot that has internal
> snapshot as parent not supported"));
> + return NULL;
> + }
> +
> + ret = g_slist_append(ret, g_steal_pointer(&data));
The usual way to work with singly-linked lists is to use the prepend
operation ...
> + }
> +
... and reverse it before returning.
> + return g_steal_pointer(&ret);
> +}
> +
> +
> typedef struct _virQEMUMomentReparent virQEMUMomentReparent;
> struct _virQEMUMomentReparent {
> const char *dir;
> @@ -2565,6 +2693,10 @@ qemuSnapshotDelete(virDomainObj *vm,
> int ret = -1;
> virDomainMomentObj *snap = NULL;
> bool metadata_only = !!(flags &
> VIR_DOMAIN_SNAPSHOT_DELETE_METADATA_ONLY);
> + bool stop_qemu = false;
> + qemuDomainObjPrivate *priv = vm->privateData;
> + virQEMUDriver *driver = priv->driver;
> + g_autoslist(qemuSnapshotDeleteExternalData) externalData = NULL;
>
> virCheckFlags(VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN |
> VIR_DOMAIN_SNAPSHOT_DELETE_METADATA_ONLY |
> @@ -2582,6 +2714,32 @@ qemuSnapshotDelete(virDomainObj *vm,
> if (!metadata_only) {
> if (qemuSnapshotDeleteValidate(vm, snap, flags) < 0)
> goto endjob;
> +
> + if (virDomainSnapshotIsExternal(snap) &&
> + !(flags & (VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN |
> + VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY))) {
> +
> + externalData = qemuSnapshotDeleteExternalPrepare(vm, snap);
> +
> + if (!virDomainObjIsActive(vm)) {
> + if (qemuProcessStart(NULL, driver, vm, NULL,
> VIR_ASYNC_JOB_SNAPSHOT,
> + NULL, -1, NULL, NULL,
> + VIR_NETDEV_VPORT_PROFILE_OP_CREATE,
> + VIR_QEMU_PROCESS_START_PAUSED) < 0) {
> + goto endjob;
> + }
> +
> + stop_qemu = true;
> +
> + /* Call the prepare again as some data require that the VM is
> + * running to get everything we need. */
> + g_slist_free_full(externalData, g_free);
> + externalData = qemuSnapshotDeleteExternalPrepare(vm, snap);
Preferrably don't reuse 'externalData'. You can e.g. instead steal the
value into a locally scoped temporary autofreed variable and then
re-create it.
> + }
> +
> + if (!externalData)
> + goto endjob;
> + }
If you deal with the lookup properly you can add:
Reviewed-by: Peter Krempa <pkre...@redhat.com>
