qemu-devel
[Top][All Lists]
Advanced

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

Re: [Qemu-devel] [PATCH 3/3] block: per caller dirty bitmap


From: Fam Zheng
Subject: Re: [Qemu-devel] [PATCH 3/3] block: per caller dirty bitmap
Date: Mon, 04 Nov 2013 14:59:30 +0800
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.1.0


On 10/30/2013 03:49 PM, Paolo Bonzini wrote:
Il 30/10/2013 08:08, Fam Zheng ha scritto:
Previously a BlockDriverState has only one dirty bitmap, so only one
caller (e.g. a block job) can keep track of writing. This changes the
dirty bitmap to a list and creates one HBitmap for each caller, the
lifecycle is managed with these new functions:

     bdrv_create_dirty_bitmap
     bdrv_release_dirty_bitmap

In place of this one:

     bdrv_set_dirty_tracking

An HBitmap pointer argument is added to these functions, since each
caller has its own dirty bitmap:

     bdrv_get_dirty
     bdrv_dirty_iter_init
     bdrv_get_dirty_count

While bdrv_set_dirty and bdrv_reset_dirty prototypes unchanged but
internally walk the list of all dirty bitmaps and set them one by one.

Signed-off-by: Fam Zheng <address@hidden>
---
  block-migration.c         | 22 ++++++++++----
  block.c                   | 74 ++++++++++++++++++++++++++---------------------
  block/mirror.c            | 23 ++++++++-------
  block/qapi.c              |  8 -----
  include/block/block.h     | 11 ++++---
  include/block/block_int.h |  2 +-
  6 files changed, 78 insertions(+), 62 deletions(-)

diff --git a/block-migration.c b/block-migration.c
index daf9ec1..08df056 100644
--- a/block-migration.c
+++ b/block-migration.c
@@ -58,6 +58,7 @@ typedef struct BlkMigDevState {
      /* Protected by block migration lock.  */
      unsigned long *aio_bitmap;
      int64_t completed_sectors;
+    HBitmap *dirty_bitmap;
  } BlkMigDevState;
