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

[Vladimir Sementsov-Ogievskiy]

24.01.2020 13:56, Juan Quintela wrote:
> 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.

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