Re: [PATCH v2 05/22] migration/block-dirty-bitmap: refactor state global

qemu-devel

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v2 05/22] migration/block-dirty-bitmap: refactor state global

From:	Andrey Shinkevich
Subject:	Re: [PATCH v2 05/22] migration/block-dirty-bitmap: refactor state global variables
Date:	Tue, 18 Feb 2020 16:05:52 +0300
User-agent:	Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.4.1



On 17/02/2020 18:02, Vladimir Sementsov-Ogievskiy wrote:

Move all state variables into one global struct. Reduce global
variable usage, utilizing opaque pointer where possible.

Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
---
  migration/block-dirty-bitmap.c | 171 ++++++++++++++++++---------------
  1 file changed, 95 insertions(+), 76 deletions(-)

diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
index 49d4cf8810..7a82b76809 100644
--- a/migration/block-dirty-bitmap.c
+++ b/migration/block-dirty-bitmap.c
@@ -128,6 +128,12 @@ typedef struct DBMSaveState {
      BdrvDirtyBitmap *prev_bitmap;
  } DBMSaveState;

+typedef struct LoadBitmapState {

+    BlockDriverState *bs;
+    BdrvDirtyBitmap *bitmap;
+    bool migrated;
+} LoadBitmapState;
+
  /* State of the dirty bitmap migration (DBM) during load process */
  typedef struct DBMLoadState {
      uint32_t flags;
@@ -135,18 +141,17 @@ typedef struct DBMLoadState {
      char bitmap_name[256];
      BlockDriverState *bs;
      BdrvDirtyBitmap *bitmap;
+
+    GSList *enabled_bitmaps;
+    QemuMutex finish_lock;
  } DBMLoadState;

-static DBMSaveState dirty_bitmap_mig_state;

+typedef struct DBMState {
+    DBMSaveState save;
+    DBMLoadState load;
+} DBMState;

-/* State of one bitmap during load process */

-typedef struct LoadBitmapState {
-    BlockDriverState *bs;
-    BdrvDirtyBitmap *bitmap;
-    bool migrated;
-} LoadBitmapState;
-static GSList *enabled_bitmaps;
-QemuMutex finish_lock;
+static DBMState dbm_state;

static uint32_t qemu_get_bitmap_flags(QEMUFile *f)

  {
@@ -169,21 +174,21 @@ static void qemu_put_bitmap_flags(QEMUFile *f, uint32_t 
flags)
      qemu_put_byte(f, flags);
  }

-static void send_bitmap_header(QEMUFile *f, SaveBitmapState *dbms,

-                               uint32_t additional_flags)
+static void send_bitmap_header(QEMUFile *f, DBMSaveState *s,
+                               SaveBitmapState *dbms, uint32_t 
additional_flags)
  {
      BlockDriverState *bs = dbms->bs;
      BdrvDirtyBitmap *bitmap = dbms->bitmap;
      uint32_t flags = additional_flags;
      trace_send_bitmap_header_enter();

- if (bs != dirty_bitmap_mig_state.prev_bs) {

-        dirty_bitmap_mig_state.prev_bs = bs;
+    if (bs != s->prev_bs) {
+        s->prev_bs = bs;
          flags |= DIRTY_BITMAP_MIG_FLAG_DEVICE_NAME;
      }

- if (bitmap != dirty_bitmap_mig_state.prev_bitmap) {

-        dirty_bitmap_mig_state.prev_bitmap = bitmap;
+    if (bitmap != s->prev_bitmap) {
+        s->prev_bitmap = bitmap;
          flags |= DIRTY_BITMAP_MIG_FLAG_BITMAP_NAME;
      }

@@ -198,19 +203,22 @@ static void send_bitmap_header(QEMUFile *f, SaveBitmapState *dbms,

      }
  }

-static void send_bitmap_start(QEMUFile *f, SaveBitmapState *dbms)

+static void send_bitmap_start(QEMUFile *f, DBMSaveState *s,
+                              SaveBitmapState *dbms)
  {
-    send_bitmap_header(f, dbms, DIRTY_BITMAP_MIG_FLAG_START);
+    send_bitmap_header(f, s, dbms, DIRTY_BITMAP_MIG_FLAG_START);
      qemu_put_be32(f, bdrv_dirty_bitmap_granularity(dbms->bitmap));
      qemu_put_byte(f, dbms->flags);
  }

-static void send_bitmap_complete(QEMUFile *f, SaveBitmapState *dbms)

+static void send_bitmap_complete(QEMUFile *f, DBMSaveState *s,
+                                 SaveBitmapState *dbms)
  {
-    send_bitmap_header(f, dbms, DIRTY_BITMAP_MIG_FLAG_COMPLETE);
+    send_bitmap_header(f, s, dbms, DIRTY_BITMAP_MIG_FLAG_COMPLETE);
  }

-static void send_bitmap_bits(QEMUFile *f, SaveBitmapState *dbms,

+static void send_bitmap_bits(QEMUFile *f, DBMSaveState *s,
+                             SaveBitmapState *dbms,
                               uint64_t start_sector, uint32_t nr_sectors)
  {
      /* align for buffer_is_zero() */
@@ -235,7 +243,7 @@ static void send_bitmap_bits(QEMUFile *f, SaveBitmapState 
*dbms,

trace_send_bitmap_bits(flags, start_sector, nr_sectors, buf_size);- send_bitmap_header(f, dbms, flags);

+    send_bitmap_header(f, s, dbms, flags);

qemu_put_be64(f, start_sector);

      qemu_put_be32(f, nr_sectors);
@@ -254,12 +262,12 @@ static void send_bitmap_bits(QEMUFile *f, SaveBitmapState 
*dbms,
  }

/* Called with iothread lock taken. */

-static void dirty_bitmap_do_save_cleanup(void)
+static void dirty_bitmap_do_save_cleanup(DBMSaveState *s)
  {
      SaveBitmapState *dbms;

- while ((dbms = QSIMPLEQ_FIRST(&dirty_bitmap_mig_state.dbms_list)) != NULL) {

-        QSIMPLEQ_REMOVE_HEAD(&dirty_bitmap_mig_state.dbms_list, entry);
+    while ((dbms = QSIMPLEQ_FIRST(&s->dbms_list)) != NULL) {
+        QSIMPLEQ_REMOVE_HEAD(&s->dbms_list, entry);
          bdrv_dirty_bitmap_set_busy(dbms->bitmap, false);
          bdrv_unref(dbms->bs);
          g_free(dbms);
@@ -267,17 +275,17 @@ static void dirty_bitmap_do_save_cleanup(void)
  }

/* Called with iothread lock taken. */

-static int init_dirty_bitmap_migration(void)
+static int init_dirty_bitmap_migration(DBMSaveState *s)
  {
      BlockDriverState *bs;
      BdrvDirtyBitmap *bitmap;
      SaveBitmapState *dbms;
      Error *local_err = NULL;

- dirty_bitmap_mig_state.bulk_completed = false;

-    dirty_bitmap_mig_state.prev_bs = NULL;
-    dirty_bitmap_mig_state.prev_bitmap = NULL;
-    dirty_bitmap_mig_state.no_bitmaps = false;
+    s->bulk_completed = false;
+    s->prev_bs = NULL;
+    s->prev_bitmap = NULL;
+    s->no_bitmaps = false;

for (bs = bdrv_next_all_states(NULL); bs; bs = bdrv_next_all_states(bs)) {

          const char *name = bdrv_get_device_or_node_name(bs);
@@ -316,35 +324,36 @@ static int init_dirty_bitmap_migration(void)
                  dbms->flags |= DIRTY_BITMAP_MIG_START_FLAG_PERSISTENT;
              }

- QSIMPLEQ_INSERT_TAIL(&dirty_bitmap_mig_state.dbms_list,

+            QSIMPLEQ_INSERT_TAIL(&s->dbms_list,
                                   dbms, entry);
          }
      }

/* unset migration flags here, to not roll back it */

-    QSIMPLEQ_FOREACH(dbms, &dirty_bitmap_mig_state.dbms_list, entry) {
+    QSIMPLEQ_FOREACH(dbms, &s->dbms_list, entry) {
          bdrv_dirty_bitmap_skip_store(dbms->bitmap, true);
      }

- if (QSIMPLEQ_EMPTY(&dirty_bitmap_mig_state.dbms_list)) {

-        dirty_bitmap_mig_state.no_bitmaps = true;
+    if (QSIMPLEQ_EMPTY(&s->dbms_list)) {
+        s->no_bitmaps = true;
      }

return 0;fail:

-    dirty_bitmap_do_save_cleanup();
+    dirty_bitmap_do_save_cleanup(s);

return -1;

/* Called with no lock taken. */

-static void bulk_phase_send_chunk(QEMUFile *f, SaveBitmapState *dbms)
+static void bulk_phase_send_chunk(QEMUFile *f, DBMSaveState *s,
+                                  SaveBitmapState *dbms)
  {
      uint32_t nr_sectors = MIN(dbms->total_sectors - dbms->cur_sector,
                               dbms->sectors_per_chunk);

- send_bitmap_bits(f, dbms, dbms->cur_sector, nr_sectors);

+    send_bitmap_bits(f, s, dbms, dbms->cur_sector, nr_sectors);

dbms->cur_sector += nr_sectors;

      if (dbms->cur_sector >= dbms->total_sectors) {
@@ -353,61 +362,66 @@ static void bulk_phase_send_chunk(QEMUFile *f, 
SaveBitmapState *dbms)
  }

/* Called with no lock taken. */

-static void bulk_phase(QEMUFile *f, bool limit)
+static void bulk_phase(QEMUFile *f, DBMSaveState *s, bool limit)
  {
      SaveBitmapState *dbms;

- QSIMPLEQ_FOREACH(dbms, &dirty_bitmap_mig_state.dbms_list, entry) {

+    QSIMPLEQ_FOREACH(dbms, &s->dbms_list, entry) {
          while (!dbms->bulk_completed) {
-            bulk_phase_send_chunk(f, dbms);
+            bulk_phase_send_chunk(f, s, dbms);
              if (limit && qemu_file_rate_limit(f)) {
                  return;
              }
          }
      }

- dirty_bitmap_mig_state.bulk_completed = true;

+    s->bulk_completed = true;
  }

/* for SaveVMHandlers */

  static void dirty_bitmap_save_cleanup(void *opaque)
  {
-    dirty_bitmap_do_save_cleanup();
+    DBMSaveState *s = &((DBMState *)opaque)->save;
+
+    dirty_bitmap_do_save_cleanup(s);
  }


Why do one need the extra nested "do" function?

static int dirty_bitmap_save_iterate(QEMUFile *f, void *opaque)

  {
+    DBMSaveState *s = &((DBMState *)opaque)->save;
+
      trace_dirty_bitmap_save_iterate(migration_in_postcopy());

- if (migration_in_postcopy() && !dirty_bitmap_mig_state.bulk_completed) {

-        bulk_phase(f, true);
+    if (migration_in_postcopy() && !s->bulk_completed) {
+        bulk_phase(f, s, true);
      }

qemu_put_bitmap_flags(f, DIRTY_BITMAP_MIG_FLAG_EOS);- return dirty_bitmap_mig_state.bulk_completed;

+    return s->bulk_completed;
  }

/* Called with iothread lock taken. */static int dirty_bitmap_save_complete(QEMUFile *f, void *opaque)

  {
+    DBMSaveState *s = &((DBMState *)opaque)->save;
      SaveBitmapState *dbms;
      trace_dirty_bitmap_save_complete_enter();

- if (!dirty_bitmap_mig_state.bulk_completed) {

-        bulk_phase(f, false);
+    if (!s->bulk_completed) {
+        bulk_phase(f, s, false);
      }

- QSIMPLEQ_FOREACH(dbms, &dirty_bitmap_mig_state.dbms_list, entry) {

-        send_bitmap_complete(f, dbms);
+    QSIMPLEQ_FOREACH(dbms, &s->dbms_list, entry) {
+        send_bitmap_complete(f, s, dbms);
      }

qemu_put_bitmap_flags(f, DIRTY_BITMAP_MIG_FLAG_EOS);trace_dirty_bitmap_save_complete_finish();- dirty_bitmap_do_save_cleanup();

+    dirty_bitmap_save_cleanup(opaque);
      return 0;
  }

@@ -417,12 +431,13 @@ static void dirty_bitmap_save_pending(QEMUFile *f, void *opaque,

                                        uint64_t *res_compatible,
                                        uint64_t *res_postcopy_only)
  {
+    DBMSaveState *s = &((DBMState *)opaque)->save;
      SaveBitmapState *dbms;
      uint64_t pending = 0;

qemu_mutex_lock_iothread();- QSIMPLEQ_FOREACH(dbms, &dirty_bitmap_mig_state.dbms_list, entry) {

+    QSIMPLEQ_FOREACH(dbms, &s->dbms_list, entry) {
          uint64_t gran = bdrv_dirty_bitmap_granularity(dbms->bitmap);
          uint64_t sectors = dbms->bulk_completed ? 0 :
                             dbms->total_sectors - dbms->cur_sector;
@@ -481,7 +496,7 @@ static int dirty_bitmap_load_start(QEMUFile *f, 
DBMLoadState *s)
          b->bs = s->bs;
          b->bitmap = s->bitmap;
          b->migrated = false;
-        enabled_bitmaps = g_slist_prepend(enabled_bitmaps, b);
+        s->enabled_bitmaps = g_slist_prepend(s->enabled_bitmaps, b);
      }

return 0;

@@ -489,11 +504,12 @@ static int dirty_bitmap_load_start(QEMUFile *f, 
DBMLoadState *s)

void dirty_bitmap_mig_before_vm_start(void)

  {
+    DBMLoadState *s = &dbm_state.load;
      GSList *item;

- qemu_mutex_lock(&finish_lock);

+    qemu_mutex_lock(&s->finish_lock);

- for (item = enabled_bitmaps; item; item = g_slist_next(item)) {

+    for (item = s->enabled_bitmaps; item; item = g_slist_next(item)) {
          LoadBitmapState *b = item->data;

if (b->migrated) {

@@ -505,10 +521,10 @@ void dirty_bitmap_mig_before_vm_start(void)
          g_free(b);
      }

- g_slist_free(enabled_bitmaps);

-    enabled_bitmaps = NULL;
+    g_slist_free(s->enabled_bitmaps);
+    s->enabled_bitmaps = NULL;

- qemu_mutex_unlock(&finish_lock);

+    qemu_mutex_unlock(&s->finish_lock);
  }

static void dirty_bitmap_load_complete(QEMUFile *f, DBMLoadState *s)

@@ -517,9 +533,9 @@ static void dirty_bitmap_load_complete(QEMUFile *f, 
DBMLoadState *s)
      trace_dirty_bitmap_load_complete();
      bdrv_dirty_bitmap_deserialize_finish(s->bitmap);

- qemu_mutex_lock(&finish_lock);

+    qemu_mutex_lock(&s->finish_lock);

- for (item = enabled_bitmaps; item; item = g_slist_next(item)) {

+    for (item = s->enabled_bitmaps; item; item = g_slist_next(item)) {
          LoadBitmapState *b = item->data;

if (b->bitmap == s->bitmap) {

@@ -530,7 +546,7 @@ 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 (enabled_bitmaps == NULL) {
+        if (s->enabled_bitmaps == NULL) {
              /* in postcopy */
              bdrv_reclaim_dirty_bitmap_locked(s->bitmap, &error_abort);
              bdrv_enable_dirty_bitmap_locked(s->bitmap);
@@ -549,7 +565,7 @@ static void dirty_bitmap_load_complete(QEMUFile *f, 
DBMLoadState *s)
          bdrv_dirty_bitmap_unlock(s->bitmap);
      }

- qemu_mutex_unlock(&finish_lock);

+    qemu_mutex_unlock(&s->finish_lock);
  }

static int dirty_bitmap_load_bits(QEMUFile *f, DBMLoadState *s)

@@ -646,7 +662,7 @@ static int dirty_bitmap_load_header(QEMUFile *f, 
DBMLoadState *s)

static int dirty_bitmap_load(QEMUFile *f, void *opaque, int version_id)

  {
-    static DBMLoadState s;
+    DBMLoadState *s = &((DBMState *)opaque)->load;
      int ret = 0;

trace_dirty_bitmap_load_enter();

@@ -656,17 +672,17 @@ static int dirty_bitmap_load(QEMUFile *f, void *opaque, 
int version_id)
      }

do {

-        ret = dirty_bitmap_load_header(f, &s);
+        ret = dirty_bitmap_load_header(f, s);
          if (ret < 0) {
              return ret;
          }

- if (s.flags & DIRTY_BITMAP_MIG_FLAG_START) {

-            ret = dirty_bitmap_load_start(f, &s);
-        } else if (s.flags & DIRTY_BITMAP_MIG_FLAG_COMPLETE) {
-            dirty_bitmap_load_complete(f, &s);
-        } else if (s.flags & DIRTY_BITMAP_MIG_FLAG_BITS) {
-            ret = dirty_bitmap_load_bits(f, &s);
+        if (s->flags & DIRTY_BITMAP_MIG_FLAG_START) {
+            ret = dirty_bitmap_load_start(f, s);
+        } else if (s->flags & DIRTY_BITMAP_MIG_FLAG_COMPLETE) {
+            dirty_bitmap_load_complete(f, s);
+        } else if (s->flags & DIRTY_BITMAP_MIG_FLAG_BITS) {
+            ret = dirty_bitmap_load_bits(f, s);
          }

if (!ret) {

@@ -676,7 +692,7 @@ static int dirty_bitmap_load(QEMUFile *f, void *opaque, int 
version_id)
          if (ret) {
              return ret;
          }
-    } while (!(s.flags & DIRTY_BITMAP_MIG_FLAG_EOS));
+    } while (!(s->flags & DIRTY_BITMAP_MIG_FLAG_EOS));

trace_dirty_bitmap_load_success();

      return 0;
@@ -684,13 +700,14 @@ static int dirty_bitmap_load(QEMUFile *f, void *opaque, 
int version_id)

static int dirty_bitmap_save_setup(QEMUFile *f, void *opaque)

  {
+    DBMSaveState *s = &((DBMState *)opaque)->save;
      SaveBitmapState *dbms = NULL;
-    if (init_dirty_bitmap_migration() < 0) {
+    if (init_dirty_bitmap_migration(s) < 0) {
          return -1;
      }

- QSIMPLEQ_FOREACH(dbms, &dirty_bitmap_mig_state.dbms_list, entry) {

-        send_bitmap_start(f, dbms);
+    QSIMPLEQ_FOREACH(dbms, &s->dbms_list, entry) {
+        send_bitmap_start(f, s, dbms);
      }
      qemu_put_bitmap_flags(f, DIRTY_BITMAP_MIG_FLAG_EOS);

@@ -699,7 +716,9 @@ static int dirty_bitmap_save_setup(QEMUFile *f, void *opaque)static bool dirty_bitmap_is_active(void *opaque)

  {
-    return migrate_dirty_bitmaps() && !dirty_bitmap_mig_state.no_bitmaps;
+    DBMSaveState *s = &((DBMState *)opaque)->save;
+
+    return migrate_dirty_bitmaps() && !s->no_bitmaps;
  }

static bool dirty_bitmap_is_active_iterate(void *opaque)

@@ -727,10 +746,10 @@ static SaveVMHandlers savevm_dirty_bitmap_handlers = {

void dirty_bitmap_mig_init(void)

  {
-    QSIMPLEQ_INIT(&dirty_bitmap_mig_state.dbms_list);
-    qemu_mutex_init(&finish_lock);
+    QSIMPLEQ_INIT(&dbm_state.save.dbms_list);
+    qemu_mutex_init(&dbm_state.load.finish_lock);

register_savevm_live("dirty-bitmap", 0, 1,

                           &savevm_dirty_bitmap_handlers,
-                         &dirty_bitmap_mig_state);
+                         &dbm_state);
  }


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

[Prev in Thread]

Current Thread

[Next in Thread]

[PATCH v2 12/22] qemu-iotests/199: fix style, (continued)
- [PATCH v2 12/22] qemu-iotests/199: fix style, Vladimir Sementsov-Ogievskiy, 2020/02/17
  - Re: [PATCH v2 12/22] qemu-iotests/199: fix style, Andrey Shinkevich, 2020/02/19
- [PATCH v2 14/22] qemu-iotests/199: better catch postcopy time, Vladimir Sementsov-Ogievskiy, 2020/02/17
  - Re: [PATCH v2 14/22] qemu-iotests/199: better catch postcopy time, Andrey Shinkevich, 2020/02/19
- [PATCH v2 22/22] qemu-iotests/199: add source-killed case to bitmaps postcopy, Vladimir Sementsov-Ogievskiy, 2020/02/17
- [PATCH v2 21/22] qemu-iotests/199: add early shutdown case to bitmaps postcopy, Vladimir Sementsov-Ogievskiy, 2020/02/17
- [PATCH v2 16/22] qemu-iotests/199: change discard patterns, Vladimir Sementsov-Ogievskiy, 2020/02/17
  - Re: [PATCH v2 16/22] qemu-iotests/199: change discard patterns, Andrey Shinkevich, 2020/02/19
    - Re: [PATCH v2 16/22] qemu-iotests/199: change discard patterns, Andrey Shinkevich, 2020/02/19
- [PATCH v2 05/22] migration/block-dirty-bitmap: refactor state global variables, Vladimir Sementsov-Ogievskiy, 2020/02/17
  - Re: [PATCH v2 05/22] migration/block-dirty-bitmap: refactor state global variables, Andrey Shinkevich <=
    - Re: [PATCH v2 05/22] migration/block-dirty-bitmap: refactor state global variables, Vladimir Sementsov-Ogievskiy, 2020/02/19
- [PATCH v2 06/22] migration/block-dirty-bitmap: rename finish_lock to just lock, Vladimir Sementsov-Ogievskiy, 2020/02/17
  - Re: [PATCH v2 06/22] migration/block-dirty-bitmap: rename finish_lock to just lock, Andrey Shinkevich, 2020/02/18
- [PATCH v2 13/22] qemu-iotests/199: drop extra constraints, Vladimir Sementsov-Ogievskiy, 2020/02/17
  - Re: [PATCH v2 13/22] qemu-iotests/199: drop extra constraints, Andrey Shinkevich, 2020/02/19
- [PATCH v2 15/22] qemu-iotests/199: improve performance: set bitmap by discard, Vladimir Sementsov-Ogievskiy, 2020/02/17
  - Re: [PATCH v2 15/22] qemu-iotests/199: improve performance: set bitmap by discard, Andrey Shinkevich, 2020/02/19
- [PATCH v2 11/22] migration/savevm: don't worry if bitmap migration postcopy failed, Vladimir Sementsov-Ogievskiy, 2020/02/17
  - Re: [PATCH v2 11/22] migration/savevm: don't worry if bitmap migration postcopy failed, Dr. David Alan Gilbert, 2020/02/17
  - Re: [PATCH v2 11/22] migration/savevm: don't worry if bitmap migration postcopy failed, Andrey Shinkevich, 2020/02/18

Prev by Date: Re: [PATCH v3 03/33] block: Add BdrvChildRole and BdrvChildRoleBits
Next by Date: Re: [PATCH v3 04/33] block: Add BdrvChildRole to BdrvChild
Previous by thread: [PATCH v2 05/22] migration/block-dirty-bitmap: refactor state global variables
Next by thread: Re: [PATCH v2 05/22] migration/block-dirty-bitmap: refactor state global variables
Index(es):
- Date
- Thread