Re: [Qemu-devel] [RFC 19/29] migration: let dst listen on port always

[Daniel P. Berrange]

On Wed, Aug 02, 2017 at 03:02:24PM +0800, Peter Xu wrote:
> On Tue, Aug 01, 2017 at 11:56:10AM +0100, Daniel P. Berrange wrote:
> > On Fri, Jul 28, 2017 at 04:06:28PM +0800, Peter Xu wrote:
> > > Signed-off-by: Peter Xu <address@hidden>
> > > ---
> > >  migration/exec.c   | 2 +-
> > >  migration/fd.c     | 2 +-
> > >  migration/socket.c | 4 ++--
> > >  3 files changed, 4 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/migration/exec.c b/migration/exec.c
> > > index 08b599e..b4412db 100644
> > > --- a/migration/exec.c
> > > +++ b/migration/exec.c
> > > @@ -49,7 +49,7 @@ static gboolean 
> > > exec_accept_incoming_migration(QIOChannel *ioc,
> > >  {
> > >      migration_channel_process_incoming(ioc);
> > >      object_unref(OBJECT(ioc));
> > > -    return FALSE; /* unregister */
> > > +    return TRUE; /* keep it registered */
> > >  }
> > >  
> > >  void exec_start_incoming_migration(const char *command, Error **errp)
> > > diff --git a/migration/fd.c b/migration/fd.c
> > > index 30f5258..865277a 100644
> > > --- a/migration/fd.c
> > > +++ b/migration/fd.c
> > > @@ -49,7 +49,7 @@ static gboolean fd_accept_incoming_migration(QIOChannel 
> > > *ioc,
> > >  {
> > >      migration_channel_process_incoming(ioc);
> > >      object_unref(OBJECT(ioc));
> > > -    return FALSE; /* unregister */
> > > +    return TRUE; /* keep it registered */
> > >  }
> > >  
> > >  void fd_start_incoming_migration(const char *infd, Error **errp)
> > > diff --git a/migration/socket.c b/migration/socket.c
> > > index 757d382..f2c2d01 100644
> > > --- a/migration/socket.c
> > > +++ b/migration/socket.c
> > > @@ -153,8 +153,8 @@ static gboolean 
> > > socket_accept_incoming_migration(QIOChannel *ioc,
> > >  
> > >  out:
> > >      /* Close listening socket as its no longer needed */
> > > -    qio_channel_close(ioc, NULL);
> > > -    return FALSE; /* unregister */
> > > +    // qio_channel_close(ioc, NULL);
> > > +    return TRUE; /* keep it registered */
> > >  }
> > 
> > 
> > This is not a very desirable approach IMHO.
> > 
> > There are two separate things at play - first we have the listener socket,
> > and second we have the I/O watch that monitors for incoming clients.
> > 
> > The current code here closes the listener, and returns FALSE to unregister
> > the event loop watch.
> > 
> > You're reversing both of these so that we keep the listener open and we
> > keep monitoring for incoming clients. Ignoring migration resume for a
> > minute, this means that the destination QEMU will now accept arbitrarily
> > many incoming clients and keep trying to start a new incoming migration.
> > 
> > The behaviour we need is diferent. We *want* to unregister the event
> > loop watch once we've accepted a client. We should only keep the socket
> > listener in existance, but *not* accept any more clients. Only once we
> > have hit a problem and want to accept a new client to do migration
> > recovery, should we be re-adding the event loop watch.
> 
> I replied with another approach in the other thread: how about we
> re-enable the listen port duing "postcopy-pause" state, and disable
> the listen port when get out of that migration state?

Thinking about this agan, I realize I only considered the socket
migration backend.  If we are using the 'fd' backend, then we
*must* have an explicit monitor command invoked on the target
host to obtain the new client connection. This is because the
'fd' that QEMU has is the actual client connection, not a listener
socket, so libvirt needs to be able to pass in a new fd.


> Here "listen port" I mean both the IO watch and the socket object. Now
> what I can think of: we keep these objects always there, meanwhile we
> introduce a new bit for migration, say, "accept_incoming", to decide
> whether we will really accept one connection. Then we drop the new
> connection if that bit is not set.
> 
> (Or a new QIOChannelFeature to temporarily refuse incoming
>  connection? E.g., QIO_CHANNEL_FEATURE_LISTEN_REFUSE?)

That feature already exists - you just unregister the event loop
watch and re-register it when you're ready again. The chardev
socket code works this way already, since it only allows a single
client at a time

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|