typedef struct BlkMigBlock {
@@ -309,12 +310,21 @@ static int mig_save_device_bulk(QEMUFile *f, 
BlkMigDevState *bmds)
/* Called with iothread lock taken. */ -static void set_dirty_tracking(int enable)
+static void set_dirty_tracking(void)
  {
      BlkMigDevState *bmds;
QSIMPLEQ_FOREACH(bmds, &block_mig_state.bmds_list, entry) {
-        bdrv_set_dirty_tracking(bmds->bs, enable ? BLOCK_SIZE : 0);
+        bmds->dirty_bitmap = bdrv_create_dirty_bitmap(bmds->bs, BLOCK_SIZE);
+    }
+}
+
+static void unset_dirty_tracking(void)
+{
+    BlkMigDevState *bmds;
+
+    QSIMPLEQ_FOREACH(bmds, &block_mig_state.bmds_list, entry) {
+        bdrv_release_dirty_bitmap(bmds->bs, bmds->dirty_bitmap);
      }
  }
@@ -432,7 +442,7 @@ static int mig_save_device_dirty(QEMUFile *f, BlkMigDevState *bmds,
          } else {
              blk_mig_unlock();
          }
-        if (bdrv_get_dirty(bmds->bs, sector)) {
+        if (bdrv_get_dirty(bmds->bs, bmds->dirty_bitmap, sector)) {
if (total_sectors - sector < BDRV_SECTORS_PER_DIRTY_CHUNK) {
                  nr_sectors = total_sectors - sector;
@@ -554,7 +564,7 @@ static int64_t get_remaining_dirty(void)
      int64_t dirty = 0;
QSIMPLEQ_FOREACH(bmds, &block_mig_state.bmds_list, entry) {
-        dirty += bdrv_get_dirty_count(bmds->bs);
+        dirty += bdrv_get_dirty_count(bmds->bs, bmds->dirty_bitmap);
      }
return dirty << BDRV_SECTOR_BITS;
@@ -569,7 +579,7 @@ static void blk_mig_cleanup(void)
bdrv_drain_all(); - set_dirty_tracking(0);
+    unset_dirty_tracking();
blk_mig_lock();
      while ((bmds = QSIMPLEQ_FIRST(&block_mig_state.bmds_list)) != NULL) {
@@ -604,7 +614,7 @@ static int block_save_setup(QEMUFile *f, void *opaque)
      init_blk_migration(f);
/* start track dirty blocks */
-    set_dirty_tracking(1);
+    set_dirty_tracking();
      qemu_mutex_unlock_iothread();
ret = flush_blks(f);
diff --git a/block.c b/block.c
index fd05a80..9975428 100644
--- a/block.c
+++ b/block.c
@@ -323,6 +323,7 @@ BlockDriverState *bdrv_new(const char *device_name)
      BlockDriverState *bs;
bs = g_malloc0(sizeof(BlockDriverState));
+    QLIST_INIT(&bs->dirty_bitmaps);
      pstrcpy(bs->device_name, sizeof(bs->device_name), device_name);
      if (device_name[0] != '\0') {
          QTAILQ_INSERT_TAIL(&bdrv_states, bs, list);
@@ -1614,7 +1615,7 @@ static void bdrv_move_feature_fields(BlockDriverState 
*bs_dest,
      bs_dest->iostatus           = bs_src->iostatus;
/* dirty bitmap */
-    bs_dest->dirty_bitmap       = bs_src->dirty_bitmap;
+    bs_dest->dirty_bitmaps      = bs_src->dirty_bitmaps;
/* reference count */
      bs_dest->refcnt             = bs_src->refcnt;
@@ -1647,7 +1648,7 @@ void bdrv_swap(BlockDriverState *bs_new, BlockDriverState 
*bs_old)
/* bs_new must be anonymous and shouldn't have anything fancy enabled */
      assert(bs_new->device_name[0] == '\0');
-    assert(bs_new->dirty_bitmap == NULL);
+    assert(QLIST_EMPTY(&bs_new->dirty_bitmaps));
      assert(bs_new->job == NULL);
      assert(bs_new->dev == NULL);
      assert(bs_new->in_use == 0);
@@ -1708,6 +1709,7 @@ static void bdrv_delete(BlockDriverState *bs)
      assert(!bs->job);
      assert(!bs->in_use);
      assert(!bs->refcnt);
+    assert(QLIST_EMPTY(&bs->dirty_bitmaps));
bdrv_close(bs); @@ -2784,9 +2786,7 @@ static int coroutine_fn bdrv_co_do_writev(BlockDriverState *bs,
          ret = bdrv_co_flush(bs);
      }
- if (bs->dirty_bitmap) {
          bdrv_set_dirty(bs, sector_num, nb_sectors);
-    }
if (bs->wr_highest_sector < sector_num + nb_sectors - 1) {
          bs->wr_highest_sector = sector_num + nb_sectors - 1;
@@ -3321,7 +3321,7 @@ int bdrv_write_compressed(BlockDriverState *bs, int64_t 
sector_num,
      if (bdrv_check_request(bs, sector_num, nb_sectors))
          return -EIO;
- assert(!bs->dirty_bitmap);
+    assert(QLIST_EMPTY(&bs->dirty_bitmaps));
return drv->bdrv_write_compressed(bs, sector_num, buf, nb_sectors);
  }
@@ -4181,9 +4181,7 @@ int coroutine_fn bdrv_co_discard(BlockDriverState *bs, 
int64_t sector_num,
          return -EROFS;
      }
- if (bs->dirty_bitmap) {
-        bdrv_reset_dirty(bs, sector_num, nb_sectors);
-    }
+    bdrv_reset_dirty(bs, sector_num, nb_sectors);
/* Do nothing if disabled. */
      if (!(bs->open_flags & BDRV_O_UNMAP)) {
@@ -4345,58 +4343,68 @@ bool bdrv_qiov_is_aligned(BlockDriverState *bs, 
QEMUIOVector *qiov)
      return true;
  }
-void bdrv_set_dirty_tracking(BlockDriverState *bs, int granularity)
+HBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs, int granularity)
  {
      int64_t bitmap_size;
+    HBitmap *bitmap;
assert((granularity & (granularity - 1)) == 0); - if (granularity) {
-        granularity >>= BDRV_SECTOR_BITS;
-        assert(!bs->dirty_bitmap);
-        bitmap_size = (bdrv_getlength(bs) >> BDRV_SECTOR_BITS);
-        bs->dirty_bitmap = hbitmap_alloc(bitmap_size, ffs(granularity) - 1);
-    } else {
-        if (bs->dirty_bitmap) {
-            hbitmap_free(bs->dirty_bitmap);
-            bs->dirty_bitmap = NULL;
+    granularity >>= BDRV_SECTOR_BITS;
+    assert(granularity);
+    bitmap_size = (bdrv_getlength(bs) >> BDRV_SECTOR_BITS);
+    bitmap = hbitmap_alloc(bitmap_size, ffs(granularity) - 1);
+    QLIST_INSERT_HEAD(&bs->dirty_bitmaps, bitmap, list);
+    return bitmap;
+}
+
+void bdrv_release_dirty_bitmap(BlockDriverState *bs, HBitmap *bitmap)
+{
+    HBitmap *bm, *next;
+    QLIST_FOREACH_SAFE(bm, &bs->dirty_bitmaps, list, next) {
+        if (bm == bitmap) {
+            QLIST_REMOVE(bitmap, list);
+            hbitmap_free(bitmap);
+            return;
          }
      }
  }
-int bdrv_get_dirty(BlockDriverState *bs, int64_t sector)
+int bdrv_get_dirty(BlockDriverState *bs, HBitmap *bitmap, int64_t sector)
  {
-    if (bs->dirty_bitmap) {
-        return hbitmap_get(bs->dirty_bitmap, sector);
+    if (bitmap) {
+        return hbitmap_get(bitmap, sector);
      } else {
          return 0;
      }
  }
-void bdrv_dirty_iter_init(BlockDriverState *bs, HBitmapIter *hbi)
+void bdrv_dirty_iter_init(BlockDriverState *bs,
+                          HBitmap *bitmap, HBitmapIter *hbi)
  {
-    hbitmap_iter_init(hbi, bs->dirty_bitmap, 0);
+    hbitmap_iter_init(hbi, bitmap, 0);
  }
void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector,
                      int nr_sectors)
  {
-    hbitmap_set(bs->dirty_bitmap, cur_sector, nr_sectors);
+    HBitmap *bitmap;
+    QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) {
+        hbitmap_set(bitmap, cur_sector, nr_sectors);
+    }
  }
-void bdrv_reset_dirty(BlockDriverState *bs, int64_t cur_sector,
-                      int nr_sectors)
+void bdrv_reset_dirty(BlockDriverState *bs, int64_t cur_sector, int nr_sectors)
  {
-    hbitmap_reset(bs->dirty_bitmap, cur_sector, nr_sectors);
+    HBitmap *bitmap;
+    QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) {
+        hbitmap_reset(bitmap, cur_sector, nr_sectors);
+    }
  }
I think callers outside block.c should only call
hbitmap_set/hbitmap_reset; resetting is typically done before processing
sectors and setting after an error (both of which happen privately to
each task).

Thus you probably should add a fourth patch which makes
bdrv_(re)set_dirty static and remove
bdrv_get_dirty/bdrv_dirty_iter_init/bdrv_get_dirty_count.
I like the idea of adding a wrapper struct (will be BdrvDirtyBitmap) to HBitmap so that patch 1 is not needed, and HBitmap becomes (almost) internal to block.c.

But I'm not sure removing bdrv_get_dirty/bdrv_dirty_iter_init/bdrv_get_dirty_count is good, as we are exposing BdrvDirtyBitmap, we should also provide operations on it, instead of let callers to handle HBitmap pointer inside. In other words, I prefer to define BdrvDirtyBitmap structure in block.c and only put a type declaration in header.

Fam




reply via email to

[Prev in Thread] Current Thread [Next in Thread]