Re: [Qemu-devel] [libvirt] [PATCH] snapshot: fix rollback failure in transaction mode

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3102 bytes --]

On 09/12/2012 12:33 PM, Jeff Cody wrote:

> If QEMU creates the snapshot file, upon transaction failure it is
> possible to have a newly created image file left, depending on when the
> failure occurs.  The running QEMU instance, however, will not be
> affected.
> 
> For instance, if we are performing qcow2 snapshots on 2 drives using the
> transaction command (e.g. drive0 and drive1), we will:
> 
> 1. create qcow2 image file for drive0 new active layer
> 2. create qcow2 image file for drive1 new active layer
> 3. If 1 & 2 were successful, then we modify the live image chain
>    structure in memory to use the newly created files.  Otherwise, we
>    abandon the change, notify libvirt of the error, and leave any newly
>    created files intact.
> 
> That means, on a snapshot failure, QEMU's live running operation will
> not be affected, but the management software (libvirt) should clean up
> any resulting image files, if appropriate.
> 
> It sounds like you expect QEMU to unlink any of the newly created
> snapshot files on failure - is that an accurate statement?

A non-empty file is being left behind by the transaction command, and it
seems like whoever did the successful open(O_CREAT) of that file should
then be tasked with the unlink() of the same file on the failure path.
But it's not quite as simple as having qemu unlink() the file.
Remember, with libvirt, we have to pre-create a 0-byte file, in order to
set appropriate SELinux labels, prior to telling qemu to make the
snapshot.  Remember, qemu is doing open(O_CREAT) but not
open(O_CREAT|O_EXCL), and has no idea whether it created a new file or
is merely reusing (and truncating) an existing file, so in a way, I'm
arguing that libvirt should be doing the unlink() if it handed a
pre-created file into qemu.

But even if unlink() on 'transaction' failure in qemu is asking too
much, it might be nice of qemu to ftruncate() the file back to 0 bytes
to return it to the state it had pre-'transaction'; any operation that
claims to have full rollback, but leaves altered state behind (even if
the altered state doesn't affect qemu operation), just seems odd.  And
while libvirt always hands in a pre-created empty file, there's still
the question of how qemu should behave when operated manually without
SELinux in the way and without libvirt driving things, where an unlink()
in qemu may actually make sense.

At any rate, you are confirming that qemu 1.1 leaves a non-empty file
behind, so even if we change qemu 1.3 to truncate the file back to 0
bytes on failure, libvirt still has to cope with the older behavior.  So
it's now up to the qemu folks to decide whether this is a bug worth
fixing, or merely an annoyance worth documenting, or even something
worth completely ignoring and throwing back at libvirt.  On libvirt's
side, Guannan's patch looks correct, although it needs better commentary
in the commit message and/or code before being applied.

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 617 bytes --]