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

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(-)

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.

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.

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;
>   }
>