[libvirt] [PATCHv2 19/20] snapshot: qemu: Add support for external snapshot deletion.
Peter Krempa
pkrempa at redhat.com
Fri Nov 9 11:36:29 UTC 2012
On 11/06/12 23:39, Eric Blake wrote:
> On 11/01/2012 10:22 AM, Peter Krempa wrote:
>> This patch adds limited support for deleting external snaphots. The
>
> s/snaphots/snapshots/
>
>> machine must not be active and only whole subtrees of snapshots can be
>> deleted as reparenting was not yet implemented for external snapshots.
>
> These are reasonable restrictions for the first implementation. We may
> relax some of them later - for example, if qemu adds support for doing
> blockcommit from the active layer, then that would let us do a live
> deletion of an external snapshot.
>
>> ---
>> This patch introduces a possible segfault, but I haven't had time to chase it.
>
> Can you describe the steps you reproduced it with? I'm not sure I saw
> it either.
>
>> +++ b/src/qemu/qemu_domain.c
>> @@ -1724,13 +1724,102 @@ qemuDomainSnapshotForEachQcow2(struct qemud_driver *driver,
>> op, try_all, def->ndisks);
>> }
>>
>> -/* Discard one snapshot (or its metadata), without reparenting any children. */
>> -int
>> -qemuDomainSnapshotDiscard(struct qemud_driver *driver,
>> - virDomainObjPtr vm,
>> - virDomainSnapshotObjPtr snap,
>> - bool update_current,
>> - bool metadata_only)
>> +/* Discard one external snapshot (or its metadata), without reparenting any children */
>> +static int
>> +qemuDomainSnapshotDiscardExternal(struct qemud_driver *driver,
>> + virDomainObjPtr vm,
>> + virDomainSnapshotObjPtr snap,
>> + bool update_current,
>> + bool metadata_only)
>> +{
>> + char *snapFile = NULL;
>> + int i;
>> + int ret = -1;
>> + virDomainSnapshotObjPtr parentsnap = NULL;
>> +
>> + if (!metadata_only) {
>> + if (virDomainObjIsActive(vm)) {
>> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>> + _("Can't delete snapshot '%s'. Domain '%s' is active."),
>> + snap->def->name, vm->def->name);
>> + goto cleanup;
>> + }
>> +
>> + if (snap->def->file) {
>> + if (unlink(snap->def->file) < 0) {
>> + virReportSystemError(errno,
>> + _("Failed to remove memory checkpoint "
>> + "save file '%s'"), snap->def->file);
>> + goto cleanup;
>> + }
>> + VIR_FREE(snap->def->file);
>> + }
>
> This part makes sense.
>
>> +
>> + for (i = 0; i < snap->def->ndisks; i++) {
>> + virDomainSnapshotDiskDefPtr disk = &(snap->def->disks[i]);
>> + if (disk->file && unlink(disk->file) < 0) {
>> + virReportSystemError(errno,
>> + _("Failed to remove disk snapshot file '%s'"),
>> + disk->file);
>> + goto cleanup;
>> + }
>> + VIR_FREE(disk->file);
>> + }
>
> But this part is rather drastic. More on this below.
>
>> + }
>> +
>
> From here...
>
>> + if (snap == vm->current_snapshot) {
>> + if (update_current && snap->def->parent) {
>> + parentsnap = virDomainSnapshotFindByName(vm->snapshots,
>> + snap->def->parent);
>> + if (!parentsnap) {
>> + VIR_WARN("missing parent snapshot matching name '%s'",
>> + snap->def->parent);
>> + } else {
>> + parentsnap->def->current = true;
>> +
>> + if (qemuDomainSnapshotWriteMetadata(vm, parentsnap,
>> + driver->snapshotDir) < 0) {
>> + VIR_WARN("failed to set parent snapshot '%s' as current",
>> + snap->def->parent);
>> + parentsnap->def->current = false;
>> + parentsnap = NULL;
>> + }
>> + }
>> + }
>> + vm->current_snapshot = parentsnap;
>> + }
>> +
>> + if (virAsprintf(&snapFile, "%s/%s/%s.xml", driver->snapshotDir,
>> + vm->def->name, snap->def->name) < 0) {
>> + virReportOOMError();
>> + goto cleanup;
>> + }
>> +
>> + if (unlink(snapFile) < 0)
>> + VIR_WARN("Failed to unlink %s", snapFile);
>> + virDomainSnapshotObjListRemove(vm->snapshots, snap);
>
> ...to here, the code is almost identical to the internal snapshot case,
> except that you reordered things so that they are unsafe (that is, the
> internal case ensures that we malloc in order to detect any OOM _prior_
> to modifying any parent snapshot). I think the tail of these two
> functions should instead be common functionality...
>
>> +
>> + ret = 0;
>> +
>> +cleanup:
>> + if (ret < 0 && !snapFile &&
>> + qemuDomainSnapshotWriteMetadata(vm, snap, driver->snapshotDir) < 0)
>> + VIR_WARN("Failed to store modified snapshot config");
>> +
>> + VIR_FREE(snapFile);
>> +
>> + return ret;
>> +}
>> +
>> +/* Discard one internal snapshot (or its metadata),
>> + * without reparenting any children.
>> + */
>> +static int
>> +qemuDomainSnapshotDiscardInternal(struct qemud_driver *driver,
>> + virDomainObjPtr vm,
>> + virDomainSnapshotObjPtr snap,
>> + bool update_current,
>> + bool metadata_only)
>> {
>> char *snapFile = NULL;
>> int ret = -1;
>> @@ -1791,6 +1880,25 @@ cleanup:
>> return ret;
>> }
>>
>> +/* Discard one snapshot (or its metadata), without reparenting any children. */
>> +int
>> +qemuDomainSnapshotDiscard(struct qemud_driver *driver,
>> + virDomainObjPtr vm,
>> + virDomainSnapshotObjPtr snap,
>> + bool update_current,
>> + bool metadata_only)
>> +{
>> + int ret;
>> +
>> + if (virDomainSnapshotIsExternal(snap))
>> + ret = qemuDomainSnapshotDiscardExternal(driver, vm, snap, update_current, metadata_only);
>> + else
>> + ret = qemuDomainSnapshotDiscardInternal(driver, vm, snap, update_current, metadata_only);
>
> ...here.
>
>> +
>> + return ret;
>> +}
>> +
>> +
>> /* Hash iterator callback to discard multiple snapshots. */
>> void qemuDomainSnapshotDiscardAll(void *payload,
>> const void *name ATTRIBUTE_UNUSED,
>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index 44bf6dc..46b7e3a 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>> @@ -12376,6 +12376,13 @@ qemuDomainSnapshotReparentChildren(void *payload,
>> return;
>> }
>>
>> + if (virDomainSnapshotIsExternal(snap)) {
>> + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
>> + _("Reparenting of external snapshots is not implemented"));
>> + rep->err = -1;
>> + return;
>> + }
>
> Why not? If I have the snapshot chain:
>
> internal_1 <- internal_2 <- external
>
> and delete just internal_2, why can't I reparent external to point to
> internal_1? I'm wondering if the memory corruption is happening here by
> returning failure in part of the tree, while the rest of the tree
> assumes success, so that the end result is a corrupted tree that points
> into free'd storage.
>
>> +
>> VIR_FREE(snap->def->parent);
>> snap->parent = rep->parent;
>>
>> @@ -12433,10 +12440,20 @@ static int qemuDomainSnapshotDelete(virDomainSnapshotPtr snapshot,
>> virDomainSnapshotForEachDescendant(snap,
>> qemuDomainSnapshotCountExternal,
>> &external);
>> - if (external) {
>> + if (external &&
>> + virDomainObjIsActive(vm)) {
>> virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>> - _("deletion of %d external disk snapshots not "
>> - "supported yet"), external);
>> + _("deletion of %d external disk snapshots is "
>> + "supported only on inactive guests"), external);
>> + goto cleanup;
>> + }
>
> Like I said earlier, this may be a reasonable first round, since we
> can't do live blockcommit yet.
>
> However, I think we're missing a bigger picture here. Remember, with
> internal snapshots, the way things work is that the same file holds both
> the snapshot (a point in time) and the delta (all changes since that
> time). Right now, the action of deleting a snapshot says to forget
> about the point in time, but preserve all changes since that point in
> time, if those changes belong to either the current state or to another
> branch of the snapshot hierarchy. The only time data is actually
> deleted is if it is not in current use and there is no other snapshot
> remaining that can get back to the data.
>
> But above, you just blindly unlink()d the qcow2 wrapper file, which in
> effect discarded the delta (all changes since the snapshot), and
> _reverted_ us back to the point in time where the snapshot was taken.
I was too carried away about wiring up the revert code that this also
ended up to be a form of revert.
> Probably not what the user meant; or if it is, this is a NEW mode of
> operation, so it should only be allowed if the user passes in a new
> flag, maybe named VIR_DOMAIN_SNAPSHOT_DELETE_REVERT, which says to
> delete the external snapshot files _and_ revert to the state at the time
> it was taken. In fact, this particular mode of operation might fit
> better with virDomainRevertToSnapshot instead of virDomainSnapshotDelete
> (again, controlled by a flag), but let's first come to an agreement on
> the scenarios that users might want...
Yes that will be definitely better to have it in the revert API.
>
> 1. I created a disk snapshot, and it created new qcow2 files. Now I no
> longer need to go to that point in time, but I want to keep the qcow2
> files, without losing my changes since the snapshot. Solution: 'virsh
> snapshot-delete --metadata' merely forget that we took the snapshot, but
> keep the backing chain as-is.
This should work as it is right now, unfortunately it leaves behind the
mess of files that are/were part of the backing chain.
> 2. I created a disk snapshot, but now I no longer need to go back, and I
> want my disk chain as short as it was before I created the snapshot, and
> without losing my changes since the snapshot. Solution: use 'qemu-img
> commit' to squash the delta back into the original file, then delete the
> backing file as no longer necessary. If any further snapshots depended
> on the backing file, then they also need a touchup (reparenting) to deal
> with the change in the backing chain, or else they must also be deleted.
> 'qemu-img commit' can only be run while the disk is offline; worse, it
> is time-consuming, so we now have to deal with the fact that deleting a
> snapshot probably ought to be an async job with status updates, rather
> than a blocking job. But if we implement this solution, we can also use
> it for 'virsh blockcommit' for offline commits. This mode most closely
> matches what we do when deleting an internal snapshot.
I'm not sure if this is something we want to do entirely in libvirt.
When committing into the original image (or other part of the backing
chain) you might invalidate a subtree that will be created with the
>
> 3. I created an external checkpoint, and now I no longer need to go that
> point in time. Solution: Delete the memory state file, then all we are
> left with is a disk snapshot, so we can now use scenario 1 or 2 to clean
> up the rest.
Agreed. When you have a separate copy at every snapshot all things get
easier. Unfortunately we can't do that with disk images :)
>
> 4. I created a disk snapshot, then did some experiments, and decided I
> don't need the result and want to go back to that point in time without
> remembering the experiments. Solution: combination revert and delete,
> where we reset the domain to use the backing file and delete the qcow2
> wrapper files.
>
> 5. I created an external checkpoint of a running machine, and now want
> to go back to that point in time. Solution: like 4, this is a
> combination revert and delete, where we delete qcow2 wrapper files, but
> we also boot using the saved memory image and backing files.
This is the original functionality I wanted to implement in patch 20/20.
>
> 6. I created an external checkpoint, did some experiments, and want to
> remember those results; but I also want to go back to the checkpoint and
> do some more experiments on a different branch. Solution: here, we need
> the ability to create NEW qcow2 files that also wrap the common base
> image. Since virDomainRevertToSnapshot doesn't have a way for us to
> pass in the new file names we wish to use, this needs to go through
> virDomainSnapshotCreateXML, which needs a new flag that says that we are
> simultaneously reverting to the state of an existing snapshot and
> creating a new snapshot from that point rather than at the current state
> of execution.
I saw your submission. But this will probably have to be a 2 step procedure:
1) create a snapshot at the original branch to have a point to return to
2) create the branch and revert to that point in time
Otherwise it will be hard to return to that original branch. Basically
we need some kind of "current" snapshot in each of the branches.
Technically that should be easy for a management application, but it
won't be that comfortable to do in virsh.
>
> 7. Once we allow creation of snapshot forks from a common base point, we
> need to worry about deleting one fork but keeping other forks alive -
> for an external checkpoint, the memory file is now effectively shared by
> multiple snapshots, and must not be deleted until the last snapshot
> referencing the file is deleted.
And also we just can't commit one of the forks into the base image as
the other fork would be invalidated. We'd need to pull that changes into
the children.
>
> 8. It is also possible to use multiple snapshots to create a longer
> chain, such as 'base <- snap1 <- snap2 <- current', and then delete
> snap1 while still remembering snap2. Right now, we can use blockcommit
> while qemu is live (and a sequence of qemu-img commit and qemu-img
> rebase calls for offline) to merge snap1 into base; we could also use
> blockpull (via qemu-img rebase while offline, but currently lacking
> support if qemu is running) to pull snap1 into snap2. That is,
> forbidding the user from deleting an external snapshot unless they also
> delete descendants is not necessarily desirable.
>
> I guess I've turned this more into a brain dump of where we should be
> headed - there is really a lot of functionality involved in reverting to
> external files, and we should be exposing choices to the user whether to
> pull into current, push into backing, discard files, or create parallel
> branches from the same starting point; and solutions to some of these
> issues touch not only snapshot deletion, but also snapshot revert and
> snapshot create. For this particular patch, though, I think we
> definitely want to introduce a new flag that says when we are doing a
> combined revert and delete, so that we can leave the option of deletion
> without losing current state (similar to how internal snapshot deletion
> works) until a later patch where we use qemu-img commit to do the work.
>
I will turn the relevant parts of this patch to the revert API as it's
semantically closer to that functionality.
Peter
More information about the libvir-list
mailing list