Re: [libvirt] [PATCHv4 4/4] snapshot: qemu: Implement reverting of external snapshots

On 11/21/2012 07:16 AM, Peter Krempa wrote: Yes, but if you were careful to shut down first before leaving a branch, that's what you want; and if you (management app) took a snapshot prior to leaving a branch, then you would revert to that NEW snapshot, rather than using revert-with-follow to the old snapshot. That's where the already-existing REVERT_RUNNING and REVERT_PAUSED flags come into play - revert-with-follow would default to shut off, but you can request that the guest be booted from the current disk state. Rather, the result of using this flag would be as if the automatic meta-snapshot was always a disk snapshot. If you leave an offline branch, all is well; if you leave an online branch, then the disk may have lost pending I/O (so don't revert from a running machine, if you want to return to that branch). The idea of doing an automatic meta-snapshot prior to leaving a branch may still make sense, but would it be internal or external? Offline snapshots (whether internal or external) are relatively fast; but online snapshots take time no matter whether they are internal or external (with internal having the further complication of having to use qcow2 disks). I guess we'll have to continue thinking about automatic snapshots before leaving a branch, but it doesn't affect the usefulness of your patches so far. Hmm, maybe we don't need a flag here after all for your patch. Thinking about the internal snapshot case, it's important to remember that there is always an implicit state (the current), as well as any number of named states in a qcow2 file. Each cluster has a reference count based on how many entry states can reach that cluster (with copy-on-write semantics any time the current state goes to write to a cluster). Consider the following setup of snapshots (in 'virsh snapshot-list --tree' format): a | +- b | +- c | | | +- d | +- e created by taking snapshot 'a', taking snapshot 'b', taking snapshot 'c', taking snapshot 'd', reverting to snapshot 'b', then taking snapshot 'e'. The action of reverting to 'b' did NOT invalidate snapshots c or d, but did discard any state changes that happened after 'd'. Another way of looking at this is that reverting to 'b' really means changing the implicit 'current' tip to initially be identical to 'b'. Looking at your patch, you are refusing to revert to anything but a leaf snapshot; that's good, because unlike the internal case, the moment we wipe an external file, we have lost any internal snapshots it may contain, and we have also invalidated any further external snapshots that use it as a backing file. So, your code will permit a revert to exactly one of two places: 'd' (if already executing on branch c/d) and 'e' (if already executing on branch e), and if there is no automatic snapshot taken at the tip, then the user is already throwing away running state, the same as they would be for reverting to an internal snapshot. Okay, I think you've convinced me that your patch doesn't need a flag, and that it matches the semantics of internal revert, or else fails because it can't match the semantics (at which point, you can use --force to throw things away, or my revert-and-commit to create a new branch from arbitrary points rather than leaf snapshots). Well, here we could recreate the checkpoint at the time of the new revert. After all, a commit operation (without a discard) says that we don't need to go back to the earlier point in time, but we still want to remember something at the current point in time. Also, this operation would be time-consuming (disk commits are slower than creating new qcow2 wrappers), so it would need to be under an async job and allow cancellation (but what does that mean to the snapshot if you cancel half-way through?). Not necessarily sugar - if you follow my argument about COMMIT needing to re-create the checkpoint state to the current point in time without this flag, then using this flag can omit that recreation step and definitely save time. But we can worry about that more when we actually get around to writing a COMMIT implementation. So the --force flag does let the user invalidate other snapshots/backing file chains/whatever, but in that case they asked for it. Meanwhile, since your version can only revert to leaf snapshots, and my revert-and-create can revert anywhere (to create a branch), and my proposed revert-and-follow flag can then switch branches (but up to the management to ensure sane state before leaving a branch), I think we are doing pretty good on the revert front. I'll post a rebase of your patch series merged with mine (as I'm finding I need some of your code to implement my revert-and-follow stuff). Indeed, now that I think about it more: via the public API, we reject things up-front if the image file already exists; and since our backdoor didn't repeat the check, then we get the right behavior for both the public API (the file doesn't exist, so create it as empty) and your revert API (forcefully recreate the file as empty). Cool! Updated plan of attack - I'll repost your series with the final tweaks I think it needs as part of my series, and you can take that as my ACK of your code. So, I'm now working on the revert-with-follow semantics, and am assuming that you are working on the delete code. -- Eric Blake eblake(a)redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org