[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 5/4] qemu: properly revert to offline snapshots



On 08/10/2011 12:24 AM, Philipp Hahn wrote:
Hello Eric,

you're patch looks very similar to mine, which I created myself yesterday, but
hadn't had time to actually send after doing the testing. I'll attach it just
FYI.

On Wednesday 10 August 2011 01:28:42 Eric Blake wrote:
qemuDomainSnapshotRevertInactive has the same FIXMEs as
qemuDomainSnapshotCreateInactive, so algorithmic fixes to properly
handle partial loop iterations should be applied later to both
functions, but we're not making the situation any worse in
this patch.

* src/qemu/qemu_driver.c (qemuDomainRevertToSnapshot): Use
qemu-img rather than 'qemu -loadvm' to revert to offline snapshot.
(qemuDomainSnapshotRevertInactive): New helper.

s/offline/inactive/ just for consistency.

Sure.


diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
...
@@ -8846,7 +8893,7 @@ static int
qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
              }
          }

-        if (qemuDomainSnapshotSetCurrentActive(vm, driver->snapshotDir)<  0)
+        if (qemuDomainSnapshotRevertInactive(vm, snap)<  0)

Now you don't call qemuDomainSnapshotSetCurrent() any more, which marks the
snapshot at being current (what ever that is important for).
That's why I in my patch also have to change qemuBuildCommandLine() to not
add -loadvm, since qemu then will refuse to start.

Right now, qemu's _only_ use of <active> (which is set to non-zero by qemuDomainSnapshotSetCurrentActive) is to tell qemuBuildCommandLine to use -loadvm to revert to that point, rather than to do a fresh boot. Your approach of keeping the call means that qemuBuildCommandLine has to special-case to not use -loadvm for inactive snapshots; whereas my approach of no longer marking the snapshot active for inactive snapshots means that the existing qemuBuildCommandLine no longer sees an active snapshot in the first place; both approaches end up omitting the -loadvm call on the next domain boot, but mine does it with less code and without using <active>.

Meanwhile, I have plans to repurpose the <active> element. Right now, it is stored as a long, but only ever contains the value 0 or 1 as used by qemu (it is not used by other drivers), and is used by at most one snapshot at a time. But since we _want_ to fix the bug where libvirtd forgets the current snapshot when it is restarted, I propose that active be repurposed slightly, as well as changed to int or even bool, since long is overkill:

qemuBuildCommandLine is already being passed current_snapshot or NULL; that should be sufficient to decide whether to add the -loadvm argument without also probing active; so all callers should be updated to pass NULL except for reverting from an active snapshot when qemu is not already running - this means a signature tweak to qemuProcessStart.

Once that is done, qemu is no longer using active for its own purposes, so conf/domain_conf can then take over <active> to uniformly identify which snapshot is current as part of the snapshot xml (that is, all but one snapshot will be <active>0</active>, and the current snapshot will be <active>1</active>). Every time the current snapshot changes, this implies updating as many as two files in the snapshot xml directory. <active> is already an internal-only xml element - domain_conf ignores it on user input, and strips it on dumpxml output visible to the user, so it is only present in the internal xml files. The idea is that if there is at least one snapshot, then there generally be a current snapshot; the only time that there is not a current snapshot but where snapshots still exist is when you create multiple trees in the hierarchy by setting the current snapshot to the root of the hierarchy, then delete that snapshot while keeping its children intact (making each child an independent root).

More patches coming later today along these lines, once I test it all.

Another goal of mine is to introduce 'virsh snapshot-list dom --tree', which shows snapshot names with a visual hierarchy relationship:

root
  child1
    grandchild1.1
    grandchild1.2
  child2
    grandchild2.1
    grandchild2.2

That would be made much easier if we had an API for virDomainSnapshotGetParentName (see also my wish for virDomainSnapshotGetName in patch 6/4); but it can also be done with xml scraping using existing API.

--
Eric Blake   eblake redhat com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]