Re: [libvirt] [PATCH 17/16] snapshot: Store qemu snapshot state in bulk

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(a)redhat.com&gt; --- 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;