On 17/02/2020 18:02, 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.
>
> To clean-up the new list the following logic is used:
> We need two events to consider bitmap migration finished:
> 1. chunk with DIRTY_BITMAP_MIG_FLAG_COMPLETE flag should be received
> 2. dirty_bitmap_mig_before_vm_start should be called
> These two events may come in any order, so we understand which one is
> last, and on the last of them we remove bitmap migration state from the
> list.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
> migration/block-dirty-bitmap.c | 64 +++++++++++++++++++++++-----------
> 1 file changed, 43 insertions(+), 21 deletions(-)
>
> diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
> index 9cc750d93b..1329db8d7d 100644
> --- a/migration/block-dirty-bitmap.c
> +++ b/migration/block-dirty-bitmap.c
> @@ -132,6 +132,7 @@ typedef struct LoadBitmapState {
> BlockDriverState *bs;
> BdrvDirtyBitmap *bitmap;
> bool migrated;
> + bool enabled;
> } LoadBitmapState;
>
> /* State of the dirty bitmap migration (DBM) during load process */
> @@ -142,8 +143,10 @@ typedef struct DBMLoadState {
> BlockDriverState *bs;
> BdrvDirtyBitmap *bitmap;
>
> - GSList *enabled_bitmaps;
> - QemuMutex lock; /* protect enabled_bitmaps */
> + bool before_vm_start_handled; /* set in dirty_bitmap_mig_before_vm_start */
> +
> + GSList *bitmaps;
> + QemuMutex lock; /* protect bitmaps */
> } DBMLoadState;
>
> typedef struct DBMState {
> @@ -458,6 +461,7 @@ static int dirty_bitmap_load_start(QEMUFile *f, DBMLoadState *s)
> Error *local_err = NULL;
> uint32_t granularity = qemu_get_be32(f);
> uint8_t flags = qemu_get_byte(f);
> + LoadBitmapState *b;
>
> if (s->bitmap) {
> error_report("Bitmap with the same name ('%s') already exists on "
> @@ -484,45 +488,59 @@ static int dirty_bitmap_load_start(QEMUFile *f, DBMLoadState *s)
>
> bdrv_disable_dirty_bitmap(s->bitmap);
> if (flags & DIRTY_BITMAP_MIG_START_FLAG_ENABLED) {
> - LoadBitmapState *b;
> -
> bdrv_dirty_bitmap_create_successor(s->bitmap, &local_err);
> if (local_err) {
> error_report_err(local_err);
> return -EINVAL;
> }
> -
> - b = g_new(LoadBitmapState, 1);
> - b->bs = s->bs;
> - b->bitmap = s->bitmap;
> - b->migrated = false;
> - s->enabled_bitmaps = g_slist_prepend(s->enabled_bitmaps, b);
> }
>
> + b = g_new(LoadBitmapState, 1);
> + b->bs = s->bs;
> + b->bitmap = s->bitmap;
> + b->migrated = false;
> + b->enabled = flags & DIRTY_BITMAP_MIG_START_FLAG_ENABLED,
> +
> + s->bitmaps = g_slist_prepend(s->bitmaps, b);
> +
> return 0;
> }
>
> -void dirty_bitmap_mig_before_vm_start(void)
> +/*
> + * before_vm_start_handle_item
> + *
> + * g_slist_foreach helper
> + *
> + * item is LoadBitmapState*
> + * opaque is DBMLoadState*
> + */
> +static void before_vm_start_handle_item(void *item, void *opaque)
> {
> - DBMLoadState *s = &dbm_state.load;
> - GSList *item;
> -
> - qemu_mutex_lock(&s->lock);
> -
> - for (item = s->enabled_bitmaps; item; item = g_slist_next(item)) {
> - LoadBitmapState *b = item->data;
> + DBMLoadState *s = opaque;
> + LoadBitmapState *b = item;
>
> + if (b->enabled) {
> if (b->migrated) {
> bdrv_enable_dirty_bitmap(b->bitmap);
> } else {
> bdrv_dirty_bitmap_enable_successor(b->bitmap);
> }
> + }
>
> + if (b->migrated) {
> + s->bitmaps = g_slist_remove(s->bitmaps, b);
> g_free(b);
> }
> +}
>
> - g_slist_free(s->enabled_bitmaps);
> - s->enabled_bitmaps = NULL;
> +void dirty_bitmap_mig_before_vm_start(void)
> +{
> + DBMLoadState *s = &dbm_state.load;
> + qemu_mutex_lock(&s->lock);
> +
> + assert(!s->before_vm_start_handled);
> + g_slist_foreach(s->bitmaps, before_vm_start_handle_item, s);
> + s->before_vm_start_handled = true;
>
> qemu_mutex_unlock(&s->lock);
> }
> @@ -539,11 +557,15 @@ static void dirty_bitmap_load_complete(QEMUFile *f, DBMLoadState *s)
> bdrv_reclaim_dirty_bitmap(s->bitmap, &error_abort);
> }
>
> - for (item = s->enabled_bitmaps; item; item = g_slist_next(item)) {
> + for (item = s->bitmaps; item; item = g_slist_next(item)) {
> LoadBitmapState *b = item->data;
>
> if (b->bitmap == s->bitmap) {
> b->migrated = true;
> + if (s->before_vm_start_handled) {
> + s->bitmaps = g_slist_remove(s->bitmaps, b);
> + g_free(b);
> + }
> break;
> }
> }
>
Reviewed-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
--
With the best regards,
Andrey Shinkevich