[PATCH] lib: direct: Disable qemu locking when opening drives
Incomplete fix for https://bugzilla.redhat.com/show_bug.cgi?id=1417306 The full fix is waiting for a libvirt change, but this can still go upstream. Rich.
Since qemu 2.10, qemu locks all drives even when they are opened readonly. This disables this locking in this safe case. Incomplete fix for https://bugzilla.redhat.com/show_bug.cgi?id=1417306 --- lib/launch-direct.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/lib/launch-direct.c b/lib/launch-direct.c index 3b848165c..e5465539d 100644 --- a/lib/launch-direct.c +++ b/lib/launch-direct.c @@ -221,8 +221,11 @@ add_drive_standard_params (guestfs_h *g, struct backend_direct_data *data, file = guestfs_int_drive_source_qemu_param (g, &drv->src); append_list_format ("file=%s", file); - if (drv->readonly) + if (drv->readonly) { append_list ("snapshot=on"); + if (guestfs_int_version_ge (&data->qemu_version, 2, 10, 0)) + append_list ("locking=off"); + } append_list_format ("cache=%s", drv->cachemode ? drv->cachemode : "writeback"); if (drv->src.format) -- 2.13.2
On Tuesday, 5 September 2017 16:55:08 CEST Richard W.M. Jones wrote:
Since qemu 2.10, qemu locks all drives even when they are opened readonly. This disables this locking in this safe case.
Incomplete fix for https://bugzilla.redhat.com/show_bug.cgi?id=1417306 ---
I guess that, according to the other message in this thread, the commit message will be amended to use RHBZ# in the subject, and drop the last paragraph?
lib/launch-direct.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/lib/launch-direct.c b/lib/launch-direct.c index 3b848165c..e5465539d 100644 --- a/lib/launch-direct.c +++ b/lib/launch-direct.c @@ -221,8 +221,11 @@ add_drive_standard_params (guestfs_h *g, struct backend_direct_data *data, file = guestfs_int_drive_source_qemu_param (g, &drv->src); append_list_format ("file=%s", file);
- if (drv->readonly) + if (drv->readonly) { append_list ("snapshot=on"); + if (guestfs_int_version_ge (&data->qemu_version, 2, 10, 0)) + append_list ("locking=off"); + } append_list_format ("cache=%s", drv->cachemode ? drv->cachemode : "writeback");
LGTM. IMHO locking=off ought to be used also for the appliance disk, otherwise starting two or more libguestfs instances for the same user will fail, no? Even if not, disabling the locking should still be good, as in not making qemu do more work than needed. -- Pino Toscano
On Tue, Sep 05, 2017 at 06:41:21PM +0200, Pino Toscano wrote:
On Tuesday, 5 September 2017 16:55:08 CEST Richard W.M. Jones wrote:
Since qemu 2.10, qemu locks all drives even when they are opened readonly. This disables this locking in this safe case.
Incomplete fix for https://bugzilla.redhat.com/show_bug.cgi?id=1417306 ---
I guess that, according to the other message in this thread, the commit message will be amended to use RHBZ# in the subject, and drop the last paragraph?
lib/launch-direct.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/lib/launch-direct.c b/lib/launch-direct.c index 3b848165c..e5465539d 100644 --- a/lib/launch-direct.c +++ b/lib/launch-direct.c @@ -221,8 +221,11 @@ add_drive_standard_params (guestfs_h *g, struct backend_direct_data *data, file = guestfs_int_drive_source_qemu_param (g, &drv->src); append_list_format ("file=%s", file);
- if (drv->readonly) + if (drv->readonly) { append_list ("snapshot=on"); + if (guestfs_int_version_ge (&data->qemu_version, 2, 10, 0)) + append_list ("locking=off"); + } append_list_format ("cache=%s", drv->cachemode ? drv->cachemode : "writeback");
LGTM. IMHO locking=off ought to be used also for the appliance disk, otherwise starting two or more libguestfs instances for the same user will fail, no? Even if not, disabling the locking should still be good, as in not making qemu do more work than needed.
Yes. I'll push it with all the changes you suggest. Thanks, Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-df lists disk usage of guests without needing to install any software inside the virtual machine. Supports Linux and Windows. http://people.redhat.com/~rjones/virt-df/
On Tue, Sep 05, 2017 at 03:55:08PM +0100, Richard W.M. Jones wrote:
Since qemu 2.10, qemu locks all drives even when they are opened readonly. This disables this locking in this safe case.
Incomplete fix for https://bugzilla.redhat.com/show_bug.cgi?id=1417306 --- lib/launch-direct.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/lib/launch-direct.c b/lib/launch-direct.c index 3b848165c..e5465539d 100644 --- a/lib/launch-direct.c +++ b/lib/launch-direct.c @@ -221,8 +221,11 @@ add_drive_standard_params (guestfs_h *g, struct backend_direct_data *data, file = guestfs_int_drive_source_qemu_param (g, &drv->src); append_list_format ("file=%s", file);
- if (drv->readonly) + if (drv->readonly) { append_list ("snapshot=on"); + if (guestfs_int_version_ge (&data->qemu_version, 2, 10, 0)) + append_list ("locking=off");
Don't we have a way to enumerate supported options? Depending on QEMU version is not going to play nice with vendor-shipped QEMU where features are often backported.
+ } append_list_format ("cache=%s", drv->cachemode ? drv->cachemode : "writeback"); if (drv->src.format)
Roman.
On Wed, Sep 06, 2017 at 10:41:24AM +0300, Roman Kagan wrote:
On Tue, Sep 05, 2017 at 03:55:08PM +0100, Richard W.M. Jones wrote:
Since qemu 2.10, qemu locks all drives even when they are opened readonly. This disables this locking in this safe case.
Incomplete fix for https://bugzilla.redhat.com/show_bug.cgi?id=1417306 --- lib/launch-direct.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/lib/launch-direct.c b/lib/launch-direct.c index 3b848165c..e5465539d 100644 --- a/lib/launch-direct.c +++ b/lib/launch-direct.c @@ -221,8 +221,11 @@ add_drive_standard_params (guestfs_h *g, struct backend_direct_data *data, file = guestfs_int_drive_source_qemu_param (g, &drv->src); append_list_format ("file=%s", file);
- if (drv->readonly) + if (drv->readonly) { append_list ("snapshot=on"); + if (guestfs_int_version_ge (&data->qemu_version, 2, 10, 0)) + append_list ("locking=off");
Don't we have a way to enumerate supported options?
Yes, although it's difficult to do: https://www.redhat.com/archives/libguestfs/2017-September/msg00039.html Rich.
Depending on QEMU version is not going to play nice with vendor-shipped QEMU where features are often backported.
+ } append_list_format ("cache=%s", drv->cachemode ? drv->cachemode : "writeback"); if (drv->src.format)
Roman.
-- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://people.redhat.com/~rjones/virt-top
On Tue, Sep 05, 2017 at 03:55:07PM +0100, Richard W.M. Jones wrote:
Incomplete fix for https://bugzilla.redhat.com/show_bug.cgi?id=1417306
The full fix is waiting for a libvirt change, but this can still go upstream.
I talked to Dan and apparently this is a full fix, since libvirt will intuit the correct locking parameters. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into KVM guests. http://libguestfs.org/virt-v2v
participants (3)
-
Pino Toscano -
Richard W.M. Jones -
Roman Kagan