Re: [PATCH v2 07/22] migration/block-dirty-bitmap: simplify dirty_bitmap_load_complete

[Andrey Shinkevich]

On 17/02/2020 18:02, Vladimir Sementsov-Ogievskiy wrote:
> 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 <address@hidden>
> ---
>   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 440c41cfca..9cc750d93b 100644
> --- a/migration/block-dirty-bitmap.c
> +++ b/migration/block-dirty-bitmap.c
> @@ -535,6 +535,10 @@ static void dirty_bitmap_load_complete(QEMUFile *f, 
> DBMLoadState *s)
>   
>       qemu_mutex_lock(&s->lock);
>   
> +    if (bdrv_dirty_bitmap_has_successor(s->bitmap)) {
What about making it sure?
           assert(!s->bitmap->successor->disabled);

> +        bdrv_reclaim_dirty_bitmap(s->bitmap, &error_abort);
> +    }
> +
>       for (item = s->enabled_bitmaps; item; item = g_slist_next(item)) {
>           LoadBitmapState *b = item->data;
>   
> @@ -544,27 +548,6 @@ static void dirty_bitmap_load_complete(QEMUFile *f, DBMLoadState *s)
>           }
>       }
>   
> -    if (bdrv_dirty_bitmap_has_successor(s->bitmap)) {
> -        bdrv_dirty_bitmap_lock(s->bitmap);
> -        if (s->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(&s->lock);
>   }
>   

Reviewed-by: Andrey Shinkevich <address@hidden>
--
With the best regards,
Andrey Shinkevich