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@xxxxxxxxxx [mailto:jdenemar@xxxxxxxxxx]
> Sent: Friday, September 27, 2013 5:06 PM
> To: Wangyufei (A)
> Cc: libvir-list@xxxxxxxxxx; Michal Privoznik; Wangrui (K)
> Subject: Re: [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@xxxxxxxxxx>
> > 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@xxxxxxxxxx>
> > ---
> > 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list