Re: [PATCH v2 4/5] migration: Fix rare hang of migration_channel_read_peek()

[Peter Xu <peterx@redhat.com>]

On Wed, Jul 29, 2026 at 03:43:43PM +0100, Daniel P. Berrangé wrote:
> On Tue, Jul 28, 2026 at 05:29:35PM -0400, Peter Xu wrote:
> > On Tue, Jul 28, 2026 at 05:04:16PM -0400, Peter Xu wrote:
> > > In an unlikely case, when a migration stream is attached to the destination
> > > QEMU and only send <4 bytes to the channel as magic, it's possible that
> > > migration_channel_read_peek() may spin forever.
> > > 
> > > Fix it by adding a manual sleep for partial read.
> > > 
> > > Since the path isn't attached to a coroutine, it means when partial read
> > > happens, there's yet not much we can do but hang the main thread, it will
> > > happen even for len==0 case.  It means monitors can hang due to this,
> > > either partial read or no data arrived (but connection established).
> > > 
> > > Leave this for later, the hope is this is extremely rare in production.
> > > 
> > > Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3889
> > > Reported-by: Feifan Qian <bea1e@proton.me>
> > > Cc: Daniel P. Berrangé <berrange@redhat.com>
> > > Signed-off-by: Peter Xu <peterx@redhat.com>
> > > ---
> > >  migration/channel.c | 11 +++++++++--
> > >  1 file changed, 9 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/migration/channel.c b/migration/channel.c
> > > index 1e2935f926..f446561b59 100644
> > > --- a/migration/channel.c
> > > +++ b/migration/channel.c
> > > @@ -296,9 +296,16 @@ int migration_channel_read_peek(QIOChannel *ioc,
> > >  
> > >          if (len == buflen) {
> > >              break;
> > > +        } else if (len == 0) {
> > 
> > I think this should be QIO_CHANNEL_ERR_BLOCK, not 0..  I'll fix it when I
> > post v3, and I'll do some more tests.
> 
> Yep, 0 == EOF, so you'll need ERR_BLOCK.
> 
> > 
> > > +            qio_channel_wait_cond(ioc, G_IO_IN);
> > > +        } else {
> > > +            /*
> > > +             * When partially ready, we can't use qio_channel_wait_cond()
> > > +             * because it will return immediately.  Apply a manual wait.
> > > +             */
> > > +            assert(!qemu_in_coroutine());
> > > +            g_usleep(1000);
> 
> So this will block the main loop, but this is OK because
> 
>  * If the network is untrusted, migration should have been configured
>    to use TLS with a certificate allow-list. This peek takes place
>    after the TLS handshake so is protected
>  * On the dest host, it doesn't hugely matter that we block the main
>    loop as there's no running guest yet that will stall. Just any
>    use of QMP commands on the dest will stall.

Yes, it's more about hanging the monitors.  I think that's indeed an
unwanted behavior, it's just that such behavior existed since the read peek
was merged so it's a while, and it won't happen in normal cases.

To fix it, we can consider creating the coroutine early, but my gut feeling
is it'll be hard because we can have quite some assumptions that all things
will be serialized during establishing all channels, so I suspect we're not
ready having >1 coroutines each on setting up some channel.

A better way might be that if we can land the threadify loadvm series:

https://lore.kernel.org/r/20251022192612.2737648-1-peterx@redhat.com

Then we can offload all channel operations into the thread and I think it's
easier creating the thread early instead, since everything will still be
serialized in that thread with no worry of half-processed coroutines. The
main thread should forward each new iochannel to the thread and continue
the event loop.

Thanks,

-- 
Peter Xu