Re: [PATCH] migration: Fix crash on second migration when cancel early

[Fabiano Rosas <farosas@suse.de>]

Fabiano Rosas <farosas@suse.de> writes:

> Peter Xu <peterx@redhat.com> writes:
>
>> Marc-André reported an issue on QEMU crash when retrying a cancelled
>> migration during early setup phase, see "Link:" for more information, and
>> also easy way to reproduce.
>>
>> This patch is a replacement of the prior fix proposed by not only switching
>> to migration_cleanup(), but also fixing it from CPR side, so that we track
>> hup_source properly to know if src QEMU is waiting or the HUP signal.
>>
>> To put it simple: this chunk of special casing in migration_cancel() should
>> not affect normal migration, but only cpr-transfer migration to cover the
>> small window when the src QEMU is waiting for a HUP signal on cpr
>> channel (so that src QEMU can continue the migration on the main channel).
>>
>> To achieve that, we'll also need to remember to detach the hup_source
>> whenenver invoked: after that point, we should always be able to cleanup
>> the migration.
>>
>> It's not a generic operation to explicitly detach a gsource from its
>> context while in its dispatch() function.  But it should be safe, because
>> gsource disptch() will only happen with a boosted refcount for the
>> dispatcher so that the gsource will not be freed until the callback
>> completes. It's also safe to return G_SOURCE_REMOVE after the gsource is
>> detached, as glib will simply ignore the G_SOURCE_REMOVE.
>>
>> One can refer to latest 2.86.5 glib code in g_main_dispatch() for that:
>>
>> https://github.com/GNOME/glib/blob/2.86.5/glib/gmain.c#L3592
>>
>> When at this, add a bunch of assertions to make sure nothing surprises us.
>>
>> After this patch applied, the 2nd migration will not crash QEMU, instead
>> it'll be in CANCELLING until the socket connection times out (it will take
>> ~2min on my Fedora default kernel).  During this process no 2nd migration
>> will be allowed, and after it timed out migration can be restarted.
>>
>> It's because so far we don't have control over socket_connect_outgoing(),
>> or anything yet managed by a task executed in qio_task_run_in_thread().
>> Speeding up the cancellation to be left for future.
>>
>> I also tested cpr-transfer by only providing cpr channel not the main
>> channel (with -incoming defer), kickoff migration on source, then cancel it
>> on source directly without providing the main channel.  It keeps working.
>>
>> I wanted to add an unit test for that but it'll need to refactor current
>> cpr-transfer tests first; let's leave it for later.
>>
>> Link: https://lore.kernel.org/r/20260417184742.293061-1-marcandre.lureau@redhat.com
>> Reported-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> Signed-off-by: Peter Xu <peterx@redhat.com>
>> ---
>>  include/migration/cpr.h  |  1 +
>>  migration/migration.h    |  5 +++++
>>  migration/cpr-transfer.c | 10 ++++++++++
>>  migration/migration.c    | 31 +++++++++++++++++++++++--------
>>  4 files changed, 39 insertions(+), 8 deletions(-)
>>
>> diff --git a/include/migration/cpr.h b/include/migration/cpr.h
>> index 5850fd1788..ebf09a2f0a 100644
>> --- a/include/migration/cpr.h
>> +++ b/include/migration/cpr.h
>> @@ -57,6 +57,7 @@ QEMUFile *cpr_transfer_input(MigrationChannel *channel, Error **errp);
>>  void cpr_transfer_add_hup_watch(MigrationState *s, QIOChannelFunc func,
>>                                  void *opaque);
>>  void cpr_transfer_source_destroy(MigrationState *s);
>> +bool cpr_transfer_source_active(MigrationState *s);
>>  
>>  void cpr_exec_init(void);
>>  QEMUFile *cpr_exec_output(Error **errp);
>> diff --git a/migration/migration.h b/migration/migration.h
>> index b6888daced..2bc2787480 100644
>> --- a/migration/migration.h
>> +++ b/migration/migration.h
>> @@ -514,6 +514,11 @@ struct MigrationState {
>>      bool postcopy_package_loaded;
>>      QemuEvent postcopy_package_loaded_event;
>>  
>> +    /*
>> +     * When set, it means cpr-transfer is waiting for the HUP signal from
>> +     * destination to continue the 2nd step of migration via the main
>> +     * channel.
>> +     */
>>      GSource *hup_source;
>>  
>>      /*
>> diff --git a/migration/cpr-transfer.c b/migration/cpr-transfer.c
>> index 61d5c9dce2..9defe7bad7 100644
>> --- a/migration/cpr-transfer.c
>> +++ b/migration/cpr-transfer.c
>> @@ -6,6 +6,7 @@
>>   */
>>  
>>  #include "qemu/osdep.h"
>> +#include "qemu/main-loop.h"
>>  #include "qapi/clone-visitor.h"
>>  #include "qapi/error.h"
>>  #include "qapi/qapi-visit-migration.h"
>> @@ -79,6 +80,7 @@ QEMUFile *cpr_transfer_input(MigrationChannel *channel, Error **errp)
>>  void cpr_transfer_add_hup_watch(MigrationState *s, QIOChannelFunc func,
>>                                  void *opaque)
>>  {
>> +    assert(bql_locked());
>>      s->hup_source = qio_channel_create_watch(cpr_state_ioc(), G_IO_HUP);
>
> Before I review the patch in detail, let me just make a high level
> comment here:
>
> I wonder if we should have a "register" in the iochannel layer for the
> several watches we create. So we could at migration_cancel time call
> g_clear_handle/g_source_destroy on all at the same time. The management
> of these source ids is getting too particular.
>
> I'm seeing that exec, fd and file migration all ignore the id returned
> by channel-watch.c. In the case of exec this is causing
> qio_channel_command_finalize() to be skipped, leaving the exec'ed
> command process behind! So what I did as an experiment was to register
> watches like this:
>
>   static inline void migration_watch_data(void) {};

Oops, not inline.