Re: [Qemu-devel] [PATCH] block/migration: Disable cache invalidate for incoming migration

[Paolo Bonzini <pbonzini@redhat.com>]

Il 09/10/2014 04:50, Alexey Kardashevskiy ha scritto:
> When migrated using libvirt with "--copy-storage-all", at the end of
> migration there is race between NBD mirroring task trying to do flush
> and migration completion, both end up invalidating cache. Since qcow2
> driver does not handle this situation very well, random crashes happen.
> 
> This disables the BDRV_O_INCOMING flag for the block device being migrated
> once the cache has been invalidated.
> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
> Changes:
> v1 (v0 was RFC):
> * added a comment to nbd_export_new() as suggested by Stefan
> ---
>  block.c     | 18 +++++-------------
>  migration.c |  1 -
>  nbd.c       |  6 ++++++
>  3 files changed, 11 insertions(+), 14 deletions(-)
> 
> diff --git a/block.c b/block.c
> index c5a251c..6314af7 100644
> --- a/block.c
> +++ b/block.c
> @@ -5048,6 +5048,11 @@ void bdrv_invalidate_cache(BlockDriverState *bs, Error **errp)
>          return;
>      }
>  
> +    if (!(bs->open_flags & BDRV_O_INCOMING)) {
> +        return;
> +    }
> +    bs->open_flags &= ~(BDRV_O_INCOMING);

Unnecessary parentheses.

> +
>      if (bs->drv->bdrv_invalidate_cache) {
>          bs->drv->bdrv_invalidate_cache(bs, &local_err);
>      } else if (bs->file) {
> @@ -5083,19 +5088,6 @@ void bdrv_invalidate_cache_all(Error **errp)
>      }
>  }
>  
> -void bdrv_clear_incoming_migration_all(void)
> -{
> -    BlockDriverState *bs;
> -
> -    QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
> -        AioContext *aio_context = bdrv_get_aio_context(bs);
> -
> -        aio_context_acquire(aio_context);
> -        bs->open_flags = bs->open_flags & ~(BDRV_O_INCOMING);
> -        aio_context_release(aio_context);
> -    }
> -}
> -
>  int bdrv_flush(BlockDriverState *bs)
>  {
>      Coroutine *co;
> diff --git a/migration.c b/migration.c
> index 8d675b3..c49a05a 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -103,7 +103,6 @@ static void process_incoming_migration_co(void *opaque)
>      }
>      qemu_announce_self();
>  
> -    bdrv_clear_incoming_migration_all();
>      /* Make sure all file formats flush their mutable metadata */
>      bdrv_invalidate_cache_all(&local_err);
>      if (local_err) {
> diff --git a/nbd.c b/nbd.c
> index e9b539b..a7bce45 100644
> --- a/nbd.c
> +++ b/nbd.c
> @@ -972,6 +972,12 @@ NBDExport *nbd_export_new(BlockDriverState *bs, off_t dev_offset,
>      exp->ctx = bdrv_get_aio_context(bs);
>      bdrv_ref(bs);
>      bdrv_add_aio_context_notifier(bs, bs_aio_attached, bs_aio_detach, exp);
> +    /*
> +     * NBD exports are used for non-shared storage migration.  Make sure
> +     * that BDRV_O_INCOMING is cleared and the image is ready for write
> +     * access since the export could be available before migration handover.
> +     */
> +    bdrv_invalidate_cache(bs, NULL);
>      return exp;
>  }
>  
> 

Apart from the above style remark,

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>