Re: [libvirt] [PATCH] qemu_migrate: Fix assign the same port when migrating concurrently
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