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

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

[Alexey Kardashevskiy]

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);
+
     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;
 }
 
-- 
2.0.0

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

[Paolo Bonzini]

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>