[PATCH 0/7] Fix crashes on early shutdown during bitmaps postcopy

[PATCH 0/7] Fix crashes on early shutdown during bitmaps postcopy

[Vladimir Sementsov-Ogievskiy]

Hi all!

Patches 5 and 6 fixes two crashes, triggered by new test case in patch
7.

Vladimir Sementsov-Ogievskiy (7):
  migration/block-dirty-bitmap: refactor incoming state to be one struct
  migration/block-dirty-bitmap: rename finish_lock to just lock
  migration/block-dirty-bitmap: simplify dirty_bitmap_load_complete
  migration/block-dirty-bitmap: keep bitmap state for all bitmaps
  migration/block-dirty-bitmap: cancel migration on shutdown
  migration: handle to_src_file on target only for ram postcopy
  qemu-iotests/199: add early shutdown case to bitmaps postcopy

 migration/migration.h          |   1 +
 migration/block-dirty-bitmap.c | 171 +++++++++++++++++++++------------
 migration/migration.c          |   7 ++
 migration/savevm.c             |  19 ++--
 tests/qemu-iotests/199         |  12 ++-
 tests/qemu-iotests/199.out     |   4 +-
 6 files changed, 142 insertions(+), 72 deletions(-)

-- 
2.21.0

[PATCH 1/7] migration/block-dirty-bitmap: refactor incoming state to be one struct

[Vladimir Sementsov-Ogievskiy]

Move enabled_bitmaps and finish_lock, which are part of incoming state
to DirtyBitmapLoadState, and make static global variable to store state
instead of static local one.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 migration/block-dirty-bitmap.c | 77 +++++++++++++++++++---------------
 1 file changed, 43 insertions(+), 34 deletions(-)

diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
index 7eafface61..281d20f41d 100644
--- a/migration/block-dirty-bitmap.c
+++ b/migration/block-dirty-bitmap.c
@@ -125,6 +125,13 @@ typedef struct DirtyBitmapMigState {
     BlockDriverState *prev_bs;
     BdrvDirtyBitmap *prev_bitmap;
 } DirtyBitmapMigState;
+static DirtyBitmapMigState dirty_bitmap_mig_state;
+
+typedef struct DirtyBitmapLoadBitmapState {
+    BlockDriverState *bs;
+    BdrvDirtyBitmap *bitmap;
+    bool migrated;
+} DirtyBitmapLoadBitmapState;
 
 typedef struct DirtyBitmapLoadState {
     uint32_t flags;
@@ -132,21 +139,15 @@ typedef struct DirtyBitmapLoadState {
     char bitmap_name[256];
     BlockDriverState *bs;
     BdrvDirtyBitmap *bitmap;
-} DirtyBitmapLoadState;
 
-static DirtyBitmapMigState dirty_bitmap_mig_state;
-
-typedef struct DirtyBitmapLoadBitmapState {
-    BlockDriverState *bs;
-    BdrvDirtyBitmap *bitmap;
-    bool migrated;
-} DirtyBitmapLoadBitmapState;
-static GSList *enabled_bitmaps;
-QemuMutex finish_lock;
+    GSList *enabled_bitmaps;
+    QemuMutex finish_lock;
+} DirtyBitmapLoadState;
+static DirtyBitmapLoadState dbm_load_state;
 
 void init_dirty_bitmap_incoming_migration(void)
 {
-    qemu_mutex_init(&finish_lock);
+    qemu_mutex_init(&dbm_load_state.finish_lock);
 }
 
 static uint32_t qemu_get_bitmap_flags(QEMUFile *f)
@@ -439,8 +440,9 @@ static void dirty_bitmap_save_pending(QEMUFile *f, void *opaque,
 }
 
 /* First occurrence of this bitmap. It should be created if doesn't exist */
-static int dirty_bitmap_load_start(QEMUFile *f, DirtyBitmapLoadState *s)
+static int dirty_bitmap_load_start(QEMUFile *f)
 {
+    DirtyBitmapLoadState *s = &dbm_load_state;
     Error *local_err = NULL;
     uint32_t granularity = qemu_get_be32(f);
     uint8_t flags = qemu_get_byte(f);
@@ -482,7 +484,8 @@ static int dirty_bitmap_load_start(QEMUFile *f, DirtyBitmapLoadState *s)
         b->bs = s->bs;
         b->bitmap = s->bitmap;
         b->migrated = false;
-        enabled_bitmaps = g_slist_prepend(enabled_bitmaps, b);
+        dbm_load_state.enabled_bitmaps =
+            g_slist_prepend(dbm_load_state.enabled_bitmaps, b);
     }
 
     return 0;
