10:22 a.m.
After failure of qemu transaction command, the snapshot file still
be there with non-zero in size. In order to unlink the file, the
patch removes the file size checking.
---
src/qemu/qemu_driver.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 8e8e00c..1fedfb8 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -10833,7 +10833,7 @@ qemuDomainSnapshotUndoSingleDiskActive(struct qemud_driver
*driver,
if (virDomainLockDiskDetach(driver->lockManager, vm, disk) < 0)
VIR_WARN("Unable to release lock on %s", disk->src);
if (need_unlink && stat(disk->src, &st) == 0 &&
- st.st_size == 0 && S_ISREG(st.st_mode) && unlink(disk->src)
< 0)
+ S_ISREG(st.st_mode) && unlink(disk->src) < 0)
VIR_WARN("Unable to remove just-created %s", disk->src);
/* Update vm in place to match changes. */
--
1.7.11.2
12:47 p.m.
On 09/12/2012 09:22 AM, Guannan Ren wrote:
After failure of qemu transaction command, the snapshot file still
be there with non-zero in size. In order to unlink the file, the
patch removes the file size checking.
Can you give some exact steps to reproduce this, so that I know who is
making the file have non-zero size? I'm worried that unlinking a
non-empty file is discarding data, so the commit message needs a lot
more details about how we are proving that the only way the file can be
non-zero size is because qemu happened to put data into a previously
empty file prior to the failed 'transaction' attempt.
That is, after re-reading context code just now, I'm fairly confident
that this code can only be reached when qemu supports the 'transaction'
monitor command, and libvirt's --reuse-ext flag was not specified, and
therefore libvirt must have just created the file. But I want that in
the commit message rather than having to re-read the code every time I
visit this commit in future reads of the git log. It may also be that
qemu has a bug in that the 'transaction' command is modifying files even
when it fails, so even while this works around the bug, I'm cc'ing Jeff
to see if qemu also needs a bug fix.
---
src/qemu/qemu_driver.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 8e8e00c..1fedfb8 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -10833,7 +10833,7 @@ qemuDomainSnapshotUndoSingleDiskActive(struct qemud_driver
*driver,
if (virDomainLockDiskDetach(driver->lockManager, vm, disk) < 0)
VIR_WARN("Unable to release lock on %s", disk->src);
if (need_unlink && stat(disk->src, &st) == 0 &&
- st.st_size == 0 && S_ISREG(st.st_mode) && unlink(disk->src)
< 0)
+ S_ISREG(st.st_mode) && unlink(disk->src) < 0)
VIR_WARN("Unable to remove just-created %s", disk->src);
/* Update vm in place to match changes. */
--
Eric Blake eblake(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
1:33 p.m.
On 09/12/2012 01:47 PM, Eric Blake wrote:
On 09/12/2012 09:22 AM, Guannan Ren wrote:
> After failure of qemu transaction command, the snapshot file still
> be there with non-zero in size. In order to unlink the file, the
> patch removes the file size checking.
Can you give some exact steps to reproduce this, so that I know who is
making the file have non-zero size? I'm worried that unlinking a
non-empty file is discarding data, so the commit message needs a lot
more details about how we are proving that the only way the file can be
non-zero size is because qemu happened to put data into a previously
empty file prior to the failed 'transaction' attempt.
That is, after re-reading context code just now, I'm fairly confident
that this code can only be reached when qemu supports the 'transaction'
monitor command, and libvirt's --reuse-ext flag was not specified, and
therefore libvirt must have just created the file. But I want that in
the commit message rather than having to re-read the code every time I
visit this commit in future reads of the git log. It may also be that
qemu has a bug in that the 'transaction' command is modifying files even
when it fails, so even while this works around the bug, I'm cc'ing Jeff
to see if qemu also needs a bug fix.
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?
> ---
> src/qemu/qemu_driver.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 8e8e00c..1fedfb8 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -10833,7 +10833,7 @@ qemuDomainSnapshotUndoSingleDiskActive(struct qemud_driver
*driver,
> if (virDomainLockDiskDetach(driver->lockManager, vm, disk) < 0)
> VIR_WARN("Unable to release lock on %s", disk->src);
> if (need_unlink && stat(disk->src, &st) == 0 &&
> - st.st_size == 0 && S_ISREG(st.st_mode) &&
unlink(disk->src) < 0)
> + S_ISREG(st.st_mode) && unlink(disk->src) < 0)
> VIR_WARN("Unable to remove just-created %s", disk->src);
>
> /* Update vm in place to match changes. */
>
3:23 p.m.
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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
10:16 p.m.
On 09/13/2012 01:47 AM, Eric Blake wrote:
On 09/12/2012 09:22 AM, Guannan Ren wrote:
> After failure of qemu transaction command, the snapshot file still
> be there with non-zero in size. In order to unlink the file, the
> patch removes the file size checking.
Can you give some exact steps to reproduce this, so that I know who is
making the file have non-zero size? I'm worried that unlinking a
non-empty file is discarding data, so the commit message needs a lot
more details about how we are proving that the only way the file can be
non-zero size is because qemu happened to put data into a previously
empty file prior to the failed 'transaction' attempt.
qemu left non-empty file.
Steps:
1, Create a qemu instance with two drive images of qcow2 type
(root user)
/usr/libexec/qemu-kvm -m 1024 -smp 1 -name "rhel6u1" \
-drive
file=/var/lib/libvirt/images/firstqcow2,if=none,id=drive-virtio-disk0,format=qcow2,cache=none
-device
virtio-blk-pci,scsi=off,bus=pci.0,addr=0x5,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1
\
-drive
file=/var/lib/libvirt/images/secondqcow2,if=none,id=drive-virtio-disk1,format=qcow2,cache=none
\
-device
virtio-blk-pci,scsi=off,bus=pci.0,addr=0x7,drive=drive-virtio-disk1,id=virtio-disk1
-qmp stdio
2, Initialize qemu qmp
{"execute":"qmp_capabilities"}
3, Remove the second drive image file
rm -f /var/lib/libvirt/images/secondqcow2
4, Run 'transaction' command with snapshot qemu commands in.
{"execute":"transaction","arguments":
{"actions":
[{"type":"blockdev-snapshot-sync","data":{"device":"drive-virtio-disk0","snapshot-file":"/var/lib/libvirt/images/firstqcow2-snapshot.img","format":"qcow2"}},
{"type":"blockdev-snapshot-sync","data":{"device":"drive-virtio-disk1","snapshot-file":"/var/lib/libvirt/images/secondqcow2-snapshot.img","format":"qcow2"}}]
},
"id":"libvirt-6"}
5, Got the error as follows:
{"id": "libvirt-6", "error": {"class":
"OpenFileFailed",
"desc": "Could not open
'/var/lib/libvirt/images/secondqcow2-snapshot.img'",
"data": {"filename":
"/var/lib/libvirt/images/secondqcow2-snapshot.img"}}}
6, List first newly-created snapshot file:
-rw-r--r--. 1 root root 262144 Sep 13 11:43
firstqcow2-snapshot.img
That is, after re-reading context code just now, I'm fairly confident
that this code can only be reached when qemu supports the 'transaction'
monitor command, and libvirt's --reuse-ext flag was not specified, and
therefore libvirt must have just created the file. But I want that in
the commit message rather than having to re-read the code every time I
visit this commit in future reads of the git log.
That's ok, I will add this information into commit log in v2.
It may also be that
qemu has a bug in that the 'transaction' command is modifying files even
when it fails, so even while this works around the bug, I'm cc'ing Jeff
to see if qemu also needs a bug fix.
4:41 a.m.
New subject: [libvirt] [PATCH v2] snapshot: fix rollback failure in transaction mode
BZ:https://bugzilla.redhat.com/show_bug.cgi?id=843372
when qemu supports the 'transaction' monitor command,
and libvirt's --reuse-ext flag was not specified, libvirt created
a stub file with zero size in first place. After the failure of
QEMU transaction command performing qcow2 snapshots on more than
one drives, the stub file is left behind with non-empty
by the QEMU transaction command.
In order to unlink the file, the patch removes the file size checking.
---
src/qemu/qemu_driver.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index a410521..d11bd19 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -10833,7 +10833,7 @@ qemuDomainSnapshotUndoSingleDiskActive(struct qemud_driver
*driver,
if (virDomainLockDiskDetach(driver->lockManager, vm, disk) < 0)
VIR_WARN("Unable to release lock on %s", disk->src);
if (need_unlink && stat(disk->src, &st) == 0 &&
- st.st_size == 0 && S_ISREG(st.st_mode) && unlink(disk->src)
< 0)
+ S_ISREG(st.st_mode) && unlink(disk->src) < 0)
VIR_WARN("Unable to remove just-created %s", disk->src);
/* Update vm in place to match changes. */
--
1.7.11.2
10:42 a.m.
New subject: [libvirt] [PATCH v2] snapshot: fix rollback failure in transaction mode
On 09/13/2012 03:41 AM, Guannan Ren wrote:
BZ:https://bugzilla.redhat.com/show_bug.cgi?id=843372
when qemu supports the 'transaction' monitor command,
and libvirt's --reuse-ext flag was not specified, libvirt created
a stub file with zero size in first place. After the failure of
QEMU transaction command performing qcow2 snapshots on more than
one drives, the stub file is left behind with non-empty
by the QEMU transaction command.
In order to unlink the file, the patch removes the file size checking.
I was still hoping you'd list the reproduction steps here in the commit
message, rather than making me chase down a link. Remember, a year down
the road, when reading 'git log', the more self-contained the log is,
the easier it will be to understand WHY the patch is appropriate.
Conditional ACK - no need to post a v3; I'm okay if you push once you
amend the commit message. Keep the mention of the BZ number, but also
add this text (from your previous message).
qemu left non-empty file.
Steps:
1, Create a qemu instance with two drive images of qcow2 type (root
user)
/usr/libexec/qemu-kvm -m 1024 -smp 1 -name "rhel6u1" \
-drive
file=/var/lib/libvirt/images/firstqcow2,if=none,id=drive-virtio-disk0,format=qcow2,cache=none
-device
virtio-blk-pci,scsi=off,bus=pci.0,addr=0x5,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1
\
-drive
file=/var/lib/libvirt/images/secondqcow2,if=none,id=drive-virtio-disk1,format=qcow2,cache=none
\
-device
virtio-blk-pci,scsi=off,bus=pci.0,addr=0x7,drive=drive-virtio-disk1,id=virtio-disk1
-qmp stdio
2, Initialize qemu qmp
{"execute":"qmp_capabilities"}
3, Remove the second drive image file
rm -f /var/lib/libvirt/images/secondqcow2
4, Run 'transaction' command with snapshot qemu commands in.
{"execute":"transaction","arguments":
{"actions":
[{"type":"blockdev-snapshot-sync","data":{"device":"drive-virtio-disk0","snapshot-file":"/var/lib/libvirt/images/firstqcow2-snapshot.img","format":"qcow2"}},
{"type":"blockdev-snapshot-sync","data":{"device":"drive-virtio-disk1","snapshot-file":"/var/lib/libvirt/images/secondqcow2-snapshot.img","format":"qcow2"}}]
},
"id":"libvirt-6"}
5, Got the error as follows:
{"id": "libvirt-6", "error": {"class":
"OpenFileFailed",
"desc": "Could not open
'/var/lib/libvirt/images/secondqcow2-snapshot.img'",
"data": {"filename":
"/var/lib/libvirt/images/secondqcow2-snapshot.img"}}}
6, List first newly-created snapshot file:
-rw-r--r--. 1 root root 262144 Sep 13 11:43
firstqcow2-snapshot.img
--
Eric Blake eblake(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
9:52 p.m.
New subject: [libvirt] [PATCH v3] snapshot: fix rollback failure in transaction mode
BZ:https://bugzilla.redhat.com/show_bug.cgi?id=843372
when qemu supports the 'transaction' monitor command,
and libvirt's --reuse-ext flag was not specified, libvirt created
a stub file with zero size in first place. After the failure of
QEMU transaction command performing qcow2 snapshots on more than
one drives, the stub file is left behind with non-empty
by the QEMU transaction command.
In order to unlink the file, the patch removes the file size checking.
Steps to reproduce the issue:
Steps:
1, Create a qemu instance with two drive images of qcow2 type (root user)
/usr/libexec/qemu-kvm -m 1024 -smp 1 -name "rhel6u1" \
-drive
file=/var/lib/libvirt/images/firstqcow2,if=none,id=drive-virtio-disk0,format=qcow2,cache=none
-device
virtio-blk-pci,scsi=off,bus=pci.0,addr=0x5,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1
\
-drive
file=/var/lib/libvirt/images/secondqcow2,if=none,id=drive-virtio-disk1,format=qcow2,cache=none
\
-device
virtio-blk-pci,scsi=off,bus=pci.0,addr=0x7,drive=drive-virtio-disk1,id=virtio-disk1 -qmp
stdio
2, Initialize qemu qmp
{"execute":"qmp_capabilities"}
3, Remove the second drive image file
rm -f /var/lib/libvirt/images/secondqcow2
4, Run 'transaction' command with snapshot qemu commands in.
{"execute":"transaction","arguments":
{"actions":
[{"type":"blockdev-snapshot-sync","data":
{"device":"drive-virtio-disk0","snapshot-file":"/var/lib/libvirt/images/firstqcow2-snapshot.img","format":"qcow2"}
},
{"type":"blockdev-snapshot-sync","data":
{"device":"drive-virtio-disk1","snapshot-file":"/var/lib/libvirt/images/secondqcow2-snapshot.img","format":"qcow2"}
}]
},
"id":"libvirt-6"}
5, Got the error as follows:
{"id": "libvirt-6",
"error": {"class": "OpenFileFailed", "desc":
"Could not open '/var/lib/libvirt/images/secondqcow2-snapshot.img'",
"data": {"filename":
"/var/lib/libvirt/images/secondqcow2-snapshot.img"}
}
}
6, List first newly-created snapshot file:
-rw-r--r--. 1 root root 262144 Sep 13 11:43 firstqcow2-snapshot.img
---
src/qemu/qemu_driver.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index a410521..d11bd19 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -10833,7 +10833,7 @@ qemuDomainSnapshotUndoSingleDiskActive(struct qemud_driver
*driver,
if (virDomainLockDiskDetach(driver->lockManager, vm, disk) < 0)
VIR_WARN("Unable to release lock on %s", disk->src);
if (need_unlink && stat(disk->src, &st) == 0 &&
- st.st_size == 0 && S_ISREG(st.st_mode) && unlink(disk->src)
< 0)
+ S_ISREG(st.st_mode) && unlink(disk->src) < 0)
VIR_WARN("Unable to remove just-created %s", disk->src);
/* Update vm in place to match changes. */
--
1.7.11.2
10 p.m.
New subject: [libvirt] [PATCH v3] snapshot: fix rollback failure in transaction mode
On 09/13/2012 08:52 PM, Guannan Ren wrote:
BZ:https://bugzilla.redhat.com/show_bug.cgi?id=843372
when qemu supports the 'transaction' monitor command,
and libvirt's --reuse-ext flag was not specified, libvirt created
a stub file with zero size in first place. After the failure of
QEMU transaction command performing qcow2 snapshots on more than
one drives, the stub file is left behind with non-empty
by the QEMU transaction command.
In order to unlink the file, the patch removes the file size checking.
ACK. Thanks for improving the commit message; it will be helpful in the
future.
--
Eric Blake eblake(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
10:16 p.m.
New subject: [libvirt] [PATCH v3] snapshot: fix rollback failure in transaction mode
On 09/14/2012 11:00 AM, Eric Blake wrote:
On 09/13/2012 08:52 PM, Guannan Ren wrote:
> BZ:https://bugzilla.redhat.com/show_bug.cgi?id=843372
> when qemu supports the 'transaction' monitor command,
> and libvirt's --reuse-ext flag was not specified, libvirt created
> a stub file with zero size in first place. After the failure of
> QEMU transaction command performing qcow2 snapshots on more than
> one drives, the stub file is left behind with non-empty
> by the QEMU transaction command.
> In order to unlink the file, the patch removes the file size checking.
ACK. Thanks for improving the commit message; it will be helpful in the
future.
Welcome.
Thanks and pushed.
Guannan
4454
days inactive
4456
days old
9 comments
3 participants
participants (3)
-
Eric Blake -
Guannan Ren -
Jeff Cody