Re: [libvirt] [Qemu-devel] Killing block migration in qemu?
by Stefan Hajnoczi
On Wed, Aug 17, 2011 at 5:51 PM, Paolo Bonzini <pbonzini(a)redhat.com> wrote:
> following discussions yesterday with Juan Quintela and Marcelo Tosatti, here
> is my humble proposal: remove block migration from qemu master. It seems to
> me that keeping block migration is going to slow down further improvements
> on migration. The main problems are:
>
> 1) there are very good reasons to move migration to a separate thread. Only
> a limited amount of extra locking, perhaps none is needed in order to do so
> for RAM and devices. But the block drivers pretty much need to run under
> the I/O thread lock, and coroutines will not help if the I/O thread is taken
> by another thread. It's hard/unreliable/pointless to ping-pong migration
> between threads.
The image streaming approach will also run in the I/O thread for the
mid-term future. Is the problem that the block migration code today
is too tied into the actual migration code path and therefore stops
from using it when migration happens in a separate thread?
>
> 2) there already are plans to reimplement block migration... it's called
> streaming :) and not coincidentially it reuses some of the block migration
> code.
What are the concrete issues with the existing block migration code?
I'm not disagreeing that we should move to a streaming approach but I
simply don't know the details of the existing block migration code.
> Here is how it would go:
This sounds reasonable. In fact we can do both pre-copy and post-copy
block migration using streaming (+mirroring).
Stefan
13 years, 8 months
[libvirt] [PATCH] qemu: Initialize GnuTLS
by Michal Privoznik
When spice_tls is set but listen_tls is not, we don't initialize
GnuTLS library. So any later gnutls call (e.g. during migration,
where we initialize a certificate) will access uninitialized GnuTLS
internal structs and throws an error.
Although, we might now initialize GnuTLS twice, it is safe according
to the documentation:
This function can be called many times,
but will only do something the first time.
---
src/qemu/qemu_driver.c | 13 +++++++++++++
1 files changed, 13 insertions(+), 0 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 421a98e..5fe20b6 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -44,6 +44,7 @@
#include <sys/ioctl.h>
#include <sys/un.h>
#include <byteswap.h>
+#include <gnutls/gnutls.h>
#include "qemu_driver.h"
@@ -537,6 +538,15 @@ qemudStartup(int privileged) {
}
VIR_FREE(driverConf);
+ if (qemu_driver->spiceTLS) {
+ /* Initialize GnuTLS. If it was initialized before,
+ * it doesn't hurt. From GnuTLS documentation:
+ * This function can be called many times,
+ * but will only do something the first time.
+ */
+ gnutls_global_init();
+ }
+
/* We should always at least have the 'nop' manager, so
* NULLs here are a fatal error
*/
@@ -754,6 +764,9 @@ qemudShutdown(void) {
qemuProcessAutoDestroyShutdown(qemu_driver);
+ if (qemu_driver->spiceTLS)
+ gnutls_global_deinit();
+
VIR_FREE(qemu_driver->configDir);
VIR_FREE(qemu_driver->autostartDir);
VIR_FREE(qemu_driver->logDir);
--
1.7.3.4
13 years, 8 months
[libvirt] [PATCH 0/2] Libvirt-java event handling
by js＠alien8.de
From: Julian Stecklina <jsteckli(a)os.inf.tu-dresden.de>
I noticed that event handling in the libvirt Java bindings is incomplete.
The first patch adds required data types and constants. The second relaxes
the visibility of the Domain constructor, otherwise Callbacks are kind
of useless.
Regards, Julian
Julian Stecklina (2):
Allow different types of callbacks (just as the C API).
Allow to construct Domain objects from inside callbacks.
src/main/java/org/libvirt/Connect.java | 3 ++-
src/main/java/org/libvirt/Domain.java | 2 +-
src/main/java/org/libvirt/jna/Libvirt.java | 22 ++++++++++++++++++++--
3 files changed, 23 insertions(+), 4 deletions(-)
--
1.7.3.4
13 years, 8 months
Re: [libvirt] patch for disk migration verbose progress
by Eric Blake
[re-adding the list]
On 08/17/2011 12:30 AM, Tom Vijlbrief wrote:
> Hi Eric
>
> This is the complete patch including json (not tested) and a typo correction
> in an error message...
>
> Tom
> ---------- Doorgestuurd bericht ----------
> Van: "Tom Vijlbrief"<tom(a)nomadbsd.v7f.eu>
> Datum: 17 aug. 2011 08:23
> Onderwerp: mon.patch
> Aan:<tvijlbrief(a)gmail.com>
>
>
> mon.patch
>
Thanks again for taking the time to contribute. It is easier to work
with patches generated by 'git format-patch' (and 'git send-email' is a
nice wrapper that both formats the patch and mails it to the list); as
written, your patch had no commit message, so I had to invent one.
I also added you to AUTHORS; let me know if I need to update any
preferred spellings.
>
> # NB, must pass the --listen flag to the libvirtd process for this to
Is this a stray line in your email, or is it pointing out a further issue?
> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index 2a9a078..618a45c 100644
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -1894,6 +1894,31 @@ qemuMonitorJSONGetMigrationStatusReply(virJSONValuePtr reply,
> _("migration was active, but RAM 'total' data was missing"));
> return -1;
> }
> +
> + virJSONValuePtr disk = virJSONValueObjectGet(ret, "disk");
> + if (!disk) {
> + return 0;
> + }
> +
> + unsigned long long t;
Our style tends to float declarations to the top, C89 style, even though
this is valid as written (we require C99 for other reasons).
> + if (virJSONValueObjectGetNumberUlong(disk, "transferred",&t)< 0) {
> + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("disk migration was active, but DISK 'transferred' data was missing"));
> + return -1;
> + }
> + (*transferred)+= t;
> + if (virJSONValueObjectGetNumberUlong(disk, "remaining",&t)< 0) {
> + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("disk migration was active, but DISK 'remaining' data was missing"));
> + return -1;
> + }
> + (*remaining)+= t;
I tweaked spacing between operators (yeah, it doesn't help that
thunderbird corrupted spacing even worse in my reply to your mail). And
the () are redundant; this can safely be:
*remaining += t;
> + if (virJSONValueObjectGetNumberUlong(disk, "total",&t)< 0) {
> + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("disk migration was active, but DISK 'total' data was missing"));
> + return -1;
> + }
> + (*total)+= t;
> }
>
> return 0;
> diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c
> index 7bf733d..fef6f36 100644
> --- a/src/qemu/qemu_monitor_text.c
> +++ b/src/qemu/qemu_monitor_text.c
> @@ -149,7 +149,7 @@ int qemuMonitorTextIOProcess(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
> passwd = strstr(start, PASSWORD_PROMPT);
> if (passwd) {
> #if DEBUG_IO
> - VIR_DEBUG("Seen a passwowrd prompt [%s]", data + used);
> + VIR_DEBUG("Seen a password prompt [%s]", data + used);
> #endif
> if (msg->passwordHandler) {
> int i;
> @@ -1183,6 +1183,9 @@ cleanup:
> #define MIGRATION_TRANSFER_PREFIX "transferred ram: "
> #define MIGRATION_REMAINING_PREFIX "remaining ram: "
> #define MIGRATION_TOTAL_PREFIX "total ram: "
> +#define MIGRATION_DISK_TRANSFER_PREFIX "transferred disk: "
> +#define MIGRATION_DISK_REMAINING_PREFIX "remaining disk: "
> +#define MIGRATION_DISK_TOTAL_PREFIX "total disk: "
>
> int qemuMonitorTextGetMigrationStatus(qemuMonitorPtr mon,
> int *status,
> @@ -1246,6 +1249,7 @@ int qemuMonitorTextGetMigrationStatus(qemuMonitorPtr mon,
> goto cleanup;
> }
> *remaining *= 1024;
> + tmp = end;
>
> if (!(tmp = strstr(tmp, MIGRATION_TOTAL_PREFIX)))
> goto done;
> @@ -1257,7 +1261,53 @@ int qemuMonitorTextGetMigrationStatus(qemuMonitorPtr mon,
> goto cleanup;
> }
> *total *= 1024;
> + tmp = end;
> +
> + /*
> + * Check for Optional Disk Migration status
> + */
> +
> + unsigned long long disk_transferred= 0;
> + unsigned long long disk_remaining= 0;
> + unsigned long long disk_total= 0;
> +
> + if (!(tmp = strstr(tmp, MIGRATION_DISK_TRANSFER_PREFIX)))
> + goto done;
> + tmp += strlen(MIGRATION_DISK_TRANSFER_PREFIX);
>
> + if (virStrToLong_ull(tmp,&end, 10,&disk_transferred)< 0) {
> + qemuReportError(VIR_ERR_INTERNAL_ERROR,
> + _("cannot parse disk migration data transferred statistic %s"), tmp);
> + goto cleanup;
> + }
> + disk_transferred *= 1024;
> + (*transferred)+= disk_transferred;
For one less assignment, I merged these two lines into:
*transferred += disk_transferred * 1024;
> + tmp = end;
> +
> + if (!(tmp = strstr(tmp, MIGRATION_DISK_REMAINING_PREFIX)))
> + goto done;
> + tmp += strlen(MIGRATION_DISK_REMAINING_PREFIX);
> +
> + if (virStrToLong_ull(tmp,&end, 10,&disk_remaining)< 0) {
> + qemuReportError(VIR_ERR_INTERNAL_ERROR,
> + _("cannot parse disk migration data remaining statistic %s"), tmp);
> + goto cleanup;
> + }
> + disk_remaining *= 1024;
> + (*remaining)+= disk_remaining;
> + tmp = end;
> +
> + if (!(tmp = strstr(tmp, MIGRATION_DISK_TOTAL_PREFIX)))
> + goto done;
> + tmp += strlen(MIGRATION_DISK_TOTAL_PREFIX);
> +
> + if (virStrToLong_ull(tmp,&end, 10,&disk_total)< 0) {
> + qemuReportError(VIR_ERR_INTERNAL_ERROR,
> + _("cannot parse disk migration data total statistic %s"), tmp);
> + goto cleanup;
> + }
> + disk_total *= 1024;
> + (*total)+= disk_total;
> }
> }
ACK and pushed with those tweaks.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
13 years, 8 months
Re: [libvirt] Patch for verbose Disk transfer
by Eric Blake
[re-adding the list]
On 08/16/2011 11:52 PM, Tom Vijlbrief wrote:
> Hi Eric,
>
> Yes json has similar info and I also patched it, but I don't know how to
> configure vir-sh so that it uses the json channel for at least testing the
> patch. I can send you the patch anyway, it's a simple change...
Use of JSON depends on which version of qemu you are running; if you are
using Fedora 15 or newer, then JSON should automatically be in use.
It's not something you can configure at the virsh command line.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
13 years, 8 months
[libvirt] [PATCH] qemu: Init reattaching related members of pciDevice before reattach
by Osier Yang
Otherwise the device will still be bound to pci-stub driver even
it's set as "managed=yes" when do detaching. Of course, it won't
triger any driver reprobing too.
---
src/qemu/qemu_hotplug.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 5f449fb..b7fdfa0 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -1761,6 +1761,7 @@ int qemuDomainDetachHostPciDevice(struct qemud_driver *driver,
pciDeviceListDel(driver->activePciHostdevs, pci);
if (pciResetDevice(pci, driver->activePciHostdevs, NULL) < 0)
ret = -1;
+ pciDeviceReAttachInit(pci);
qemuReattachPciDevice(pci, driver);
pciFreeDevice(pci);
}
--
1.7.6
13 years, 8 months
[libvirt] [PATCH] conf: Do not load domain if the domain with same already exists
by Osier Yang
We don't allow to define domain with same name and different UUID,
or with same UUID, so it's reasonable to not load the domain config
if there is domain with same name already exists.
Otherwise it can cause problem like:
1) % cp /etc/libvirt/qemu/dom.xml /etc/libvirt/qemu/dom_diffuuid.xml
2) remove the line with "uuid" in the "dom_diffuuid.xml"
3) % for i in {1..10}; do kill -SIGHUP $(pidof libvirtd); done
4) % virsh list --all
There will be 11 domains listed with the same name, as if there is
no UUID specified in domain XML, libvirt will generate one for it,
which will be definitely different with the original one.
---
src/conf/domain_conf.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index ce1f3c5..0bed929 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -10535,7 +10535,8 @@ static virDomainObjPtr virDomainLoadConfig(virCapsPtr caps,
/* if the domain is already in our hashtable, we only need to
* update the autostart flag
*/
- if ((dom = virDomainFindByUUID(doms, def->uuid))) {
+ if ((dom = virDomainFindByUUID(doms, def->uuid)) ||
+ (dom = virDomainFindByName(doms, def->name))) {
dom->autostart = autostart;
VIR_FREE(configFile);
--
1.7.6
13 years, 9 months
[libvirt] [PATCH] Ensure stream is aborted when exiting console
by Daniel P. Berrange
From: "Daniel P. Berrange" <berrange(a)redhat.com>
After running 'virsh console' in interactive mode, there was a
missing call to virStreamAbort, which meant the server kept the
stream resources open
* tools/console.c: Abort stream when exiting
---
tools/console.c | 6 ++++--
1 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/tools/console.c b/tools/console.c
index 171ebc9..82ef537 100644
--- a/tools/console.c
+++ b/tools/console.c
@@ -90,9 +90,11 @@ static void
virConsoleShutdown(virConsolePtr con)
{
con->quit = true;
- virStreamEventRemoveCallback(con->st);
- if (con->st)
+ if (con->st) {
+ virStreamEventRemoveCallback(con->st);
+ virStreamAbort(con->st);
virStreamFree(con->st);
+ }
if (con->stdinWatch != -1)
virEventRemoveHandle(con->stdinWatch);
if (con->stdoutWatch != -1)
--
1.7.6
13 years, 9 months
[libvirt] [PATCH] Ensure async packets never get marked for sync replies
by Daniel P. Berrange
From: "Daniel P. Berrange" <berrange(a)redhat.com>
If a client had initiated a stream abort, it will have a call
waiting for a reply in the queue. If more data continues to
arrive on the stream, the abort command could mistakenly get
signalled as complete. Remove the code from async data processing
that looked for waiting calls. Add a sanity check to ensure no
async call can ever be marked as needing a reply
* src/rpc/virnetclient.c: Ensure async data packets can't
trigger a reply
---
src/rpc/virnetclient.c | 12 +++++++-----
1 files changed, 7 insertions(+), 5 deletions(-)
diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c
index efaac88..055361d 100644
--- a/src/rpc/virnetclient.c
+++ b/src/rpc/virnetclient.c
@@ -627,11 +627,6 @@ static int virNetClientCallDispatchStream(virNetClientPtr client)
case VIR_NET_CONTINUE: {
if (virNetClientStreamQueuePacket(st, &client->msg) < 0)
return -1;
-
- if (thecall && thecall->expectReply) {
- VIR_DEBUG("Got sync data packet completion");
- thecall->mode = VIR_NET_CLIENT_MODE_COMPLETE;
- }
return 0;
}
@@ -1193,6 +1188,13 @@ int virNetClientSend(virNetClientPtr client,
virNetClientCallPtr call;
int ret = -1;
+ if (expectReply &&
+ (msg->header.status == VIR_NET_CONTINUE)) {
+ virNetError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Attempt to send an asynchronous message with a synchronous reply"));
+ return -1;
+ }
+
if (VIR_ALLOC(call) < 0) {
virReportOOMError();
return -1;
--
1.7.6
13 years, 9 months
[libvirt] [PATCH] Don't attempt to read from a stream if it is closed
by Daniel P. Berrange
From: "Daniel P. Berrange" <berrange(a)redhat.com>
The I/O event callback processes incoming packets first, and then
does outgoing packets. If the incoming packet caused the stream to
close, then the attempt to process outgoing data resulted in an
error. This caused libvirt to then send an error back to the client,
but the stream had already been stopped. This confused the client
since it sees 2 error events.
* daemon/stream.c: Don't attempt read if stream is closed
---
daemon/stream.c | 10 +++++++++-
1 files changed, 9 insertions(+), 1 deletions(-)
diff --git a/daemon/stream.c b/daemon/stream.c
index 7dd9ae7..7d2b367 100644
--- a/daemon/stream.c
+++ b/daemon/stream.c
@@ -678,7 +678,15 @@ daemonStreamHandleRead(virNetServerClientPtr client,
size_t bufferLen = VIR_NET_MESSAGE_PAYLOAD_MAX;
int ret;
- VIR_DEBUG("client=%p, stream=%p", client, stream);
+ VIR_DEBUG("client=%p, stream=%p tx=%d closed=%d",
+ client, stream, stream->tx, stream->closed);
+
+ /* We might have had an event pending before we shut
+ * down the stream, so if we're marked as closed,
+ * then do nothing
+ */
+ if (stream->closed)
+ return 0;
/* Shouldn't ever be called unless we're marked able to
* transmit, but doesn't hurt to check */
--
1.7.6
13 years, 9 months
