[libvirt PATCH v2 00/24] introduce external snapshot revert support
This implements virDomainRevertToSnapshot to work with external snapshots. In addition it modifies virDomainSnapshotDelete to work correctly when we revert to non-leaf snapshot or when there is non-linear snapshot tree with multiple branches. Gitlab repo with the patches: https://gitlab.com/phrdina/libvirt/-/tree/snapshot-revert-external Pavel Hrdina (24): libvirt_private: list virDomainMomentDefPostParse snapshot_conf: export virDomainSnapshotDiskDefClear snapshot_conf: use alternate domain definition in virDomainSnapshotDefAssignExternalNames snapshot_conf: introduce <revertDisks> metadata element snapshot_conf: add new argument to virDomainSnapshotAlignDisks qemu_snapshot: introduce qemuSnapshotDomainDefUpdateDisk qemu_snapshot: use virDomainDiskByName while updating domain def qemu_snapshot: introduce qemuSnapshotCreateQcow2Files qemu_snapshot: allow using alternate domain definition when creating qcow2 files qemu_snapshot: move external disk prepare to single function qemu_snapshot: use VIR_ASYNC_JOB_SNAPSHOT when reverting snapshot qemu_snapshot: introduce external snapshot revert support qemu_snapshot: rename qemuSnapshotDeleteExternalPrepare qemu_snapshot: extract external snapshot delete prepare to function qemu_snapshot: add merge to external snapshot delete prepare data qemu_snapshot: prepare data for non-active leaf external snapshot deletion qemu_snapshot: add support to delete external snapshot without block commit qemu_snapshot: delete: properly update parent snapshot with revert data qemu_snapshot: remove revertdisks when creating new snapshot virdomainmomentobjlist: introduce virDomainMomentIsAncestor qemu_snapshot: update backing store after deleting external snapshot qemu_snapshot: check only once if snapshot is external qemu_snapshot: add checks for external snapshot deletion qemu_snapshot: allow snapshot revert for external snapshots src/conf/schemas/domainsnapshot.rng | 7 + src/conf/snapshot_conf.c | 52 +- src/conf/snapshot_conf.h | 11 +- src/conf/virdomainmomentobjlist.c | 17 + src/conf/virdomainmomentobjlist.h | 4 + src/libvirt_private.syms | 6 + src/qemu/qemu_snapshot.c | 874 ++++++++++++++++++++++------ src/test/test_driver.c | 2 +- 8 files changed, 780 insertions(+), 193 deletions(-) -- 2.41.0
We will need to call this function from qemu_snapshot when introducing external snapshot revert support. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> Reviewed-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt_private.syms | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index fb7ad9c855..eed4c17d49 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -807,6 +807,10 @@ virInterfaceDefParseString; virInterfaceDefParseXML; +# conf/moment_conf.h +virDomainMomentDefPostParse; + + # conf/netdev_bandwidth_conf.h virDomainClearNetBandwidth; virNetDevBandwidthFormat; -- 2.41.0
We will need to call this function from qemu_snapshot when introducing external snapshot revert support. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> Reviewed-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/snapshot_conf.c | 2 +- src/conf/snapshot_conf.h | 3 +++ src/libvirt_private.syms | 1 + 3 files changed, 5 insertions(+), 1 deletion(-) diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index 0acba95d7f..cc59bddbc8 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -74,7 +74,7 @@ VIR_ENUM_IMPL(virDomainSnapshotState, ); /* Snapshot Def functions */ -static void +void virDomainSnapshotDiskDefClear(virDomainSnapshotDiskDef *disk) { VIR_FREE(disk->name); diff --git a/src/conf/snapshot_conf.h b/src/conf/snapshot_conf.h index 96c77ef42b..ad49990a1e 100644 --- a/src/conf/snapshot_conf.h +++ b/src/conf/snapshot_conf.h @@ -59,6 +59,9 @@ struct _virDomainSnapshotDiskDef { virStorageSource *src; }; +void +virDomainSnapshotDiskDefClear(virDomainSnapshotDiskDef *disk); + void virDomainSnapshotDiskDefFree(virDomainSnapshotDiskDef *disk); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index eed4c17d49..2b1d4e4512 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1037,6 +1037,7 @@ virDomainSnapshotDefFormat; virDomainSnapshotDefIsExternal; virDomainSnapshotDefNew; virDomainSnapshotDefParseString; +virDomainSnapshotDiskDefClear; virDomainSnapshotDiskDefFree; virDomainSnapshotDiskDefParseXML; virDomainSnapshotFormatConvertXMLFlags; -- 2.41.0
Commit <ef3f3884a2432958bdd4ea0ce45509d47a91a453> introduced new argument for virDomainSnapshotAlignDisks() that allows passing alternate domain definition in case the snapshot parent.dom is NULL. In case of redefining snapshot it will not hit the part of code that unconditionally uses parent.dom as there will not be need to generate default external file names. It should be still fixed to make it safe. Future external snapshot revert code will use this to generate default file names and in this case it would crash. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> Reviewed-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/snapshot_conf.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index cc59bddbc8..ac5aba1753 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -486,12 +486,14 @@ virDomainSnapshotRedefineValidate(virDomainSnapshotDef *def, /** * virDomainSnapshotDefAssignExternalNames: * @def: snapshot def object + * @domdef: domain def object * * Generate default external file names for snapshot targets. Returns 0 on * success, -1 on error. */ static int -virDomainSnapshotDefAssignExternalNames(virDomainSnapshotDef *def) +virDomainSnapshotDefAssignExternalNames(virDomainSnapshotDef *def, + virDomainDef *domdef) { const char *origpath; char *tmppath; @@ -514,7 +516,7 @@ virDomainSnapshotDefAssignExternalNames(virDomainSnapshotDef *def) return -1; } - if (!(origpath = virDomainDiskGetSource(def->parent.dom->disks[i]))) { + if (!(origpath = virDomainDiskGetSource(domdef->disks[i]))) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("cannot generate external snapshot name for disk '%1$s' without source"), disk->name); @@ -702,7 +704,7 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDef *snapdef, } /* Generate default external file names for external snapshot locations */ - if (virDomainSnapshotDefAssignExternalNames(snapdef) < 0) + if (virDomainSnapshotDefAssignExternalNames(snapdef, domdef) < 0) return -1; return 0; -- 2.41.0
This new element will hold the new disk overlay created when reverting to non-leaf snapshot in order to remember the files libvirt created. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> Reviewed-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/schemas/domainsnapshot.rng | 7 +++++++ src/conf/snapshot_conf.c | 27 +++++++++++++++++++++++++++ src/conf/snapshot_conf.h | 5 +++++ 3 files changed, 39 insertions(+) diff --git a/src/conf/schemas/domainsnapshot.rng b/src/conf/schemas/domainsnapshot.rng index 45f01b96cd..2549c47b22 100644 --- a/src/conf/schemas/domainsnapshot.rng +++ b/src/conf/schemas/domainsnapshot.rng @@ -60,6 +60,13 @@ </zeroOrMore> </element> </optional> + <optional> + <element name="revertDisks"> + <zeroOrMore> + <ref name="disksnapshot"/> + </zeroOrMore> + </element> + </optional> <optional> <element name="active"> <choice> diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index ac5aba1753..f6725c0e7b 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -376,6 +376,22 @@ virDomainSnapshotDefParse(xmlXPathContextPtr ctxt, return NULL; } + if (flags & VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE) { + g_autofree xmlNodePtr *revertDiskNodes = NULL; + + if ((n = virXPathNodeSet("./revertDisks/*", ctxt, &revertDiskNodes)) < 0) + return NULL; + if (n) + def->revertdisks = g_new0(virDomainSnapshotDiskDef, n); + def->nrevertdisks = n; + for (i = 0; i < def->nrevertdisks; i++) { + if (virDomainSnapshotDiskDefParseXML(revertDiskNodes[i], ctxt, + &def->revertdisks[i], + flags, xmlopt) < 0) + return NULL; + } + } + if (flags & VIR_DOMAIN_SNAPSHOT_PARSE_INTERNAL) { int active; @@ -834,6 +850,17 @@ virDomainSnapshotDefFormatInternal(virBuffer *buf, virBufferAddLit(buf, "</disks>\n"); } + if (def->nrevertdisks > 0) { + g_auto(virBuffer) childBuf = VIR_BUFFER_INIT_CHILD(buf); + + for (i = 0; i < def->nrevertdisks; i++) { + if (virDomainSnapshotDiskDefFormat(&childBuf, &def->revertdisks[i], xmlopt) < 0) + return -1; + } + + virXMLFormatElement(buf, "revertDisks", NULL, &childBuf); + } + if (def->parent.dom) { if (virDomainDefFormatInternal(def->parent.dom, xmlopt, buf, domainflags) < 0) diff --git a/src/conf/snapshot_conf.h b/src/conf/snapshot_conf.h index ad49990a1e..ab76af604a 100644 --- a/src/conf/snapshot_conf.h +++ b/src/conf/snapshot_conf.h @@ -80,6 +80,11 @@ struct _virDomainSnapshotDef { size_t ndisks; /* should not exceed dom->ndisks */ virDomainSnapshotDiskDef *disks; + /* When we revert to non-leaf snapshot we need to + * store the new overlay disks. */ + size_t nrevertdisks; + virDomainSnapshotDiskDef *revertdisks; + virObject *cookie; }; -- 2.41.0
This new option will be used by external snapshot revert code. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/conf/snapshot_conf.c | 15 ++++++++++++--- src/conf/snapshot_conf.h | 3 ++- src/qemu/qemu_snapshot.c | 2 +- src/test/test_driver.c | 2 +- 4 files changed, 16 insertions(+), 6 deletions(-) diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index f6725c0e7b..d8ebf786cf 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -578,6 +578,8 @@ virDomainSnapshotDefAssignExternalNames(virDomainSnapshotDef *def, * @default_snapshot: snapshot location to assign to disks which don't have any * @uniform_internal_snapshot: Require that for an internal snapshot all disks * take part in the internal snapshot + * @force_default_location: Always use @default_snapshot even if domain def + * has different default value * * Align snapdef->disks to domain definition, filling in any missing disks or * snapshot state defaults given by the domain, with a fallback to @@ -595,6 +597,10 @@ virDomainSnapshotDefAssignExternalNames(virDomainSnapshotDef *def, * in the internal snapshot. This is for hypervisors where granularity of an * internal snapshot can't be controlled. * + * When @force_default_location is true we will always use @default_snapshot + * even if domain definition has different default set. This is required to + * create new snapshot definition when reverting external snapshots. + * * Convert paths to disk targets for uniformity. * * On error -1 is returned and a libvirt error is reported. @@ -603,7 +609,8 @@ int virDomainSnapshotAlignDisks(virDomainSnapshotDef *snapdef, virDomainDef *existingDomainDef, virDomainSnapshotLocation default_snapshot, - bool uniform_internal_snapshot) + bool uniform_internal_snapshot, + bool force_default_location) { virDomainDef *domdef = snapdef->parent.dom; g_autoptr(GHashTable) map = virHashNew(NULL); @@ -711,7 +718,7 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDef *snapdef, /* Don't snapshot empty drives */ if (virStorageSourceIsEmpty(domdef->disks[i]->src)) snapdisk->snapshot = VIR_DOMAIN_SNAPSHOT_LOCATION_NO; - else + else if (!force_default_location) snapdisk->snapshot = domdef->disks[i]->snapshot; snapdisk->src->type = VIR_STORAGE_TYPE_FILE; @@ -967,8 +974,10 @@ virDomainSnapshotRedefinePrep(virDomainObj *vm, virDomainSnapshotDefIsExternal(snapdef)) align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL; - if (virDomainSnapshotAlignDisks(snapdef, otherDomDef, align_location, true) < 0) + if (virDomainSnapshotAlignDisks(snapdef, otherDomDef, align_location, + true, false) < 0) { return -1; + } return 0; } diff --git a/src/conf/snapshot_conf.h b/src/conf/snapshot_conf.h index ab76af604a..14254d1c86 100644 --- a/src/conf/snapshot_conf.h +++ b/src/conf/snapshot_conf.h @@ -126,7 +126,8 @@ char *virDomainSnapshotDefFormat(const char *uuidstr, int virDomainSnapshotAlignDisks(virDomainSnapshotDef *snapshot, virDomainDef *existingDomainDef, virDomainSnapshotLocation default_snapshot, - bool uniform_internal_snapshot); + bool uniform_internal_snapshot, + bool force_default_location); bool virDomainSnapshotDefIsExternal(virDomainSnapshotDef *def); bool virDomainSnapshotIsExternal(virDomainMomentObj *snap); diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 91de8b0c31..844b02d427 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -1585,7 +1585,7 @@ qemuSnapshotCreateAlignDisks(virDomainObj *vm, else def->memory = VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL; } - if (virDomainSnapshotAlignDisks(def, NULL, align_location, true) < 0) + if (virDomainSnapshotAlignDisks(def, NULL, align_location, true, false) < 0) return -1; return 0; diff --git a/src/test/test_driver.c b/src/test/test_driver.c index e7fce053b4..83d97e629c 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -8721,7 +8721,7 @@ testDomainSnapshotAlignDisks(virDomainObj *vm, VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL; } - return virDomainSnapshotAlignDisks(def, NULL, align_location, true); + return virDomainSnapshotAlignDisks(def, NULL, align_location, true, false); } -- 2.41.0
Preferrably: virDomainSnapshotAlignDisks: Allow overriding user-configured snapshot default On Tue, Jun 27, 2023 at 17:07:08 +0200, Pavel Hrdina wrote:
This new option will be used by external snapshot revert code.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/conf/snapshot_conf.c | 15 ++++++++++++--- src/conf/snapshot_conf.h | 3 ++- src/qemu/qemu_snapshot.c | 2 +- src/test/test_driver.c | 2 +- 4 files changed, 16 insertions(+), 6 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Extract the code that updates disks in domain definition while creating external snapshots. We will use it later in the external snapshot revert code. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> Reviewed-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_snapshot.c | 63 ++++++++++++++++++++++++---------------- 1 file changed, 38 insertions(+), 25 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 844b02d427..72a0f71d4f 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -142,6 +142,42 @@ qemuSnapshotFSThaw(virDomainObj *vm, } +static int +qemuSnapshotDomainDefUpdateDisk(virDomainDef *domdef, + virDomainSnapshotDef *snapdef, + bool reuse) +{ + size_t i; + + for (i = 0; i < snapdef->ndisks; i++) { + g_autoptr(virStorageSource) newsrc = NULL; + virDomainSnapshotDiskDef *snapdisk = &(snapdef->disks[i]); + virDomainDiskDef *defdisk = domdef->disks[i]; + + if (snapdisk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) + continue; + + if (!(newsrc = virStorageSourceCopy(snapdisk->src, false))) + return -1; + + if (virStorageSourceInitChainElement(newsrc, defdisk->src, false) < 0) + return -1; + + if (!reuse && + virStorageSourceHasBacking(defdisk->src)) { + defdisk->src->readonly = true; + newsrc->backingStore = g_steal_pointer(&defdisk->src); + } else { + virObjectUnref(defdisk->src); + } + + defdisk->src = g_steal_pointer(&newsrc); + } + + return 0; +} + + /* The domain is expected to be locked and inactive. */ static int qemuSnapshotCreateInactiveInternal(virQEMUDriver *driver, @@ -216,31 +252,8 @@ qemuSnapshotCreateInactiveExternal(virQEMUDriver *driver, } /* update disk definitions */ - for (i = 0; i < snapdef->ndisks; i++) { - g_autoptr(virStorageSource) newsrc = NULL; - - snapdisk = &(snapdef->disks[i]); - defdisk = vm->def->disks[i]; - - if (snapdisk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) - continue; - - if (!(newsrc = virStorageSourceCopy(snapdisk->src, false))) - goto cleanup; - - if (virStorageSourceInitChainElement(newsrc, defdisk->src, false) < 0) - goto cleanup; - - if (!reuse && - virStorageSourceHasBacking(defdisk->src)) { - defdisk->src->readonly = true; - newsrc->backingStore = g_steal_pointer(&defdisk->src); - } else { - virObjectUnref(defdisk->src); - } - - defdisk->src = g_steal_pointer(&newsrc); - } + if (qemuSnapshotDomainDefUpdateDisk(vm->def, snapdef, reuse) < 0) + goto cleanup; if (virDomainDefSave(vm->def, driver->xmlopt, cfg->configDir) < 0) goto cleanup; -- 2.41.0
When creating external snapshot this function is called only when the VM is not running so there is only one definition to care about. However, it will be used by external snapshot revert code for active and inactive definition and they may be different if a disk was (un)plugged only for the active or inactive definition. The current code would crash so use virDomainDiskByName() to get the correct disk from the domain definition based on the disk name and make sure it exists. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> Reviewed-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_snapshot.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 72a0f71d4f..8e1eb21b5d 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -152,11 +152,14 @@ qemuSnapshotDomainDefUpdateDisk(virDomainDef *domdef, for (i = 0; i < snapdef->ndisks; i++) { g_autoptr(virStorageSource) newsrc = NULL; virDomainSnapshotDiskDef *snapdisk = &(snapdef->disks[i]); - virDomainDiskDef *defdisk = domdef->disks[i]; + virDomainDiskDef *defdisk = virDomainDiskByName(domdef, snapdisk->name, false); if (snapdisk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) continue; + if (!defdisk) + continue; + if (!(newsrc = virStorageSourceCopy(snapdisk->src, false))) return -1; -- 2.41.0
Extract creation of qcow2 files for external snapshots to separate function as we will need it for external snapshot revert code. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 67 +++++++++++++++++++++++++--------------- 1 file changed, 42 insertions(+), 25 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 8e1eb21b5d..227c201195 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -181,35 +181,21 @@ qemuSnapshotDomainDefUpdateDisk(virDomainDef *domdef, } -/* The domain is expected to be locked and inactive. */ static int -qemuSnapshotCreateInactiveInternal(virQEMUDriver *driver, - virDomainObj *vm, - virDomainMomentObj *snap) -{ - return qemuDomainSnapshotForEachQcow2(driver, vm->def, snap, "-c", false); -} - - -/* The domain is expected to be locked and inactive. */ -static int -qemuSnapshotCreateInactiveExternal(virQEMUDriver *driver, - virDomainObj *vm, - virDomainMomentObj *snap, - bool reuse) +qemuSnapshotCreateQcow2Files(virDomainObj *vm, + virDomainSnapshotDef *snapdef, + virBitmap *created, + bool reuse) { size_t i; - virDomainSnapshotDiskDef *snapdisk; - virDomainDiskDef *defdisk; const char *qemuImgPath; - g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); - int ret = -1; g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; - virDomainSnapshotDef *snapdef = virDomainSnapshotObjGetDef(snap); - g_autoptr(virBitmap) created = virBitmapNew(snapdef->ndisks); + virQEMUDriver *driver = ((qemuDomainObjPrivate *) vm->privateData)->driver; + virDomainSnapshotDiskDef *snapdisk = NULL; + virDomainDiskDef *defdisk = NULL; if (!(qemuImgPath = qemuFindQemuImgBinary(driver))) - goto cleanup; + return -1; /* If reuse is true, then qemuSnapshotPrepare already * ensured that the new files exist, and it was up to the user to @@ -218,6 +204,7 @@ qemuSnapshotCreateInactiveExternal(virQEMUDriver *driver, g_autoptr(virCommand) cmd = NULL; snapdisk = &(snapdef->disks[i]); defdisk = vm->def->disks[i]; + if (snapdisk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) continue; @@ -225,7 +212,7 @@ qemuSnapshotCreateInactiveExternal(virQEMUDriver *driver, snapdisk->src->format = VIR_STORAGE_FILE_QCOW2; if (qemuDomainStorageSourceValidateDepth(defdisk->src, 1, defdisk->dst) < 0) - goto cleanup; + return -1; /* creates cmd line args: qemu-img create -f qcow2 -o */ if (!(cmd = virCommandNewArgList(qemuImgPath, @@ -234,7 +221,7 @@ qemuSnapshotCreateInactiveExternal(virQEMUDriver *driver, virStorageFileFormatTypeToString(snapdisk->src->format), "-o", NULL))) - goto cleanup; + return -1; /* adds cmd line arg: backing_fmt=format,backing_file=/path/to/backing/file */ virBufferAsprintf(&buf, "backing_fmt=%s,backing_file=", @@ -251,9 +238,39 @@ qemuSnapshotCreateInactiveExternal(virQEMUDriver *driver, ignore_value(virBitmapSetBit(created, i)); if (virCommandRun(cmd, NULL) < 0) - goto cleanup; + return -1; } + return 0; +} + + +/* The domain is expected to be locked and inactive. */ +static int +qemuSnapshotCreateInactiveInternal(virQEMUDriver *driver, + virDomainObj *vm, + virDomainMomentObj *snap) +{ + return qemuDomainSnapshotForEachQcow2(driver, vm->def, snap, "-c", false); +} + + +/* The domain is expected to be locked and inactive. */ +static int +qemuSnapshotCreateInactiveExternal(virQEMUDriver *driver, + virDomainObj *vm, + virDomainMomentObj *snap, + bool reuse) +{ + virDomainSnapshotDiskDef *snapdisk; + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); + int ret = -1; + virDomainSnapshotDef *snapdef = virDomainSnapshotObjGetDef(snap); + g_autoptr(virBitmap) created = virBitmapNew(snapdef->ndisks); + + if (qemuSnapshotCreateQcow2Files(vm, snapdef, created, reuse) < 0) + goto cleanup; + /* update disk definitions */ if (qemuSnapshotDomainDefUpdateDisk(vm->def, snapdef, reuse) < 0) goto cleanup; -- 2.41.0
On Tue, Jun 27, 2023 at 17:07:11 +0200, Pavel Hrdina wrote:
Extract creation of qcow2 files for external snapshots to separate function as we will need it for external snapshot revert code.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 67 +++++++++++++++++++++++++--------------- 1 file changed, 42 insertions(+), 25 deletions(-)
diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 8e1eb21b5d..227c201195 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -181,35 +181,21 @@ qemuSnapshotDomainDefUpdateDisk(virDomainDef *domdef, }
-/* The domain is expected to be locked and inactive. */ static int -qemuSnapshotCreateInactiveInternal(virQEMUDriver *driver, - virDomainObj *vm, - virDomainMomentObj *snap) -{ - return qemuDomainSnapshotForEachQcow2(driver, vm->def, snap, "-c", false); -} - - -/* The domain is expected to be locked and inactive. */ -static int -qemuSnapshotCreateInactiveExternal(virQEMUDriver *driver, - virDomainObj *vm, - virDomainMomentObj *snap, - bool reuse)
Please document the arguments of the new helper.
+qemuSnapshotCreateQcow2Files(virDomainObj *vm, + virDomainSnapshotDef *snapdef, + virBitmap *created, + bool reuse)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
To create new overlay files when external snapshot revert support is introduced we will be using different domain definition than what is currently used by the domain. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 227c201195..069c4c8ba7 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -183,6 +183,7 @@ qemuSnapshotDomainDefUpdateDisk(virDomainDef *domdef, static int qemuSnapshotCreateQcow2Files(virDomainObj *vm, + virDomainDef *def, virDomainSnapshotDef *snapdef, virBitmap *created, bool reuse) @@ -194,6 +195,9 @@ qemuSnapshotCreateQcow2Files(virDomainObj *vm, virDomainSnapshotDiskDef *snapdisk = NULL; virDomainDiskDef *defdisk = NULL; + if (!def) + def = vm->def; + if (!(qemuImgPath = qemuFindQemuImgBinary(driver))) return -1; @@ -203,7 +207,7 @@ qemuSnapshotCreateQcow2Files(virDomainObj *vm, for (i = 0; i < snapdef->ndisks && !reuse; i++) { g_autoptr(virCommand) cmd = NULL; snapdisk = &(snapdef->disks[i]); - defdisk = vm->def->disks[i]; + defdisk = def->disks[i]; if (snapdisk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) continue; @@ -268,7 +272,7 @@ qemuSnapshotCreateInactiveExternal(virQEMUDriver *driver, virDomainSnapshotDef *snapdef = virDomainSnapshotObjGetDef(snap); g_autoptr(virBitmap) created = virBitmapNew(snapdef->ndisks); - if (qemuSnapshotCreateQcow2Files(vm, snapdef, created, reuse) < 0) + if (qemuSnapshotCreateQcow2Files(vm, NULL, snapdef, created, reuse) < 0) goto cleanup; /* update disk definitions */ -- 2.41.0
On Tue, Jun 27, 2023 at 17:07:12 +0200, Pavel Hrdina wrote:
To create new overlay files when external snapshot revert support is introduced we will be using different domain definition than what is currently used by the domain.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 227c201195..069c4c8ba7 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -183,6 +183,7 @@ qemuSnapshotDomainDefUpdateDisk(virDomainDef *domdef,
static int qemuSnapshotCreateQcow2Files(virDomainObj *vm, + virDomainDef *def,
At this point I'd strongly suggest removing 'vm' argument which is at this point used only to get the virQEMUDriver pointer from the private data. That way you will not have to explain in the comment I've requested in the previous review why both 'vm' and 'def' are needed and why 'def' isn't picked from 'vm'.
virDomainSnapshotDef *snapdef, virBitmap *created, bool reuse) @@ -194,6 +195,9 @@ qemuSnapshotCreateQcow2Files(virDomainObj *vm, virDomainSnapshotDiskDef *snapdisk = NULL; virDomainDiskDef *defdisk = NULL;
+ if (!def) + def = vm->def;
And drop this.
+ if (!(qemuImgPath = qemuFindQemuImgBinary(driver))) return -1;
@@ -203,7 +207,7 @@ qemuSnapshotCreateQcow2Files(virDomainObj *vm, for (i = 0; i < snapdef->ndisks && !reuse; i++) { g_autoptr(virCommand) cmd = NULL; snapdisk = &(snapdef->disks[i]); - defdisk = vm->def->disks[i]; + defdisk = def->disks[i];
if (snapdisk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) continue; @@ -268,7 +272,7 @@ qemuSnapshotCreateInactiveExternal(virQEMUDriver *driver, virDomainSnapshotDef *snapdef = virDomainSnapshotObjGetDef(snap); g_autoptr(virBitmap) created = virBitmapNew(snapdef->ndisks);
- if (qemuSnapshotCreateQcow2Files(vm, snapdef, created, reuse) < 0) + if (qemuSnapshotCreateQcow2Files(vm, NULL, snapdef, created, reuse) < 0)a
and adjust this to pass the vm def explicitly. With adjustments: Reviewed-by: Peter Krempa <pkrempa@redhat.com>
We will need to reuse the functionality when reverting external snapshots. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 38 +++++++++++++++++++------------------- 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 069c4c8ba7..5d2ffdeee6 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -520,6 +520,25 @@ qemuSnapshotPrepareDiskExternal(virDomainDiskDef *disk, bool active, bool reuse) { + if (!snapdisk->src->format) { + snapdisk->src->format = VIR_STORAGE_FILE_QCOW2; + } else if (snapdisk->src->format != VIR_STORAGE_FILE_QCOW2 && + snapdisk->src->format != VIR_STORAGE_FILE_QED) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("external snapshot format for disk %1$s is unsupported: %2$s"), + snapdisk->name, + virStorageFileFormatTypeToString(snapdisk->src->format)); + return -1; + } + + if (snapdisk->src->metadataCacheMaxSize > 0) { + if (snapdisk->src->format != VIR_STORAGE_FILE_QCOW2) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("metadata cache max size control is supported only with qcow2 images")); + return -1; + } + } + if (qemuTranslateSnapshotDiskSourcePool(snapdisk) < 0) return -1; @@ -700,25 +719,6 @@ qemuSnapshotPrepare(virDomainObj *vm, break; case VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL: - if (!disk->src->format) { - disk->src->format = VIR_STORAGE_FILE_QCOW2; - } else if (disk->src->format != VIR_STORAGE_FILE_QCOW2 && - disk->src->format != VIR_STORAGE_FILE_QED) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("external snapshot format for disk %1$s is unsupported: %2$s"), - disk->name, - virStorageFileFormatTypeToString(disk->src->format)); - return -1; - } - - if (disk->src->metadataCacheMaxSize > 0) { - if (disk->src->format != VIR_STORAGE_FILE_QCOW2) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("metadata cache max size control is supported only with qcow2 images")); - return -1; - } - } - if (qemuSnapshotPrepareDiskExternal(dom_disk, disk, active, reuse) < 0) return -1; -- 2.41.0
On Tue, Jun 27, 2023 at 17:07:13 +0200, Pavel Hrdina wrote:
We will need to reuse the functionality when reverting external snapshots.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 38 +++++++++++++++++++------------------- 1 file changed, 19 insertions(+), 19 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 5d2ffdeee6..1cb0ea55de 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -2001,7 +2001,7 @@ qemuSnapshotRevertActive(virDomainObj *vm, /* Transitions 5, 6, 8, 9 */ qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT, - VIR_ASYNC_JOB_START, 0); + VIR_ASYNC_JOB_SNAPSHOT, 0); virDomainAuditStop(vm, "from-snapshot"); detail = VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT; event = virDomainEventLifecycleNewFromObj(vm, @@ -2026,7 +2026,7 @@ qemuSnapshotRevertActive(virDomainObj *vm, rc = qemuProcessStart(snapshot->domain->conn, driver, vm, cookie ? cookie->cpu : NULL, - VIR_ASYNC_JOB_START, NULL, -1, NULL, snap, + VIR_ASYNC_JOB_SNAPSHOT, NULL, -1, NULL, snap, VIR_NETDEV_VPORT_PROFILE_OP_CREATE, start_flags); virDomainAuditStart(vm, "from-snapshot", rc >= 0); @@ -2059,7 +2059,7 @@ qemuSnapshotRevertActive(virDomainObj *vm, } rc = qemuProcessStartCPUs(driver, vm, VIR_DOMAIN_RUNNING_FROM_SNAPSHOT, - VIR_ASYNC_JOB_START); + VIR_ASYNC_JOB_SNAPSHOT); if (rc < 0) return -1; } @@ -2122,7 +2122,7 @@ qemuSnapshotRevertInactive(virDomainObj *vm, if (virDomainObjIsActive(vm)) { /* Transitions 4, 7 */ qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT, - VIR_ASYNC_JOB_START, 0); + VIR_ASYNC_JOB_SNAPSHOT, 0); virDomainAuditStop(vm, "from-snapshot"); detail = VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT; event = virDomainEventLifecycleNewFromObj(vm, @@ -2149,7 +2149,7 @@ qemuSnapshotRevertInactive(virDomainObj *vm, start_flags |= paused ? VIR_QEMU_PROCESS_START_PAUSED : 0; rc = qemuProcessStart(snapshot->domain->conn, driver, vm, NULL, - VIR_ASYNC_JOB_START, NULL, -1, NULL, NULL, + VIR_ASYNC_JOB_SNAPSHOT, NULL, -1, NULL, NULL, VIR_NETDEV_VPORT_PROFILE_OP_CREATE, start_flags); virDomainAuditStart(vm, "from-snapshot", rc >= 0); @@ -2216,10 +2216,11 @@ qemuSnapshotRevert(virDomainObj *vm, return -1; } - if (qemuProcessBeginJob(vm, - VIR_DOMAIN_JOB_OPERATION_SNAPSHOT_REVERT, - flags) < 0) + if (virDomainObjBeginAsyncJob(vm, VIR_ASYNC_JOB_SNAPSHOT, + VIR_DOMAIN_JOB_OPERATION_SNAPSHOT_REVERT, + flags) < 0) { return -1; + } if (!(snap = qemuSnapObjFromSnapshot(vm, snapshot))) goto endjob; @@ -2269,7 +2270,7 @@ qemuSnapshotRevert(virDomainObj *vm, } endjob: - qemuProcessEndJob(vm); + virDomainObjEndAsyncJob(vm); return ret; } -- 2.41.0
On Tue, Jun 27, 2023 at 17:07:14 +0200, Pavel Hrdina wrote: So what is this actually doing?
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-)
diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 5d2ffdeee6..1cb0ea55de 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -2001,7 +2001,7 @@ qemuSnapshotRevertActive(virDomainObj *vm, /* Transitions 5, 6, 8, 9 */ qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT, - VIR_ASYNC_JOB_START, 0); + VIR_ASYNC_JOB_SNAPSHOT, 0);
It looks like we are declaring that an async job is used here ...
virDomainAuditStop(vm, "from-snapshot"); detail = VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT; event = virDomainEventLifecycleNewFromObj(vm, @@ -2026,7 +2026,7 @@ qemuSnapshotRevertActive(virDomainObj *vm,
rc = qemuProcessStart(snapshot->domain->conn, driver, vm, cookie ? cookie->cpu : NULL, - VIR_ASYNC_JOB_START, NULL, -1, NULL, snap, + VIR_ASYNC_JOB_SNAPSHOT, NULL, -1, NULL, snap, VIR_NETDEV_VPORT_PROFILE_OP_CREATE, start_flags); virDomainAuditStart(vm, "from-snapshot", rc >= 0); @@ -2059,7 +2059,7 @@ qemuSnapshotRevertActive(virDomainObj *vm, } rc = qemuProcessStartCPUs(driver, vm, VIR_DOMAIN_RUNNING_FROM_SNAPSHOT, - VIR_ASYNC_JOB_START); + VIR_ASYNC_JOB_SNAPSHOT); if (rc < 0) return -1; } @@ -2122,7 +2122,7 @@ qemuSnapshotRevertInactive(virDomainObj *vm, if (virDomainObjIsActive(vm)) { /* Transitions 4, 7 */ qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT, - VIR_ASYNC_JOB_START, 0); + VIR_ASYNC_JOB_SNAPSHOT, 0); virDomainAuditStop(vm, "from-snapshot"); detail = VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT; event = virDomainEventLifecycleNewFromObj(vm, @@ -2149,7 +2149,7 @@ qemuSnapshotRevertInactive(virDomainObj *vm, start_flags |= paused ? VIR_QEMU_PROCESS_START_PAUSED : 0;
rc = qemuProcessStart(snapshot->domain->conn, driver, vm, NULL, - VIR_ASYNC_JOB_START, NULL, -1, NULL, NULL, + VIR_ASYNC_JOB_SNAPSHOT, NULL, -1, NULL, NULL, VIR_NETDEV_VPORT_PROFILE_OP_CREATE, start_flags); virDomainAuditStart(vm, "from-snapshot", rc >= 0); @@ -2216,10 +2216,11 @@ qemuSnapshotRevert(virDomainObj *vm, return -1; }
- if (qemuProcessBeginJob(vm, - VIR_DOMAIN_JOB_OPERATION_SNAPSHOT_REVERT, - flags) < 0)
... but it was never initiated as async. It is unclear if you are fixing a bug, or doing a change for consistency or what is happening here actually.
On Tue, Jul 25, 2023 at 03:54:54PM +0200, Peter Krempa wrote:
On Tue, Jun 27, 2023 at 17:07:14 +0200, Pavel Hrdina wrote:
So what is this actually doing?
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-)
diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 5d2ffdeee6..1cb0ea55de 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -2001,7 +2001,7 @@ qemuSnapshotRevertActive(virDomainObj *vm, /* Transitions 5, 6, 8, 9 */ qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT, - VIR_ASYNC_JOB_START, 0); + VIR_ASYNC_JOB_SNAPSHOT, 0);
It looks like we are declaring that an async job is used here ...
virDomainAuditStop(vm, "from-snapshot"); detail = VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT; event = virDomainEventLifecycleNewFromObj(vm, @@ -2026,7 +2026,7 @@ qemuSnapshotRevertActive(virDomainObj *vm,
rc = qemuProcessStart(snapshot->domain->conn, driver, vm, cookie ? cookie->cpu : NULL, - VIR_ASYNC_JOB_START, NULL, -1, NULL, snap, + VIR_ASYNC_JOB_SNAPSHOT, NULL, -1, NULL, snap, VIR_NETDEV_VPORT_PROFILE_OP_CREATE, start_flags); virDomainAuditStart(vm, "from-snapshot", rc >= 0); @@ -2059,7 +2059,7 @@ qemuSnapshotRevertActive(virDomainObj *vm, } rc = qemuProcessStartCPUs(driver, vm, VIR_DOMAIN_RUNNING_FROM_SNAPSHOT, - VIR_ASYNC_JOB_START); + VIR_ASYNC_JOB_SNAPSHOT); if (rc < 0) return -1; } @@ -2122,7 +2122,7 @@ qemuSnapshotRevertInactive(virDomainObj *vm, if (virDomainObjIsActive(vm)) { /* Transitions 4, 7 */ qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT, - VIR_ASYNC_JOB_START, 0); + VIR_ASYNC_JOB_SNAPSHOT, 0); virDomainAuditStop(vm, "from-snapshot"); detail = VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT; event = virDomainEventLifecycleNewFromObj(vm, @@ -2149,7 +2149,7 @@ qemuSnapshotRevertInactive(virDomainObj *vm, start_flags |= paused ? VIR_QEMU_PROCESS_START_PAUSED : 0;
rc = qemuProcessStart(snapshot->domain->conn, driver, vm, NULL, - VIR_ASYNC_JOB_START, NULL, -1, NULL, NULL, + VIR_ASYNC_JOB_SNAPSHOT, NULL, -1, NULL, NULL, VIR_NETDEV_VPORT_PROFILE_OP_CREATE, start_flags); virDomainAuditStart(vm, "from-snapshot", rc >= 0); @@ -2216,10 +2216,11 @@ qemuSnapshotRevert(virDomainObj *vm, return -1; }
- if (qemuProcessBeginJob(vm, - VIR_DOMAIN_JOB_OPERATION_SNAPSHOT_REVERT, - flags) < 0)
... but it was never initiated as async.
It is unclear if you are fixing a bug, or doing a change for consistency or what is happening here actually.
qemuProcessBeginJob is just a wrapper for: if (virDomainObjBeginAsyncJob(vm, VIR_ASYNC_JOB_START, operation, apiFlags) < 0) return -1; qemuDomainObjSetAsyncJobMask(vm, VIR_JOB_NONE); return 0; so we actually did start the job as async. Yeah missing details in commit message bites me now as I don't remember if this was only cosmetic change as CREATE and DELETE both use VIR_ASYNC_JOB_SNAPSHOT instead of VIR_ASYNC_JOB_START or if there was any other reason for this patch. The only change this introduces is that now when revert job is running query jobs are allowed as previously no other jobs were allowed. Pavel
When reverting to external snapshot we need to create new overlay qcow2 files from the disk files the VM had when the snapshot was taken. There are some specifics and limitations when reverting to a snapshot: 1) When reverting to last snapshot we need to first create new overlay files before we can safely delete the old overlay files in case the creation fails so we have still recovery option when we error out. These new files will not have the suffix as when the snapshot was created as renaming the original files in order to use the same file names as when the snapshot was created would add unnecessary complexity to the code. 2) When reverting to any snapshot we will always create overlay files for every disk the VM had when the snapshot was done. Otherwise we would have to figure out if there is any other qcow2 image already using any of the VM disks as backing store and that itself might be extremely complex and in some cases impossible. 3) When reverting from any state the current overlay files will be always removed as that VM state is not meant to be saved. It's the same as with internal snapshots. If user want's to keep the current state before reverting they need to create a new snapshot. For now this will only work if the current snapshot is the last. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 232 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 228 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 1cb0ea55de..dbf2cdd5db 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -18,6 +18,8 @@ #include <config.h> +#include <fcntl.h> + #include "qemu_snapshot.h" #include "qemu_monitor.h" @@ -1975,6 +1977,183 @@ qemuSnapshotRevertWriteMetadata(virDomainObj *vm, } +static int +qemuSnapshotRevertExternalPrepare(virDomainObj *vm, + virDomainSnapshotDef *tmpsnapdef, + virDomainMomentObj *snap, + virDomainDef *config, + virDomainDef *inactiveConfig, + int *memsnapFD, + char **memsnapPath) +{ + ssize_t i; + bool active = virDomainObjIsActive(vm); + virDomainDef *domdef = NULL; + virDomainSnapshotLocation location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL; + virDomainSnapshotDef *snapdef = virDomainSnapshotObjGetDef(snap); + + if (config) { + domdef = config; + } else { + domdef = inactiveConfig; + } + + /* We need this to generate creation timestamp that is used as default + * snapshot name. */ + if (virDomainMomentDefPostParse(&tmpsnapdef->parent) < 0) + return -1; + + if (virDomainSnapshotAlignDisks(tmpsnapdef, domdef, location, false, true) < 0) + return -1; + + for (i = 0; i < tmpsnapdef->ndisks; i++) { + virDomainSnapshotDiskDef *snapdisk = &tmpsnapdef->disks[i]; + virDomainDiskDef *domdisk = domdef->disks[i]; + + if (qemuSnapshotPrepareDiskExternal(domdisk, snapdisk, active, false) < 0) + return -1; + } + + if (memsnapFD && memsnapPath && snapdef->memorysnapshotfile) { + virQEMUDriver *driver = ((qemuDomainObjPrivate *) vm->privateData)->driver; + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); + + *memsnapPath = snapdef->memorysnapshotfile; + *memsnapFD = qemuDomainOpenFile(cfg, NULL, *memsnapPath, O_RDONLY, NULL); + } + + return 0; +} + + +static int +qemuSnapshotRevertExternalActive(virDomainObj *vm, + virDomainSnapshotDef *tmpsnapdef) +{ + ssize_t i; + g_autoptr(GHashTable) blockNamedNodeData = NULL; + g_autoptr(qemuSnapshotDiskContext) snapctxt = NULL; + + snapctxt = qemuSnapshotDiskContextNew(tmpsnapdef->ndisks, vm, VIR_ASYNC_JOB_SNAPSHOT); + + if (!(blockNamedNodeData = qemuBlockGetNamedNodeData(vm, VIR_ASYNC_JOB_SNAPSHOT))) + return -1; + + for (i = 0; i < tmpsnapdef->ndisks; i++) { + if (qemuSnapshotDiskPrepareOne(snapctxt, + vm->def->disks[i], + tmpsnapdef->disks + i, + blockNamedNodeData, + false, + true) < 0) + return -1; + } + + if (qemuSnapshotDiskCreate(snapctxt) < 0) + return -1; + + return 0; +} + + +static int +qemuSnapshotRevertExternalInactive(virDomainObj *vm, + virDomainSnapshotDef *tmpsnapdef, + virDomainDef *config, + virDomainDef *inactiveConfig) +{ + virDomainDef *domdef = NULL; + g_autoptr(virBitmap) created = NULL; + + created = virBitmapNew(tmpsnapdef->ndisks); + + if (config) { + domdef = config; + } else { + domdef = inactiveConfig; + } + + if (config) { + if (qemuSnapshotDomainDefUpdateDisk(config, tmpsnapdef, false) < 0) + return -1; + } + + if (qemuSnapshotDomainDefUpdateDisk(inactiveConfig, tmpsnapdef, false) < 0) + return -1; + + if (qemuSnapshotCreateQcow2Files(vm, domdef, tmpsnapdef, created, false) < 0) { + ssize_t bit = -1; + + while ((bit = virBitmapNextSetBit(created, bit)) >= 0) { + virDomainSnapshotDiskDef *snapdisk = &(tmpsnapdef->disks[bit]); + + if (virStorageSourceInit(snapdisk->src) < 0 || + virStorageSourceUnlink(snapdisk->src) < 0) { + VIR_WARN("Failed to remove snapshot image '%s'", + snapdisk->src->path); + } + } + + return -1; + } + + return 0; +} + + +static void +qemuSnapshotRevertExternalFinish(virDomainObj *vm, + virDomainSnapshotDef *tmpsnapdef, + virDomainMomentObj *snap) +{ + size_t i; + virDomainMomentObj *curSnap = virDomainSnapshotGetCurrent(vm->snapshots); + virDomainSnapshotDef *curdef = virDomainSnapshotObjGetDef(curSnap); + virDomainSnapshotDef *snapdef = virDomainSnapshotObjGetDef(snap); + + if (curdef->revertdisks) { + for (i = 0; i < curdef->nrevertdisks; i++) { + virDomainSnapshotDiskDef *snapdisk = &(curdef->revertdisks[i]); + + if (virStorageSourceInit(snapdisk->src) < 0 || + virStorageSourceUnlink(snapdisk->src) < 0) { + VIR_WARN("Failed to remove snapshot image '%s'", + snapdisk->src->path); + } + + virDomainSnapshotDiskDefClear(snapdisk); + } + + g_clear_pointer(&curdef->revertdisks, g_free); + curdef->nrevertdisks = 0; + } else { + for (i = 0; i < curdef->ndisks; i++) { + virDomainSnapshotDiskDef *snapdisk = &(curdef->disks[i]); + + if (virStorageSourceInit(snapdisk->src) < 0 || + virStorageSourceUnlink(snapdisk->src) < 0) { + VIR_WARN("Failed to remove snapshot image '%s'", + snapdisk->src->path); + } + } + } + + if (snap->nchildren != 0) { + snapdef->revertdisks = g_steal_pointer(&tmpsnapdef->disks); + snapdef->nrevertdisks = tmpsnapdef->ndisks; + tmpsnapdef->ndisks = 0; + } else { + for (i = 0; i < snapdef->ndisks; i++) { + virDomainSnapshotDiskDefClear(&snapdef->disks[i]); + } + g_free(snapdef->disks); + snapdef->disks = g_steal_pointer(&tmpsnapdef->disks); + snapdef->ndisks = tmpsnapdef->ndisks; + tmpsnapdef->ndisks = 0; + } +} + + static int qemuSnapshotRevertActive(virDomainObj *vm, virDomainSnapshotPtr snapshot, @@ -1989,10 +2168,14 @@ qemuSnapshotRevertActive(virDomainObj *vm, { virObjectEvent *event = NULL; virObjectEvent *event2 = NULL; + virDomainMomentObj *loadSnap = NULL; + VIR_AUTOCLOSE memsnapFD = -1; + char *memsnapPath = NULL; int detail; bool defined = false; qemuDomainSaveCookie *cookie = (qemuDomainSaveCookie *) snapdef->cookie; int rc; + g_autoptr(virDomainSnapshotDef) tmpsnapdef = NULL; start_flags |= VIR_QEMU_PROCESS_START_PAUSED; @@ -2010,6 +2193,19 @@ qemuSnapshotRevertActive(virDomainObj *vm, virObjectEventStateQueue(driver->domainEventState, event); } + if (virDomainSnapshotIsExternal(snap)) { + if (!(tmpsnapdef = virDomainSnapshotDefNew())) + return -1; + + if (qemuSnapshotRevertExternalPrepare(vm, tmpsnapdef, snap, + *config, *inactiveConfig, + &memsnapFD, &memsnapPath) < 0) { + return -1; + } + } else { + loadSnap = snap; + } + if (*inactiveConfig) { virDomainObjAssignDef(vm, inactiveConfig, false, NULL); defined = true; @@ -2026,7 +2222,8 @@ qemuSnapshotRevertActive(virDomainObj *vm, rc = qemuProcessStart(snapshot->domain->conn, driver, vm, cookie ? cookie->cpu : NULL, - VIR_ASYNC_JOB_SNAPSHOT, NULL, -1, NULL, snap, + VIR_ASYNC_JOB_SNAPSHOT, NULL, memsnapFD, + memsnapPath, loadSnap, VIR_NETDEV_VPORT_PROFILE_OP_CREATE, start_flags); virDomainAuditStart(vm, "from-snapshot", rc >= 0); @@ -2038,6 +2235,14 @@ qemuSnapshotRevertActive(virDomainObj *vm, if (rc < 0) return -1; + + if (virDomainSnapshotIsExternal(snap)) { + if (qemuSnapshotRevertExternalActive(vm, tmpsnapdef) < 0) + return -1; + + qemuSnapshotRevertExternalFinish(vm, tmpsnapdef, snap); + } + /* Touch up domain state. */ if (!(flags & VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING) && (snapdef->state == VIR_DOMAIN_SNAPSHOT_PAUSED || @@ -2112,6 +2317,7 @@ qemuSnapshotRevertInactive(virDomainObj *vm, int detail; bool defined = false; int rc; + g_autoptr(virDomainSnapshotDef) tmpsnapdef = NULL; /* Transitions 1, 4, 7 */ /* Newer qemu -loadvm refuses to revert to the state of a snapshot @@ -2131,9 +2337,27 @@ qemuSnapshotRevertInactive(virDomainObj *vm, virObjectEventStateQueue(driver->domainEventState, event); } - if (qemuSnapshotInternalRevertInactive(driver, vm, snap) < 0) { - qemuDomainRemoveInactive(driver, vm, 0, false); - return -1; + if (virDomainSnapshotIsExternal(snap)) { + if (!(tmpsnapdef = virDomainSnapshotDefNew())) + return -1; + + if (qemuSnapshotRevertExternalPrepare(vm, tmpsnapdef, snap, + NULL, *inactiveConfig, + NULL, NULL) < 0) { + return -1; + } + + if (qemuSnapshotRevertExternalInactive(vm, tmpsnapdef, + NULL, *inactiveConfig) < 0) { + return -1; + } + + qemuSnapshotRevertExternalFinish(vm, tmpsnapdef, snap); + } else { + if (qemuSnapshotInternalRevertInactive(driver, vm, snap) < 0) { + qemuDomainRemoveInactive(driver, vm, 0, false); + return -1; + } } if (*inactiveConfig) { -- 2.41.0
On Tue, Jun 27, 2023 at 17:07:15 +0200, Pavel Hrdina wrote:
When reverting to external snapshot we need to create new overlay qcow2 files from the disk files the VM had when the snapshot was taken.
There are some specifics and limitations when reverting to a snapshot:
1) When reverting to last snapshot we need to first create new overlay files before we can safely delete the old overlay files in case the creation fails so we have still recovery option when we error out.
These new files will not have the suffix as when the snapshot was created as renaming the original files in order to use the same file names as when the snapshot was created would add unnecessary complexity to the code.
2) When reverting to any snapshot we will always create overlay files for every disk the VM had when the snapshot was done. Otherwise we would have to figure out if there is any other qcow2 image already using any of the VM disks as backing store and that itself might be extremely complex and in some cases impossible.
3) When reverting from any state the current overlay files will be always removed as that VM state is not meant to be saved. It's the same as with internal snapshots. If user want's to keep the current state before reverting they need to create a new snapshot. For now this will only work if the current snapshot is the last.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 232 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 228 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 1cb0ea55de..dbf2cdd5db 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -18,6 +18,8 @@
#include <config.h>
+#include <fcntl.h> + #include "qemu_snapshot.h"
#include "qemu_monitor.h" @@ -1975,6 +1977,183 @@ qemuSnapshotRevertWriteMetadata(virDomainObj *vm, }
Please document all new functions both any non-obvious parameter and what it is supposed to do.
+static int +qemuSnapshotRevertExternalPrepare(virDomainObj *vm, + virDomainSnapshotDef *tmpsnapdef, + virDomainMomentObj *snap, + virDomainDef *config, + virDomainDef *inactiveConfig, + int *memsnapFD, + char **memsnapPath) +{ + ssize_t i;
Normally we declare 'i' as unsigned.
+ bool active = virDomainObjIsActive(vm); + virDomainDef *domdef = NULL; + virDomainSnapshotLocation location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL;
This constant is used in one place only.
+ virDomainSnapshotDef *snapdef = virDomainSnapshotObjGetDef(snap); + + if (config) { + domdef = config; + } else { + domdef = inactiveConfig; + } + + /* We need this to generate creation timestamp that is used as default + * snapshot name. */ + if (virDomainMomentDefPostParse(&tmpsnapdef->parent) < 0) + return -1; + + if (virDomainSnapshotAlignDisks(tmpsnapdef, domdef, location, false, true) < 0) + return -1; + + for (i = 0; i < tmpsnapdef->ndisks; i++) { + virDomainSnapshotDiskDef *snapdisk = &tmpsnapdef->disks[i]; + virDomainDiskDef *domdisk = domdef->disks[i]; + + if (qemuSnapshotPrepareDiskExternal(domdisk, snapdisk, active, false) < 0) + return -1; + } + + if (memsnapFD && memsnapPath && snapdef->memorysnapshotfile) { + virQEMUDriver *driver = ((qemuDomainObjPrivate *) vm->privateData)->driver; + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); + + *memsnapPath = snapdef->memorysnapshotfile; + *memsnapFD = qemuDomainOpenFile(cfg, NULL, *memsnapPath, O_RDONLY, NULL);
Since the caller inherits the FD this must be documented.
+ } + + return 0; +} + + +static int +qemuSnapshotRevertExternalActive(virDomainObj *vm, + virDomainSnapshotDef *tmpsnapdef) +{ + ssize_t i;
'size_t'
+ g_autoptr(GHashTable) blockNamedNodeData = NULL; + g_autoptr(qemuSnapshotDiskContext) snapctxt = NULL; + + snapctxt = qemuSnapshotDiskContextNew(tmpsnapdef->ndisks, vm, VIR_ASYNC_JOB_SNAPSHOT); + + if (!(blockNamedNodeData = qemuBlockGetNamedNodeData(vm, VIR_ASYNC_JOB_SNAPSHOT))) + return -1; + + for (i = 0; i < tmpsnapdef->ndisks; i++) { + if (qemuSnapshotDiskPrepareOne(snapctxt, + vm->def->disks[i], + tmpsnapdef->disks + i, + blockNamedNodeData, + false, + true) < 0) + return -1; + } + + if (qemuSnapshotDiskCreate(snapctxt) < 0) + return -1; + + return 0; +} + + +static int +qemuSnapshotRevertExternalInactive(virDomainObj *vm, + virDomainSnapshotDef *tmpsnapdef, + virDomainDef *config, + virDomainDef *inactiveConfig)
This function is named '..Inactive'
+{ + virDomainDef *domdef = NULL; + g_autoptr(virBitmap) created = NULL; + + created = virBitmapNew(tmpsnapdef->ndisks); + + if (config) { + domdef = config; + } else { + domdef = inactiveConfig; + }
So this logic is weird. Inactive VMs have only one definition.
+ + if (config) { + if (qemuSnapshotDomainDefUpdateDisk(config, tmpsnapdef, false) < 0) + return -1; + } + + if (qemuSnapshotDomainDefUpdateDisk(inactiveConfig, tmpsnapdef, false) < 0) + return -1; + + if (qemuSnapshotCreateQcow2Files(vm, domdef, tmpsnapdef, created, false) < 0) { + ssize_t bit = -1;
Consider storing the error here ...
+ + while ((bit = virBitmapNextSetBit(created, bit)) >= 0) { + virDomainSnapshotDiskDef *snapdisk = &(tmpsnapdef->disks[bit]); + + if (virStorageSourceInit(snapdisk->src) < 0 || + virStorageSourceUnlink(snapdisk->src) < 0) {
... so that this doesn't overwrite it.
+ VIR_WARN("Failed to remove snapshot image '%s'", + snapdisk->src->path); + } + } + + return -1; + } + + return 0; +}
The rest seems to make sense: Reviewed-by: Peter Krempa <pkrempa@redhat.com>
The new name reflects that we prepare data for external snapshot deletion and the old name will be used later for different part of code. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> Reviewed-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_snapshot.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index dbf2cdd5db..c5de24917b 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -2570,9 +2570,9 @@ qemuSnapshotFindParentSnapForDisk(virDomainMomentObj *snap, static int -qemuSnapshotDeleteExternalPrepare(virDomainObj *vm, - virDomainMomentObj *snap, - GSList **externalData) +qemuSnapshotDeleteExternalPrepareData(virDomainObj *vm, + virDomainMomentObj *snap, + GSList **externalData) { ssize_t i; virDomainSnapshotDef *snapdef = virDomainSnapshotObjGetDef(snap); @@ -3429,7 +3429,7 @@ qemuSnapshotDelete(virDomainObj *vm, g_autoslist(qemuSnapshotDeleteExternalData) tmpData = NULL; /* this also serves as validation whether the snapshot can be deleted */ - if (qemuSnapshotDeleteExternalPrepare(vm, snap, &tmpData) < 0) + if (qemuSnapshotDeleteExternalPrepareData(vm, snap, &tmpData) < 0) goto endjob; if (!virDomainObjIsActive(vm)) { @@ -3444,7 +3444,7 @@ qemuSnapshotDelete(virDomainObj *vm, /* Call the prepare again as some data require that the VM is * running to get everything we need. */ - if (qemuSnapshotDeleteExternalPrepare(vm, snap, &externalData) < 0) + if (qemuSnapshotDeleteExternalPrepareData(vm, snap, &externalData) < 0) goto endjob; } else { qemuDomainJobPrivate *jobPriv = vm->job->privateData; -- 2.41.0
This part of code is about to grow to make deletion work when user reverts to non-leaf snapshot. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> Reviewed-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_snapshot.c | 89 +++++++++++++++++++++++++--------------- 1 file changed, 55 insertions(+), 34 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index c5de24917b..08cff2a9a2 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -2653,6 +2653,58 @@ qemuSnapshotDeleteExternalPrepareData(virDomainObj *vm, } +static int +qemuSnapshotDeleteExternalPrepare(virDomainObj *vm, + virDomainMomentObj *snap, + unsigned int flags, + GSList **externalData, + bool *stop_qemu) +{ + virQEMUDriver *driver = ((qemuDomainObjPrivate *) vm->privateData)->driver; + g_autoslist(qemuSnapshotDeleteExternalData) tmpData = NULL; + + if (!virDomainSnapshotIsExternal(snap)) + return 0; + + if (flags & (VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN | + VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY)) { + return 0; + } + + /* this also serves as validation whether the snapshot can be deleted */ + if (qemuSnapshotDeleteExternalPrepareData(vm, snap, &tmpData) < 0) + return -1; + + 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) { + return -1; + } + + *stop_qemu = true; + + /* Call the prepare again as some data require that the VM is + * running to get everything we need. */ + if (qemuSnapshotDeleteExternalPrepareData(vm, snap, externalData) < 0) + return -1; + } else { + qemuDomainJobPrivate *jobPriv = vm->job->privateData; + + *externalData = g_steal_pointer(&tmpData); + + /* If the VM is running we need to indicate that the async snapshot + * job is snapshot delete job. */ + jobPriv->snapshotDelete = true; + + qemuDomainSaveStatus(vm); + } + + return 0; +} + + typedef struct _virQEMUMomentReparent virQEMUMomentReparent; struct _virQEMUMomentReparent { const char *dir; @@ -3423,40 +3475,9 @@ qemuSnapshotDelete(virDomainObj *vm, if (qemuSnapshotDeleteValidate(vm, snap, flags) < 0) goto endjob; - if (virDomainSnapshotIsExternal(snap) && - !(flags & (VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN | - VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY))) { - g_autoslist(qemuSnapshotDeleteExternalData) tmpData = NULL; - - /* this also serves as validation whether the snapshot can be deleted */ - if (qemuSnapshotDeleteExternalPrepareData(vm, snap, &tmpData) < 0) - goto endjob; - - 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. */ - if (qemuSnapshotDeleteExternalPrepareData(vm, snap, &externalData) < 0) - goto endjob; - } else { - qemuDomainJobPrivate *jobPriv = vm->job->privateData; - - externalData = g_steal_pointer(&tmpData); - - /* If the VM is running we need to indicate that the async snapshot - * job is snapshot delete job. */ - jobPriv->snapshotDelete = true; - - qemuDomainSaveStatus(vm); - } + if (qemuSnapshotDeleteExternalPrepare(vm, snap, flags, + &externalData, &stop_qemu) < 0) { + goto endjob; } } -- 2.41.0
On Tue, Jun 27, 2023 at 17:07:17 +0200, Pavel Hrdina wrote:
This part of code is about to grow to make deletion work when user reverts to non-leaf snapshot.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> Reviewed-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_snapshot.c | 89 +++++++++++++++++++++++++--------------- 1 file changed, 55 insertions(+), 34 deletions(-)
Please document the new function.
Before external snapshot revert every delete operation did block commit in order to delete a snapshot. But now when user reverts to non-leaf snapshot deleting leaf snapshot will not have any overlay files so we can just simply delete the snapshot images. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 93 ++++++++++++++++++++++++++-------------- 1 file changed, 60 insertions(+), 33 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 08cff2a9a2..9c4d26bad5 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -2569,9 +2569,26 @@ qemuSnapshotFindParentSnapForDisk(virDomainMomentObj *snap, } +/** + * qemuSnapshotDeleteExternalPrepareData: + * @vm: domain object + * @snap: snapshot object + * @merge: whether we are just deleting image or not + * @externalData: prepared data to delete external snapshot + * + * Validate if we can delete selected snapshot @snap and prepare all necessary + * data that will be used when deleting snapshot as @externalData. + * + * If @merge is set to true we will merge the deleted snapshot into parent one + * instead of just deleting it. This is necessary when operating on snapshot + * that has existing overlay files. + * + * Returns -1 on error, 0 on success. + */ static int qemuSnapshotDeleteExternalPrepareData(virDomainObj *vm, virDomainMomentObj *snap, + bool merge, GSList **externalData) { ssize_t i; @@ -2579,7 +2596,6 @@ qemuSnapshotDeleteExternalPrepareData(virDomainObj *vm, 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_EXTERNAL) @@ -2591,14 +2607,18 @@ qemuSnapshotDeleteExternalPrepareData(virDomainObj *vm, snapDisk->name); return -1; } + } + + for (i = 0; i < snapdef->ndisks; i++) { + virDomainSnapshotDiskDef *snapDisk = &(snapdef->disks[i]); + g_autofree qemuSnapshotDeleteExternalData *data = NULL; + + if (snapDisk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) + continue; data = g_new0(qemuSnapshotDeleteExternalData, 1); data->snapDisk = snapDisk; - data->domDisk = qemuDomainDiskByName(vm->def, snapDisk->name); - if (!data->domDisk) - return -1; - data->parentDomDisk = virDomainDiskByTarget(snapdef->parent.dom, data->snapDisk->name); if (!data->parentDomDisk) { @@ -2608,39 +2628,46 @@ qemuSnapshotDeleteExternalPrepareData(virDomainObj *vm, return -1; } - if (virDomainObjIsActive(vm)) { - data->diskSrc = virStorageSourceChainLookupBySource(data->domDisk->src, - data->snapDisk->src, - &data->prevDiskSrc); - if (!data->diskSrc) + if (merge) { + data->domDisk = qemuDomainDiskByName(vm->def, snapDisk->name); + if (!data->domDisk) return -1; - 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 -1; - } + if (virDomainObjIsActive(vm)) { + data->diskSrc = virStorageSourceChainLookupBySource(data->domDisk->src, + data->snapDisk->src, + &data->prevDiskSrc); + if (!data->diskSrc) + return -1; - data->parentDiskSrc = data->diskSrc->backingStore; - if (!virStorageSourceIsBacking(data->parentDiskSrc)) { - virReportError(VIR_ERR_OPERATION_FAILED, "%s", - _("failed to find parent disk source in backing chain")); - return -1; - } + 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 -1; + } - 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 -1; + data->parentDiskSrc = data->diskSrc->backingStore; + if (!virStorageSourceIsBacking(data->parentDiskSrc)) { + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("failed to find parent disk source in backing chain")); + return -1; + } + + 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 -1; + } } - } - data->parentSnap = qemuSnapshotFindParentSnapForDisk(snap, data->snapDisk); + 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 -1; + if (data->parentSnap && !virDomainSnapshotIsExternal(data->parentSnap)) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("deleting external snapshot that has internal snapshot as parent not supported")); + return -1; + } } ret = g_slist_prepend(ret, g_steal_pointer(&data)); @@ -2672,7 +2699,7 @@ qemuSnapshotDeleteExternalPrepare(virDomainObj *vm, } /* this also serves as validation whether the snapshot can be deleted */ - if (qemuSnapshotDeleteExternalPrepareData(vm, snap, &tmpData) < 0) + if (qemuSnapshotDeleteExternalPrepareData(vm, snap, true, &tmpData) < 0) return -1; if (!virDomainObjIsActive(vm)) { @@ -2687,7 +2714,7 @@ qemuSnapshotDeleteExternalPrepare(virDomainObj *vm, /* Call the prepare again as some data require that the VM is * running to get everything we need. */ - if (qemuSnapshotDeleteExternalPrepareData(vm, snap, externalData) < 0) + if (qemuSnapshotDeleteExternalPrepareData(vm, snap, true, externalData) < 0) return -1; } else { qemuDomainJobPrivate *jobPriv = vm->job->privateData; -- 2.41.0
On Tue, Jun 27, 2023 at 17:07:18 +0200, Pavel Hrdina wrote:
Before external snapshot revert every delete operation did block commit in order to delete a snapshot. But now when user reverts to non-leaf snapshot deleting leaf snapshot will not have any overlay files so we can just simply delete the snapshot images.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 93 ++++++++++++++++++++++++++-------------- 1 file changed, 60 insertions(+), 33 deletions(-)
diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 08cff2a9a2..9c4d26bad5 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -2569,9 +2569,26 @@ qemuSnapshotFindParentSnapForDisk(virDomainMomentObj *snap, }
+/** + * qemuSnapshotDeleteExternalPrepareData: + * @vm: domain object + * @snap: snapshot object + * @merge: whether we are just deleting image or not + * @externalData: prepared data to delete external snapshot + * + * Validate if we can delete selected snapshot @snap and prepare all necessary + * data that will be used when deleting snapshot as @externalData. + * + * If @merge is set to true we will merge the deleted snapshot into parent one + * instead of just deleting it. This is necessary when operating on snapshot + * that has existing overlay files. + * + * Returns -1 on error, 0 on success. + */ static int qemuSnapshotDeleteExternalPrepareData(virDomainObj *vm, virDomainMomentObj *snap, + bool merge, GSList **externalData) { ssize_t i; @@ -2579,7 +2596,6 @@ qemuSnapshotDeleteExternalPrepareData(virDomainObj *vm, 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_EXTERNAL) @@ -2591,14 +2607,18 @@ qemuSnapshotDeleteExternalPrepareData(virDomainObj *vm, snapDisk->name); return -1; } + } + + for (i = 0; i < snapdef->ndisks; i++) {
I didn't really find a reason why you've added another loop here. The only thing the function does is to prepare data, so I don't think there is anything that'd require another pass through the disks.
+ virDomainSnapshotDiskDef *snapDisk = &(snapdef->disks[i]); + g_autofree qemuSnapshotDeleteExternalData *data = NULL; + + if (snapDisk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) + continue;
With the extra loop removed: Reviewed-by: Peter Krempa <pkrempa@redhat.com>
In this case there is no need to run block commit and using qemu process at all. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> Reviewed-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_snapshot.c | 55 +++++++++++++++++++++++----------------- 1 file changed, 32 insertions(+), 23 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 9c4d26bad5..be94e97340 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -2698,34 +2698,43 @@ qemuSnapshotDeleteExternalPrepare(virDomainObj *vm, return 0; } - /* this also serves as validation whether the snapshot can be deleted */ - if (qemuSnapshotDeleteExternalPrepareData(vm, snap, true, &tmpData) < 0) - return -1; - - 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) { - return -1; - } - - *stop_qemu = true; - - /* Call the prepare again as some data require that the VM is - * running to get everything we need. */ - if (qemuSnapshotDeleteExternalPrepareData(vm, snap, true, externalData) < 0) + /* Leaf non-active snapshot doesn't have overlay files for the disk images + * so there is no need to do any merge and we can just delete the files + * directly. */ + if (snap != virDomainSnapshotGetCurrent(vm->snapshots) && + snap->nchildren == 0) { + if (qemuSnapshotDeleteExternalPrepareData(vm, snap, false, externalData) < 0) return -1; } else { - qemuDomainJobPrivate *jobPriv = vm->job->privateData; + /* this also serves as validation whether the snapshot can be deleted */ + if (qemuSnapshotDeleteExternalPrepareData(vm, snap, true, &tmpData) < 0) + return -1; - *externalData = g_steal_pointer(&tmpData); + 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) { + return -1; + } - /* If the VM is running we need to indicate that the async snapshot - * job is snapshot delete job. */ - jobPriv->snapshotDelete = true; + *stop_qemu = true; - qemuDomainSaveStatus(vm); + /* Call the prepare again as some data require that the VM is + * running to get everything we need. */ + if (qemuSnapshotDeleteExternalPrepareData(vm, snap, true, externalData) < 0) + return -1; + } else { + qemuDomainJobPrivate *jobPriv = vm->job->privateData; + + *externalData = g_steal_pointer(&tmpData); + + /* If the VM is running we need to indicate that the async snapshot + * job is snapshot delete job. */ + jobPriv->snapshotDelete = true; + + qemuDomainSaveStatus(vm); + } } return 0; -- 2.41.0
When block commit is not needed we can just simply unlink the disk files. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 56 ++++++++++++++++++++++++++-------------- 1 file changed, 36 insertions(+), 20 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index be94e97340..b08e06d312 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -2510,6 +2510,7 @@ typedef struct _qemuSnapshotDeleteExternalData { virStorageSource *prevDiskSrc; /* source of disk for which @diskSrc is backing disk */ qemuBlockJobData *job; + bool merge; } qemuSnapshotDeleteExternalData; @@ -2618,6 +2619,7 @@ qemuSnapshotDeleteExternalPrepareData(virDomainObj *vm, data = g_new0(qemuSnapshotDeleteExternalData, 1); data->snapDisk = snapDisk; + data->merge = merge; data->parentDomDisk = virDomainDiskByTarget(snapdef->parent.dom, data->snapDisk->name); @@ -2628,7 +2630,7 @@ qemuSnapshotDeleteExternalPrepareData(virDomainObj *vm, return -1; } - if (merge) { + if (data->merge) { data->domDisk = qemuDomainDiskByName(vm->def, snapDisk->name); if (!data->domDisk) return -1; @@ -3053,31 +3055,42 @@ qemuSnapshotDiscardExternal(virDomainObj *vm, virTristateBool autofinalize = VIR_TRISTATE_BOOL_NO; unsigned int commitFlags = VIR_DOMAIN_BLOCK_COMMIT_DELETE; - if (data->domDisk->src == data->diskSrc) { - commitFlags |= VIR_DOMAIN_BLOCK_COMMIT_ACTIVE; - autofinalize = VIR_TRISTATE_BOOL_YES; + if (data->merge) { + if (data->domDisk->src == data->diskSrc) { + commitFlags |= VIR_DOMAIN_BLOCK_COMMIT_ACTIVE; + autofinalize = VIR_TRISTATE_BOOL_YES; + } + + if (qemuSnapshotSetInvalid(vm, data->parentSnap, data->snapDisk, true) < 0) + goto error; + + data->job = qemuBlockCommit(vm, + data->domDisk, + data->parentDiskSrc, + data->diskSrc, + data->prevDiskSrc, + 0, + VIR_ASYNC_JOB_SNAPSHOT, + autofinalize, + commitFlags); + + if (!data->job) + goto error; + } else { + if (virStorageSourceInit(data->parentDomDisk->src) < 0 || + virStorageSourceUnlink(data->parentDomDisk->src) < 0) { + VIR_WARN("Failed to remove snapshot image '%s'", + data->snapDisk->name); + } } - - if (qemuSnapshotSetInvalid(vm, data->parentSnap, data->snapDisk, true) < 0) - goto error; - - data->job = qemuBlockCommit(vm, - data->domDisk, - data->parentDiskSrc, - data->diskSrc, - data->prevDiskSrc, - 0, - VIR_ASYNC_JOB_SNAPSHOT, - autofinalize, - commitFlags); - - if (!data->job) - goto error; } for (cur = externalData; cur; cur = g_slist_next(cur)) { qemuSnapshotDeleteExternalData *data = cur->data; + if (!data->merge) + continue; + if (qemuSnapshotDeleteBlockJobRunning(vm, data->job) < 0) goto error; @@ -3092,6 +3105,9 @@ qemuSnapshotDiscardExternal(virDomainObj *vm, for (cur = externalData; cur; cur = g_slist_next(cur)) { qemuSnapshotDeleteExternalData *data = cur->data; + if (!data->merge) + continue; + if (data->job->state == QEMU_BLOCKJOB_STATE_READY) { if (qemuBlockPivot(vm, data->job, VIR_ASYNC_JOB_SNAPSHOT, NULL) < 0) goto error; -- 2.41.0
On Tue, Jun 27, 2023 at 17:07:20 +0200, Pavel Hrdina wrote:
When block commit is not needed we can just simply unlink the disk files.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 56 ++++++++++++++++++++++++++-------------- 1 file changed, 36 insertions(+), 20 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
When deleting external snapshot and parent snapshot is the currently active snapshot as user reverted to it we need to properly update the parent snapshot metadata. After the delete is done the new overlay files will be the currently used files created when snapshot revert was done, replacing the original overlay files. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> Reviewed-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_snapshot.c | 40 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index b08e06d312..a206f015c4 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -3168,6 +3168,41 @@ qemuSnapshotDiscardExternal(virDomainObj *vm, } +static int +qemuSnapshotDeleteUpdateParent(virDomainObj *vm, + virDomainMomentObj *parent) +{ + size_t i; + virQEMUDriver *driver = QEMU_DOMAIN_PRIVATE(vm)->driver; + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); + virDomainSnapshotDef *parentDef = virDomainSnapshotObjGetDef(parent); + + if (!parentDef) + return 0; + + if (!parentDef->revertdisks) + return 0; + + for (i = 0; i < parentDef->ndisks; i++) { + virDomainSnapshotDiskDefClear(&parentDef->disks[i]); + } + g_free(parentDef->disks); + + parentDef->disks = g_steal_pointer(&parentDef->revertdisks); + parentDef->ndisks = parentDef->nrevertdisks; + parentDef->nrevertdisks = 0; + + if (qemuDomainSnapshotWriteMetadata(vm, + parent, + driver->xmlopt, + cfg->snapshotDir) < 0) { + return -1; + } + + return 0; +} + + static int qemuSnapshotDiscardMetadata(virDomainObj *vm, virDomainMomentObj *snap, @@ -3207,6 +3242,11 @@ qemuSnapshotDiscardMetadata(virDomainObj *vm, virDomainMomentMoveChildren(snap, snap->parent); } + if (update_parent && snap->parent) { + if (qemuSnapshotDeleteUpdateParent(vm, snap->parent) < 0) + ret = -1; + } + snapFile = g_strdup_printf("%s/%s/%s.xml", cfg->snapshotDir, vm->def->name, snap->def->name); -- 2.41.0
When user creates a new snapshot after reverting to non-leaf snapshot we no longer need to store the temporary overlays as they will be part of the VM XMLs stored in the newly created snapshot. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index a206f015c4..2950ad7d77 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -1652,6 +1652,23 @@ qemuSnapshotCreateWriteMetadata(virDomainObj *vm, } +static void +qemuSnapshotClearRevertdisks(virDomainMomentObj *current) +{ + virDomainSnapshotDef *curdef = NULL; + + if (!current) + return; + + curdef = virDomainSnapshotObjGetDef(current); + + if (curdef->revertdisks) { + g_clear_pointer(&curdef->revertdisks, g_free); + curdef->nrevertdisks = 0; + } +} + + static virDomainSnapshotPtr qemuSnapshotRedefine(virDomainObj *vm, virDomainPtr domain, @@ -1661,6 +1678,7 @@ qemuSnapshotRedefine(virDomainObj *vm, unsigned int flags) { virDomainMomentObj *snap = NULL; + virDomainMomentObj *current = virDomainSnapshotGetCurrent(vm->snapshots); virDomainSnapshotPtr ret = NULL; g_autoptr(virDomainSnapshotDef) snapdef = virObjectRef(snapdeftmp); @@ -1678,8 +1696,10 @@ qemuSnapshotRedefine(virDomainObj *vm, * makes sense, such as checking that qemu-img recognizes the * snapshot name in at least one of the domain's disks? */ - if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT) + if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT) { + qemuSnapshotClearRevertdisks(current); qemuSnapshotSetCurrent(vm, snap); + } if (qemuSnapshotCreateWriteMetadata(vm, snap, driver, cfg) < 0) goto error; @@ -1758,6 +1778,7 @@ qemuSnapshotCreate(virDomainObj *vm, } if (!tmpsnap) { + qemuSnapshotClearRevertdisks(current); qemuSnapshotSetCurrent(vm, snap); if (qemuSnapshotCreateWriteMetadata(vm, snap, driver, cfg) < 0) -- 2.41.0
On Tue, Jun 27, 2023 at 17:07:22 +0200, Pavel Hrdina wrote:
When user creates a new snapshot after reverting to non-leaf snapshot we no longer need to store the temporary overlays as they will be part of the VM XMLs stored in the newly created snapshot.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index a206f015c4..2950ad7d77 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -1652,6 +1652,23 @@ qemuSnapshotCreateWriteMetadata(virDomainObj *vm, }
+static void +qemuSnapshotClearRevertdisks(virDomainMomentObj *current) +{ + virDomainSnapshotDef *curdef = NULL; + + if (!current) + return; + + curdef = virDomainSnapshotObjGetDef(current); + + if (curdef->revertdisks) { + g_clear_pointer(&curdef->revertdisks, g_free);
At least in one instance the structure's 'name' and 'src' fields are filled with allocated pointers, so this looks like it will leak memory. I also didn't find code which frees the 'reverdisks' field when the snapshot definition is being freed.
This new helper will allow us to check if we are able to delete external snapshot after user did revert to non-leaf snapshot. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> Reviewed-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/virdomainmomentobjlist.c | 17 +++++++++++++++++ src/conf/virdomainmomentobjlist.h | 4 ++++ src/libvirt_private.syms | 1 + 3 files changed, 22 insertions(+) diff --git a/src/conf/virdomainmomentobjlist.c b/src/conf/virdomainmomentobjlist.c index e5cdd9a141..ea9850df8c 100644 --- a/src/conf/virdomainmomentobjlist.c +++ b/src/conf/virdomainmomentobjlist.c @@ -582,3 +582,20 @@ virDomainMomentFindLeaf(virDomainMomentObjList *list) return moment; return NULL; } + + +/* Check if @moment is descendant of @ancestor. */ +bool +virDomainMomentIsAncestor(virDomainMomentObj *moment, + virDomainMomentObj *ancestor) +{ + if (moment == ancestor) + return false; + + for (moment = moment->parent; moment; moment = moment->parent) { + if (moment == ancestor) + return true; + } + + return false; +} diff --git a/src/conf/virdomainmomentobjlist.h b/src/conf/virdomainmomentobjlist.h index d2ab3b46b1..2ea6b181c0 100644 --- a/src/conf/virdomainmomentobjlist.h +++ b/src/conf/virdomainmomentobjlist.h @@ -157,3 +157,7 @@ virDomainMomentCheckCycles(virDomainMomentObjList *list, virDomainMomentObj * virDomainMomentFindLeaf(virDomainMomentObjList *list); + +bool +virDomainMomentIsAncestor(virDomainMomentObj *moment, + virDomainMomentObj *ancestor); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 2b1d4e4512..2f4ab607e5 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1221,6 +1221,7 @@ virDomainMomentDropChildren; virDomainMomentDropParent; virDomainMomentForEachChild; virDomainMomentForEachDescendant; +virDomainMomentIsAncestor; virDomainMomentMoveChildren; virDomainMomentObjFree; virDomainMomentObjNew; -- 2.41.0
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@redhat.com> --- 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)); + + 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)); + } +} + + 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
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@redhat.com> --- 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
There will be more external snapshot checks introduced by following patch so group them together. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> Reviewed-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_snapshot.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 337c83f151..9246c02f12 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -3635,18 +3635,18 @@ qemuSnapshotDeleteValidate(virDomainObj *vm, } } - if (virDomainSnapshotIsExternal(snap) && - qemuDomainHasBlockjob(vm, false)) { - virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", - _("cannot delete external snapshots when there is another active block job")); - return -1; - } + if (virDomainSnapshotIsExternal(snap)) { + if (qemuDomainHasBlockjob(vm, false)) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("cannot delete external snapshots when there is another active block job")); + return -1; + } - if (virDomainSnapshotIsExternal(snap) && - (flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("deletion of external disk snapshots with children not supported")); - return -1; + if (flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("deletion of external disk snapshots with children not supported")); + return -1; + } } return 0; -- 2.41.0
With the introduction of external snapshot revert support we need to error out in some cases when trying to delete some snapshots. If users reverts to non-leaf snapshots and would try to delete it after the revert is done it would not work currently as this operation would require using block-stream which is not implemented for now as in this case the snapshot has two children so the disk files have multiple overlays. Similarly if user reverts to non-leaf snapshot and would try to delete snapshot that is non-leaf but not in currently active snapshot chain we would still need to use block-commit operation. The issue here is that in order to do that we would have to start new qemu process with different domain definition than what is currently used by the domain. If the current domain would be running it would complicate things even more so this operation is not yet supported. If user creates new snapshot after reverting to non-leaf snapshot it creates a new branch. Deleting snapshot with multiple children will require block-stream which is not implemented for now. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 9246c02f12..9e8a7f2f9f 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -3636,6 +3636,8 @@ qemuSnapshotDeleteValidate(virDomainObj *vm, } if (virDomainSnapshotIsExternal(snap)) { + virDomainMomentObj *current = virDomainSnapshotGetCurrent(vm->snapshots); + if (qemuDomainHasBlockjob(vm, false)) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", _("cannot delete external snapshots when there is another active block job")); @@ -3647,6 +3649,25 @@ qemuSnapshotDeleteValidate(virDomainObj *vm, _("deletion of external disk snapshots with children not supported")); return -1; } + + if (snap == current && snap->nchildren != 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("deletion of active external snapshot that is not a leaf snapshot is not supported")); + return -1; + } + + if (snap != current && snap->nchildren != 0 && + virDomainMomentIsAncestor(snap, current)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("deletion of non-leaf external snapshot that is not in active chain is not supported")); + return -1; + } + + if (snap->nchildren > 1) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("deletion of external disk snapshot with multiple children snapshots not supported")); + return -1; + } } return 0; -- 2.41.0
On Tue, Jun 27, 2023 at 17:07:26 +0200, Pavel Hrdina wrote:
With the introduction of external snapshot revert support we need to error out in some cases when trying to delete some snapshots.
If users reverts to non-leaf snapshots and would try to delete it after the revert is done it would not work currently as this operation would require using block-stream which is not implemented for now as in this case the snapshot has two children so the disk files have multiple overlays.
Similarly if user reverts to non-leaf snapshot and would try to delete snapshot that is non-leaf but not in currently active snapshot chain we would still need to use block-commit operation. The issue here is that in order to do that we would have to start new qemu process with different domain definition than what is currently used by the domain. If the current domain would be running it would complicate things even more so this operation is not yet supported.
If user creates new snapshot after reverting to non-leaf snapshot it creates a new branch. Deleting snapshot with multiple children will require block-stream which is not implemented for now.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 21 +++++++++++++++++++++
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Now that the support to revert external snapshots is implemented we can drop this check. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 9e8a7f2f9f..5150e8685a 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -1880,12 +1880,6 @@ qemuSnapshotRevertValidate(virDomainObj *vm, return -1; } - if (virDomainSnapshotIsExternal(snap)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("revert to external snapshot not supported yet")); - return -1; - } - if (!snap->def->dom) { virReportError(VIR_ERR_SNAPSHOT_REVERT_RISKY, _("snapshot '%1$s' lacks domain '%2$s' rollback info"), -- 2.41.0
On Tue, Jun 27, 2023 at 17:07:27 +0200, Pavel Hrdina wrote:
Now that the support to revert external snapshots is implemented we can drop this check.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 6 ------ 1 file changed, 6 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
participants (2)
-
Pavel Hrdina -
Peter Krempa