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

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

19.02.2020 18:30, Vladimir Sementsov-Ogievskiy wrote:
> 18.02.2020 17:26, Andrey Shinkevich wrote:
>> 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 <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 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);
> 
> I'm afraid we can't as BdrvDirtyBitmap is not public structure
> 
>>
>>> +        bdrv_reclaim_dirty_bitmap(s->bitmap, &error_abort);
> 
> But we can assert that resulting bitmap is enabled.

Or not, as bitmap may be not yet enabled, if guest is not yet started.

> 
>>> +    }
>>> +
>>>       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 <andrey.shinkevich@virtuozzo.com>
> 
> 


-- 
Best regards,
Vladimir