On Tue, Jun 27, 2023 at 17:07:24 +0200, Pavel Hrdina wrote:
> With introduction of external snapshot revert we will have to update
> backing store of qcow images not actively used be QEMU manually.
> The need for this patch comes from the fact that we stop and start QEMU
> process therefore after revert not all existing snapshots will be known
> to that QEMU process due to reverting to non-leaf snapshot or having
> multiple branches.
>
> We need to loop over all existing snapshots and check all disks to see
> if they happen to have the image we are deleting as backing store and
> update them to point to the new image except for images currently used
> by the running QEMU process doing the merge operation.
>
> Signed-off-by: Pavel Hrdina <phrdina@xxxxxxxxxx>
> ---
> src/qemu/qemu_snapshot.c | 95 ++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 95 insertions(+)
>
> diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
> index 2950ad7d77..337c83f151 100644
> --- a/src/qemu/qemu_snapshot.c
> +++ b/src/qemu/qemu_snapshot.c
> @@ -2530,6 +2530,8 @@ typedef struct _qemuSnapshotDeleteExternalData {
> virStorageSource *parentDiskSrc; /* backing disk source of the @diskSrc */
> virStorageSource *prevDiskSrc; /* source of disk for which @diskSrc is
> backing disk */
> + GSList *disksWithBacking; /* list of storage source for which the
> + deleted storage source is backing store */
> qemuBlockJobData *job;
> bool merge;
> } qemuSnapshotDeleteExternalData;
> @@ -2542,6 +2544,7 @@ qemuSnapshotDeleteExternalDataFree(qemuSnapshotDeleteExternalData *data)
> return;
>
> virObjectUnref(data->job);
> + g_slist_free(data->disksWithBacking);
>
> g_free(data);
> }
> @@ -2591,6 +2594,60 @@ qemuSnapshotFindParentSnapForDisk(virDomainMomentObj *snap,
> }
>
>
> +struct _qemuSnapshotDisksWithBackingStoreData {
> + virDomainMomentObj *current;
> + virStorageSource *diskSrc;
> + GSList **disksWithBacking;
> +};
> +
> +
> +static int
> +qemuSnapshotDiskHasBackingDisk(void *payload,
> + const char *name G_GNUC_UNUSED,
> + void *opaque)
> +{
> + virDomainMomentObj *snap = payload;
> + virDomainSnapshotDef *snapdef = virDomainSnapshotObjGetDef(snap);
> + struct _qemuSnapshotDisksWithBackingStoreData *data = opaque;
> + ssize_t i;
> +
> + /* skip snapshots that are within the active snapshot tree as it will be handled
> + * by qemu */
> + if (virDomainMomentIsAncestor(data->current, snap) || data->current == snap)
> + return 0;
> +
> + for (i = 0; i < snapdef->parent.dom->ndisks; i++) {
> + virDomainDiskDef *disk = snapdef->parent.dom->disks[i];
> +
> + if (!virStorageSourceIsLocalStorage(disk->src))
> + continue;
> +
> + if (!disk->src->backingStore)
> + ignore_value(virStorageSourceGetMetadata(disk->src, -1, -1, 1, false));
Please note that in case of snapshots created by libvirt the definition
will contain the backing chain declared in the XML.
This is done when creating the snapshot in
qemuSnapshotDiskUpdateSource() where the previous backing chain is moved
as new 'backingStore' of the new image. This is done also for the
persistent definition.
Also note that if such definition is found in the XML libvirt will not
probe metadata from images, but rather take what's in the XML
definition.
This means that if you don't update the metadata internally, we'd still
attempt to instruct qemu to open the image regardless of what the qcow2
metadata say.
Also note on NFS usage. Using -1 for uid/gid will result in the code not
working on root-squash nfs.
> +
> + if (virStorageSourceIsSameLocation(disk->src->backingStore, data->diskSrc))
> + *data->disksWithBacking = g_slist_prepend(*data->disksWithBacking, disk->src);
> + }
> +
> + return 0;
> +}
> +
> +
> +static void
> +qemuSnapshotGetDisksWithBackingStore(virDomainObj *vm,
> + virDomainMomentObj *snap,
> + qemuSnapshotDeleteExternalData *data)
> +{
> + struct _qemuSnapshotDisksWithBackingStoreData iterData;
> +
> + iterData.current = virDomainSnapshotGetCurrent(vm->snapshots);
> + iterData.diskSrc = data->diskSrc;
> + iterData.disksWithBacking = &data->disksWithBacking;
> +
> + virDomainMomentForEachDescendant(snap, qemuSnapshotDiskHasBackingDisk, &iterData);
> +}
> +
> +
> /**
> * qemuSnapshotDeleteExternalPrepareData:
> * @vm: domain object
> @@ -2682,6 +2739,8 @@ qemuSnapshotDeleteExternalPrepareData(virDomainObj *vm,
> _("snapshot VM disk source and parent disk source are not the same"));
> return -1;
> }
> +
> + qemuSnapshotGetDisksWithBackingStore(vm, snap, data);
> }
>
> data->parentSnap = qemuSnapshotFindParentSnapForDisk(snap, data->snapDisk);
> @@ -3063,6 +3122,40 @@ qemuSnapshotSetInvalid(virDomainObj *vm,
> }
>
>
> +static void
> +qemuSnapshotUpdateBackingStore(virDomainObj *vm,
> + qemuSnapshotDeleteExternalData *data)
> +{
> + GSList *cur = NULL;
> + const char *qemuImgPath;
> + virQEMUDriver *driver = QEMU_DOMAIN_PRIVATE(vm)->driver;
> +
> + if (!(qemuImgPath = qemuFindQemuImgBinary(driver)))
> + return;
> +
> + for (cur = data->disksWithBacking; cur; cur = g_slist_next(cur)) {
> + virStorageSource *diskSrc = cur->data;
> + g_autoptr(virCommand) cmd = NULL;
> +
> + /* creates cmd line args: qemu-img create -f qcow2 -o */
> + if (!(cmd = virCommandNewArgList(qemuImgPath,
> + "rebase",
> + "-u",
> + "-F",
> + virStorageFileFormatTypeToString(data->parentDiskSrc->format),
> + "-f",
> + virStorageFileFormatTypeToString(diskSrc->format),
> + "-b",
> + data->parentDiskSrc->path,
> + diskSrc->path,
> + NULL)))
> + continue;
> +
> + ignore_value(virCommandRun(cmd, NULL));
... similarly here. If the image is stored on root-squash nfs this will
not work as we'll try to run qemu-img as root.
> + }
> +}
> +
> +
> static int
> qemuSnapshotDiscardExternal(virDomainObj *vm,
> virDomainMomentObj *snap,
> @@ -3149,6 +3242,8 @@ qemuSnapshotDiscardExternal(virDomainObj *vm,
>
> qemuBlockJobSyncEnd(vm, data->job, VIR_ASYNC_JOB_SNAPSHOT);
>
> + qemuSnapshotUpdateBackingStore(vm, data);
> +
> if (qemuSnapshotSetInvalid(vm, data->parentSnap, data->snapDisk, false) < 0)
> goto error;
> }
> --
> 2.41.0
>
