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

2013-10-30 Thread Fam Zheng 
Eric,

This touches qapi code, forgot to Cc you.

Fam

On Wed, 10/30 15:08, Fam Zheng wrote:
> 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 
> ---
>  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);
>  asser

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

2013-10-30 Thread Paolo Bonzini 
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 
> ---
>  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);
> +

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

2013-11-03 Thread Fam Zheng 


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 
---
  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 *

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

2013-11-04 Thread Paolo Bonzini 
Il 04/11/2013 07:59, Fam Zheng ha scritto:
>>>
>> 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.

If you want to expose BdrvDirtyBitmap, having wrappers is fine.  I was
thinking of exposing HBitmap instead and keeping BdrvDirtyBitmap internal.

Either way is fine, and as the person who writes the code you have the
privilege of making the choice. :)

Paolo