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

Peter Xu <[email protected]> writes:

> On Tue, Jul 28, 2026 at 04:59:20PM -0300, Fabiano Rosas wrote:
>> Peter Xu <[email protected]> writes:
>> 
>> > On Tue, Jul 28, 2026 at 05:24:36PM +0100, Daniel P. Berrangé wrote:
>> >> On Tue, Jul 28, 2026 at 11:52:46AM -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 without yielding in the main
>> >> > thread causing two unwanted consequences:
>> >> > 
>> >> > - CPU will spin 100% waiting for the rest bytes until it reaches 4
>> >> > - (more importantly..) Main thread is stuck during this process as the qio
>> >> >   operation won't really yield the coroutine
>> >> > 
>> >> > Fix it by consuming the bytes that arrived.
>> >> > 
>> >> > Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3889
>> >> > Cc: Daniel P. Berrangé <[email protected]>
>> >> > Reported-by: Feifan Qian <[email protected]>
>> >> > Signed-off-by: Peter Xu <[email protected]>
>> >> > ---
>> >> >  migration/channel.c | 26 +++++++++++++++++++++++---
>> >> >  1 file changed, 23 insertions(+), 3 deletions(-)
>> >> > 
>> >> > diff --git a/migration/channel.c b/migration/channel.c
>> >> > index 1e2935f926..28fe1d2906 100644
>> >> > --- a/migration/channel.c
>> >> > +++ b/migration/channel.c
>> >> > @@ -294,11 +294,31 @@ int migration_channel_read_peek(QIOChannel *ioc,
>> >> >              return -1;
>> >> >          }
>> >> >  
>> >> > -        if (len == buflen) {
>> >> > +        if (len == iov.iov_len) {
>> >> >              break;
>> >> > -        }
>> >> > +        } else if (len == 0) {
>> >> > +            qio_channel_wait_cond(ioc, G_IO_IN);
>> >> > +        } else {
>> >> > +            ssize_t received = len;
>> >> >  
>> >> > -        qio_channel_wait_cond(ioc, G_IO_IN);
>> >> > +            /*
>> >> > +             * Partially arrived, read out to make qio_channel_wait_cond()
>> >> > +             * won't return immediately, causing an unwanted spin on this
>> >> > +             * CPU.
>> >> > +             */
>> >> > +            iov.iov_len = len;
>> >> > +            len = qio_channel_readv_full(ioc, &iov, 1, NULL, NULL, 0, errp);
>> >> 
>> >> Sure this breaks the API behaviour that the caller is expecting to
>> >> see.
>> >> 
>> >> migration_channel_identify will call migration_channel_read_peek
>> >> to match the magic bytes.
>> >> 
>> >> But something later in the flow will actually try to read the magic
>> >> bytes.  By consuming them in this migration_channel_read_peek
>> >> method, surely we're breaking the code that wants to read the bytes
>> >> later.
>> >
>> > Ah right, stupid me.  The best then is for partial read we apply a manual
>> > wait.
>> >
>> > I can also revert 604bb1badc ("migration: Properly wait on G_IO_IN when
>> > peeking messages"), looping with 1ms interval for data isn't too bad.  But
>> > the best is we only do that for partial, so:
>> >
>> > diff --git a/migration/channel.c b/migration/channel.c
>> > index 1e2935f926..88f89521f8 100644
>> > --- a/migration/channel.c
>> > +++ b/migration/channel.c
>> > @@ -296,9 +296,19 @@ int migration_channel_read_peek(QIOChannel *ioc,
>> >
>> >          if (len == buflen) {
>> >              break;
>> > +        } else if (len == 0) {
>> > +            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.
>> > +             */
>> > +            if (qemu_in_coroutine()) {
>> 
>> There's no coroutine at this point, could replace this with an assert.
>
> Hmm, true..  Then it means this can stuck the main thread even if no data
> arrived (len==0), hang monitors..  I hope it's not a major issue and
> shouldn't easily happen in real life, because normally when src connected,
> at least the headers will be dumped very soon.
>

If I'm not mistaken the watch only dispatches when there's IO.

> I'll switch to assert for now.
>
>> 
>> > +                qemu_co_sleep_ns(QEMU_CLOCK_REALTIME, 1000000);
>> > +            } else {
>> > +                g_usleep(1000);
>> > +            }
>> >          }
>> > -
>> > -        qio_channel_wait_cond(ioc, G_IO_IN);
>> >      }
>> >
>> >      return 0;
>> >
>> > Any preference?
>> >
>> > Thanks,
>> >
>> >> 
>> >> > +            /*
>> >> > +             * QIO_CHANNEL_ERR_BLOCK also shouldn't happen, due to the
>> >> > +             * prior peek just happened.  We should be pretty sure we will
>> >> > +             * read what we peeked, or the channel was broken.
>> >> > +             */
>> >> > +            if (len != received) {
>> >> > +                return -1;
>> >> > +            }
>> >> > +            iov.iov_base += received;
>> >> > +            iov.iov_len = buflen - received;
>> >> > +        }
>> >> >      }
>> >> >  
>> >> >      return 0;
>> >> > -- 
>> >> > 2.54.0
>> >> > 
>> >> 
>> >> With regards,
>> >> Daniel
>> >> -- 
>> >> |: https://berrange.com       ~~        https://hachyderm.io/@berrange :|
>> >> |: https://libvirt.org          ~~          https://entangle-photo.org :|
>> >> |: https://pixelfed.art/berrange   ~~    https://fstop138.berrange.com :|
>> >> 
>>