Re: [PATCH 1/4] migration: Do not try to start VM if disk activation fails

[Fabiano Rosas <farosas@suse.de>]

Juraj Marcin <jmarcin@redhat.com> writes:

> From: Peter Xu <peterx@redhat.com>
>
> If a rare split brain happens (e.g. dest QEMU started running somehow,
> taking shared drive locks), src QEMU may not be able to activate the
> drives anymore.  In this case, src QEMU shouldn't start the VM or it might
> crash the block layer later with something like:
>
> bdrv_co_write_req_prepare: Assertion `!(bs->open_flags &
> BDRV_O_INACTIVE)' failed.

This patch doesn't fix the assert, so I think it's best not to mention
it at all. We've had a few instances of this assert (or similar) and
there's a fair chance someone still looks at git log searching for a
backport.

>
> Meanwhile, src QEMU cannot try to continue either even if dest QEMU can
> release the drive locks (e.g. by QMP "stop").  Because as long as dest QEMU
> started running, it means dest QEMU's RAM is the only version that is
> consistent with current status of the shared storage.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  migration/migration.c | 29 ++++++++++++++++++++++++-----
>  1 file changed, 24 insertions(+), 5 deletions(-)
>
> diff --git a/migration/migration.c b/migration/migration.c
> index 10c216d25d..54dac3db88 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -3502,6 +3502,8 @@ static MigIterateState migration_iteration_run(MigrationState *s)
>  
>  static void migration_iteration_finish(MigrationState *s)
>  {
> +    Error *local_err = NULL;
> +
>      bql_lock();
>  
>      /*
> @@ -3525,11 +3527,28 @@ static void migration_iteration_finish(MigrationState *s)
>      case MIGRATION_STATUS_FAILED:
>      case MIGRATION_STATUS_CANCELLED:
>      case MIGRATION_STATUS_CANCELLING:
> -        /*
> -         * Re-activate the block drives if they're inactivated.  Note, COLO
> -         * shouldn't use block_active at all, so it should be no-op there.
> -         */
> -        migration_block_activate(NULL);
> +        if (!migration_block_activate(&local_err)) {
> +            /*
> +            * Re-activate the block drives if they're inactivated.

Comment formatting is wrong.

> +            *
> +            * If it fails (e.g. in case of a split brain, where dest QEMU
> +            * might have taken some of the drive locks and running!), do
> +            * not start VM, instead wait for mgmt to decide the next step.
> +            *
> +            * If dest already started, it means dest QEMU should contain
> +            * all the data it needs and it properly owns all the drive
> +            * locks.  Then even if src QEMU got a FAILED in migration, it
> +            * normally should mean we should treat the migration as
> +            * COMPLETED.
> +            *
> +            * NOTE: it's not safe anymore to start VM on src now even if
> +            * dest would release the drive locks.  It's because as long as
> +            * dest started running then only dest QEMU's RAM is consistent
> +            * with the shared storage.
> +            */
> +            error_free(local_err);
> +            break;
> +        }
>          if (runstate_is_live(s->vm_old_state)) {
>              if (!runstate_check(RUN_STATE_SHUTDOWN)) {
>                  vm_start();

Reviewed-by: Fabiano Rosas <farosas@suse.de>