[libvirt] [PATCH] qemu_migrate: Fix assign the same port when migrating concurrently
From: WangYufei <james.wangyufei@huawei.com> Date: Fri, 27 Sep 2013 11:53:57 +0800 Subject: [PATCH] qemu_migrate: Fix assign the same port when migrating concurrently When we migrate vms concurrently, there's a chance that libvirtd on destination assign the same port for different migrations, which will lead to migration failed during migration prepare phase on destination. So we change the port increase to atomic operation to solve the problem. Signed-off-by: WangYufei <james.wangyufei@huawei.com> --- src/qemu/qemu_migration.c | 5 +++-- 1 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 3a1aab7..0f496f4 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -57,6 +57,7 @@ #include "virhook.h" #include "virstring.h" #include "virtypedparam.h" +#include "viratomic.h" #define VIR_FROM_THIS VIR_FROM_QEMU @@ -2521,7 +2522,7 @@ qemuMigrationPrepareDirect(virQEMUDriverPtr driver, * to be a correct hostname which refers to the target machine). */ if (uri_in == NULL) { - this_port = QEMUD_MIGRATION_FIRST_PORT + port++; + this_port = QEMUD_MIGRATION_FIRST_PORT + virAtomicIntInc(&port); if (port == QEMUD_MIGRATION_NUM_PORTS) port = 0; /* Get hostname */ @@ -2578,7 +2579,7 @@ qemuMigrationPrepareDirect(virQEMUDriverPtr driver, if (uri->port == 0) { /* Generate a port */ - this_port = QEMUD_MIGRATION_FIRST_PORT + port++; + this_port = QEMUD_MIGRATION_FIRST_PORT + virAtomicIntInc(&port); if (port == QEMUD_MIGRATION_NUM_PORTS) port = 0; -- 1.7.3.1.msysgit.0 Best Regards, -WangYufei
On Fri, Sep 27, 2013 at 06:28:50 +0000, Wangyufei (A) wrote:
From: WangYufei <james.wangyufei@huawei.com> Date: Fri, 27 Sep 2013 11:53:57 +0800 Subject: [PATCH] qemu_migrate: Fix assign the same port when migrating concurrently
When we migrate vms concurrently, there's a chance that libvirtd on destination assign the same port for different migrations, which will lead to migration failed during migration prepare phase on destination. So we change the port increase to atomic operation to solve the problem.
Oops, this was apparently latent until the big qemu driver lock was removed.
Signed-off-by: WangYufei <james.wangyufei@huawei.com> --- src/qemu/qemu_migration.c | 5 +++-- 1 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 3a1aab7..0f496f4 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -57,6 +57,7 @@ #include "virhook.h" #include "virstring.h" #include "virtypedparam.h" +#include "viratomic.h"
#define VIR_FROM_THIS VIR_FROM_QEMU
@@ -2521,7 +2522,7 @@ qemuMigrationPrepareDirect(virQEMUDriverPtr driver, * to be a correct hostname which refers to the target machine). */ if (uri_in == NULL) { - this_port = QEMUD_MIGRATION_FIRST_PORT + port++; + this_port = QEMUD_MIGRATION_FIRST_PORT + virAtomicIntInc(&port); if (port == QEMUD_MIGRATION_NUM_PORTS) port = 0;
/* Get hostname */ @@ -2578,7 +2579,7 @@ qemuMigrationPrepareDirect(virQEMUDriverPtr driver,
if (uri->port == 0) { /* Generate a port */ - this_port = QEMUD_MIGRATION_FIRST_PORT + port++; + this_port = QEMUD_MIGRATION_FIRST_PORT + virAtomicIntInc(&port); if (port == QEMUD_MIGRATION_NUM_PORTS) port = 0;
Unfortunately, this patch is incomplete. The increments are now atomic but the wrapping at QEMUD_MIGRATION_NUM_PORTS is not. I think this should use virPortAllocator instead. Jirka
Yes, I get your point. this_port = QEMUD_MIGRATION_FIRST_PORT + port++; if (port == QEMUD_MIGRATION_NUM_PORTS) port = 0; the operations above should be atomic, otherwise, port may be bigger than QEMUD_MIGRATION_NUM_PORTS and out of control at last. Right? Using virPortAllocator is much more complicated, I need to think about it. I guess qemu driver lock removed has sent some devils out, we'll see. Thanks for your reply.
-----Original Message----- From: jdenemar@redhat.com [mailto:jdenemar@redhat.com] Sent: Friday, September 27, 2013 5:06 PM To: Wangyufei (A) Cc: libvir-list@redhat.com; Michal Privoznik; Wangrui (K) Subject: Re: [libvirt] [PATCH] qemu_migrate: Fix assign the same port when migrating concurrently
On Fri, Sep 27, 2013 at 06:28:50 +0000, Wangyufei (A) wrote:
From: WangYufei <james.wangyufei@huawei.com> Date: Fri, 27 Sep 2013 11:53:57 +0800 Subject: [PATCH] qemu_migrate: Fix assign the same port when migrating concurrently
When we migrate vms concurrently, there's a chance that libvirtd on destination assign the same port for different migrations, which will lead to migration failed during migration prepare phase on destination. So we change the port increase to atomic operation to solve the problem.
Oops, this was apparently latent until the big qemu driver lock was removed.
Signed-off-by: WangYufei <james.wangyufei@huawei.com> --- src/qemu/qemu_migration.c | 5 +++-- 1 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 3a1aab7..0f496f4 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -57,6 +57,7 @@ #include "virhook.h" #include "virstring.h" #include "virtypedparam.h" +#include "viratomic.h"
#define VIR_FROM_THIS VIR_FROM_QEMU
@@ -2521,7 +2522,7 @@ qemuMigrationPrepareDirect(virQEMUDriverPtr
driver,
* to be a correct hostname which refers to the target machine). */ if (uri_in == NULL) { - this_port = QEMUD_MIGRATION_FIRST_PORT + port++; + this_port = QEMUD_MIGRATION_FIRST_PORT +
virAtomicIntInc(&port);
if (port == QEMUD_MIGRATION_NUM_PORTS) port = 0;
/* Get hostname */ @@ -2578,7 +2579,7 @@ qemuMigrationPrepareDirect(virQEMUDriverPtr
driver,
if (uri->port == 0) { /* Generate a port */ - this_port = QEMUD_MIGRATION_FIRST_PORT + port++; + this_port = QEMUD_MIGRATION_FIRST_PORT +
virAtomicIntInc(&port);
if (port == QEMUD_MIGRATION_NUM_PORTS) port = 0;
Unfortunately, this patch is incomplete. The increments are now atomic but the wrapping at QEMUD_MIGRATION_NUM_PORTS is not. I think this should use virPortAllocator instead.
Jirka
On 27.09.2013 11:36, Wangyufei (A) wrote:
Yes, I get your point.
this_port = QEMUD_MIGRATION_FIRST_PORT + port++; if (port == QEMUD_MIGRATION_NUM_PORTS) port = 0;
In fact this won't work either. What if this_port is already taken by another app? virPortAllocator ensures such situation won't happen (*). Michal (*) Okay, I should rather say 'virPortAllocator minimizes the chance of race' as there is still a small window for race. The virPortAllocator is used for allocating a VNC port, for instance. So whenever a domain with autoport for VNC is start, a free port is searched and bind()-ed. Then, the port number is put onto qemu cmd line. And here comes the race: libvirtd fork()s and close all FDs that are not to be transferred to the qemu process. After this the execve("qemu", ...) is called. So the race window begins at the FDs closing and ends when qemu decide to bind() to the given port. But there's no better way to do this (until qemu learns that VNC port socket can be passed as a FD).
participants (3)
-
jdenemar＠redhat.com -
Michal Privoznik -
Wangyufei (A)