Rather than one file per snapshot, store all qemu snapshots in a
single file, using the recently added bulk snapshot list
operations. For now, this doesn't change how often libvirt writes a
snapshot file, but it does open the door for the next patch to update
the signature to qemuDomainSnapshotWriteMetadata() and call it less
frequently. One of the main benefits for doing a bulk write is that
you only have to do a single file system write at the end of an
operation, rather than potentially 3 during
virDomainSnapshotCreateXML(REDEFINE|CURRENT) (delete the old snapshot
definition being redefined, rewrite the previous current snapshot to
longer be current, and store the new snapshot definition) or even more
during virDomainSnapshotDelete(DESCENDENTS) (a file system hit per
snapshot being deleted). It also makes it perhaps a bit more feasible
to roll back to earlier state if something fails horribly midway
through an operation (until you write the new file, the old file is
still a reliable record of what state to roll back to), compared to
the current code which has to track lots of things locally; although I
did not attempt to play with any patches along those lines.
Another benefit of the bulk write - it's less code to maintain, and
will make it easier for me to model qemu's checkpoint storage in the
same way (and for checkpoints, I don't even have to worry about legacy
parsing).
This is a one-way upgrade - if you have snapshots created by an older
libvirt, the new libvirt will correctly load those snapshots and
convert to the new format. But as the new libvirt will no longer
output the old format, reverting back to the old libvirt will make it
appear that all snapshots have disappeared (merely hidden until you
upgrade libvirt again). But then again, we've never promised that
downgrading libvirt after an upgrade was supposed to work flawlessly.
There is a slight chance for confusion if a user named two separate
domains 'foo' and 'foo.xml'; the new scheme expects 'foo.xml' to be a
regular file for the former domain, while the old scheme expected
'foo.xml' to be a directory for the latter domain; if we are worried
about that, we could tweak the code to instead output new state in a
file named 'foo' instead of the more typical 'foo.xml' and just key
off of whether the file is regular or a directory when deciding if
this is the first libvirt run after an upgrade. But I felt that the
chances of a user abusing domain names like that is not worth the
effort.
The bulk snapshot file can be huge (over RPC, we allow <domain> to be
up to 4M, and we allow up to 16k snapshots; since each each snapshot
includes a <domain>, a worst-case domain would result in gigabytes of
bulk data); it is no worse on overall filesystem usage than before,
but now in a single file vs. a series of files it requires more memory
to read in at once. I don't know if we have to worry about that in
practice, but my patch does cap things to read in no more than an
arbitrarily-picked 128M, which we may have to raise in the future.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
src/qemu/qemu_domain.c | 59 ++++++++-------------------
src/qemu/qemu_driver.c | 93 +++++++++++++++++++++++++++++++++---------
2 files changed, 91 insertions(+), 61 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index ea7b31dab3..424f839a00 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -8448,45 +8448,28 @@ qemuFindQemuImgBinary(virQEMUDriverPtr driver)
int
qemuDomainSnapshotWriteMetadata(virDomainObjPtr vm,
- virDomainMomentObjPtr snapshot,
+ virDomainMomentObjPtr snapshot ATTRIBUTE_UNUSED,
virCapsPtr caps,
virDomainXMLOptionPtr xmlopt,
const char *snapshotDir)
{
- char *newxml = NULL;
- int ret = -1;
- char *snapDir = NULL;
- char *snapFile = NULL;
char uuidstr[VIR_UUID_STRING_BUFLEN];
- unsigned int flags = VIR_DOMAIN_SNAPSHOT_FORMAT_SECURE |
- VIR_DOMAIN_SNAPSHOT_FORMAT_INTERNAL;
- virDomainSnapshotDefPtr def = virDomainSnapshotObjGetDef(snapshot);
+ unsigned int flags = VIR_DOMAIN_SNAPSHOT_FORMAT_SECURE;
+ VIR_AUTOFREE(char *) newxml = NULL;
+ VIR_AUTOFREE(char *) snapDir = NULL;
+ VIR_AUTOFREE(char *) snapFile = NULL;
+ VIR_AUTOCLEAN(virBuffer) buf = VIR_BUFFER_INITIALIZER;
- if (virDomainSnapshotGetCurrent(vm->snapshots) == snapshot)
- flags |= VIR_DOMAIN_SNAPSHOT_FORMAT_CURRENT;
virUUIDFormat(vm->def->uuid, uuidstr);
- newxml = virDomainSnapshotDefFormat(uuidstr, def, caps, xmlopt, flags);
- if (newxml == NULL)
+ if (virDomainSnapshotObjListFormat(&buf, uuidstr, vm->snapshots, caps,
+ xmlopt, flags) < 0)
return -1;
- if (virAsprintf(&snapDir, "%s/%s", snapshotDir, vm->def->name) < 0)
- goto cleanup;
- if (virFileMakePath(snapDir) < 0) {
- virReportSystemError(errno, _("cannot create snapshot directory '%s'"),
- snapDir);
- goto cleanup;
- }
-
- if (virAsprintf(&snapFile, "%s/%s.xml", snapDir, def->common.name) < 0)
- goto cleanup;
-
- ret = virXMLSaveFile(snapFile, NULL, "snapshot-edit", newxml);
+ if (virAsprintf(&snapFile, "%s/%s.xml", snapshotDir, vm->def->name) < 0)
+ return -1;
- cleanup:
- VIR_FREE(snapFile);
- VIR_FREE(snapDir);
- VIR_FREE(newxml);
- return ret;
+ newxml = virBufferContentAndReset(&buf);
+ return virXMLSaveFile(snapFile, NULL, "snapshot-edit", newxml);
}
/* The domain is expected to be locked and inactive. Return -1 on normal
@@ -8589,7 +8572,6 @@ qemuDomainSnapshotDiscard(virQEMUDriverPtr driver,
bool update_parent,
bool metadata_only)
{
- char *snapFile = NULL;
int ret = -1;
qemuDomainObjPrivatePtr priv;
virDomainMomentObjPtr parentsnap = NULL;
@@ -8610,10 +8592,6 @@ qemuDomainSnapshotDiscard(virQEMUDriverPtr driver,
}
}
- if (virAsprintf(&snapFile, "%s/%s/%s.xml", cfg->snapshotDir,
- vm->def->name, snap->def->name) < 0)
- goto cleanup;
-
if (snap == virDomainSnapshotGetCurrent(vm->snapshots)) {
virDomainSnapshotSetCurrent(vm->snapshots, NULL);
if (update_parent && snap->def->parent) {
@@ -8635,8 +8613,6 @@ qemuDomainSnapshotDiscard(virQEMUDriverPtr driver,
}
}
- if (unlink(snapFile) < 0)
- VIR_WARN("Failed to unlink %s", snapFile);
if (update_parent)
virDomainMomentDropParent(snap);
virDomainSnapshotObjListRemove(vm->snapshots, snap);
@@ -8644,7 +8620,6 @@ qemuDomainSnapshotDiscard(virQEMUDriverPtr driver,
ret = 0;
cleanup:
- VIR_FREE(snapFile);
virObjectUnref(cfg);
return ret;
}
@@ -8691,7 +8666,7 @@ qemuDomainRemoveInactiveCommon(virQEMUDriverPtr driver,
virDomainObjPtr vm)
{
virQEMUDriverConfigPtr cfg;
- VIR_AUTOFREE(char *) snapDir = NULL;
+ VIR_AUTOFREE(char *) snapFile = NULL;
cfg = virQEMUDriverGetConfig(driver);
@@ -8699,12 +8674,12 @@ qemuDomainRemoveInactiveCommon(virQEMUDriverPtr driver,
if (qemuDomainSnapshotDiscardAllMetadata(driver, vm) < 0) {
VIR_WARN("unable to remove all snapshots for domain %s",
vm->def->name);
- } else if (virAsprintf(&snapDir, "%s/%s", cfg->snapshotDir,
+ } else if (virAsprintf(&snapFile, "%s/%s.xml", cfg->snapshotDir,
vm->def->name) < 0) {
- VIR_WARN("unable to remove snapshot directory %s/%s",
+ VIR_WARN("unable to remove snapshots storage %s/%s.xml",
cfg->snapshotDir, vm->def->name);
- } else if (rmdir(snapDir) < 0 && errno != ENOENT) {
- VIR_WARN("unable to remove snapshot directory %s", snapDir);
+ } else if (unlink(snapFile) < 0 && errno != ENOENT) {
+ VIR_WARN("unable to remove snapshots storage %s", snapFile);
}
qemuExtDevicesCleanupHost(driver, vm->def);
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 9c2245b095..018d1cdc87 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -406,12 +406,14 @@ qemuSecurityInit(virQEMUDriverPtr driver)
}
+/* Older qemu used a series of $dir/snapshot/domainname/snap.xml
+ * files, instead of the modern $dir/snapshot/domainname.xml bulk
+ * file. Called while vm is locked. */
static int
-qemuDomainSnapshotLoad(virDomainObjPtr vm,
- void *data)
+qemuDomainSnapshotLoadLegacy(virDomainObjPtr vm,
+ char *snapDir,
+ virCapsPtr caps)
{
- char *baseDir = (char *)data;
- char *snapDir = NULL;
DIR *dir = NULL;
struct dirent *entry;
char *xmlStr;
@@ -424,21 +426,8 @@ qemuDomainSnapshotLoad(virDomainObjPtr vm,
VIR_DOMAIN_SNAPSHOT_PARSE_DISKS |
VIR_DOMAIN_SNAPSHOT_PARSE_INTERNAL);
int ret = -1;
- virCapsPtr caps = NULL;
int direrr;
- virObjectLock(vm);
- if (virAsprintf(&snapDir, "%s/%s", baseDir, vm->def->name) < 0) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Failed to allocate memory for "
- "snapshot directory for domain %s"),
- vm->def->name);
- goto cleanup;
- }
-
- if (!(caps = virQEMUDriverGetCapabilities(qemu_driver, false)))
- goto cleanup;
-
VIR_INFO("Scanning for snapshots for domain %s in %s", vm->def->name,
snapDir);
@@ -503,6 +492,74 @@ qemuDomainSnapshotLoad(virDomainObjPtr vm,
_("Snapshots have inconsistent relations for domain %s"),
vm->def->name);
+ virResetLastError();
+
+ ret = 0;
+ cleanup:
+ VIR_DIR_CLOSE(dir);
+ return ret;
+}
+
+
+/* Load all snapshots associated with domain */
+static int
+qemuDomainSnapshotLoad(virDomainObjPtr vm,
+ void *data)
+{
+ char *baseDir = (char *)data;
+ unsigned int flags = (VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE |
+ VIR_DOMAIN_SNAPSHOT_PARSE_DISKS);
+ int ret = -1;
+ virCapsPtr caps = NULL;
+ VIR_AUTOFREE(char *) snapFile = NULL;
+ VIR_AUTOFREE(char *) snapDir = NULL;
+ VIR_AUTOFREE(char *) xmlStr = NULL;
+
+ virObjectLock(vm);
+ VIR_INFO("Scanning for snapshots for domain %s in %s", vm->def->name,
+ baseDir);
+ if (virAsprintf(&snapFile, "%s/%s.xml", baseDir, vm->def->name) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Failed to allocate memory for "
+ "snapshots storage for domain %s"),
+ vm->def->name);
+ goto cleanup;
+ }
+
+ if (!(caps = virQEMUDriverGetCapabilities(qemu_driver, false)))
+ goto cleanup;
+
+ if (virFileExists(snapFile)) {
+ /* State last saved by modern libvirt in single file. As each
+ * snapshot contains a <domain>, it can be quite large. */
+ if (virFileReadAll(snapFile, 128*1024*1024*1, &xmlStr) < 0) {
+ /* Nothing we can do here */
+ virReportSystemError(errno,
+ _("Failed to read snapshot file %s"),
+ snapFile);
+ goto cleanup;
+ }
+
+ ret = virDomainSnapshotObjListParse(xmlStr, vm->def->uuid,
+ vm->snapshots, caps,
+ qemu_driver->xmlopt, flags);
+ if (ret < 0)
+ goto cleanup;
+ VIR_INFO("Read in %d snapshots for domain %s", ret, vm->def->name);
+ } else if (virAsprintf(&snapDir, "%s/%s", baseDir, vm->def->name) >= 0 &&
+ virFileExists(snapDir)) {
+ /* State may have been saved by earlier libvirt; if so, try to
+ * read it in, convert to modern style, and remove the old
+ * directory if successful. */
+ if (qemuDomainSnapshotLoadLegacy(vm, snapDir, caps) < 0)
+ goto cleanup;
+ if (qemuDomainSnapshotWriteMetadata(vm, NULL, caps,
+ qemu_driver->xmlopt, baseDir) < 0)
+ goto cleanup;
+ if (virFileDeleteTree(snapDir) < 0)
+ VIR_WARN("Unable to remove legacy snapshot directory %s", snapDir);
+ }
+
/* FIXME: qemu keeps internal track of snapshots. We can get access
* to this info via the "info snapshots" monitor command for running
* domains, or via "qemu-img snapshot -l" for shutoff domains. It would
@@ -516,8 +573,6 @@ qemuDomainSnapshotLoad(virDomainObjPtr vm,
ret = 0;
cleanup:
- VIR_DIR_CLOSE(dir);
- VIR_FREE(snapDir);
virObjectUnref(caps);
virObjectUnlock(vm);
return ret;
--
2.20.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 3/20/19 5:32 PM, Eric Blake wrote:
> Rather than one file per snapshot, store all qemu snapshots in a
> single file, using the recently added bulk snapshot list
> operations. For now, this doesn't change how often libvirt writes a
> snapshot file, but it does open the door for the next patch to update
> the signature to qemuDomainSnapshotWriteMetadata() and call it less
> frequently. One of the main benefits for doing a bulk write is that
> you only have to do a single file system write at the end of an
> operation, rather than potentially 3 during
> virDomainSnapshotCreateXML(REDEFINE|CURRENT) (delete the old snapshot
> definition being redefined, rewrite the previous current snapshot to
> longer be current, and store the new snapshot definition) or even more
> during virDomainSnapshotDelete(DESCENDENTS) (a file system hit per
> snapshot being deleted). It also makes it perhaps a bit more feasible
> to roll back to earlier state if something fails horribly midway
> through an operation (until you write the new file, the old file is
> still a reliable record of what state to roll back to), compared to
> the current code which has to track lots of things locally; although I
> did not attempt to play with any patches along those lines.
>
> Another benefit of the bulk write - it's less code to maintain, and
> will make it easier for me to model qemu's checkpoint storage in the
> same way (and for checkpoints, I don't even have to worry about legacy
> parsing).
>
> This is a one-way upgrade - if you have snapshots created by an older
> libvirt, the new libvirt will correctly load those snapshots and
> convert to the new format. But as the new libvirt will no longer
> output the old format, reverting back to the old libvirt will make it
> appear that all snapshots have disappeared (merely hidden until you
> upgrade libvirt again). But then again, we've never promised that
> downgrading libvirt after an upgrade was supposed to work flawlessly.
This kind of information I would think would need to be heavily if not
overly documented. I assume someone could say this should be "stuck
behind" a forced decision by the consumer to do the conversion to the
new style. What worries me most is the issues in the subsequent
paragraphs that could bite someone.
To some degree we could be 'stuck with' the model for snapshots, but
that doesn't mean checkpoints couldn't make use of some newer
functionality that stores everything in one file and not allow the
storage into multiple files.
This is one of those gray areas for me that from an overall
architectural level that we struggle with related to dealing with our
technical debt because we've guaranteed some sort of backcompat for
various things "forever".
I also wonder what harm it does to keep the old style around and *force*
the consumer to do the deletion. Whether that's by them running rmdir on
the old directory or running some libvirt command to clean out the
legacy. I lean towards leaving the old stuff there - if nothing else it
provides some strange sort of comfort. Maybe even rename the directory
to have something like ".old" in the name.
I'm OK with the choice and arguments you make for the algorithm you
chose, but a ping of Daniel to make sure he's OK with it probably would
help here, especially since he commented on the last pile and brought
the issue to the forefront.
>
> There is a slight chance for confusion if a user named two separate
> domains 'foo' and 'foo.xml'; the new scheme expects 'foo.xml' to be a
> regular file for the former domain, while the old scheme expected
> 'foo.xml' to be a directory for the latter domain; if we are worried
> about that, we could tweak the code to instead output new state in a
> file named 'foo' instead of the more typical 'foo.xml' and just key
> off of whether the file is regular or a directory when deciding if
> this is the first libvirt run after an upgrade. But I felt that the
> chances of a user abusing domain names like that is not worth the
> effort.
I think this is just another thing to document. As in, if you've done
this and use snapshots, then "fix your stupidity" ;-). Why anyone other
that QE to test all possibilities would name a domain with ".xml" in the
name suffix goes beyond common sense to me.
>
> The bulk snapshot file can be huge (over RPC, we allow <domain> to be
> up to 4M, and we allow up to 16k snapshots; since each each snapshot
> includes a <domain>, a worst-case domain would result in gigabytes of
> bulk data); it is no worse on overall filesystem usage than before,
> but now in a single file vs. a series of files it requires more memory
> to read in at once. I don't know if we have to worry about that in
> practice, but my patch does cap things to read in no more than an
> arbitrarily-picked 128M, which we may have to raise in the future.
and even more. I think when I first reviewed this I thought of the RPC
limit stuff - at least w/r/t how the stats code is the other place where
we've hit this limit. But for some reason I felt that perhaps some under
the covers code in that code we solved something that allowed for things
to work, but now I'm not so sure.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
> src/qemu/qemu_domain.c | 59 ++++++++-------------------
> src/qemu/qemu_driver.c | 93 +++++++++++++++++++++++++++++++++---------
> 2 files changed, 91 insertions(+), 61 deletions(-)
>
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index ea7b31dab3..424f839a00 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -8448,45 +8448,28 @@ qemuFindQemuImgBinary(virQEMUDriverPtr driver)
>
> int
> qemuDomainSnapshotWriteMetadata(virDomainObjPtr vm,
> - virDomainMomentObjPtr snapshot,
> + virDomainMomentObjPtr snapshot ATTRIBUTE_UNUSED,
> virCapsPtr caps,
> virDomainXMLOptionPtr xmlopt,
> const char *snapshotDir)
> {
> - char *newxml = NULL;
> - int ret = -1;
> - char *snapDir = NULL;
> - char *snapFile = NULL;
> char uuidstr[VIR_UUID_STRING_BUFLEN];
> - unsigned int flags = VIR_DOMAIN_SNAPSHOT_FORMAT_SECURE |
> - VIR_DOMAIN_SNAPSHOT_FORMAT_INTERNAL;
> - virDomainSnapshotDefPtr def = virDomainSnapshotObjGetDef(snapshot);
> + unsigned int flags = VIR_DOMAIN_SNAPSHOT_FORMAT_SECURE;
> + VIR_AUTOFREE(char *) newxml = NULL;
> + VIR_AUTOFREE(char *) snapDir = NULL;
@snapDir is not used
> + VIR_AUTOFREE(char *) snapFile = NULL;
> + VIR_AUTOCLEAN(virBuffer) buf = VIR_BUFFER_INITIALIZER;
>
> - if (virDomainSnapshotGetCurrent(vm->snapshots) == snapshot)
> - flags |= VIR_DOMAIN_SNAPSHOT_FORMAT_CURRENT;
> virUUIDFormat(vm->def->uuid, uuidstr);
> - newxml = virDomainSnapshotDefFormat(uuidstr, def, caps, xmlopt, flags);
> - if (newxml == NULL)
> + if (virDomainSnapshotObjListFormat(&buf, uuidstr, vm->snapshots, caps,
> + xmlopt, flags) < 0)
> return -1;
>
> - if (virAsprintf(&snapDir, "%s/%s", snapshotDir, vm->def->name) < 0)
> - goto cleanup;
> - if (virFileMakePath(snapDir) < 0) {
> - virReportSystemError(errno, _("cannot create snapshot directory '%s'"),
> - snapDir);
> - goto cleanup;
> - }
> -
> - if (virAsprintf(&snapFile, "%s/%s.xml", snapDir, def->common.name) < 0)
> - goto cleanup;
> -
> - ret = virXMLSaveFile(snapFile, NULL, "snapshot-edit", newxml);
> + if (virAsprintf(&snapFile, "%s/%s.xml", snapshotDir, vm->def->name) < 0)
> + return -1;
>
> - cleanup:
> - VIR_FREE(snapFile);
> - VIR_FREE(snapDir);
> - VIR_FREE(newxml);
> - return ret;
> + newxml = virBufferContentAndReset(&buf);
> + return virXMLSaveFile(snapFile, NULL, "snapshot-edit", newxml);
> }
>
> /* The domain is expected to be locked and inactive. Return -1 on normal
> @@ -8589,7 +8572,6 @@ qemuDomainSnapshotDiscard(virQEMUDriverPtr driver,
> bool update_parent,
> bool metadata_only)
> {
> - char *snapFile = NULL;
> int ret = -1;
> qemuDomainObjPrivatePtr priv;
> virDomainMomentObjPtr parentsnap = NULL;
> @@ -8610,10 +8592,6 @@ qemuDomainSnapshotDiscard(virQEMUDriverPtr driver,
> }
> }
>
> - if (virAsprintf(&snapFile, "%s/%s/%s.xml", cfg->snapshotDir,
> - vm->def->name, snap->def->name) < 0)
> - goto cleanup;
> -
So "theoretically" we'll never clean up the old way? Maybe this should
survive and have a prefix of virFileExists before the unlink below? or
some sort of rename for posterity (depends on the opinion about keeping
the historical references).
> if (snap == virDomainSnapshotGetCurrent(vm->snapshots)) {
> virDomainSnapshotSetCurrent(vm->snapshots, NULL);
> if (update_parent && snap->def->parent) {
> @@ -8635,8 +8613,6 @@ qemuDomainSnapshotDiscard(virQEMUDriverPtr driver,
> }
> }
>
> - if (unlink(snapFile) < 0)
> - VIR_WARN("Failed to unlink %s", snapFile);
> if (update_parent)
> virDomainMomentDropParent(snap);
> virDomainSnapshotObjListRemove(vm->snapshots, snap);
> @@ -8644,7 +8620,6 @@ qemuDomainSnapshotDiscard(virQEMUDriverPtr driver,
> ret = 0;
>
> cleanup:
> - VIR_FREE(snapFile);
> virObjectUnref(cfg);
> return ret;
> }
> @@ -8691,7 +8666,7 @@ qemuDomainRemoveInactiveCommon(virQEMUDriverPtr driver,
> virDomainObjPtr vm)
> {
> virQEMUDriverConfigPtr cfg;
> - VIR_AUTOFREE(char *) snapDir = NULL;
> + VIR_AUTOFREE(char *) snapFile = NULL;
>
FWIW: Similar thoughts here about keeping the historical reference and
then "forcing" the user to delete it themselves or provide some sort of
new flag to delete the old historical files.
> cfg = virQEMUDriverGetConfig(driver);
>
> @@ -8699,12 +8674,12 @@ qemuDomainRemoveInactiveCommon(virQEMUDriverPtr driver,
> if (qemuDomainSnapshotDiscardAllMetadata(driver, vm) < 0) {
> VIR_WARN("unable to remove all snapshots for domain %s",
> vm->def->name);
> - } else if (virAsprintf(&snapDir, "%s/%s", cfg->snapshotDir,
> + } else if (virAsprintf(&snapFile, "%s/%s.xml", cfg->snapshotDir,
> vm->def->name) < 0) {
> - VIR_WARN("unable to remove snapshot directory %s/%s",
> + VIR_WARN("unable to remove snapshots storage %s/%s.xml",
> cfg->snapshotDir, vm->def->name);
> - } else if (rmdir(snapDir) < 0 && errno != ENOENT) {
> - VIR_WARN("unable to remove snapshot directory %s", snapDir);
> + } else if (unlink(snapFile) < 0 && errno != ENOENT) {
> + VIR_WARN("unable to remove snapshots storage %s", snapFile);
> }
> qemuExtDevicesCleanupHost(driver, vm->def);
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 9c2245b095..018d1cdc87 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -406,12 +406,14 @@ qemuSecurityInit(virQEMUDriverPtr driver)
> }
>
>
> +/* Older qemu used a series of $dir/snapshot/domainname/snap.xml
> + * files, instead of the modern $dir/snapshot/domainname.xml bulk
> + * file. Called while vm is locked. */
> static int
> -qemuDomainSnapshotLoad(virDomainObjPtr vm,
> - void *data)
> +qemuDomainSnapshotLoadLegacy(virDomainObjPtr vm,
> + char *snapDir,
> + virCapsPtr caps)
> {
> - char *baseDir = (char *)data;
> - char *snapDir = NULL;
> DIR *dir = NULL;
> struct dirent *entry;
> char *xmlStr;
> @@ -424,21 +426,8 @@ qemuDomainSnapshotLoad(virDomainObjPtr vm,
> VIR_DOMAIN_SNAPSHOT_PARSE_DISKS |
> VIR_DOMAIN_SNAPSHOT_PARSE_INTERNAL);
> int ret = -1;
> - virCapsPtr caps = NULL;
> int direrr;
>
> - virObjectLock(vm);
> - if (virAsprintf(&snapDir, "%s/%s", baseDir, vm->def->name) < 0) {
> - virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("Failed to allocate memory for "
> - "snapshot directory for domain %s"),
> - vm->def->name);
> - goto cleanup;
> - }
> -
> - if (!(caps = virQEMUDriverGetCapabilities(qemu_driver, false)))
> - goto cleanup;
> -
> VIR_INFO("Scanning for snapshots for domain %s in %s", vm->def->name,
> snapDir);
>
> @@ -503,6 +492,74 @@ qemuDomainSnapshotLoad(virDomainObjPtr vm,
> _("Snapshots have inconsistent relations for domain %s"),
> vm->def->name);
>
> + virResetLastError();
> +
> + ret = 0;
> + cleanup:
> + VIR_DIR_CLOSE(dir);
> + return ret;
> +}
> +
> +
> +/* Load all snapshots associated with domain */
> +static int
> +qemuDomainSnapshotLoad(virDomainObjPtr vm,
> + void *data)
> +{
> + char *baseDir = (char *)data;
> + unsigned int flags = (VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE |
> + VIR_DOMAIN_SNAPSHOT_PARSE_DISKS);
> + int ret = -1;
> + virCapsPtr caps = NULL;
> + VIR_AUTOFREE(char *) snapFile = NULL;
> + VIR_AUTOFREE(char *) snapDir = NULL;
> + VIR_AUTOFREE(char *) xmlStr = NULL;
> +
> + virObjectLock(vm);
> + VIR_INFO("Scanning for snapshots for domain %s in %s", vm->def->name,
> + baseDir);
> + if (virAsprintf(&snapFile, "%s/%s.xml", baseDir, vm->def->name) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Failed to allocate memory for "
> + "snapshots storage for domain %s"),
> + vm->def->name);
> + goto cleanup;
> + }
> +
> + if (!(caps = virQEMUDriverGetCapabilities(qemu_driver, false)))
> + goto cleanup;
> +
> + if (virFileExists(snapFile)) {
> + /* State last saved by modern libvirt in single file. As each
> + * snapshot contains a <domain>, it can be quite large. */
> + if (virFileReadAll(snapFile, 128*1024*1024*1, &xmlStr) < 0) {
> + /* Nothing we can do here */
> + virReportSystemError(errno,
> + _("Failed to read snapshot file %s"),
> + snapFile);
> + goto cleanup;
> + }
> +
> + ret = virDomainSnapshotObjListParse(xmlStr, vm->def->uuid,
> + vm->snapshots, caps,
> + qemu_driver->xmlopt, flags);
> + if (ret < 0)
> + goto cleanup;
> + VIR_INFO("Read in %d snapshots for domain %s", ret, vm->def->name);
> + } else if (virAsprintf(&snapDir, "%s/%s", baseDir, vm->def->name) >= 0 &&
> + virFileExists(snapDir)) {
> + /* State may have been saved by earlier libvirt; if so, try to
> + * read it in, convert to modern style, and remove the old
> + * directory if successful. */
> + if (qemuDomainSnapshotLoadLegacy(vm, snapDir, caps) < 0)
> + goto cleanup;
> + if (qemuDomainSnapshotWriteMetadata(vm, NULL, caps,
> + qemu_driver->xmlopt, baseDir) < 0)
> + goto cleanup;
> + if (virFileDeleteTree(snapDir) < 0)
> + VIR_WARN("Unable to remove legacy snapshot directory %s", snapDir);
This is where I wonder if it'd be better to keep the historical
reference and then perhaps in the "if" portion of this code do a similar
generation of snapDir, virFileExists (OK, Directory), and then VIR_WARN
that old stuff still exists...
For what's here - sure it seems to do the right thing, but let's be sure
we think we're making the right decision before an R-by, fair enough?
John
> + }
> +
> /* FIXME: qemu keeps internal track of snapshots. We can get access
> * to this info via the "info snapshots" monitor command for running
> * domains, or via "qemu-img snapshot -l" for shutoff domains. It would
> @@ -516,8 +573,6 @@ qemuDomainSnapshotLoad(virDomainObjPtr vm,
>
> ret = 0;
> cleanup:
> - VIR_DIR_CLOSE(dir);
> - VIR_FREE(snapDir);
> virObjectUnref(caps);
> virObjectUnlock(vm);
> return ret;
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[keeping context, because of adding Dan in cc]
On 3/22/19 9:35 AM, John Ferlan wrote:
>
>
> On 3/20/19 5:32 PM, Eric Blake wrote:
>> Rather than one file per snapshot, store all qemu snapshots in a
>> single file, using the recently added bulk snapshot list
>> operations. For now, this doesn't change how often libvirt writes a
>> snapshot file, but it does open the door for the next patch to update
>> the signature to qemuDomainSnapshotWriteMetadata() and call it less
>> frequently. One of the main benefits for doing a bulk write is that
>> you only have to do a single file system write at the end of an
>> operation, rather than potentially 3 during
>> virDomainSnapshotCreateXML(REDEFINE|CURRENT) (delete the old snapshot
>> definition being redefined, rewrite the previous current snapshot to
>> longer be current, and store the new snapshot definition) or even more
>> during virDomainSnapshotDelete(DESCENDENTS) (a file system hit per
>> snapshot being deleted). It also makes it perhaps a bit more feasible
>> to roll back to earlier state if something fails horribly midway
>> through an operation (until you write the new file, the old file is
>> still a reliable record of what state to roll back to), compared to
>> the current code which has to track lots of things locally; although I
>> did not attempt to play with any patches along those lines.
>>
>> Another benefit of the bulk write - it's less code to maintain, and
>> will make it easier for me to model qemu's checkpoint storage in the
>> same way (and for checkpoints, I don't even have to worry about legacy
>> parsing).
>>
>> This is a one-way upgrade - if you have snapshots created by an older
>> libvirt, the new libvirt will correctly load those snapshots and
>> convert to the new format. But as the new libvirt will no longer
>> output the old format, reverting back to the old libvirt will make it
>> appear that all snapshots have disappeared (merely hidden until you
>> upgrade libvirt again). But then again, we've never promised that
>> downgrading libvirt after an upgrade was supposed to work flawlessly.
>
> This kind of information I would think would need to be heavily if not
> overly documented. I assume someone could say this should be "stuck
> behind" a forced decision by the consumer to do the conversion to the
> new style. What worries me most is the issues in the subsequent
> paragraphs that could bite someone.
Indeed, I would need to provide a patch to release notes to match this
commit, if we decide to go with it.
>
> To some degree we could be 'stuck with' the model for snapshots, but
> that doesn't mean checkpoints couldn't make use of some newer
> functionality that stores everything in one file and not allow the
> storage into multiple files.
>
> This is one of those gray areas for me that from an overall
> architectural level that we struggle with related to dealing with our
> technical debt because we've guaranteed some sort of backcompat for
> various things "forever".
It's not the first time where upgrading libvirt performs a one-way of
the old-style internals to the new-style, where you then can't downgrade
libvirt, but at least the bulk of the libvirt code no longer has to
worry about generating both old- and new-style output. But yes, getting
a second opinion won't hurt.
>
> I also wonder what harm it does to keep the old style around and *force*
> the consumer to do the deletion. Whether that's by them running rmdir on
> the old directory or running some libvirt command to clean out the
> legacy. I lean towards leaving the old stuff there - if nothing else it
> provides some strange sort of comfort. Maybe even rename the directory
> to have something like ".old" in the name.
>
> I'm OK with the choice and arguments you make for the algorithm you
> chose, but a ping of Daniel to make sure he's OK with it probably would
> help here, especially since he commented on the last pile and brought
> the issue to the forefront.
added in cc.
>
>>
>> There is a slight chance for confusion if a user named two separate
>> domains 'foo' and 'foo.xml'; the new scheme expects 'foo.xml' to be a
>> regular file for the former domain, while the old scheme expected
>> 'foo.xml' to be a directory for the latter domain; if we are worried
>> about that, we could tweak the code to instead output new state in a
>> file named 'foo' instead of the more typical 'foo.xml' and just key
>> off of whether the file is regular or a directory when deciding if
>> this is the first libvirt run after an upgrade. But I felt that the
>> chances of a user abusing domain names like that is not worth the
>> effort.
>
> I think this is just another thing to document. As in, if you've done
> this and use snapshots, then "fix your stupidity" ;-). Why anyone other
> that QE to test all possibilities would name a domain with ".xml" in the
> name suffix goes beyond common sense to me.
>
>>
>> The bulk snapshot file can be huge (over RPC, we allow <domain> to be
>> up to 4M, and we allow up to 16k snapshots; since each each snapshot
>> includes a <domain>, a worst-case domain would result in gigabytes of
>> bulk data); it is no worse on overall filesystem usage than before,
>> but now in a single file vs. a series of files it requires more memory
>> to read in at once. I don't know if we have to worry about that in
>> practice, but my patch does cap things to read in no more than an
>> arbitrarily-picked 128M, which we may have to raise in the future.
>
> and even more. I think when I first reviewed this I thought of the RPC
> limit stuff - at least w/r/t how the stats code is the other place where
> we've hit this limit. But for some reason I felt that perhaps some under
> the covers code in that code we solved something that allowed for things
> to work, but now I'm not so sure.
>
I don't know if virXML has some way of visiting a seekable stream, and
parsing in only chunks at a time. You still have to PARSE the entire
file to learn where the chunks are, but if the parser is stream based
and keeps track of offsets where interesting chunks start, it is perhaps
feasable that you could cap your memory usage by revisiting chunks as
needed. (But doing that to save memory ALSO means reparsing portions as
you reload them into memory, so you're slower than if you had kept the
full file in memory anyway).
Or maybe we need to think about ways to compress the information (most
of the <domain>s in the overall <snapshotlist> will share a lot of
commonality, even if the user hot-plugged devices between snapshots).
But that's a big project, which I don't want to tackle.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>> ---
>> src/qemu/qemu_domain.c | 59 ++++++++-------------------
>> src/qemu/qemu_driver.c | 93 +++++++++++++++++++++++++++++++++---------
>> 2 files changed, 91 insertions(+), 61 deletions(-)
>>
>> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>> index ea7b31dab3..424f839a00 100644
>> --- a/src/qemu/qemu_domain.c
>> +++ b/src/qemu/qemu_domain.c
>> @@ -8448,45 +8448,28 @@ qemuFindQemuImgBinary(virQEMUDriverPtr driver)
>>
>> int
>> qemuDomainSnapshotWriteMetadata(virDomainObjPtr vm,
>> - virDomainMomentObjPtr snapshot,
>> + virDomainMomentObjPtr snapshot ATTRIBUTE_UNUSED,
>> virCapsPtr caps,
>> virDomainXMLOptionPtr xmlopt,
>> const char *snapshotDir)
>> {
>> - char *newxml = NULL;
>> - int ret = -1;
>> - char *snapDir = NULL;
>> - char *snapFile = NULL;
>> char uuidstr[VIR_UUID_STRING_BUFLEN];
>> - unsigned int flags = VIR_DOMAIN_SNAPSHOT_FORMAT_SECURE |
>> - VIR_DOMAIN_SNAPSHOT_FORMAT_INTERNAL;
>> - virDomainSnapshotDefPtr def = virDomainSnapshotObjGetDef(snapshot);
>> + unsigned int flags = VIR_DOMAIN_SNAPSHOT_FORMAT_SECURE;
>> + VIR_AUTOFREE(char *) newxml = NULL;
>> + VIR_AUTOFREE(char *) snapDir = NULL;
>
> @snapDir is not used
Rebase fluff (I played around with multiple ideas before settling on
this patch); will scrub.
>
>> + VIR_AUTOFREE(char *) snapFile = NULL;
>> + VIR_AUTOCLEAN(virBuffer) buf = VIR_BUFFER_INITIALIZER;
>>
>> - if (virDomainSnapshotGetCurrent(vm->snapshots) == snapshot)
>> - flags |= VIR_DOMAIN_SNAPSHOT_FORMAT_CURRENT;
>> virUUIDFormat(vm->def->uuid, uuidstr);
>> - newxml = virDomainSnapshotDefFormat(uuidstr, def, caps, xmlopt, flags);
>> - if (newxml == NULL)
>> + if (virDomainSnapshotObjListFormat(&buf, uuidstr, vm->snapshots, caps,
>> + xmlopt, flags) < 0)
>> return -1;
>>
>> - if (virAsprintf(&snapDir, "%s/%s", snapshotDir, vm->def->name) < 0)
>> - goto cleanup;
>> - if (virFileMakePath(snapDir) < 0) {
>> - virReportSystemError(errno, _("cannot create snapshot directory '%s'"),
>> - snapDir);
>> - goto cleanup;
>> - }
>> -
>> - if (virAsprintf(&snapFile, "%s/%s.xml", snapDir, def->common.name) < 0)
>> - goto cleanup;
>> -
>> - ret = virXMLSaveFile(snapFile, NULL, "snapshot-edit", newxml);
>> + if (virAsprintf(&snapFile, "%s/%s.xml", snapshotDir, vm->def->name) < 0)
>> + return -1;
>>
>> - cleanup:
>> - VIR_FREE(snapFile);
>> - VIR_FREE(snapDir);
>> - VIR_FREE(newxml);
>> - return ret;
>> + newxml = virBufferContentAndReset(&buf);
>> + return virXMLSaveFile(snapFile, NULL, "snapshot-edit", newxml);
>> }
>>
>> /* The domain is expected to be locked and inactive. Return -1 on normal
>> @@ -8589,7 +8572,6 @@ qemuDomainSnapshotDiscard(virQEMUDriverPtr driver,
>> bool update_parent,
>> bool metadata_only)
>> {
>> - char *snapFile = NULL;
>> int ret = -1;
>> qemuDomainObjPrivatePtr priv;
>> virDomainMomentObjPtr parentsnap = NULL;
>> @@ -8610,10 +8592,6 @@ qemuDomainSnapshotDiscard(virQEMUDriverPtr driver,
>> }
>> }
>>
>> - if (virAsprintf(&snapFile, "%s/%s/%s.xml", cfg->snapshotDir,
>> - vm->def->name, snap->def->name) < 0)
>> - goto cleanup;
>> -
>
> So "theoretically" we'll never clean up the old way? Maybe this should
> survive and have a prefix of virFileExists before the unlink below? or
> some sort of rename for posterity (depends on the opinion about keeping
> the historical references).
Hmm - you have a point. My upgrade path parsed the old way, created the
new way, then tried to clean the old way. But if the attempt to clean
the old way failed, future starts will see only the new way, leaving the
old way to litter the storage. So yeah, opinions on the best logic to
follow are worthwhile.
>
>> if (snap == virDomainSnapshotGetCurrent(vm->snapshots)) {
>> virDomainSnapshotSetCurrent(vm->snapshots, NULL);
>> if (update_parent && snap->def->parent) {
>> @@ -8635,8 +8613,6 @@ qemuDomainSnapshotDiscard(virQEMUDriverPtr driver,
>> }
>> }
>>
>> - if (unlink(snapFile) < 0)
>> - VIR_WARN("Failed to unlink %s", snapFile);
>> if (update_parent)
>> virDomainMomentDropParent(snap);
>> virDomainSnapshotObjListRemove(vm->snapshots, snap);
>> @@ -8644,7 +8620,6 @@ qemuDomainSnapshotDiscard(virQEMUDriverPtr driver,
>> ret = 0;
>>
>> cleanup:
>> - VIR_FREE(snapFile);
>> virObjectUnref(cfg);
>> return ret;
>> }
>> @@ -8691,7 +8666,7 @@ qemuDomainRemoveInactiveCommon(virQEMUDriverPtr driver,
>> virDomainObjPtr vm)
>> {
>> virQEMUDriverConfigPtr cfg;
>> - VIR_AUTOFREE(char *) snapDir = NULL;
>> + VIR_AUTOFREE(char *) snapFile = NULL;
>>
>
> FWIW: Similar thoughts here about keeping the historical reference and
> then "forcing" the user to delete it themselves or provide some sort of
> new flag to delete the old historical files.
I'm open to whatever Dan may suggest.
>
>> cfg = virQEMUDriverGetConfig(driver);
>>
>> @@ -8699,12 +8674,12 @@ qemuDomainRemoveInactiveCommon(virQEMUDriverPtr driver,
>> if (qemuDomainSnapshotDiscardAllMetadata(driver, vm) < 0) {
>> VIR_WARN("unable to remove all snapshots for domain %s",
>> vm->def->name);
>> - } else if (virAsprintf(&snapDir, "%s/%s", cfg->snapshotDir,
>> + } else if (virAsprintf(&snapFile, "%s/%s.xml", cfg->snapshotDir,
>> vm->def->name) < 0) {
>> - VIR_WARN("unable to remove snapshot directory %s/%s",
>> + VIR_WARN("unable to remove snapshots storage %s/%s.xml",
>> cfg->snapshotDir, vm->def->name);
>> - } else if (rmdir(snapDir) < 0 && errno != ENOENT) {
>> - VIR_WARN("unable to remove snapshot directory %s", snapDir);
>> + } else if (unlink(snapFile) < 0 && errno != ENOENT) {
>> + VIR_WARN("unable to remove snapshots storage %s", snapFile);
>> }
>> qemuExtDevicesCleanupHost(driver, vm->def);
>>
>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index 9c2245b095..018d1cdc87 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>> @@ -406,12 +406,14 @@ qemuSecurityInit(virQEMUDriverPtr driver)
>> }
>>
>>
>> +/* Older qemu used a series of $dir/snapshot/domainname/snap.xml
>> + * files, instead of the modern $dir/snapshot/domainname.xml bulk
>> + * file. Called while vm is locked. */
>> static int
>> -qemuDomainSnapshotLoad(virDomainObjPtr vm,
>> - void *data)
>> +qemuDomainSnapshotLoadLegacy(virDomainObjPtr vm,
>> + char *snapDir,
>> + virCapsPtr caps)
>> {
>> - char *baseDir = (char *)data;
>> - char *snapDir = NULL;
>> DIR *dir = NULL;
>> struct dirent *entry;
>> char *xmlStr;
>> @@ -424,21 +426,8 @@ qemuDomainSnapshotLoad(virDomainObjPtr vm,
>> VIR_DOMAIN_SNAPSHOT_PARSE_DISKS |
>> VIR_DOMAIN_SNAPSHOT_PARSE_INTERNAL);
>> int ret = -1;
>> - virCapsPtr caps = NULL;
>> int direrr;
>>
>> - virObjectLock(vm);
>> - if (virAsprintf(&snapDir, "%s/%s", baseDir, vm->def->name) < 0) {
>> - virReportError(VIR_ERR_INTERNAL_ERROR,
>> - _("Failed to allocate memory for "
>> - "snapshot directory for domain %s"),
>> - vm->def->name);
>> - goto cleanup;
>> - }
>> -
>> - if (!(caps = virQEMUDriverGetCapabilities(qemu_driver, false)))
>> - goto cleanup;
>> -
>> VIR_INFO("Scanning for snapshots for domain %s in %s", vm->def->name,
>> snapDir);
>>
>> @@ -503,6 +492,74 @@ qemuDomainSnapshotLoad(virDomainObjPtr vm,
>> _("Snapshots have inconsistent relations for domain %s"),
>> vm->def->name);
>>
>> + virResetLastError();
>> +
>> + ret = 0;
>> + cleanup:
>> + VIR_DIR_CLOSE(dir);
>> + return ret;
>> +}
>> +
>> +
>> +/* Load all snapshots associated with domain */
>> +static int
>> +qemuDomainSnapshotLoad(virDomainObjPtr vm,
>> + void *data)
>> +{
>> + char *baseDir = (char *)data;
>> + unsigned int flags = (VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE |
>> + VIR_DOMAIN_SNAPSHOT_PARSE_DISKS);
>> + int ret = -1;
>> + virCapsPtr caps = NULL;
>> + VIR_AUTOFREE(char *) snapFile = NULL;
>> + VIR_AUTOFREE(char *) snapDir = NULL;
>> + VIR_AUTOFREE(char *) xmlStr = NULL;
>> +
>> + virObjectLock(vm);
>> + VIR_INFO("Scanning for snapshots for domain %s in %s", vm->def->name,
>> + baseDir);
>> + if (virAsprintf(&snapFile, "%s/%s.xml", baseDir, vm->def->name) < 0) {
>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>> + _("Failed to allocate memory for "
>> + "snapshots storage for domain %s"),
>> + vm->def->name);
>> + goto cleanup;
>> + }
>> +
>> + if (!(caps = virQEMUDriverGetCapabilities(qemu_driver, false)))
>> + goto cleanup;
>> +
>> + if (virFileExists(snapFile)) {
>> + /* State last saved by modern libvirt in single file. As each
>> + * snapshot contains a <domain>, it can be quite large. */
>> + if (virFileReadAll(snapFile, 128*1024*1024*1, &xmlStr) < 0) {
>> + /* Nothing we can do here */
>> + virReportSystemError(errno,
>> + _("Failed to read snapshot file %s"),
>> + snapFile);
>> + goto cleanup;
>> + }
>> +
>> + ret = virDomainSnapshotObjListParse(xmlStr, vm->def->uuid,
>> + vm->snapshots, caps,
>> + qemu_driver->xmlopt, flags);
>> + if (ret < 0)
>> + goto cleanup;
>> + VIR_INFO("Read in %d snapshots for domain %s", ret, vm->def->name);
>> + } else if (virAsprintf(&snapDir, "%s/%s", baseDir, vm->def->name) >= 0 &&
>> + virFileExists(snapDir)) {
>> + /* State may have been saved by earlier libvirt; if so, try to
>> + * read it in, convert to modern style, and remove the old
>> + * directory if successful. */
>> + if (qemuDomainSnapshotLoadLegacy(vm, snapDir, caps) < 0)
>> + goto cleanup;
>> + if (qemuDomainSnapshotWriteMetadata(vm, NULL, caps,
>> + qemu_driver->xmlopt, baseDir) < 0)
>> + goto cleanup;
>> + if (virFileDeleteTree(snapDir) < 0)
>> + VIR_WARN("Unable to remove legacy snapshot directory %s", snapDir);
>
> This is where I wonder if it'd be better to keep the historical
> reference and then perhaps in the "if" portion of this code do a similar
> generation of snapDir, virFileExists (OK, Directory), and then VIR_WARN
> that old stuff still exists...
>
> For what's here - sure it seems to do the right thing, but let's be sure
> we think we're making the right decision before an R-by, fair enough?
Indeed.
>
> John
>
>> + }
>> +
>> /* FIXME: qemu keeps internal track of snapshots. We can get access
>> * to this info via the "info snapshots" monitor command for running
>> * domains, or via "qemu-img snapshot -l" for shutoff domains. It would
>> @@ -516,8 +573,6 @@ qemuDomainSnapshotLoad(virDomainObjPtr vm,
>>
>> ret = 0;
>> cleanup:
>> - VIR_DIR_CLOSE(dir);
>> - VIR_FREE(snapDir);
>> virObjectUnref(caps);
>> virObjectUnlock(vm);
>> return ret;
>>
>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Fri, Mar 22, 2019 at 01:07:04PM -0500, Eric Blake wrote: > [keeping context, because of adding Dan in cc] > > To some degree we could be 'stuck with' the model for snapshots, but > > that doesn't mean checkpoints couldn't make use of some newer > > functionality that stores everything in one file and not allow the > > storage into multiple files. I think it would be a really bad idea to use a different approach for snapshots vs checkpoints given how similar they are in every other respect. > > > > This is one of those gray areas for me that from an overall > > architectural level that we struggle with related to dealing with our > > technical debt because we've guaranteed some sort of backcompat for > > various things "forever". > > It's not the first time where upgrading libvirt performs a one-way of > the old-style internals to the new-style, where you then can't downgrade > libvirt, but at least the bulk of the libvirt code no longer has to > worry about generating both old- and new-style output. But yes, getting > a second opinion won't hurt. We've never considered libvirt downgrades to be supported, especially not with running VMs. Downgrades will offline VMs haven't been supported either but have been muuuuuuch less likely to break in general. Since this is in reference to critical VM state info, I'm a bit more concerned about this downgrade issue. If we decide its important though, the only solutions become to not change the format, or to always write both old and new formats which makes the new format rather pointless to support. > >> There is a slight chance for confusion if a user named two separate > >> domains 'foo' and 'foo.xml'; the new scheme expects 'foo.xml' to be a > >> regular file for the former domain, while the old scheme expected > >> 'foo.xml' to be a directory for the latter domain; if we are worried > >> about that, we could tweak the code to instead output new state in a > >> file named 'foo' instead of the more typical 'foo.xml' and just key > >> off of whether the file is regular or a directory when deciding if > >> this is the first libvirt run after an upgrade. But I felt that the > >> chances of a user abusing domain names like that is not worth the > >> effort. > > > > I think this is just another thing to document. As in, if you've done > > this and use snapshots, then "fix your stupidity" ;-). Why anyone other > > that QE to test all possibilities would name a domain with ".xml" in the > > name suffix goes beyond common sense to me. Relying on common sense isn't a good idea. This kind of naming clash is the kind of thing that is useful for exploiting a service to access data you shouldn't be allowed to. I think we should change the naming here to avoid risk of the clash if we're going to do this. > >> The bulk snapshot file can be huge (over RPC, we allow <domain> to be > >> up to 4M, and we allow up to 16k snapshots; since each each snapshot > >> includes a <domain>, a worst-case domain would result in gigabytes of > >> bulk data); it is no worse on overall filesystem usage than before, > >> but now in a single file vs. a series of files it requires more memory > >> to read in at once. I don't know if we have to worry about that in > >> practice, but my patch does cap things to read in no more than an > >> arbitrarily-picked 128M, which we may have to raise in the future. > > > > and even more. I think when I first reviewed this I thought of the RPC > > limit stuff - at least w/r/t how the stats code is the other place where > > we've hit this limit. But for some reason I felt that perhaps some under > > the covers code in that code we solved something that allowed for things > > to work, but now I'm not so sure. > > > > I don't know if virXML has some way of visiting a seekable stream, and > parsing in only chunks at a time. You still have to PARSE the entire > file to learn where the chunks are, but if the parser is stream based > and keeps track of offsets where interesting chunks start, it is perhaps > feasable that you could cap your memory usage by revisiting chunks as > needed. (But doing that to save memory ALSO means reparsing portions as > you reload them into memory, so you're slower than if you had kept the > full file in memory anyway). As soon as we talk about making the stream seekable & parsing chunks for efficiency, this is a red flag to me saying that we should not go down this route. We already have a filesystem layout that lets us read just the individual snapshots that we care about one at a time. Creating this problem by putting all snapshots into a single file and then trying to solve it by creating indexes for chunks inside the file is basically creating a filesystem within a file. > Or maybe we need to think about ways to compress the information (most > of the <domain>s in the overall <snapshotlist> will share a lot of > commonality, even if the user hot-plugged devices between snapshots). > But that's a big project, which I don't want to tackle. On disk storage space is practically free, so I don't see compression being neccessary, especially if you compare the XML size to the actual disk image snapshot data. Overall, I'm struggling to be convinced that putting all snapshots into a single file is a compelling enough think todo. I can see there are some benefits, but there are also benefits to the current per-file approach too. I worry that the bulk file will create efficiency problems later that will require us to go back to per-file impl later, or reinvent filesystems-within-a-file. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 3/25/19 12:19 PM, Daniel P. Berrangé wrote: > On Fri, Mar 22, 2019 at 01:07:04PM -0500, Eric Blake wrote: >> [keeping context, because of adding Dan in cc] >>> To some degree we could be 'stuck with' the model for snapshots, but >>> that doesn't mean checkpoints couldn't make use of some newer >>> functionality that stores everything in one file and not allow the >>> storage into multiple files. > > I think it would be a really bad idea to use a different approach for > snapshots vs checkpoints given how similar they are in every other > respect. > Agreed, which is why deciding what to do here will impact what code I end up using for checkpoints. I will either end up reverting the patches adding low-level bulk code for snapshots in commits 86c0ed6f and 1b57269c (it's been refactored a bit in the meantime, but since we dropped the API additions, there is no current client to the low-level code), or I will have similar low-level bulk code for checkpoints. >>> and even more. I think when I first reviewed this I thought of the RPC >>> limit stuff - at least w/r/t how the stats code is the other place where >>> we've hit this limit. But for some reason I felt that perhaps some under >>> the covers code in that code we solved something that allowed for things >>> to work, but now I'm not so sure. >>> >> >> I don't know if virXML has some way of visiting a seekable stream, and >> parsing in only chunks at a time. You still have to PARSE the entire >> file to learn where the chunks are, but if the parser is stream based >> and keeps track of offsets where interesting chunks start, it is perhaps >> feasable that you could cap your memory usage by revisiting chunks as >> needed. (But doing that to save memory ALSO means reparsing portions as >> you reload them into memory, so you're slower than if you had kept the >> full file in memory anyway). > > As soon as we talk about making the stream seekable & parsing chunks > for efficiency, this is a red flag to me saying that we should not > go down this route. We already have a filesystem layout that lets us > read just the individual snapshots that we care about one at a time. > Creating this problem by putting all snapshots into a single file > and then trying to solve it by creating indexes for chunks inside the > file is basically creating a filesystem within a file. > >> Or maybe we need to think about ways to compress the information (most >> of the <domain>s in the overall <snapshotlist> will share a lot of >> commonality, even if the user hot-plugged devices between snapshots). >> But that's a big project, which I don't want to tackle. > > On disk storage space is practically free, so I don't see compression > being neccessary, especially if you compare the XML size to the actual > disk image snapshot data. > > > > Overall, I'm struggling to be convinced that putting all snapshots into > a single file is a compelling enough think todo. I can see there are > some benefits, but there are also benefits to the current per-file > approach too. I worry that the bulk file will create efficiency problems > later that will require us to go back to per-file impl later, or reinvent > filesystems-within-a-file. One other thing I just thought of - one of the potential advantages I could think of for bulk code was that I only had to write the file system once per API call (rather than multiple times), and in doing so, I was able to track the current snapshot via <snapshots current='name'> rather than via <domainsnapshot>...<active>1</active></domainsnapshot>, so that I no longer could run into an inconsistent situation where various file system fiascos could end up reporting more than one current snapshot. But if maintaining <active> correctly is painful during per-snapshot files, it would be possible to instead store the name (only) of the active snapshot in <domain> on disk, where individual snapshots don't need a per-snapshot <active>. But that still doesn't require a low-level bulk parse/format. Okay, I think that for 5.2, I'm best off reverting the bulk functions as not having any client, and just making sure that checkpoints copy the style of snapshots in tracking one xml file per entity, rather than pursuing this bulk code any further for now. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Remove a now-unused parameter from qemuDomainSnapshotWriteMetadata().
Then, instead of calling it after every individual change to a given
snapshot, call it only once at the end of the API function that
resulted in any overall changes.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
src/qemu/qemu_domain.h | 1 -
src/qemu/qemu_domain.c | 13 ++---------
src/qemu/qemu_driver.c | 50 ++++++++++++++----------------------------
3 files changed, 19 insertions(+), 45 deletions(-)
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index ca24de15e5..5014f62463 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -681,7 +681,6 @@ int qemuDomainLogAppendMessage(virQEMUDriverPtr driver,
const char *qemuFindQemuImgBinary(virQEMUDriverPtr driver);
int qemuDomainSnapshotWriteMetadata(virDomainObjPtr vm,
- virDomainMomentObjPtr snapshot,
virCapsPtr caps,
virDomainXMLOptionPtr xmlopt,
const char *snapshotDir);
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 424f839a00..e8756a0cea 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -8448,7 +8448,6 @@ qemuFindQemuImgBinary(virQEMUDriverPtr driver)
int
qemuDomainSnapshotWriteMetadata(virDomainObjPtr vm,
- virDomainMomentObjPtr snapshot ATTRIBUTE_UNUSED,
virCapsPtr caps,
virDomainXMLOptionPtr xmlopt,
const char *snapshotDir)
@@ -8597,19 +8596,11 @@ qemuDomainSnapshotDiscard(virQEMUDriverPtr driver,
if (update_parent && snap->def->parent) {
parentsnap = virDomainSnapshotFindByName(vm->snapshots,
snap->def->parent);
- if (!parentsnap) {
+ if (!parentsnap)
VIR_WARN("missing parent snapshot matching name '%s'",
snap->def->parent);
- } else {
+ else
virDomainSnapshotSetCurrent(vm->snapshots, parentsnap);
- if (qemuDomainSnapshotWriteMetadata(vm, parentsnap, driver->caps,
- driver->xmlopt,
- cfg->snapshotDir) < 0) {
- VIR_WARN("failed to set parent snapshot '%s' as current",
- snap->def->parent);
- virDomainSnapshotSetCurrent(vm->snapshots, NULL);
- }
- }
}
}
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 018d1cdc87..862d832598 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -553,7 +553,7 @@ qemuDomainSnapshotLoad(virDomainObjPtr vm,
* directory if successful. */
if (qemuDomainSnapshotLoadLegacy(vm, snapDir, caps) < 0)
goto cleanup;
- if (qemuDomainSnapshotWriteMetadata(vm, NULL, caps,
+ if (qemuDomainSnapshotWriteMetadata(vm, caps,
qemu_driver->xmlopt, baseDir) < 0)
goto cleanup;
if (virFileDeleteTree(snapDir) < 0)
@@ -15919,13 +15919,8 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
if (!redefine &&
VIR_STRDUP(snap->def->parent, current->def->name) < 0)
goto endjob;
- if (update_current) {
+ if (update_current)
virDomainSnapshotSetCurrent(vm->snapshots, NULL);
- if (qemuDomainSnapshotWriteMetadata(vm, current,
- driver->caps, driver->xmlopt,
- cfg->snapshotDir) < 0)
- goto endjob;
- }
}
/* actually do the snapshot */
@@ -15972,7 +15967,7 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
if (snapshot && !(flags & VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA)) {
if (update_current)
virDomainSnapshotSetCurrent(vm->snapshots, snap);
- if (qemuDomainSnapshotWriteMetadata(vm, snap, driver->caps,
+ if (qemuDomainSnapshotWriteMetadata(vm, driver->caps,
driver->xmlopt,
cfg->snapshotDir) < 0) {
/* if writing of metadata fails, error out rather than trying
@@ -16499,10 +16494,6 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
current = virDomainSnapshotGetCurrent(vm->snapshots);
if (current) {
virDomainSnapshotSetCurrent(vm->snapshots, NULL);
- if (qemuDomainSnapshotWriteMetadata(vm, current,
- driver->caps, driver->xmlopt,
- cfg->snapshotDir) < 0)
- goto endjob;
/* XXX Should we restore the current snapshot after this point
* in the failure cases where we know there was no change? */
}
@@ -16783,10 +16774,12 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
cleanup:
if (ret == 0) {
virDomainSnapshotSetCurrent(vm->snapshots, snap);
- if (qemuDomainSnapshotWriteMetadata(vm, snap, driver->caps,
- driver->xmlopt,
+ if (qemuDomainSnapshotWriteMetadata(vm, driver->caps, driver->xmlopt,
cfg->snapshotDir) < 0) {
+ /* We've already reverted; not much we can do now */
virDomainSnapshotSetCurrent(vm->snapshots, NULL);
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("unable to save metadata for snapshots"));
ret = -1;
}
} else if (snap) {
@@ -16839,14 +16832,9 @@ qemuDomainSnapshotReparentChildren(void *payload,
VIR_FREE(snap->def->parent);
if (rep->parent->def &&
- VIR_STRDUP(snap->def->parent, rep->parent->def->name) < 0) {
+ VIR_STRDUP(snap->def->parent, rep->parent->def->name) < 0)
rep->err = -1;
- return 0;
- }
- rep->err = qemuDomainSnapshotWriteMetadata(rep->vm, snap,
- rep->caps, rep->xmlopt,
- rep->cfg->snapshotDir);
return 0;
}
@@ -16912,20 +16900,8 @@ qemuDomainSnapshotDelete(virDomainSnapshotPtr snapshot,
&rem);
if (rem.err < 0)
goto endjob;
- if (rem.current) {
+ if (rem.current)
virDomainSnapshotSetCurrent(vm->snapshots, snap);
- if (flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY) {
- if (qemuDomainSnapshotWriteMetadata(vm, snap, driver->caps,
- driver->xmlopt,
- cfg->snapshotDir) < 0) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("failed to set snapshot '%s' as current"),
- snap->def->name);
- virDomainSnapshotSetCurrent(vm->snapshots, NULL);
- goto endjob;
- }
- }
- }
} else if (snap->nchildren) {
rep.cfg = cfg;
rep.parent = snap->parent;
@@ -16949,6 +16925,14 @@ qemuDomainSnapshotDelete(virDomainSnapshotPtr snapshot,
}
endjob:
+ if (ret == 0 &&
+ qemuDomainSnapshotWriteMetadata(vm, driver->caps, driver->xmlopt,
+ cfg->snapshotDir) < 0) {
+ /* We've already deleted everything; not much we can do now */
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("unable to save metadata for snapshots"));
+ ret = -1;
+ }
qemuDomainObjEndJob(driver, vm);
cleanup:
--
2.20.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 3/20/19 5:32 PM, Eric Blake wrote:
> Remove a now-unused parameter from qemuDomainSnapshotWriteMetadata().
> Then, instead of calling it after every individual change to a given
> snapshot, call it only once at the end of the API function that
> resulted in any overall changes.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
> src/qemu/qemu_domain.h | 1 -
> src/qemu/qemu_domain.c | 13 ++---------
> src/qemu/qemu_driver.c | 50 ++++++++++++++----------------------------
> 3 files changed, 19 insertions(+), 45 deletions(-)
>
I guess proper review of this depends on the decision from 17/16. Still
if the decision going forward it to have in one large file, then this
patch appears to be right to me. If there's some pushback that says no
we have to give the consumer a decision to keep saving single snapshot
files, then this cannot be done.
Still in reading this patch, I'm not sure we could keep a hybrid model,
so that swings the pendulum towards let's go with the one file although
the RPC limitation is a concern.
Like I said in the last one - if snapshots continue to use the old way
and checkpoints fall in the other direction of using one file - that's
fine. Sucks to keep two models around though.
Hard to know without empirical data that someone has *that many*
snapshots, but I hate to assume anything at this point in the game
because that bites us hard. Or at least it did with the stats data and
some config w/ more disks than ever thought reasonably possible ;-).
John
> diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
> index ca24de15e5..5014f62463 100644
> --- a/src/qemu/qemu_domain.h
> +++ b/src/qemu/qemu_domain.h
> @@ -681,7 +681,6 @@ int qemuDomainLogAppendMessage(virQEMUDriverPtr driver,
> const char *qemuFindQemuImgBinary(virQEMUDriverPtr driver);
>
> int qemuDomainSnapshotWriteMetadata(virDomainObjPtr vm,
> - virDomainMomentObjPtr snapshot,
> virCapsPtr caps,
> virDomainXMLOptionPtr xmlopt,
> const char *snapshotDir);
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 424f839a00..e8756a0cea 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -8448,7 +8448,6 @@ qemuFindQemuImgBinary(virQEMUDriverPtr driver)
>
> int
> qemuDomainSnapshotWriteMetadata(virDomainObjPtr vm,
> - virDomainMomentObjPtr snapshot ATTRIBUTE_UNUSED,
> virCapsPtr caps,
> virDomainXMLOptionPtr xmlopt,
> const char *snapshotDir)
> @@ -8597,19 +8596,11 @@ qemuDomainSnapshotDiscard(virQEMUDriverPtr driver,
> if (update_parent && snap->def->parent) {
> parentsnap = virDomainSnapshotFindByName(vm->snapshots,
> snap->def->parent);
> - if (!parentsnap) {
> + if (!parentsnap)
> VIR_WARN("missing parent snapshot matching name '%s'",
> snap->def->parent);
> - } else {
> + else
> virDomainSnapshotSetCurrent(vm->snapshots, parentsnap);
> - if (qemuDomainSnapshotWriteMetadata(vm, parentsnap, driver->caps,
> - driver->xmlopt,
> - cfg->snapshotDir) < 0) {
> - VIR_WARN("failed to set parent snapshot '%s' as current",
> - snap->def->parent);
> - virDomainSnapshotSetCurrent(vm->snapshots, NULL);
> - }
> - }
> }
> }
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 018d1cdc87..862d832598 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -553,7 +553,7 @@ qemuDomainSnapshotLoad(virDomainObjPtr vm,
> * directory if successful. */
> if (qemuDomainSnapshotLoadLegacy(vm, snapDir, caps) < 0)
> goto cleanup;
> - if (qemuDomainSnapshotWriteMetadata(vm, NULL, caps,
> + if (qemuDomainSnapshotWriteMetadata(vm, caps,
> qemu_driver->xmlopt, baseDir) < 0)
> goto cleanup;
> if (virFileDeleteTree(snapDir) < 0)
> @@ -15919,13 +15919,8 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
> if (!redefine &&
> VIR_STRDUP(snap->def->parent, current->def->name) < 0)
> goto endjob;
> - if (update_current) {
> + if (update_current)
> virDomainSnapshotSetCurrent(vm->snapshots, NULL);
> - if (qemuDomainSnapshotWriteMetadata(vm, current,
> - driver->caps, driver->xmlopt,
> - cfg->snapshotDir) < 0)
> - goto endjob;
> - }
> }
>
> /* actually do the snapshot */
> @@ -15972,7 +15967,7 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
> if (snapshot && !(flags & VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA)) {
> if (update_current)
> virDomainSnapshotSetCurrent(vm->snapshots, snap);
> - if (qemuDomainSnapshotWriteMetadata(vm, snap, driver->caps,
> + if (qemuDomainSnapshotWriteMetadata(vm, driver->caps,
> driver->xmlopt,
> cfg->snapshotDir) < 0) {
> /* if writing of metadata fails, error out rather than trying
> @@ -16499,10 +16494,6 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
> current = virDomainSnapshotGetCurrent(vm->snapshots);
> if (current) {
> virDomainSnapshotSetCurrent(vm->snapshots, NULL);
> - if (qemuDomainSnapshotWriteMetadata(vm, current,
> - driver->caps, driver->xmlopt,
> - cfg->snapshotDir) < 0)
> - goto endjob;
> /* XXX Should we restore the current snapshot after this point
> * in the failure cases where we know there was no change? */
> }
> @@ -16783,10 +16774,12 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
> cleanup:
> if (ret == 0) {
> virDomainSnapshotSetCurrent(vm->snapshots, snap);
> - if (qemuDomainSnapshotWriteMetadata(vm, snap, driver->caps,
> - driver->xmlopt,
> + if (qemuDomainSnapshotWriteMetadata(vm, driver->caps, driver->xmlopt,
> cfg->snapshotDir) < 0) {
> + /* We've already reverted; not much we can do now */
> virDomainSnapshotSetCurrent(vm->snapshots, NULL);
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("unable to save metadata for snapshots"));
> ret = -1;
> }
> } else if (snap) {
> @@ -16839,14 +16832,9 @@ qemuDomainSnapshotReparentChildren(void *payload,
> VIR_FREE(snap->def->parent);
>
> if (rep->parent->def &&
> - VIR_STRDUP(snap->def->parent, rep->parent->def->name) < 0) {
> + VIR_STRDUP(snap->def->parent, rep->parent->def->name) < 0)
> rep->err = -1;
> - return 0;
> - }
>
> - rep->err = qemuDomainSnapshotWriteMetadata(rep->vm, snap,
> - rep->caps, rep->xmlopt,
> - rep->cfg->snapshotDir);
> return 0;
> }
>
> @@ -16912,20 +16900,8 @@ qemuDomainSnapshotDelete(virDomainSnapshotPtr snapshot,
> &rem);
> if (rem.err < 0)
> goto endjob;
> - if (rem.current) {
> + if (rem.current)
> virDomainSnapshotSetCurrent(vm->snapshots, snap);
> - if (flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY) {
> - if (qemuDomainSnapshotWriteMetadata(vm, snap, driver->caps,
> - driver->xmlopt,
> - cfg->snapshotDir) < 0) {
> - virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("failed to set snapshot '%s' as current"),
> - snap->def->name);
> - virDomainSnapshotSetCurrent(vm->snapshots, NULL);
> - goto endjob;
> - }
> - }
> - }
> } else if (snap->nchildren) {
> rep.cfg = cfg;
> rep.parent = snap->parent;
> @@ -16949,6 +16925,14 @@ qemuDomainSnapshotDelete(virDomainSnapshotPtr snapshot,
> }
>
> endjob:
> + if (ret == 0 &&
> + qemuDomainSnapshotWriteMetadata(vm, driver->caps, driver->xmlopt,
> + cfg->snapshotDir) < 0) {
> + /* We've already deleted everything; not much we can do now */
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("unable to save metadata for snapshots"));
> + ret = -1;
> + }
> qemuDomainObjEndJob(driver, vm);
>
> cleanup:
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Fri, Mar 22, 2019 at 10:49:04AM -0400, John Ferlan wrote: > > > On 3/20/19 5:32 PM, Eric Blake wrote: > > Remove a now-unused parameter from qemuDomainSnapshotWriteMetadata(). > > Then, instead of calling it after every individual change to a given > > snapshot, call it only once at the end of the API function that > > resulted in any overall changes. > > > > Signed-off-by: Eric Blake <eblake@redhat.com> > > --- > > src/qemu/qemu_domain.h | 1 - > > src/qemu/qemu_domain.c | 13 ++--------- > > src/qemu/qemu_driver.c | 50 ++++++++++++++---------------------------- > > 3 files changed, 19 insertions(+), 45 deletions(-) > > > > I guess proper review of this depends on the decision from 17/16. Still > if the decision going forward it to have in one large file, then this > patch appears to be right to me. If there's some pushback that says no > we have to give the consumer a decision to keep saving single snapshot > files, then this cannot be done. > > Still in reading this patch, I'm not sure we could keep a hybrid model, > so that swings the pendulum towards let's go with the one file although > the RPC limitation is a concern. No matter what we do for on-disk storage, we must never expose a bulk query via the API or RPC. This shouldn't be affected by the choice of storage format, though if we never expose bulk format in the APIs, that pushes me to preferring to mirror that in the storage & stick with one file per snapshot. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2026 Red Hat, Inc.