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

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

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