[libvirt] [PATCH 18/16] snapshot: Consolidate writing snapshot state to disk
John Ferlan
jferlan at redhat.com
Fri Mar 22 14:49:04 UTC 2019
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 at 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:
>
More information about the libvir-list
mailing list