@@ -492,9 +495,11 @@ void dirty_bitmap_mig_before_vm_start(void)
 {
     GSList *item;
 
-    qemu_mutex_lock(&finish_lock);
+    qemu_mutex_lock(&dbm_load_state.finish_lock);
 
-    for (item = enabled_bitmaps; item; item = g_slist_next(item)) {
+    for (item = dbm_load_state.enabled_bitmaps; item;
+         item = g_slist_next(item))
+    {
         DirtyBitmapLoadBitmapState *b = item->data;
 
         if (b->migrated) {
@@ -506,21 +511,24 @@ void dirty_bitmap_mig_before_vm_start(void)
         g_free(b);
     }
 
-    g_slist_free(enabled_bitmaps);
-    enabled_bitmaps = NULL;
+    g_slist_free(dbm_load_state.enabled_bitmaps);
+    dbm_load_state.enabled_bitmaps = NULL;
 
-    qemu_mutex_unlock(&finish_lock);
+    qemu_mutex_unlock(&dbm_load_state.finish_lock);
 }
 
-static void dirty_bitmap_load_complete(QEMUFile *f, DirtyBitmapLoadState *s)
+static void dirty_bitmap_load_complete(QEMUFile *f)
 {
+    DirtyBitmapLoadState *s = &dbm_load_state;
     GSList *item;
     trace_dirty_bitmap_load_complete();
     bdrv_dirty_bitmap_deserialize_finish(s->bitmap);
 
-    qemu_mutex_lock(&finish_lock);
+    qemu_mutex_lock(&dbm_load_state.finish_lock);
 
-    for (item = enabled_bitmaps; item; item = g_slist_next(item)) {
+    for (item = dbm_load_state.enabled_bitmaps; item;
+         item = g_slist_next(item))
+    {
         DirtyBitmapLoadBitmapState *b = item->data;
 
         if (b->bitmap == s->bitmap) {
@@ -531,7 +539,7 @@ static void dirty_bitmap_load_complete(QEMUFile *f, DirtyBitmapLoadState *s)
 
     if (bdrv_dirty_bitmap_has_successor(s->bitmap)) {
         bdrv_dirty_bitmap_lock(s->bitmap);
-        if (enabled_bitmaps == NULL) {
+        if (dbm_load_state.enabled_bitmaps == NULL) {
             /* in postcopy */
             bdrv_reclaim_dirty_bitmap_locked(s->bitmap, &error_abort);
             bdrv_enable_dirty_bitmap_locked(s->bitmap);
@@ -550,11 +558,12 @@ static void dirty_bitmap_load_complete(QEMUFile *f, DirtyBitmapLoadState *s)
         bdrv_dirty_bitmap_unlock(s->bitmap);
     }
 
-    qemu_mutex_unlock(&finish_lock);
+    qemu_mutex_unlock(&dbm_load_state.finish_lock);
 }
 
-static int dirty_bitmap_load_bits(QEMUFile *f, DirtyBitmapLoadState *s)
+static int dirty_bitmap_load_bits(QEMUFile *f)
 {
+    DirtyBitmapLoadState *s = &dbm_load_state;
     uint64_t first_byte = qemu_get_be64(f) << BDRV_SECTOR_BITS;
     uint64_t nr_bytes = (uint64_t)qemu_get_be32(f) << BDRV_SECTOR_BITS;
     trace_dirty_bitmap_load_bits_enter(first_byte >> BDRV_SECTOR_BITS,
@@ -598,8 +607,9 @@ static int dirty_bitmap_load_bits(QEMUFile *f, DirtyBitmapLoadState *s)
     return 0;
 }
 
-static int dirty_bitmap_load_header(QEMUFile *f, DirtyBitmapLoadState *s)
+static int dirty_bitmap_load_header(QEMUFile *f)
 {
+    DirtyBitmapLoadState *s = &dbm_load_state;
     Error *local_err = NULL;
     bool nothing;
     s->flags = qemu_get_bitmap_flags(f);
@@ -647,7 +657,6 @@ static int dirty_bitmap_load_header(QEMUFile *f, DirtyBitmapLoadState *s)
 
 static int dirty_bitmap_load(QEMUFile *f, void *opaque, int version_id)
 {
-    static DirtyBitmapLoadState s;
     int ret = 0;
 
     trace_dirty_bitmap_load_enter();
@@ -657,17 +666,17 @@ static int dirty_bitmap_load(QEMUFile *f, void *opaque, int version_id)
     }
 
     do {
-        ret = dirty_bitmap_load_header(f, &s);
+        ret = dirty_bitmap_load_header(f);
         if (ret < 0) {
             return ret;
         }
 
-        if (s.flags & DIRTY_BITMAP_MIG_FLAG_START) {
-            ret = dirty_bitmap_load_start(f, &s);
-        } else if (s.flags & DIRTY_BITMAP_MIG_FLAG_COMPLETE) {
-            dirty_bitmap_load_complete(f, &s);
-        } else if (s.flags & DIRTY_BITMAP_MIG_FLAG_BITS) {
-            ret = dirty_bitmap_load_bits(f, &s);
+        if (dbm_load_state.flags & DIRTY_BITMAP_MIG_FLAG_START) {
+            ret = dirty_bitmap_load_start(f);
+        } else if (dbm_load_state.flags & DIRTY_BITMAP_MIG_FLAG_COMPLETE) {
+            dirty_bitmap_load_complete(f);
+        } else if (dbm_load_state.flags & DIRTY_BITMAP_MIG_FLAG_BITS) {
+            ret = dirty_bitmap_load_bits(f);
         }
 
         if (!ret) {
@@ -677,7 +686,7 @@ static int dirty_bitmap_load(QEMUFile *f, void *opaque, int version_id)
         if (ret) {
             return ret;
         }
-    } while (!(s.flags & DIRTY_BITMAP_MIG_FLAG_EOS));
+    } while (!(dbm_load_state.flags & DIRTY_BITMAP_MIG_FLAG_EOS));
 
     trace_dirty_bitmap_load_success();
     return 0;
-- 
2.21.0

Re: [PATCH 1/7] migration/block-dirty-bitmap: refactor incoming state to be one struct

[Juan Quintela]

Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
> Move enabled_bitmaps and finish_lock, which are part of incoming state
> to DirtyBitmapLoadState, and make static global variable to store state
> instead of static local one.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  migration/block-dirty-bitmap.c | 77 +++++++++++++++++++---------------
>  1 file changed, 43 insertions(+), 34 deletions(-)
>
> diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
> index 7eafface61..281d20f41d 100644
> --- a/migration/block-dirty-bitmap.c
> +++ b/migration/block-dirty-bitmap.c
> @@ -125,6 +125,13 @@ typedef struct DirtyBitmapMigState {
>      BlockDriverState *prev_bs;
>      BdrvDirtyBitmap *prev_bitmap;
>  } DirtyBitmapMigState;
> +static DirtyBitmapMigState dirty_bitmap_mig_state;

Missing new line.

> +
> +typedef struct DirtyBitmapLoadBitmapState {
> +    BlockDriverState *bs;
> +    BdrvDirtyBitmap *bitmap;
> +    bool migrated;
> +} DirtyBitmapLoadBitmapState;
>  
>  typedef struct DirtyBitmapLoadState {
>      uint32_t flags;
> @@ -132,21 +139,15 @@ typedef struct DirtyBitmapLoadState {
>      char bitmap_name[256];
>      BlockDriverState *bs;
>      BdrvDirtyBitmap *bitmap;
> -} DirtyBitmapLoadState;
>  
> -static DirtyBitmapMigState dirty_bitmap_mig_state;
> -
> -typedef struct DirtyBitmapLoadBitmapState {
> -    BlockDriverState *bs;
> -    BdrvDirtyBitmap *bitmap;
> -    bool migrated;
> -} DirtyBitmapLoadBitmapState;
> -static GSList *enabled_bitmaps;
> -QemuMutex finish_lock;
> +    GSList *enabled_bitmaps;
> +    QemuMutex finish_lock;
> +} DirtyBitmapLoadState;
> +static DirtyBitmapLoadState dbm_load_state;

You move two global variables to an struct (good)
But you create a even bigger global variable (i.e. state that was not
shared before.)

>  /* First occurrence of this bitmap. It should be created if doesn't exist */
> -static int dirty_bitmap_load_start(QEMUFile *f, DirtyBitmapLoadState *s)
> +static int dirty_bitmap_load_start(QEMUFile *f)
>  {
> +    DirtyBitmapLoadState *s = &dbm_load_state;

You create a local alias.

>      Error *local_err = NULL;
>      uint32_t granularity = qemu_get_be32(f);
>      uint8_t flags = qemu_get_byte(f);
> @@ -482,7 +484,8 @@ static int dirty_bitmap_load_start(QEMUFile *f, DirtyBitmapLoadState *s)
>          b->bs = s->bs;
>          b->bitmap = s->bitmap;
>          b->migrated = false;
> -        enabled_bitmaps = g_slist_prepend(enabled_bitmaps, b);
> +        dbm_load_state.enabled_bitmaps =
> +            g_slist_prepend(dbm_load_state.enabled_bitmaps, b);

And then you access it using the global variable?

> -static void dirty_bitmap_load_complete(QEMUFile *f, DirtyBitmapLoadState *s)
> +static void dirty_bitmap_load_complete(QEMUFile *f)
>  {
> +    DirtyBitmapLoadState *s = &dbm_load_state;

Exactly the same on this function.

I still don't understand why you are removing the pass as parameters of
this function.


> -    static DirtyBitmapLoadState s;

Aha, this is why you are moving it as a global.

But, why can't you put this inside dirty_bitmap_mig_state?  Then you:
a- don't have to have "yet" another global variable
b- you can clean it up on save_cleanup handler.

not related to this patch, but to the file in general:

static void dirty_bitmap_save_cleanup(void *opaque)
{
    dirty_bitmap_mig_cleanup();
}

We have opaque here, that we can do:

DirtyBitmapMigBitmapState *dbms = opaque;

And then pass dbms to dirty_bitmap_mig_cleanup().

/* Called with iothread lock taken.  */
static void dirty_bitmap_mig_cleanup(void)
{
    DirtyBitmapMigBitmapState *dbms;

    while ((dbms = QSIMPLEQ_FIRST(&dirty_bitmap_mig_state.dbms_list)) != NULL) {
        QSIMPLEQ_REMOVE_HEAD(&dirty_bitmap_mig_state.dbms_list, entry);
        bdrv_dirty_bitmap_set_busy(dbms->bitmap, false);
        bdrv_unref(dbms->bs);
        g_free(dbms);
    }
}


Because here we just use the global variable.

Later, Juan.

Re: [PATCH 1/7] migration/block-dirty-bitmap: refactor incoming state to be one struct

[Vladimir Sementsov-Ogievskiy]

24.01.2020 13:56, Juan Quintela wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
>> Move enabled_bitmaps and finish_lock, which are part of incoming state
>> to DirtyBitmapLoadState, and make static global variable to store state
>> instead of static local one.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   migration/block-dirty-bitmap.c | 77 +++++++++++++++++++---------------
>>   1 file changed, 43 insertions(+), 34 deletions(-)
>>
>> diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
>> index 7eafface61..281d20f41d 100644
>> --- a/migration/block-dirty-bitmap.c
>> +++ b/migration/block-dirty-bitmap.c
>> @@ -125,6 +125,13 @@ typedef struct DirtyBitmapMigState {
>>       BlockDriverState *prev_bs;
>>       BdrvDirtyBitmap *prev_bitmap;
>>   } DirtyBitmapMigState;
>> +static DirtyBitmapMigState dirty_bitmap_mig_state;
> 
> Missing new line.
> 
>> +
>> +typedef struct DirtyBitmapLoadBitmapState {
>> +    BlockDriverState *bs;
>> +    BdrvDirtyBitmap *bitmap;
>> +    bool migrated;
>> +} DirtyBitmapLoadBitmapState;
>>   
>>   typedef struct DirtyBitmapLoadState {
>>       uint32_t flags;
>> @@ -132,21 +139,15 @@ typedef struct DirtyBitmapLoadState {
>>       char bitmap_name[256];
>>       BlockDriverState *bs;
>>       BdrvDirtyBitmap *bitmap;
>> -} DirtyBitmapLoadState;
>>   
>> -static DirtyBitmapMigState dirty_bitmap_mig_state;
>> -
>> -typedef struct DirtyBitmapLoadBitmapState {
>> -    BlockDriverState *bs;
>> -    BdrvDirtyBitmap *bitmap;
>> -    bool migrated;
>> -} DirtyBitmapLoadBitmapState;
>> -static GSList *enabled_bitmaps;
>> -QemuMutex finish_lock;
>> +    GSList *enabled_bitmaps;
>> +    QemuMutex finish_lock;
>> +} DirtyBitmapLoadState;
>> +static DirtyBitmapLoadState dbm_load_state;
> 
> You move two global variables to an struct (good)
> But you create a even bigger global variable (i.e. state that was not
> shared before.)
> 
>>   /* First occurrence of this bitmap. It should be created if doesn't exist */
>> -static int dirty_bitmap_load_start(QEMUFile *f, DirtyBitmapLoadState *s)
>> +static int dirty_bitmap_load_start(QEMUFile *f)
>>   {
>> +    DirtyBitmapLoadState *s = &dbm_load_state;
> 
> You create a local alias.
> 
>>       Error *local_err = NULL;
>>       uint32_t granularity = qemu_get_be32(f);
>>       uint8_t flags = qemu_get_byte(f);
>> @@ -482,7 +484,8 @@ static int dirty_bitmap_load_start(QEMUFile *f, DirtyBitmapLoadState *s)
>>           b->bs = s->bs;
>>           b->bitmap = s->bitmap;
>>           b->migrated = false;
>> -        enabled_bitmaps = g_slist_prepend(enabled_bitmaps, b);
>> +        dbm_load_state.enabled_bitmaps =
>> +            g_slist_prepend(dbm_load_state.enabled_bitmaps, b);
> 
> And then you access it using the global variable?
> 
>> -static void dirty_bitmap_load_complete(QEMUFile *f, DirtyBitmapLoadState *s)
>> +static void dirty_bitmap_load_complete(QEMUFile *f)
>>   {
>> +    DirtyBitmapLoadState *s = &dbm_load_state;
> 
> Exactly the same on this function.
> 
> I still don't understand why you are removing the pass as parameters of
> this function.
> 
> 
>> -    static DirtyBitmapLoadState s;
> 
> Aha, this is why you are moving it as a global.
> 
> But, why can't you put this inside dirty_bitmap_mig_state?  Then you:
> a- don't have to have "yet" another global variable
> b- you can clean it up on save_cleanup handler.

Because dirty_bitmap_mig_state is source state, and new dbm_load_state is
destination state. So, at least [b] will not work...

Do you think it's good to combine both source and destination states into one
struct, and use opaque everywhere?

It will look like

DirtyBitmapMigSourceState *s = ((DirtyBitmapMigState *)opaque)->source_state;

in save functions

and
DirtyBitmapIncomingState *s = ((DirtyBitmapMigState *)opaque)->incoming_state;

in load function

> 
> not related to this patch, but to the file in general:
> 
> static void dirty_bitmap_save_cleanup(void *opaque)
> {
>      dirty_bitmap_mig_cleanup();
> }
> 
> We have opaque here, that we can do:
> 
> DirtyBitmapMigBitmapState *dbms = opaque;
> 
> And then pass dbms to dirty_bitmap_mig_cleanup().
> 
> /* Called with iothread lock taken.  */
> static void dirty_bitmap_mig_cleanup(void)
> {
>      DirtyBitmapMigBitmapState *dbms;
> 
>      while ((dbms = QSIMPLEQ_FIRST(&dirty_bitmap_mig_state.dbms_list)) != NULL) {
>          QSIMPLEQ_REMOVE_HEAD(&dirty_bitmap_mig_state.dbms_list, entry);
>          bdrv_dirty_bitmap_set_busy(dbms->bitmap, false);
>          bdrv_unref(dbms->bs);
>          g_free(dbms);
>      }
> }
> 
> 
> Because here we just use the global variable.
> 
> Later, Juan.
> 


-- 
Best regards,
Vladimir

[PATCH 2/7] migration/block-dirty-bitmap: rename finish_lock to just lock

[Vladimir Sementsov-Ogievskiy]

finish_lock is bad name, as lock used not only on process end.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 migration/block-dirty-bitmap.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
index 281d20f41d..502e858c31 100644
--- a/migration/block-dirty-bitmap.c
+++ b/migration/block-dirty-bitmap.c
@@ -141,13 +141,13 @@ typedef struct DirtyBitmapLoadState {
     BdrvDirtyBitmap *bitmap;
 
     GSList *enabled_bitmaps;
-    QemuMutex finish_lock;
+    QemuMutex lock; /* protect enabled_bitmaps */
 } DirtyBitmapLoadState;
 static DirtyBitmapLoadState dbm_load_state;
 
 void init_dirty_bitmap_incoming_migration(void)
 {
-    qemu_mutex_init(&dbm_load_state.finish_lock);
+    qemu_mutex_init(&dbm_load_state.lock);
 }
 
 static uint32_t qemu_get_bitmap_flags(QEMUFile *f)
@@ -495,7 +495,7 @@ void dirty_bitmap_mig_before_vm_start(void)
 {
     GSList *item;
 
-    qemu_mutex_lock(&dbm_load_state.finish_lock);
+    qemu_mutex_lock(&dbm_load_state.lock);
 
     for (item = dbm_load_state.enabled_bitmaps; item;
          item = g_slist_next(item))
@@ -514,7 +514,7 @@ void dirty_bitmap_mig_before_vm_start(void)
     g_slist_free(dbm_load_state.enabled_bitmaps);
     dbm_load_state.enabled_bitmaps = NULL;
 
-    qemu_mutex_unlock(&dbm_load_state.finish_lock);
+    qemu_mutex_unlock(&dbm_load_state.lock);
 }
 
 static void dirty_bitmap_load_complete(QEMUFile *f)
@@ -524,7 +524,7 @@ static void dirty_bitmap_load_complete(QEMUFile *f)
     trace_dirty_bitmap_load_complete();
     bdrv_dirty_bitmap_deserialize_finish(s->bitmap);
 
-    qemu_mutex_lock(&dbm_load_state.finish_lock);
+    qemu_mutex_lock(&dbm_load_state.lock);
 
     for (item = dbm_load_state.enabled_bitmaps; item;
          item = g_slist_next(item))
@@ -558,7 +558,7 @@ static void dirty_bitmap_load_complete(QEMUFile *f)
         bdrv_dirty_bitmap_unlock(s->bitmap);
     }
 
-    qemu_mutex_unlock(&dbm_load_state.finish_lock);
+    qemu_mutex_unlock(&dbm_load_state.lock);
 }
 
 static int dirty_bitmap_load_bits(QEMUFile *f)
-- 
2.21.0

Re: [PATCH 2/7] migration/block-dirty-bitmap: rename finish_lock to just lock

[Juan Quintela]

Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
> finish_lock is bad name, as lock used not only on process end.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

I still would like the cleanup suggested on the previous patch, but this
one is ok.

[PATCH 3/7] migration/block-dirty-bitmap: simplify dirty_bitmap_load_complete

[Vladimir Sementsov-Ogievskiy]

bdrv_enable_dirty_bitmap_locked() call does nothing, as if we are in
postcopy, bitmap successor must be enabled, and reclaim operation will
enable the bitmap.

So, actually we need just call _reclaim_ in both if branches, and
making differences only to add an assertion seems not really good. The
logic becomes simple: on load complete we do reclaim and that's all.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 migration/block-dirty-bitmap.c | 25 ++++---------------------
 1 file changed, 4 insertions(+), 21 deletions(-)

diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
index 502e858c31..eeaab2174e 100644
--- a/migration/block-dirty-bitmap.c
+++ b/migration/block-dirty-bitmap.c
@@ -526,6 +526,10 @@ static void dirty_bitmap_load_complete(QEMUFile *f)
 
     qemu_mutex_lock(&dbm_load_state.lock);
 
+    if (bdrv_dirty_bitmap_has_successor(s->bitmap)) {
+        bdrv_reclaim_dirty_bitmap(s->bitmap, &error_abort);
+    }
+
     for (item = dbm_load_state.enabled_bitmaps; item;
          item = g_slist_next(item))
     {
@@ -537,27 +541,6 @@ static void dirty_bitmap_load_complete(QEMUFile *f)
         }
     }
 
-    if (bdrv_dirty_bitmap_has_successor(s->bitmap)) {
-        bdrv_dirty_bitmap_lock(s->bitmap);
-        if (dbm_load_state.enabled_bitmaps == NULL) {
-            /* in postcopy */
-            bdrv_reclaim_dirty_bitmap_locked(s->bitmap, &error_abort);
-            bdrv_enable_dirty_bitmap_locked(s->bitmap);
-        } else {
-            /* target not started, successor must be empty */
-            int64_t count = bdrv_get_dirty_count(s->bitmap);
-            BdrvDirtyBitmap *ret = bdrv_reclaim_dirty_bitmap_locked(s->bitmap,
-                                                                    NULL);
-            /* bdrv_reclaim_dirty_bitmap can fail only on no successor (it
-             * must be) or on merge fail, but merge can't fail when second
-             * bitmap is empty
-             */
-            assert(ret == s->bitmap &&
-                   count == bdrv_get_dirty_count(s->bitmap));
-        }
-        bdrv_dirty_bitmap_unlock(s->bitmap);
-    }
-
     qemu_mutex_unlock(&dbm_load_state.lock);
 }
 
-- 
2.21.0

[PATCH 4/7] migration/block-dirty-bitmap: keep bitmap state for all bitmaps

[Vladimir Sementsov-Ogievskiy]

Keep bitmap state for disabled bitmaps too. Keep the state until the
end of the process. It's needed for the following commit to implement
bitmap postcopy canceling.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 migration/block-dirty-bitmap.c | 59 ++++++++++++++++++++++------------
 1 file changed, 38 insertions(+), 21 deletions(-)

diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
index eeaab2174e..f96458113c 100644
--- a/migration/block-dirty-bitmap.c
+++ b/migration/block-dirty-bitmap.c
@@ -131,6 +131,7 @@ typedef struct DirtyBitmapLoadBitmapState {
     BlockDriverState *bs;
     BdrvDirtyBitmap *bitmap;
     bool migrated;
+    bool enabled;
 } DirtyBitmapLoadBitmapState;
 
 typedef struct DirtyBitmapLoadState {
@@ -140,8 +141,11 @@ typedef struct DirtyBitmapLoadState {
     BlockDriverState *bs;
     BdrvDirtyBitmap *bitmap;
 
-    GSList *enabled_bitmaps;
-    QemuMutex lock; /* protect enabled_bitmaps */
+    bool bitmaps_enabled; /* set in dirty_bitmap_mig_before_vm_start */
+    bool stream_ended; /* set when all migrated data handled */
+
+    GSList *bitmaps;
+    QemuMutex lock; /* protect bitmaps */
 } DirtyBitmapLoadState;
 static DirtyBitmapLoadState dbm_load_state;
 
@@ -446,6 +450,7 @@ static int dirty_bitmap_load_start(QEMUFile *f)
     Error *local_err = NULL;
     uint32_t granularity = qemu_get_be32(f);
     uint8_t flags = qemu_get_byte(f);
+    DirtyBitmapLoadBitmapState *b;
 
     if (s->bitmap) {
         error_report("Bitmap with the same name ('%s') already exists on "
@@ -472,22 +477,23 @@ static int dirty_bitmap_load_start(QEMUFile *f)
 
     bdrv_disable_dirty_bitmap(s->bitmap);
     if (flags & DIRTY_BITMAP_MIG_START_FLAG_ENABLED) {
-        DirtyBitmapLoadBitmapState *b;
-
         bdrv_dirty_bitmap_create_successor(s->bitmap, &local_err);
         if (local_err) {
             error_report_err(local_err);
             return -EINVAL;
         }
-
-        b = g_new(DirtyBitmapLoadBitmapState, 1);
-        b->bs = s->bs;
-        b->bitmap = s->bitmap;
-        b->migrated = false;
-        dbm_load_state.enabled_bitmaps =
-            g_slist_prepend(dbm_load_state.enabled_bitmaps, b);
     }
 
+    b = g_new(DirtyBitmapLoadBitmapState, 1);
+    *b = (DirtyBitmapLoadBitmapState) {
+        .bs = s->bs,
+        .bitmap = s->bitmap,
+        .migrated = false,
+        .enabled = flags & DIRTY_BITMAP_MIG_START_FLAG_ENABLED,
+    };
+
+    dbm_load_state.bitmaps = g_slist_prepend(dbm_load_state.bitmaps, b);
+
     return 0;
 }
 
@@ -497,22 +503,25 @@ void dirty_bitmap_mig_before_vm_start(void)
 
     qemu_mutex_lock(&dbm_load_state.lock);
 
-    for (item = dbm_load_state.enabled_bitmaps; item;
-         item = g_slist_next(item))
-    {
+    for (item = dbm_load_state.bitmaps; item; item = g_slist_next(item)) {
         DirtyBitmapLoadBitmapState *b = item->data;
 
+        if (!b->enabled) {
+            continue;
+        }
+
         if (b->migrated) {
             bdrv_enable_dirty_bitmap_locked(b->bitmap);
         } else {
             bdrv_dirty_bitmap_enable_successor(b->bitmap);
         }
-
-        g_free(b);
     }
 
-    g_slist_free(dbm_load_state.enabled_bitmaps);
-    dbm_load_state.enabled_bitmaps = NULL;
+    dbm_load_state.bitmaps_enabled = true;
+    if (dbm_load_state.stream_ended) {
+        g_slist_free_full(dbm_load_state.bitmaps, g_free);
+        dbm_load_state.bitmaps = NULL;
+    }
 
     qemu_mutex_unlock(&dbm_load_state.lock);
 }
@@ -530,9 +539,7 @@ static void dirty_bitmap_load_complete(QEMUFile *f)
         bdrv_reclaim_dirty_bitmap(s->bitmap, &error_abort);
     }
 
-    for (item = dbm_load_state.enabled_bitmaps; item;
-         item = g_slist_next(item))
-    {
+    for (item = dbm_load_state.bitmaps; item; item = g_slist_next(item)) {
         DirtyBitmapLoadBitmapState *b = item->data;
 
         if (b->bitmap == s->bitmap) {
@@ -671,6 +678,16 @@ static int dirty_bitmap_load(QEMUFile *f, void *opaque, int version_id)
         }
     } while (!(dbm_load_state.flags & DIRTY_BITMAP_MIG_FLAG_EOS));
 
+    qemu_mutex_lock(&dbm_load_state.lock);
+
+    dbm_load_state.stream_ended = true;
+    if (dbm_load_state.bitmaps_enabled) {
+        g_slist_free_full(dbm_load_state.bitmaps, g_free);
+        dbm_load_state.bitmaps = NULL;
+    }
+
+    qemu_mutex_unlock(&dbm_load_state.lock);
+
     trace_dirty_bitmap_load_success();
     return 0;
 }
-- 
2.21.0

Re: [PATCH 4/7] migration/block-dirty-bitmap: keep bitmap state for all bitmaps

[Juan Quintela]

Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
> Keep bitmap state for disabled bitmaps too. Keep the state until the
> end of the process. It's needed for the following commit to implement
> bitmap postcopy canceling.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> -
> -        b = g_new(DirtyBitmapLoadBitmapState, 1);
> -        b->bs = s->bs;
> -        b->bitmap = s->bitmap;
> -        b->migrated = false;
> -        dbm_load_state.enabled_bitmaps =
> -            g_slist_prepend(dbm_load_state.enabled_bitmaps, b);
>      }
>  
> +    b = g_new(DirtyBitmapLoadBitmapState, 1);
> +    *b = (DirtyBitmapLoadBitmapState) {
> +        .bs = s->bs,
> +        .bitmap = s->bitmap,
> +        .migrated = false,
> +        .enabled = flags & DIRTY_BITMAP_MIG_START_FLAG_ENABLED,
> +    };

What is wrong with:
     b->bs = s->bs;
     b->bitmap = s->bitmap;
     b->migrated = false;
     b->enabled = flags & DIRTY_BITMAP_MIG_START_FLAG_ENABLED;

???

Later, Juan.

Re: [PATCH 4/7] migration/block-dirty-bitmap: keep bitmap state for all bitmaps

[Vladimir Sementsov-Ogievskiy]

24.01.2020 14:01, Juan Quintela wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
>> Keep bitmap state for disabled bitmaps too. Keep the state until the
>> end of the process. It's needed for the following commit to implement
>> bitmap postcopy canceling.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> -
>> -        b = g_new(DirtyBitmapLoadBitmapState, 1);
>> -        b->bs = s->bs;
>> -        b->bitmap = s->bitmap;
>> -        b->migrated = false;
>> -        dbm_load_state.enabled_bitmaps =
>> -            g_slist_prepend(dbm_load_state.enabled_bitmaps, b);
>>       }
>>   
>> +    b = g_new(DirtyBitmapLoadBitmapState, 1);
>> +    *b = (DirtyBitmapLoadBitmapState) {
>> +        .bs = s->bs,
>> +        .bitmap = s->bitmap,
>> +        .migrated = false,
>> +        .enabled = flags & DIRTY_BITMAP_MIG_START_FLAG_ENABLED,
>> +    };
> 
> What is wrong with:
>       b->bs = s->bs;
>       b->bitmap = s->bitmap;
>       b->migrated = false;
>       b->enabled = flags & DIRTY_BITMAP_MIG_START_FLAG_ENABLED;
> 
> ???

Nothing wrong. Compound literal is a bit better, as it will initialize to zero all skipped fields.
Still nothing missed here. The change is actually unrelated to the patch, I can drop it.


-- 
Best regards,
Vladimir

Re: [PATCH 4/7] migration/block-dirty-bitmap: keep bitmap state for all bitmaps

[Vladimir Sementsov-Ogievskiy]

22.01.2020 16:23, Vladimir Sementsov-Ogievskiy wrote:
> Keep bitmap state for disabled bitmaps too. Keep the state until the
> end of the process. It's needed for the following commit to implement
> bitmap postcopy canceling.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   migration/block-dirty-bitmap.c | 59 ++++++++++++++++++++++------------
>   1 file changed, 38 insertions(+), 21 deletions(-)
> 
> diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
> index eeaab2174e..f96458113c 100644
> --- a/migration/block-dirty-bitmap.c
> +++ b/migration/block-dirty-bitmap.c
> @@ -131,6 +131,7 @@ typedef struct DirtyBitmapLoadBitmapState {
>       BlockDriverState *bs;
>       BdrvDirtyBitmap *bitmap;
>       bool migrated;
> +    bool enabled;
>   } DirtyBitmapLoadBitmapState;
>   
>   typedef struct DirtyBitmapLoadState {
> @@ -140,8 +141,11 @@ typedef struct DirtyBitmapLoadState {
>       BlockDriverState *bs;
>       BdrvDirtyBitmap *bitmap;
>   
> -    GSList *enabled_bitmaps;
> -    QemuMutex lock; /* protect enabled_bitmaps */
> +    bool bitmaps_enabled; /* set in dirty_bitmap_mig_before_vm_start */
> +    bool stream_ended; /* set when all migrated data handled */
> +
> +    GSList *bitmaps;
> +    QemuMutex lock; /* protect bitmaps */
>   } DirtyBitmapLoadState;
>   static DirtyBitmapLoadState dbm_load_state;
>   
> @@ -446,6 +450,7 @@ static int dirty_bitmap_load_start(QEMUFile *f)
>       Error *local_err = NULL;
>       uint32_t granularity = qemu_get_be32(f);
>       uint8_t flags = qemu_get_byte(f);
> +    DirtyBitmapLoadBitmapState *b;
>   
>       if (s->bitmap) {
>           error_report("Bitmap with the same name ('%s') already exists on "
> @@ -472,22 +477,23 @@ static int dirty_bitmap_load_start(QEMUFile *f)
>   
>       bdrv_disable_dirty_bitmap(s->bitmap);
>       if (flags & DIRTY_BITMAP_MIG_START_FLAG_ENABLED) {
> -        DirtyBitmapLoadBitmapState *b;
> -
>           bdrv_dirty_bitmap_create_successor(s->bitmap, &local_err);
>           if (local_err) {
>               error_report_err(local_err);
>               return -EINVAL;
>           }
> -
> -        b = g_new(DirtyBitmapLoadBitmapState, 1);
> -        b->bs = s->bs;
> -        b->bitmap = s->bitmap;
> -        b->migrated = false;
> -        dbm_load_state.enabled_bitmaps =
> -            g_slist_prepend(dbm_load_state.enabled_bitmaps, b);
>       }
>   
> +    b = g_new(DirtyBitmapLoadBitmapState, 1);
> +    *b = (DirtyBitmapLoadBitmapState) {
> +        .bs = s->bs,
> +        .bitmap = s->bitmap,
> +        .migrated = false,
> +        .enabled = flags & DIRTY_BITMAP_MIG_START_FLAG_ENABLED,
> +    };
> +
> +    dbm_load_state.bitmaps = g_slist_prepend(dbm_load_state.bitmaps, b);
> +
>       return 0;
>   }
>   
> @@ -497,22 +503,25 @@ void dirty_bitmap_mig_before_vm_start(void)
>   
>       qemu_mutex_lock(&dbm_load_state.lock);
>   
> -    for (item = dbm_load_state.enabled_bitmaps; item;
> -         item = g_slist_next(item))
> -    {
> +    for (item = dbm_load_state.bitmaps; item; item = g_slist_next(item)) {
>           DirtyBitmapLoadBitmapState *b = item->data;
>   
> +        if (!b->enabled) {
> +            continue;
> +        }
> +
>           if (b->migrated) {
>               bdrv_enable_dirty_bitmap_locked(b->bitmap);
>           } else {
>               bdrv_dirty_bitmap_enable_successor(b->bitmap);
>           }
> -
> -        g_free(b);
>       }
>   
> -    g_slist_free(dbm_load_state.enabled_bitmaps);
> -    dbm_load_state.enabled_bitmaps = NULL;
> +    dbm_load_state.bitmaps_enabled = true;
> +    if (dbm_load_state.stream_ended) {
> +        g_slist_free_full(dbm_load_state.bitmaps, g_free);
> +        dbm_load_state.bitmaps = NULL;
> +    }
>   
>       qemu_mutex_unlock(&dbm_load_state.lock);
>   }
> @@ -530,9 +539,7 @@ static void dirty_bitmap_load_complete(QEMUFile *f)
>           bdrv_reclaim_dirty_bitmap(s->bitmap, &error_abort);
>       }
>   
> -    for (item = dbm_load_state.enabled_bitmaps; item;
> -         item = g_slist_next(item))
> -    {
> +    for (item = dbm_load_state.bitmaps; item; item = g_slist_next(item)) {
>           DirtyBitmapLoadBitmapState *b = item->data;
>   
>           if (b->bitmap == s->bitmap) {
> @@ -671,6 +678,16 @@ static int dirty_bitmap_load(QEMUFile *f, void *opaque, int version_id)
>           }
>       } while (!(dbm_load_state.flags & DIRTY_BITMAP_MIG_FLAG_EOS));
>   
> +    qemu_mutex_lock(&dbm_load_state.lock);
> +
> +    dbm_load_state.stream_ended = true;

This is wrong. EOS doesn't mean that bitmap migration finished: only one set of sequential
bitmap chunks finished. EOS may be sent several times during migration and it is needed to
switch to other things migration.

> +    if (dbm_load_state.bitmaps_enabled) {
> +        g_slist_free_full(dbm_load_state.bitmaps, g_free);
> +        dbm_load_state.bitmaps = NULL;
> +    }
> +
> +    qemu_mutex_unlock(&dbm_load_state.lock);
> +
>       trace_dirty_bitmap_load_success();
>       return 0;
>   }
> 


-- 
Best regards,
Vladimir

[PATCH 5/7] migration/block-dirty-bitmap: cancel migration on shutdown

[Vladimir Sementsov-Ogievskiy]

If target is turned of prior to postcopy finished, we crash because
busy bitmaps are found at shutdown.

Let's fix it by removing all unfinished bitmaps on shutdown.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 migration/migration.h          |  1 +
 migration/block-dirty-bitmap.c | 44 ++++++++++++++++++++++++++++++----
 migration/migration.c          |  7 ++++++
 3 files changed, 48 insertions(+), 4 deletions(-)

diff --git a/migration/migration.h b/migration/migration.h
index aa9ff6f27b..a3927b93bb 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -331,6 +331,7 @@ 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_incoming(void);
 void init_dirty_bitmap_incoming_migration(void);
 void migrate_add_address(SocketAddress *address);
 
diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
index f96458113c..5a98543672 100644
--- a/migration/block-dirty-bitmap.c
+++ b/migration/block-dirty-bitmap.c
@@ -143,6 +143,7 @@ typedef struct DirtyBitmapLoadState {
 
     bool bitmaps_enabled; /* set in dirty_bitmap_mig_before_vm_start */
     bool stream_ended; /* set when all migrated data handled */
+    bool cancelled;
 
     GSList *bitmaps;
     QemuMutex lock; /* protect bitmaps */
@@ -533,8 +534,6 @@ static void dirty_bitmap_load_complete(QEMUFile *f)
     trace_dirty_bitmap_load_complete();
     bdrv_dirty_bitmap_deserialize_finish(s->bitmap);
 
-    qemu_mutex_lock(&dbm_load_state.lock);
-
     if (bdrv_dirty_bitmap_has_successor(s->bitmap)) {
         bdrv_reclaim_dirty_bitmap(s->bitmap, &error_abort);
     }
@@ -547,8 +546,6 @@ static void dirty_bitmap_load_complete(QEMUFile *f)
             break;
         }
     }
-
-    qemu_mutex_unlock(&dbm_load_state.lock);
 }
 
 static int dirty_bitmap_load_bits(QEMUFile *f)
@@ -656,6 +653,13 @@ static int dirty_bitmap_load(QEMUFile *f, void *opaque, int version_id)
     }
 
     do {
+        qemu_mutex_lock(&dbm_load_state.lock);
+
+        if (dbm_load_state.cancelled) {
+            qemu_mutex_unlock(&dbm_load_state.lock);
+            break;
+        }
+
         ret = dirty_bitmap_load_header(f);
         if (ret < 0) {
             return ret;
@@ -676,6 +680,8 @@ static int dirty_bitmap_load(QEMUFile *f, void *opaque, int version_id)
         if (ret) {
             return ret;
         }
+
+        qemu_mutex_unlock(&dbm_load_state.lock);
     } while (!(dbm_load_state.flags & DIRTY_BITMAP_MIG_FLAG_EOS));
 
     qemu_mutex_lock(&dbm_load_state.lock);
@@ -692,6 +698,36 @@ static int dirty_bitmap_load(QEMUFile *f, void *opaque, int version_id)
     return 0;
 }
 
+void dirty_bitmap_mig_cancel_incoming(void)
+{
+    GSList *item;
+
+    qemu_mutex_lock(&dbm_load_state.lock);
+
+    if (dbm_load_state.bitmaps_enabled && dbm_load_state.stream_ended) {
+        qemu_mutex_unlock(&dbm_load_state.lock);
+        return;
+    }
+
+    dbm_load_state.cancelled = true;
+
+    for (item = dbm_load_state.bitmaps; item; item = g_slist_next(item)) {
+        DirtyBitmapLoadBitmapState *b = item->data;
+
+        if (!dbm_load_state.bitmaps_enabled || !b->migrated) {
+            if (bdrv_dirty_bitmap_has_successor(b->bitmap)) {
+                bdrv_reclaim_dirty_bitmap(b->bitmap, &error_abort);
+            }
+            bdrv_release_dirty_bitmap(b->bitmap);
+        }
+    }
+
+    g_slist_free_full(dbm_load_state.bitmaps, g_free);
+    dbm_load_state.bitmaps = NULL;
+
+    qemu_mutex_unlock(&dbm_load_state.lock);
+}
+
 static int dirty_bitmap_save_setup(QEMUFile *f, void *opaque)
 {
     DirtyBitmapMigBitmapState *dbms = NULL;
diff --git a/migration/migration.c b/migration/migration.c
index 990bff00c0..12d161165d 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -182,6 +182,13 @@ void migration_shutdown(void)
      */
     migrate_fd_cancel(current_migration);
     object_unref(OBJECT(current_migration));
+
+    /*
+     * 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 */
-- 
2.21.0

[PATCH 6/7] migration: handle to_src_file on target only for ram postcopy

[Vladimir Sementsov-Ogievskiy]

If only bitmaps postcopy migration enabled and not ram, this assertion
will fire, as we don't have to_src_file for bitmaps postcopy migration.

migrate_postcopy_ram() accesses migrations state, which may be freed in
main thread, so, we should ref/unref it in postcopy incoming thread.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 migration/savevm.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/migration/savevm.c b/migration/savevm.c
index adfdca26ac..143755389e 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1832,6 +1832,9 @@ static void *postcopy_ram_listen_thread(void *opaque)
     MigrationIncomingState *mis = migration_incoming_get_current();
     QEMUFile *f = mis->from_src_file;
     int load_res;
+    MigrationState *migr = migrate_get_current();
+
+    object_ref(OBJECT(migr));
 
     migrate_set_state(&mis->state, MIGRATION_STATUS_ACTIVE,
                                    MIGRATION_STATUS_POSTCOPY_ACTIVE);
@@ -1898,6 +1901,8 @@ static void *postcopy_ram_listen_thread(void *opaque)
     mis->have_listen_thread = false;
     postcopy_state_set(POSTCOPY_INCOMING_END);
 
+    object_unref(OBJECT(migr));
+
     return NULL;
 }
 
@@ -2457,12 +2462,14 @@ static bool postcopy_pause_incoming(MigrationIncomingState *mis)
     qemu_fclose(mis->from_src_file);
     mis->from_src_file = NULL;
 
-    assert(mis->to_src_file);
-    qemu_file_shutdown(mis->to_src_file);
-    qemu_mutex_lock(&mis->rp_mutex);
-    qemu_fclose(mis->to_src_file);
-    mis->to_src_file = NULL;
-    qemu_mutex_unlock(&mis->rp_mutex);
+    if (migrate_postcopy_ram()) {
+        assert(mis->to_src_file);
+        qemu_file_shutdown(mis->to_src_file);
+        qemu_mutex_lock(&mis->rp_mutex);
+        qemu_fclose(mis->to_src_file);
+        mis->to_src_file = NULL;
+        qemu_mutex_unlock(&mis->rp_mutex);
+    }
 
     migrate_set_state(&mis->state, MIGRATION_STATUS_POSTCOPY_ACTIVE,
                       MIGRATION_STATUS_POSTCOPY_PAUSED);
-- 
2.21.0

[PATCH 7/7] qemu-iotests/199: add early shutdown case to bitmaps postcopy

[Vladimir Sementsov-Ogievskiy]

Previous patches fixed two crashes which may occur on shutdown prior to
bitmaps postcopy finished. Check that it works now.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 tests/qemu-iotests/199     | 12 +++++++++++-
 tests/qemu-iotests/199.out |  4 ++--
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/199 b/tests/qemu-iotests/199
index a2c8ecab5a..a3f6c73aed 100755
--- a/tests/qemu-iotests/199
+++ b/tests/qemu-iotests/199
@@ -47,7 +47,7 @@ class TestDirtyBitmapPostcopyMigration(iotests.QMPTestCase):
         self.vm_a.launch()
         self.vm_b.launch()
 
-    def test_postcopy(self):
+    def do_test_postcopy(self, early_shutdown):
         write_size = 0x40000000
         granularity = 512
         chunk = 4096
@@ -99,6 +99,10 @@ class TestDirtyBitmapPostcopyMigration(iotests.QMPTestCase):
             if event['data']['status'] == 'completed':
                 break
 
+        if early_shutdown:
+            self.vm_b.qmp('quit')
+            return
+
         s = 0x8000
         while s < write_size:
             self.vm_b.hmp_qemu_io('drive0', 'write %d %d' % (s, chunk))
@@ -114,6 +118,12 @@ class TestDirtyBitmapPostcopyMigration(iotests.QMPTestCase):
 
         self.assert_qmp(result, 'return/sha256', sha256);
 
+    def test_postcopy(self):
+        self.do_test_postcopy(False)
+
+    def test_postcopy_early_shutdown(self):
+        self.do_test_postcopy(True)
+
 if __name__ == '__main__':
     iotests.main(supported_fmts=['qcow2'], supported_cache_modes=['none'],
                  supported_protocols=['file'])
diff --git a/tests/qemu-iotests/199.out b/tests/qemu-iotests/199.out
index ae1213e6f8..fbc63e62f8 100644
--- a/tests/qemu-iotests/199.out
+++ b/tests/qemu-iotests/199.out
@@ -1,5 +1,5 @@
-.
+..
 ----------------------------------------------------------------------
-Ran 1 tests
+Ran 2 tests
 
 OK
-- 
2.21.0