Re: [PATCH 1/2] migration/multifd: Cleanup TLS iochannel referencing

[Peter Xu <peterx@redhat.com>]

On Thu, Feb 08, 2024 at 04:10:58PM +0200, Avihai Horon wrote:
> 
> On 08/02/2024 5:51, peterx@redhat.com wrote:
> > External email: Use caution opening links or attachments
> > 
> > 
> > From: Peter Xu <peterx@redhat.com>
> > 
> > Commit a1af605bd5 ("migration/multifd: fix hangup with TLS-Multifd due to
> > blocking handshake") introduced a thread for TLS channels, which will
> > resolve the issue on blocking the main thread.  However in the same commit
> > p->c is slightly abused just to be able to pass over the pointer "p" into
> > the thread.
> > 
> > That's the major reason we'll need to conditionally free the io channel in
> > the fault paths.
> > 
> > To clean it up, using a separate structure to pass over both "p" and "tioc"
> > in the tls handshake thread.  Then we can make it a rule that p->c will
> > never be set until the channel is completely setup.  With that, we can drop
> > the tricky conditional unref of the io channel in the error path.
> > 
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >   migration/multifd.c | 37 +++++++++++++++++++++++--------------
> >   1 file changed, 23 insertions(+), 14 deletions(-)
> > 
> > diff --git a/migration/multifd.c b/migration/multifd.c
> > index adfe8c9a0a..4a85a6b7b3 100644
> > --- a/migration/multifd.c
> > +++ b/migration/multifd.c
> > @@ -873,16 +873,22 @@ out:
> > 
> >   static void multifd_new_send_channel_async(QIOTask *task, gpointer opaque);
> > 
> > +typedef struct {
> > +    MultiFDSendParams *p;
> > +    QIOChannelTLS *tioc;
> > +} MultiFDTLSThreadArgs;
> > +
> >   static void *multifd_tls_handshake_thread(void *opaque)
> >   {
> > -    MultiFDSendParams *p = opaque;
> > -    QIOChannelTLS *tioc = QIO_CHANNEL_TLS(p->c);
> > +    MultiFDTLSThreadArgs *args = opaque;
> > 
> > -    qio_channel_tls_handshake(tioc,
> > +    qio_channel_tls_handshake(args->tioc,
> >                                 multifd_new_send_channel_async,
> > -                              p,
> > +                              args->p,
> >                                 NULL,
> >                                 NULL);
> > +    g_free(args);
> > +
> >       return NULL;
> >   }
> > 
> > @@ -892,6 +898,7 @@ static bool multifd_tls_channel_connect(MultiFDSendParams *p,
> >   {
> >       MigrationState *s = migrate_get_current();
> >       const char *hostname = s->hostname;
> > +    MultiFDTLSThreadArgs *args;
> >       QIOChannelTLS *tioc;
> > 
> >       tioc = migration_tls_client_create(ioc, hostname, errp);
> > @@ -906,11 +913,14 @@ static bool multifd_tls_channel_connect(MultiFDSendParams *p,
> >       object_unref(OBJECT(ioc));
> >       trace_multifd_tls_outgoing_handshake_start(ioc, tioc, hostname);
> >       qio_channel_set_name(QIO_CHANNEL(tioc), "multifd-tls-outgoing");
> > -    p->c = QIO_CHANNEL(tioc);
> > +
> > +    args = g_new0(MultiFDTLSThreadArgs, 1);
> > +    args->tioc = tioc;
> > +    args->p = p;
> > 
> >       p->tls_thread_created = true;
> >       qemu_thread_create(&p->tls_thread, "multifd-tls-handshake-worker",
> > -                       multifd_tls_handshake_thread, p,
> > +                       multifd_tls_handshake_thread, args,
> >                          QEMU_THREAD_JOINABLE);
> >       return true;
> >   }
> > @@ -923,6 +933,7 @@ static bool multifd_channel_connect(MultiFDSendParams *p,
> > 
> >       migration_ioc_register_yank(ioc);
> >       p->registered_yank = true;
> > +    /* Setup p->c only if the channel is completely setup */
> >       p->c = ioc;
> > 
> >       p->thread_created = true;
> > @@ -976,14 +987,12 @@ out:
> > 
> >       trace_multifd_new_send_channel_async_error(p->id, local_err);
> >       multifd_send_set_error(local_err);
> > -    if (!p->c) {
> > -        /*
> > -         * If no channel has been created, drop the initial
> > -         * reference. Otherwise cleanup happens at
> > -         * multifd_send_channel_destroy()
> > -         */
> > -        object_unref(OBJECT(ioc));
> > -    }
> > +    /*
> > +     * For error cases (TLS or non-TLS), IO channel is always freed here
> > +     * rather than when cleanup multifd: since p->c is not set, multifd
> > +     * cleanup code doesn't even know its existance.
> 
> Small nit:
> s/existance/existence
> 
> BTW, I just noticed that multifd_channel_connect() can't fail, probably
> would be good to turn it into a void function.

Yes we can.  I'll add a patch and fix the spell, thanks.

-- 
Peter Xu