[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 1/1] yank: Unregister function when using TLS migration
From: |
Peter Xu |
Subject: |
Re: [PATCH 1/1] yank: Unregister function when using TLS migration |
Date: |
Thu, 27 May 2021 08:23:52 -0400 |
On Thu, May 27, 2021 at 09:46:54AM +0100, Daniel P. Berrangé wrote:
> On Wed, May 26, 2021 at 05:58:58PM -0400, Peter Xu wrote:
> > On Wed, May 26, 2021 at 11:21:03PM +0200, Lukas Straub wrote:
> > > On Wed, 26 May 2021 16:40:35 -0400
> > > Peter Xu <peterx@redhat.com> wrote:
> > >
> > > > On Wed, May 26, 2021 at 05:05:40PM -0300, Leonardo Bras wrote:
> > > > > After yank feature was introduced, whenever migration is started
> > > > > using TLS,
> > > > > the following error happens in both source and destination hosts:
> > > > >
> > > > > (qemu) qemu-kvm: ../util/yank.c:107: yank_unregister_instance:
> > > > > Assertion `QLIST_EMPTY(&entry->yankfns)' failed.
> > > > >
> > > > > This happens because of a missing yank_unregister_function() when
> > > > > using
> > > > > qio-channel-tls.
> > > > >
> > > > > Fix this by also allowing TYPE_QIO_CHANNEL_TLS object type to perform
> > > > > yank_unregister_function() in channel_close() and
> > > > > multifd_load_cleanup().
> > > > >
> > > > > Fixes: 50186051f ("Introduce yank feature")
> > > > > Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1964326
> > > > > Signed-off-by: Leonardo Bras <leobras.c@gmail.com>
> > > >
> > > > Leo,
> > > >
> > > > Thanks for looking into it!
> > > >
> > > > So before looking int the fix... I do have a doubt on why we only
> > > > enable yank
> > > > on socket typed, as I think tls should also work with
> > > > qio_channel_shutdown().
> > > >
> > > > IIUC the confused thing here is we register only for qio-socket,
> > > > however tls
> > > > will actually call migration_channel_connect() twice, first with a
> > > > qio-socket,
> > > > then with the real tls-socket. For tls I feel like we have registered
> > > > with the
> > > > wrong channel - instead of the wrapper socket ioc, we should register
> > > > to the
> > > > final tls ioc?
> > > >
> > > > Lukas, is there a reason?
> > > >
> > >
> > > Hi,
> > > There is no specific reason. Both ways work equally well in preventing
> > > qemu from hanging. shutdown() for tls-channel just makes it abort a
> > > little sooner (by not attempting to encrypt and send data anymore).
> > >
> > > I don't lean either way. I guess registering it on the tls-channel
> > > makes is a bit more explicit and clearer.
> >
> > Agreed, because IMHO logically the migration code should not be aware of
> > internals of IOChannels, e.g., we shouldn't need to know ioc->master is the
> > socket ioc of tls ioc to unregister.
>
> I think it is atually better to ignore the TLS channel and *always* yank
> on the undering socket IO channel. The yank functionality is intended to
> be used in a scenario where we know the channels are broken. If yank
> calls the high level IO channel it is potentially going to try to do a
> cleanup shutdown that we know will fail because of the broken network.
Could you elaborate what's the "cleanup shutdown"?
The yank calls migration_yank_iochannel:
void migration_yank_iochannel(void *opaque)
{
QIOChannel *ioc = QIO_CHANNEL(opaque);
qio_channel_shutdown(ioc, QIO_CHANNEL_SHUTDOWN_BOTH, NULL);
}
Where qio_channel_shutdown for tls is nothing but delivers that to the master
channel:
static int qio_channel_tls_shutdown(QIOChannel *ioc,
QIOChannelShutdown how,
Error **errp)
{
QIOChannelTLS *tioc = QIO_CHANNEL_TLS(ioc);
qatomic_or(&tioc->shutdown, how);
return qio_channel_shutdown(tioc->master, how, errp);
}
So I thought it was a nice wrapper just for things like this, and I didn't see
anything it does more than the io_shutdown for the socket channel. Did I miss
something?
Thanks,
--
Peter Xu
- [PATCH 1/1] yank: Unregister function when using TLS migration, Leonardo Bras, 2021/05/26
- Re: [PATCH 1/1] yank: Unregister function when using TLS migration, Peter Xu, 2021/05/26
- Re: [PATCH 1/1] yank: Unregister function when using TLS migration, Lukas Straub, 2021/05/26
- Re: [PATCH 1/1] yank: Unregister function when using TLS migration, Peter Xu, 2021/05/26
- Re: [PATCH 1/1] yank: Unregister function when using TLS migration, Daniel P . Berrangé, 2021/05/27
- Re: [PATCH 1/1] yank: Unregister function when using TLS migration,
Peter Xu <=
- Re: [PATCH 1/1] yank: Unregister function when using TLS migration, Daniel P . Berrangé, 2021/05/27
- Re: [PATCH 1/1] yank: Unregister function when using TLS migration, Peter Xu, 2021/05/27
- Re: [PATCH 1/1] yank: Unregister function when using TLS migration, Daniel P . Berrangé, 2021/05/27
- Re: [PATCH 1/1] yank: Unregister function when using TLS migration, Dr. David Alan Gilbert, 2021/05/27
- Re: [PATCH 1/1] yank: Unregister function when using TLS migration, Peter Xu, 2021/05/27
- Re: [PATCH 1/1] yank: Unregister function when using TLS migration, Lukas Straub, 2021/05/27
Re: [PATCH 1/1] yank: Unregister function when using TLS migration, Lukas Straub, 2021/05/26