1:28 a.m.
From: WangYufei <james.wangyufei(a)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(a)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
4:06 a.m.
On Fri, Sep 27, 2013 at 06:28:50 +0000, Wangyufei (A) wrote:
From: WangYufei <james.wangyufei(a)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(a)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
4:36 a.m.
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(a)redhat.com [mailto:jdenemar@redhat.com]
Sent: Friday, September 27, 2013 5:06 PM
To: Wangyufei (A)
Cc: libvir-list(a)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(a)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(a)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
4:56 a.m.
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).
4075
days inactive
4075
days old
3 comments
3 participants
participants (3)
-
jdenemar＠redhat.com -
Michal Privoznik -
Wangyufei (A)