Re: [Qemu-devel] [PATCH 1/2] migration: move bdrv_invalidate_cache_all of of coroutine context

From: Fam Zheng <famz@redhat.com>
To: "Denis V. Lunev" <den@openvz.org>
Cc: Amit Shah <amit.shah@redhat.com>,
	Paolo Bonzini <pbonzini@redhat.com>,
	qemu-devel@nongnu.org, Juan Quintela <quintela@redhat.com>
Subject: Re: [Qemu-devel] [PATCH 1/2] migration: move bdrv_invalidate_cache_all of of coroutine context
Date: Tue, 23 Feb 2016 20:54:16 +0800	[thread overview]
Message-ID: <20160223125416.GB26812@ad.usersys.redhat.com> (raw)
In-Reply-To: <1455259174-3384-2-git-send-email-den@openvz.org>

On Fri, 02/12 09:39, Denis V. Lunev wrote:
> There is a possibility to hit an assert in qcow2_get_specific_info that
> s->qcow_version is undefined. This happens when VM in starting from
> suspended state, i.e. it processes incoming migration, and in the same
> time 'info block' is called.
> 
> The problem is that qcow2_invalidate_cache() closes the image and
> memset()s BDRVQcowState in the middle.
> 
> The patch moves processing of bdrv_invalidate_cache_all out of
> coroutine context for standard migration to avoid that.
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Paolo Bonzini <pbonzini@redhat.com>
> CC: Juan Quintela <quintela@redhat.com>
> CC: Amit Shah <amit.shah@redhat.com>
> ---
>  migration/migration.c | 89 ++++++++++++++++++++++++++++-----------------------
>  1 file changed, 49 insertions(+), 40 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index a64cfcd..1f8535e 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -323,13 +323,59 @@ void qemu_start_incoming_migration(const char *uri, Error **errp)
>      }
>  }
>  
> +static void process_incoming_migration_bh(void *opaque)
> +{
> +    Error *local_err = NULL;
> +    MigrationIncomingState *mis = opaque;
> +
> +    /* Make sure all file formats flush their mutable metadata */
> +    bdrv_invalidate_cache_all(&local_err);
> +    if (local_err) {
> +        migrate_set_state(&mis->state, MIGRATION_STATUS_ACTIVE,
> +                          MIGRATION_STATUS_FAILED);
> +        error_report_err(local_err);
> +        migrate_decompress_threads_join();
> +        exit(EXIT_FAILURE);
> +    }
> +
> +    /*
> +     * This must happen after all error conditions are dealt with and
> +     * we're sure the VM is going to be running on this host.
> +     */
> +    qemu_announce_self();
> +
> +    /* If global state section was not received or we are in running
> +       state, we need to obey autostart. Any other state is set with
> +       runstate_set. */
> +
> +    if (!global_state_received() ||
> +        global_state_get_runstate() == RUN_STATE_RUNNING) {
> +        if (autostart) {
> +            vm_start();
> +        } else {
> +            runstate_set(RUN_STATE_PAUSED);
> +        }
> +    } else {
> +        runstate_set(global_state_get_runstate());
> +    }
> +    migrate_decompress_threads_join();
> +    /*
> +     * This must happen after any state changes since as soon as an external
> +     * observer sees this event they might start to prod at the VM assuming
> +     * it's ready to use.
> +     */
> +    migrate_set_state(&mis->state, MIGRATION_STATUS_ACTIVE,
> +                      MIGRATION_STATUS_COMPLETED);
> +    migration_incoming_state_destroy();
> +}
> +
>  static void process_incoming_migration_co(void *opaque)
>  {
>      QEMUFile *f = opaque;
> -    Error *local_err = NULL;
>      MigrationIncomingState *mis;
>      PostcopyState ps;
>      int ret;
> +    QEMUBH *bh;
>  
>      mis = migration_incoming_state_new(f);
>      postcopy_state_set(POSTCOPY_INCOMING_NONE);
> @@ -369,45 +415,8 @@ static void process_incoming_migration_co(void *opaque)
>          exit(EXIT_FAILURE);
>      }
>  
> -    /* Make sure all file formats flush their mutable metadata */
> -    bdrv_invalidate_cache_all(&local_err);
> -    if (local_err) {
> -        migrate_set_state(&mis->state, MIGRATION_STATUS_ACTIVE,
> -                          MIGRATION_STATUS_FAILED);
> -        error_report_err(local_err);
> -        migrate_decompress_threads_join();
> -        exit(EXIT_FAILURE);
> -    }
> -
> -    /*
> -     * This must happen after all error conditions are dealt with and
> -     * we're sure the VM is going to be running on this host.
> -     */
> -    qemu_announce_self();
> -
> -    /* If global state section was not received or we are in running
> -       state, we need to obey autostart. Any other state is set with
> -       runstate_set. */
> -
> -    if (!global_state_received() ||
> -        global_state_get_runstate() == RUN_STATE_RUNNING) {
> -        if (autostart) {
> -            vm_start();
> -        } else {
> -            runstate_set(RUN_STATE_PAUSED);
> -        }
> -    } else {
> -        runstate_set(global_state_get_runstate());
> -    }
> -    migrate_decompress_threads_join();
> -    /*
> -     * This must happen after any state changes since as soon as an external
> -     * observer sees this event they might start to prod at the VM assuming
> -     * it's ready to use.
> -     */
> -    migrate_set_state(&mis->state, MIGRATION_STATUS_ACTIVE,
> -                      MIGRATION_STATUS_COMPLETED);
> -    migration_incoming_state_destroy();
> +    bh = qemu_bh_new(process_incoming_migration_bh, mis);

Don't you need to call qemu_bh_delete somewhere? (Also applies to the other
patch)

Fam

> +    qemu_bh_schedule(bh);
>  }
>  
>  void process_incoming_migration(QEMUFile *f)
> -- 
> 2.1.4
> 
> 