Re: [PATCH v2 10/22] migration/block-dirty-bitmap: cancel migration on shutdown

[Andrey Shinkevich]

On 17/02/2020 18:02, Vladimir Sementsov-Ogievskiy wrote:
> If target is turned of prior to postcopy finished, target crashes
> because busy bitmaps are found at shutdown.
> Canceling incoming migration helps, as it removes all unfinished (and
> therefore busy) bitmaps.
>
> Similarly on source we crash in bdrv_close_all which asserts that all
> bdrv states are removed, because bdrv states involved into dirty bitmap
> migration are referenced by it. So, we need to cancel outgoing
> migration as well.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> ---
>   migration/migration.h          |  2 ++
>   migration/block-dirty-bitmap.c | 16 ++++++++++++++++
>   migration/migration.c          | 13 +++++++++++++
>   3 files changed, 31 insertions(+)
>
> diff --git a/migration/migration.h b/migration/migration.h
> index 2948f2387b..2de6b8bbe2 100644
> --- a/migration/migration.h
> +++ b/migration/migration.h
> @@ -332,6 +332,8 @@ void migrate_send_rp_recv_bitmap(MigrationIncomingState 
> *mis,
>   void migrate_send_rp_resume_ack(MigrationIncomingState *mis, uint32_t value);
>   
>   void dirty_bitmap_mig_before_vm_start(void);
> +void dirty_bitmap_mig_cancel_outgoing(void);
> +void dirty_bitmap_mig_cancel_incoming(void);
>   void migrate_add_address(SocketAddress *address);
>   
>   int foreach_not_ignored_block(RAMBlockIterFunc func, void *opaque);
> diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
> index aea5326804..3ca425d95e 100644
> --- a/migration/block-dirty-bitmap.c
> +++ b/migration/block-dirty-bitmap.c
> @@ -585,6 +585,22 @@ static void cancel_incoming_locked(DBMLoadState *s)
>       s->bitmaps = NULL;
>   }
>   
> +void dirty_bitmap_mig_cancel_outgoing(void)
> +{
> +    dirty_bitmap_do_save_cleanup(&dbm_state.save);

The comment above the dirty_bitmap_do_save_cleanup() says:
"Called with iothread lock taken"

> +}
> +
> +void dirty_bitmap_mig_cancel_incoming(void)
> +{
> +    DBMLoadState *s = &dbm_state.load;
> +
> +    qemu_mutex_lock(&s->lock);
> +
> +    cancel_incoming_locked(s);
> +
> +    qemu_mutex_unlock(&s->lock);
> +}
> +
>   static void dirty_bitmap_load_complete(QEMUFile *f, DBMLoadState *s)
>   {
>       GSList *item;
> diff --git a/migration/migration.c b/migration/migration.c
> index 515047932c..7c605ba218 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -181,6 +181,19 @@ void migration_shutdown(void)
>        */
>       migrate_fd_cancel(current_migration);
>       object_unref(OBJECT(current_migration));
> +
> +    /*
> +     * Cancel outgoing migration of dirty bitmaps. It should
> +     * at least unref used block nodes.
> +     */
> +    dirty_bitmap_mig_cancel_outgoing();
> +
> +    /*
> +     * Cancel incoming migration of dirty bitmaps. Dirty bitmaps
> +     * are non-critical data, and their loss never considered as
> +     * something serious.
> +     */
> +    dirty_bitmap_mig_cancel_incoming();
>   }
>   
>   /* For outgoing */

Reviewed-by: Andrey Shinkevich <address@hidden>
--
With the best regards,
Andrey Shinkevich