On Wed, Sep 07, 2022 at 03:06:51PM +0200, Peter Krempa wrote:
> On Tue, Aug 23, 2022 at 18:32:25 +0200, Pavel Hrdina wrote:
> > With external snapshots we need to modify the metadata bit more then
> > what is required for internal snapshots. Mainly the storage source
> > location changes with every external snapshot.
> >
> > This means that if we delete non-leaf snapshot we need to update all
> > children snapshots and modify the disk sources for all affected disks.
> >
> > Signed-off-by: Pavel Hrdina <phrdina@xxxxxxxxxx>
> > ---
> > src/qemu/qemu_snapshot.c | 116 +++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 116 insertions(+)
> >
> > diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
> > index 64ee395230..37ae3f04d0 100644
> > --- a/src/qemu/qemu_snapshot.c
> > +++ b/src/qemu/qemu_snapshot.c
> > @@ -2380,6 +2380,109 @@ qemuSnapshotChildrenReparent(void *payload,
> > }
> >
> >
> > +typedef struct _qemuSnapshotUpdateDisksData qemuSnapshotUpdateDisksData;
> > +struct _qemuSnapshotUpdateDisksData {
> > + virDomainMomentObj *snap;
> > + virDomainObj *vm;
> > + virQEMUDriver *driver;
>
> 'driver' is present in private data of 'vm'
>
> > + int error;
> > + int (*writeMetadata)(virDomainObj *, virDomainMomentObj *,
> > + virDomainXMLOption *, const char *);
>
> The only function pointer that is ever populated here is
> qemuDomainSnapshotWriteMetadata. So you can use that directly from the
> function that uses this.
>
> > +};
> > +
> > +
> > +static int
> > +qemuSnapshotUpdateDisksSingle(virDomainMomentObj *snap,
> > + virDomainDef *def,
> > + virDomainDef *parentDef,
> > + virDomainSnapshotDiskDef *snapDisk)
> > +{
> > + virDomainDiskDef *disk = NULL;
> > +
> > + if (!(disk = qemuDomainDiskByName(def, snapDisk->name)))
> > + return -1;
> > +
> > + if (virDomainSnapshotIsExternal(snap)) {
>
> So IIUC, this bit is done only for external snapshots ...
>
> > + virDomainDiskDef *parentDisk = NULL;
> > +
> > + if (!(parentDisk = qemuDomainDiskByName(parentDef, snapDisk->name)))
> > + return -1;
> > +
> > + if (virStorageSourceIsSameLocation(snapDisk->src, disk->src)) {
> > + virObjectUnref(disk->src);
> > + disk->src = virStorageSourceCopy(parentDisk->src, false);
> > + }
> > + }
> > +
> > + if (disk->src->backingStore) {
>
> ... but this even for internal ones? That doesn't seem right.
Needs to be documented as pointed elsewhere. The first part is relevant
only to external snapshots because when you delete one the whole image
disappears and you need to update other metadata files to not point to
the deleted file anymore. So for example:
test.qcow2
|
+-test.snap1
|
+-test.snap2
There the snap2 metadata file will have in the domain def for the state
before snapshot 'test.snap1' as a disk source. If you delete the snap1
we need to change the source to 'test.qcow2' in the snap2 metadata.
But the next part that modifies the backing chain in the domain
definition is relevant to internal snapshot metadata files as well.
Pavel
> > + virStorageSource *cur = disk->src;
> > + virStorageSource *next = disk->src->backingStore;
> > +
> > + while (next) {
> > + if (virStorageSourceIsSameLocation(snapDisk->src, next)) {
> > + cur->backingStore = next->backingStore;
> > + next->backingStore = NULL;
> > + virObjectUnref(next);
> > + break;
> > + }
>
> So you are looking up the image corresponding to the snapshot disk and
> trying to move it one level up, right?
This removes source we are deleting from the backing chain. Could
probably rename next -> cur and cur -> prev.
> > +
> > + cur = next;
> > + next = cur->backingStore;
> > + }
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +
> > +static int
> > +qemuSnapshotDeleteUpdateDisks(void *payload,
> > + const char *name G_GNUC_UNUSED,
> > + void *opaque)
> > +{
> > + virDomainMomentObj *snap = payload;
> > + qemuSnapshotUpdateDisksData *data = opaque;
> > + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(data->driver);
> > + virDomainSnapshotDef *snapdef = virDomainSnapshotObjGetDef(data->snap);
> > + ssize_t i;
> > +
> > + if (data->error < 0)
> > + return 0;
>
> At this point I'm afraid we can't afford to deal with errors the usual
> way but have to try to update as much metadata as possible.
>
> If the first error breaks up everything then the state of the disk
> images will not correspond to the metadata.
>
> > +
> > + for (i = 0; i < snapdef->ndisks; i++) {
> > + virDomainSnapshotDiskDef *snapDisk = &(snapdef->disks[i]);
> > +
> > + if (snapDisk->snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_NO)
> > + continue;
> > +
> > + if (qemuSnapshotUpdateDisksSingle(snap, snap->def->dom,
> > + data->snap->def->dom, snapDisk) < 0) {
> > + data->error = -1;
> > + return 0;
> > + }
> > +
> > + if (snap->def->inactiveDom) {
> > + virDomainDef *dom = data->snap->def->inactiveDom;
> > +
> > + if (!dom)
> > + dom = data->snap->def->dom;
> > +
> > + if (qemuSnapshotUpdateDisksSingle(snap, snap->def->inactiveDom,
> > + dom, snapDisk) < 0) {
>
> The failure of qemuDomainDiskByName, in this case probably should not be
> fatal, if the next-boot config diverges from the running config.
>
>
> > + data->error = -1;
> > + return 0;
> > + }
> > + }
> > + }
> > +
> > + data->error = data->writeMetadata(data->vm,
> > + snap,
> > + data->driver->xmlopt,
> > + cfg->snapshotDir);
> > + return 0;
> > +}
>
Attachment:
signature.asc
Description: PGP signature
