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

[Juan Quintela]

Vladimir Sementsov-Ogievskiy <address@hidden> 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 <address@hidden>
> ---
>  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.

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.