This is virt-inspector-1.0.34-1.3 on CentOS 5:
$ virt-inspector --ro-fish --xml da.qcow2 qemu: loading initrd (0x33a576d bytes) at 0x0000000014c4a000 umount_all: guestfs_umount_all: call launch() before using this function at /usr/bin/virt-inspector line 375.
Charles Duffy wrote:
This is virt-inspector-1.0.34-1.3 on CentOS 5:
$ virt-inspector --ro-fish --xml da.qcow2 qemu: loading initrd (0x33a576d bytes) at 0x0000000014c4a000 umount_all: guestfs_umount_all: call launch() before using this function at /usr/bin/virt-inspector line 375.
Running with LIBGUESTFS_DEBUG=1, it appears that this happened due to the guest trying to do ext3 journal recovery against a read-only image.
If telling libguestfs to run in read-only mode implied passing the -snapshot flag to qemu, and passing the snapshot=on option to each -drive, this should allow changes to be *temporarily* created (stored to unlinked copy-on-write files for the duration of the run), mooting issues of this kind.
On Wed, May 27, 2009 at 05:16:40PM -0500, Charles Duffy wrote:
Charles Duffy wrote:
This is virt-inspector-1.0.34-1.3 on CentOS 5:
$ virt-inspector --ro-fish --xml da.qcow2 qemu: loading initrd (0x33a576d bytes) at 0x0000000014c4a000 umount_all: guestfs_umount_all: call launch() before using this function at /usr/bin/virt-inspector line 375.
Running with LIBGUESTFS_DEBUG=1, it appears that this happened due to the guest trying to do ext3 journal recovery against a read-only image.
ext3 tries to do a journal recovery if we mount -o ro ?
If telling libguestfs to run in read-only mode implied passing the -snapshot flag to qemu, and passing the snapshot=on option to each -drive, this should allow changes to be *temporarily* created (stored to unlinked copy-on-write files for the duration of the run), mooting issues of this kind.
I want to get the ability to mount drives completely read-only into qemu. Turns out that it's a bit more complicated than just opening the underlying file O_RDONLY - we have to communicate this all the way through IDE / virtio drivers.
But using snapshots actually isn't such a bad idea. Also works with previous and current versions of qemu too.
Rich.
Charles, can you see if this fixes the problem?
It's completely untested, and might even segfault the Perl bindings because of BZ #501892 (which really needs to be fixed).
Rich.
Richard W.M. Jones wrote:
Charles, can you see if this fixes the problem?
It's completely untested, and might even segfault the Perl bindings because of BZ #501892 (which really needs to be fixed).
It doesn't fix things completely, due to what appears to be a qemu bug -- apparently, -snapshot doesn't work correctly with -drive, and snapshot=on needs to be used instead. I've tested this by setting LIBGUESTFS_QEMU to a wrapper which adds ,cache=off,snapshot=on to each -drive parameter, which works correctly; using only -snapshot fails as below.
The generated command line correctly includes -snapshot, and qemu correctly creates a read-only file to store changes to, but we still fail (albeit differently) on attempted journal replay:
hda: max request size: 128KiB hda: 2016 sectors (1 MB) w/256KiB Cache, CHS=2/16/63, (U)DMA hda: cache flushes supported hda:<4>hda: dma_timer_expiry: dma status == 0x61 hda: DMA timeout error sock_read_event: 0x9ec8f0 g->state = 1, fd = 3, events = 0x1 stdout_event: 0x9ec8f0: child process died wait_ready: guestfs_wait_ready failed, see earlier error messages at /usr/bin/virt-inspector line 219. closing guestfs handle 0x9ec8f0 (state 0)
Manually creating a new, writable backing store (qemu-img create -b da.qcow2 -f qcow2 da.tmp.qcow2) and running against that (with --force), virt-inspector works correctly, just as it does with snapshot=on passed to the individual -drive options and no global -snapshot flag at all.
One other issue with that patch is that it passes an empty string rather than no secondary parameter at all in place of the null:
/usr/bin/qemu-kvm -snapshot '' -drive file=da.qcow2 [...]
On Thu, May 28, 2009 at 11:50:00AM -0500, Charles Duffy wrote:
Richard W.M. Jones wrote:
Charles, can you see if this fixes the problem?
It's completely untested, and might even segfault the Perl bindings because of BZ #501892 (which really needs to be fixed).
I'm still fixing this one ...
It doesn't fix things completely, due to what appears to be a qemu bug -- apparently, -snapshot doesn't work correctly with -drive, and snapshot=on needs to be used instead. I've tested this by setting LIBGUESTFS_QEMU to a wrapper which adds ,cache=off,snapshot=on to each -drive parameter, which works correctly; using only -snapshot fails as below.
The generated command line correctly includes -snapshot, and qemu correctly creates a read-only file to store changes to, but we still fail (albeit differently) on attempted journal replay:
hda: max request size: 128KiB hda: 2016 sectors (1 MB) w/256KiB Cache, CHS=2/16/63, (U)DMA hda: cache flushes supported hda:<4>hda: dma_timer_expiry: dma status == 0x61 hda: DMA timeout error sock_read_event: 0x9ec8f0 g->state = 1, fd = 3, events = 0x1 stdout_event: 0x9ec8f0: child process died wait_ready: guestfs_wait_ready failed, see earlier error messages at /usr/bin/virt-inspector line 219. closing guestfs handle 0x9ec8f0 (state 0)
Manually creating a new, writable backing store (qemu-img create -b da.qcow2 -f qcow2 da.tmp.qcow2) and running against that (with --force), virt-inspector works correctly, just as it does with snapshot=on passed to the individual -drive options and no global -snapshot flag at all.
One other issue with that patch is that it passes an empty string rather than no secondary parameter at all in place of the null:
/usr/bin/qemu-kvm -snapshot '' -drive file=da.qcow2 [...]
I think that's the same bug in the Perl bindings actually. The C code looks correct.
Rich.
On Thu, May 28, 2009 at 05:58:30PM +0100, Richard W.M. Jones wrote:
On Thu, May 28, 2009 at 11:50:00AM -0500, Charles Duffy wrote:
Richard W.M. Jones wrote:
It's completely untested, and might even segfault the Perl bindings because of BZ #501892 (which really needs to be fixed).
I'm still fixing this one ...
This ridiculously large patch fixes 501892 and a number of other bugs in the language bindings, and also adds detailed regression tests so hopefully we won't end up adding any bugs like this back in future:
http://git.et.redhat.com/?p=libguestfs.git;a=commitdiff;h=babc0846cc911b01a5...
One other issue with that patch is that it passes an empty string rather than no secondary parameter at all in place of the null:
/usr/bin/qemu-kvm -snapshot '' -drive file=da.qcow2 [...]
I think that's the same bug in the Perl bindings actually. The C code looks correct.
The Perl binding was turning undef into '', incorrectly of course. This is fixed by the above patch.
Rich.
On Thu, May 28, 2009 at 04:03:57PM -0500, Charles Duffy wrote:
Richard W.M. Jones wrote:
Charles, can you see if this fixes the problem?
It's completely untested, and might even segfault the Perl bindings because of BZ #501892 (which really needs to be fixed).
With 1.0.35, this works correctly.
1.0.35 had a broken test. 1.0.36 is on its way ...
I've also built 1.0.35 in EPEL now, so it should be possible to use that directly once it is signed and in the EPEL repositories.
Rich.
Richard W.M. Jones wrote:
On Thu, May 28, 2009 at 04:03:57PM -0500, Charles Duffy wrote:
Richard W.M. Jones wrote:
Charles, can you see if this fixes the problem?
It's completely untested, and might even segfault the Perl bindings because of BZ #501892 (which really needs to be fixed).
With 1.0.35, this works correctly.
1.0.35 had a broken test. 1.0.36 is on its way ...
I've also built 1.0.35 in EPEL now, so it should be possible to use that directly once it is signed and in the EPEL repositories.
Just curious -- do you intend to push the patch ("-snapshot" in virt-inspector) into the tree?
On Thu, May 28, 2009 at 04:19:21PM -0500, Charles Duffy wrote:
Richard W.M. Jones wrote:
On Thu, May 28, 2009 at 04:03:57PM -0500, Charles Duffy wrote:
Richard W.M. Jones wrote:
Charles, can you see if this fixes the problem?
It's completely untested, and might even segfault the Perl bindings because of BZ #501892 (which really needs to be fixed).
With 1.0.35, this works correctly.
1.0.35 had a broken test. 1.0.36 is on its way ...
I've also built 1.0.35 in EPEL now, so it should be possible to use that directly once it is signed and in the EPEL repositories.
Just curious -- do you intend to push the patch ("-snapshot" in virt-inspector) into the tree?
I guess we can, but is there a better way?
Rich.
Richard W.M. Jones wrote:
I guess we can, but is there a better way?
The guests could use "blockdev --setro" prior to any read-only mount calls to prevent the ext3 code from being able to replay its journal.
If the journal replay is necessary for the filesystem's metadata to be consistent, though, I'm not sure that this is really "better".
On Thu, May 28, 2009 at 05:01:02PM -0500, Charles Duffy wrote:
Richard W.M. Jones wrote:
I guess we can, but is there a better way?
The guests could use "blockdev --setro" prior to any read-only mount calls to prevent the ext3 code from being able to replay its journal.
Ah well ... I discovered that 'blockdev --setro' doesn't work on Fedora 10+.
It only works on RHEL 5.
Rich.
On Thu, May 28, 2009 at 10:53:20PM +0100, Richard W.M. Jones wrote:
On Thu, May 28, 2009 at 04:19:21PM -0500, Charles Duffy wrote:
Just curious -- do you intend to push the patch ("-snapshot" in virt-inspector) into the tree?
I guess we can, but is there a better way?
Right, had a think about this over the weekend, found and read the qemu documentation, and I have added this patch:
http://git.et.redhat.com/?p=libguestfs.git;a=commitdiff;h=bfdc03be234d6d95f1...
It adds a 'guestfs_add_drive_ro' call, which is equivalent to the qemu parameter: -drive file=<filename>,snapshot=on
I've also updated virt-inspector so that it mounts all drives using this flag.
That copes with the simple/common case. For more complex cases, use the guestfs_config function. I'm not intending to map the entire qemu parameter space into the libguestfs API. For a start, they keep changing it, and we're tied to qemu anyhow.
Rich.
On Tue, Jun 2, 2009 at 8:37 AM, Richard W.M. Jones rjones@redhat.comwrote:
On Thu, May 28, 2009 at 10:53:20PM +0100, Richard W.M. Jones wrote:
On Thu, May 28, 2009 at 04:19:21PM -0500, Charles Duffy wrote:
Just curious -- do you intend to push the patch ("-snapshot" in virt-inspector) into the tree?
I guess we can, but is there a better way?
Right, had a think about this over the weekend, found and read the qemu documentation, and I have added this patch:
http://git.et.redhat.com/?p=libguestfs.git;a=commitdiff;h=bfdc03be234d6d95f1...
It adds a 'guestfs_add_drive_ro' call, which is equivalent to the qemu parameter: -drive file=<filename>,snapshot=on
Yay!
That said -- might I suggest that guestfish use add_drive_ro() for all --add options if --ro is specified on the command line at startup?
I'll happily work up a patch myself once I get into the office in a few hours, if you'd prefer.
On Tue, Jun 02, 2009 at 08:51:53AM -0500, Charles Duffy wrote:
On Tue, Jun 2, 2009 at 8:37 AM, Richard W.M. Jones rjones@redhat.comwrote:
On Thu, May 28, 2009 at 10:53:20PM +0100, Richard W.M. Jones wrote:
On Thu, May 28, 2009 at 04:19:21PM -0500, Charles Duffy wrote:
Just curious -- do you intend to push the patch ("-snapshot" in virt-inspector) into the tree?
I guess we can, but is there a better way?
Right, had a think about this over the weekend, found and read the qemu documentation, and I have added this patch:
http://git.et.redhat.com/?p=libguestfs.git;a=commitdiff;h=bfdc03be234d6d95f1...
It adds a 'guestfs_add_drive_ro' call, which is equivalent to the qemu parameter: -drive file=<filename>,snapshot=on
Yay!
That said -- might I suggest that guestfish use add_drive_ro() for all --add options if --ro is specified on the command line at startup?
I'll happily work up a patch myself once I get into the office in a few hours, if you'd prefer.
Yes, good idea, please send a patch.
Rich.
On Tue, Jun 02, 2009 at 12:05:51PM -0500, Charles Duffy wrote:
Richard W.M. Jones wrote:
On Tue, Jun 02, 2009 at 08:51:53AM -0500, Charles Duffy wrote:
That said -- might I suggest that guestfish use add_drive_ro() for all --add options if --ro is specified on the command line at startup?
Yes, good idea, please send a patch.
Attached.
Thanks - I've applied it.
Rich.
On Thu, May 28, 2009 at 3:39 AM, Richard W.M. Jones rjones@redhat.comwrote:
Running with LIBGUESTFS_DEBUG=1, it appears that this happened due to the guest trying to do ext3 journal recovery against a read-only image.
ext3 tries to do a journal recovery if we mount -o ro ?
Yup. Quoting from Stephen Tweedie's ext3 readme:
If you use an older version of e2fsck from e2fsprogs-1.17 or later, then you can now run e2fsck quite happily on the filesystem, but *only as long as the filesystem was unmounted cleanly*. If it wasn't, then you'll need to get the kernel code to recover the journal from the disk by mounting the filesystem *(even a readonly mount will cause a journal recovery to happen)* and umounting it again (or, for the root filesystem, remounting it readonly with "mount -o remount,ro /").
But using snapshots actually isn't such a bad idea. Also works with
previous and current versions of qemu too.
It'd almost be tempting to provide optional support for snapshots in read-write mode, letting the user do a commit to persist their changes or to exit without one to discard them.
-
Charles Duffy -
Richard W.M. Jones