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

2013-11-12 Thread Kevin Wolf 
Am 04.11.2013 um 10:30 hat Fam Zheng geschrieben:
 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 a BdrvDirtyBitmap for each caller, the
 lifecycle is managed with these new functions:
 
 bdrv_create_dirty_bitmap
 bdrv_release_dirty_bitmap
 
 Where BdrvDirtyBitmap is a linked list wrapper structure of HBitmap.
 
 In place of bdrv_set_dirty_tracking, a BdrvDirtyBitmap 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
 
 bdrv_set_dirty and bdrv_reset_dirty prototypes are unchanged but will
 internally walk the list of all dirty bitmaps and set them one by one.
 
 Signed-off-by: Fam Zheng f...@redhat.com

 diff --git a/block/qapi.c b/block/qapi.c
 index 5880b3e..6b0cdcf 100644
 --- a/block/qapi.c
 +++ b/block/qapi.c
 @@ -204,14 +204,6 @@ void bdrv_query_info(BlockDriverState *bs,
  info-io_status = bs-iostatus;
  }
  
 -if (bs-dirty_bitmap) {
 -info-has_dirty = true;
 -info-dirty = g_malloc0(sizeof(*info-dirty));
 -info-dirty-count = bdrv_get_dirty_count(bs) * BDRV_SECTOR_SIZE;
 -info-dirty-granularity =
 - ((int64_t) BDRV_SECTOR_SIZE  
 hbitmap_granularity(bs-dirty_bitmap));
 -}
 -
  if (bs-drv) {
  info-has_inserted = true;
  info-inserted = g_malloc0(sizeof(*info-inserted));

The dirty field should probably be removed from qapi-schema.json if it
never gets set any more.

It was optional, so perhaps removing it doesn't cause any trouble
indeed, but I'd like to hear Eric on this matter before merging the
patch. Though if libvirt does make use of it, we have a problem because
it doesn't really make sense any more after these changes.

Kevin

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

2013-11-12 Thread Eric Blake 
On 11/12/2013 03:46 AM, Kevin Wolf wrote:

 +++ b/block/qapi.c
 @@ -204,14 +204,6 @@ void bdrv_query_info(BlockDriverState *bs,
  info-io_status = bs-iostatus;
  }
  
 -if (bs-dirty_bitmap) {
 -info-has_dirty = true;
 -info-dirty = g_malloc0(sizeof(*info-dirty));
 -info-dirty-count = bdrv_get_dirty_count(bs) * BDRV_SECTOR_SIZE;
 -info-dirty-granularity =
 - ((int64_t) BDRV_SECTOR_SIZE  
 hbitmap_granularity(bs-dirty_bitmap));
 -}
 -
  if (bs-drv) {
  info-has_inserted = true;
  info-inserted = g_malloc0(sizeof(*info-inserted));
 
 The dirty field should probably be removed from qapi-schema.json if it
 never gets set any more.
 
 It was optional, so perhaps removing it doesn't cause any trouble
 indeed, but I'd like to hear Eric on this matter before merging the
 patch. Though if libvirt does make use of it, we have a problem because
 it doesn't really make sense any more after these changes.

Removing an optional output-only field is okay (it is the same as
stating that we are ALWAYS taking the option of not including it).
Since it has always been marked optional, management can't be relying on
it; more specifically, libvirt does not have any code checking for it.
So I'm okay with removing it from the schema as a backwards-compatible
change.

[Removing an optional input field is wrong, but BlockInfo is an output
type, not an input type.]

Eventually, libvirt would like to use persistent dirty bitmaps, in order
to resume long-running operations such as drive-mirror even across
migration between different qemu processes, but I don't think we are
quite there yet, and I also don't think that removal of 'dirty' from
BlockInfo impedes in that goal.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

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

2013-11-11 Thread Stefan Hajnoczi 
On Mon, Nov 04, 2013 at 11:55:47AM +0100, Paolo Bonzini wrote:
 Il 04/11/2013 11:47, Fam Zheng ha scritto:
 
  -void bdrv_set_dirty_tracking(BlockDriverState *bs, int granularity);
  -int bdrv_get_dirty(BlockDriverState *bs, int64_t sector);
  +typedef struct BdrvDirtyBitmap BdrvDirtyBitmap;
  +BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs, int
  granularity);
  +void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap
  *bitmap);
  +int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
  int64_t sector);
void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector, int
  nr_sectors);
void bdrv_reset_dirty(BlockDriverState *bs, int64_t cur_sector, int
  nr_sectors);
  -void bdrv_dirty_iter_init(BlockDriverState *bs, struct HBitmapIter
  *hbi);
  -int64_t bdrv_get_dirty_count(BlockDriverState *bs);
  +void bdrv_dirty_iter_init(BlockDriverState *bs,
  +  BdrvDirtyBitmap *bitmap, struct
  HBitmapIter *hbi);
  +int64_t bdrv_get_dirty_count(BlockDriverState *bs, BdrvDirtyBitmap
  *bitmap);
  void bdrv_enable_copy_on_read(BlockDriverState *bs);
void bdrv_disable_copy_on_read(BlockDriverState *bs);
  You do not really need the BDS argument to the functions, do you?  (Or
  do you have other plans?)
  
  I just wanted to keep the pattern of those bdrv_* family, no other plans
  for it.
 
 Kevin, Stefan, any second opinions?

I was thinking the same thing and arrived at the conclusion that since
it's bdrv_*() and not bbitmap_*() we should keep the explicit BDS
argument.

Stefan

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

2013-11-11 Thread Stefan Hajnoczi 
On Mon, Nov 04, 2013 at 05:30:10PM +0800, Fam Zheng wrote:
 @@ -2785,9 +2792,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);
 -}

Forgot to reduce indentation?

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

2013-11-11 Thread Stefan Hajnoczi 
On Mon, Nov 04, 2013 at 05:30:10PM +0800, 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 a BdrvDirtyBitmap for each caller, the
 lifecycle is managed with these new functions:
 
 bdrv_create_dirty_bitmap
 bdrv_release_dirty_bitmap
 
 Where BdrvDirtyBitmap is a linked list wrapper structure of HBitmap.
 
 In place of bdrv_set_dirty_tracking, a BdrvDirtyBitmap 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
 
 bdrv_set_dirty and bdrv_reset_dirty prototypes are unchanged but will
 internally walk the list of all dirty bitmaps and set them one by one.
 
 Signed-off-by: Fam Zheng f...@redhat.com
 
 ---
 v2: Added BdrvDirtyBitmap [Paolo]
 
 Signed-off-by: Fam Zheng f...@redhat.com
 ---
  block-migration.c | 22 +
  block.c   | 81 
 ---
  block/mirror.c| 23 --
  block/qapi.c  |  8 -
  include/block/block.h | 11 ---
  include/block/block_int.h |  2 +-
  6 files changed, 85 insertions(+), 62 deletions(-)

Happy with this modulo the indentation fixup I commented on.

Reviewed-by: Stefan Hajnoczi stefa...@redhat.com

[Qemu-devel] [PATCH v2] block: per caller dirty bitmap

2013-11-04 Thread Fam Zheng 
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 a BdrvDirtyBitmap for each caller, the
lifecycle is managed with these new functions:

bdrv_create_dirty_bitmap
bdrv_release_dirty_bitmap

Where BdrvDirtyBitmap is a linked list wrapper structure of HBitmap.

In place of bdrv_set_dirty_tracking, a BdrvDirtyBitmap 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

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

Signed-off-by: Fam Zheng f...@redhat.com

---
v2: Added BdrvDirtyBitmap [Paolo]

Signed-off-by: Fam Zheng f...@redhat.com
---
 block-migration.c | 22 +
 block.c   | 81 ---
 block/mirror.c| 23 --
 block/qapi.c  |  8 -
 include/block/block.h | 11 ---
 include/block/block_int.h |  2 +-
 6 files changed, 85 insertions(+), 62 deletions(-)

diff --git a/block-migration.c b/block-migration.c
index daf9ec1..8f4e826 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;
+BdrvDirtyBitmap *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 58efb5b..ef7a55f 100644
--- a/block.c
+++ b/block.c
@@ -49,6 +49,11 @@
 #include windows.h
 #endif
 
+typedef struct BdrvDirtyBitmap {
+HBitmap *bitmap;
+QLIST_ENTRY(BdrvDirtyBitmap) list;
+} BdrvDirtyBitmap;
+
 #define NOT_DONE 0x7fff /* used while emulated sync operation in progress 
*/
 
 typedef enum {
@@ -323,6 +328,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);
@@ -1615,7 +1621,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;
@@ -1648,7 +1654,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);
@@ -1709,6 +1715,7 @@ static void bdrv_delete(BlockDriverState *bs)

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

2013-11-04 Thread Paolo Bonzini 
Il 04/11/2013 10:30, Fam Zheng ha scritto:
 diff --git a/include/block/block.h b/include/block/block.h
 index 3560deb..06f424c 100644
 --- a/include/block/block.h
 +++ b/include/block/block.h
 @@ -388,12 +388,15 @@ void *qemu_blockalign(BlockDriverState *bs, size_t 
 size);
  bool bdrv_qiov_is_aligned(BlockDriverState *bs, QEMUIOVector *qiov);
  
  struct HBitmapIter;
 -void bdrv_set_dirty_tracking(BlockDriverState *bs, int granularity);
 -int bdrv_get_dirty(BlockDriverState *bs, int64_t sector);
 +typedef struct BdrvDirtyBitmap BdrvDirtyBitmap;
 +BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs, int 
 granularity);
 +void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap 
 *bitmap);
 +int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, int64_t 
 sector);
  void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector, int 
 nr_sectors);
  void bdrv_reset_dirty(BlockDriverState *bs, int64_t cur_sector, int 
 nr_sectors);
 -void bdrv_dirty_iter_init(BlockDriverState *bs, struct HBitmapIter *hbi);
 -int64_t bdrv_get_dirty_count(BlockDriverState *bs);
 +void bdrv_dirty_iter_init(BlockDriverState *bs,
 +  BdrvDirtyBitmap *bitmap, struct HBitmapIter *hbi);
 +int64_t bdrv_get_dirty_count(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
  
  void bdrv_enable_copy_on_read(BlockDriverState *bs);
  void bdrv_disable_copy_on_read(BlockDriverState *bs);

You do not really need the BDS argument to the functions, do you?  (Or
do you have other plans?)

Apart from this, looks good.

Paolo

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

2013-11-04 Thread Fam Zheng 


On 11/04/2013 06:37 PM, Paolo Bonzini wrote:

Il 04/11/2013 10:30, Fam Zheng ha scritto:

diff --git a/include/block/block.h b/include/block/block.h
index 3560deb..06f424c 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -388,12 +388,15 @@ void *qemu_blockalign(BlockDriverState *bs, size_t size);
  bool bdrv_qiov_is_aligned(BlockDriverState *bs, QEMUIOVector *qiov);
  
  struct HBitmapIter;

-void bdrv_set_dirty_tracking(BlockDriverState *bs, int granularity);
-int bdrv_get_dirty(BlockDriverState *bs, int64_t sector);
+typedef struct BdrvDirtyBitmap BdrvDirtyBitmap;
+BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs, int 
granularity);
+void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
+int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, int64_t 
sector);
  void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector, int nr_sectors);
  void bdrv_reset_dirty(BlockDriverState *bs, int64_t cur_sector, int 
nr_sectors);
-void bdrv_dirty_iter_init(BlockDriverState *bs, struct HBitmapIter *hbi);
-int64_t bdrv_get_dirty_count(BlockDriverState *bs);
+void bdrv_dirty_iter_init(BlockDriverState *bs,
+  BdrvDirtyBitmap *bitmap, struct HBitmapIter *hbi);
+int64_t bdrv_get_dirty_count(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
  
  void bdrv_enable_copy_on_read(BlockDriverState *bs);

  void bdrv_disable_copy_on_read(BlockDriverState *bs);

You do not really need the BDS argument to the functions, do you?  (Or
do you have other plans?)


I just wanted to keep the pattern of those bdrv_* family, no other plans 
for it.


Fam

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

2013-11-04 Thread Paolo Bonzini 
Il 04/11/2013 11:47, Fam Zheng ha scritto:

 -void bdrv_set_dirty_tracking(BlockDriverState *bs, int granularity);
 -int bdrv_get_dirty(BlockDriverState *bs, int64_t sector);
 +typedef struct BdrvDirtyBitmap BdrvDirtyBitmap;
 +BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs, int
 granularity);
 +void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap
 *bitmap);
 +int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
 int64_t sector);
   void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector, int
 nr_sectors);
   void bdrv_reset_dirty(BlockDriverState *bs, int64_t cur_sector, int
 nr_sectors);
 -void bdrv_dirty_iter_init(BlockDriverState *bs, struct HBitmapIter
 *hbi);
 -int64_t bdrv_get_dirty_count(BlockDriverState *bs);
 +void bdrv_dirty_iter_init(BlockDriverState *bs,
 +  BdrvDirtyBitmap *bitmap, struct
 HBitmapIter *hbi);
 +int64_t bdrv_get_dirty_count(BlockDriverState *bs, BdrvDirtyBitmap
 *bitmap);
 void bdrv_enable_copy_on_read(BlockDriverState *bs);
   void bdrv_disable_copy_on_read(BlockDriverState *bs);
 You do not really need the BDS argument to the functions, do you?  (Or
 do you have other plans?)
 
 I just wanted to keep the pattern of those bdrv_* family, no other plans
 for it.

Kevin, Stefan, any second opinions?

Paolo

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

2013-11-04 Thread Benoît Canet 
Le Monday 04 Nov 2013 à 17:30:10 (+0800), Fam Zheng a écrit :
 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 a BdrvDirtyBitmap for each caller, the
 lifecycle is managed with these new functions:
 
 bdrv_create_dirty_bitmap
 bdrv_release_dirty_bitmap
 
 Where BdrvDirtyBitmap is a linked list wrapper structure of HBitmap.
 
 In place of bdrv_set_dirty_tracking, a BdrvDirtyBitmap 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
 
 bdrv_set_dirty and bdrv_reset_dirty prototypes are unchanged but will
 internally walk the list of all dirty bitmaps and set them one by one.
 
 Signed-off-by: Fam Zheng f...@redhat.com
 
 ---
 v2: Added BdrvDirtyBitmap [Paolo]
 
 Signed-off-by: Fam Zheng f...@redhat.com
 ---
  block-migration.c | 22 +
  block.c   | 81 
 ---
  block/mirror.c| 23 --
  block/qapi.c  |  8 -
  include/block/block.h | 11 ---
  include/block/block_int.h |  2 +-
  6 files changed, 85 insertions(+), 62 deletions(-)
 
 diff --git a/block-migration.c b/block-migration.c
 index daf9ec1..8f4e826 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;
 +BdrvDirtyBitmap *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 58efb5b..ef7a55f 100644
 --- a/block.c
 +++ b/block.c
 @@ -49,6 +49,11 @@
  #include windows.h
  #endif
  
 +typedef struct BdrvDirtyBitmap {
 +HBitmap *bitmap;
 +QLIST_ENTRY(BdrvDirtyBitmap) list;
 +} BdrvDirtyBitmap;
 +
  #define NOT_DONE 0x7fff /* used while emulated sync operation in 
 progress */
  
  typedef enum {
 @@ -323,6 +328,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);
 @@ -1615,7 +1621,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;
 @@ -1648,7 +1654,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);
 +

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

2013-11-04 Thread Fam Zheng 


On 11/04/2013 10:38 PM, Benoît Canet wrote:

Le Monday 04 Nov 2013 à 17:30:10 (+0800), Fam Zheng a écrit :

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 a BdrvDirtyBitmap for each caller, the
lifecycle is managed with these new functions:

 bdrv_create_dirty_bitmap
 bdrv_release_dirty_bitmap

Where BdrvDirtyBitmap is a linked list wrapper structure of HBitmap.

In place of bdrv_set_dirty_tracking, a BdrvDirtyBitmap 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

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

Signed-off-by: Fam Zheng f...@redhat.com

---
v2: Added BdrvDirtyBitmap [Paolo]

Signed-off-by: Fam Zheng f...@redhat.com
---
  block-migration.c | 22 +
  block.c   | 81 ---
  block/mirror.c| 23 --
  block/qapi.c  |  8 -
  include/block/block.h | 11 ---
  include/block/block_int.h |  2 +-
  6 files changed, 85 insertions(+), 62 deletions(-)

diff --git a/block-migration.c b/block-migration.c
index daf9ec1..8f4e826 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;
+BdrvDirtyBitmap *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 58efb5b..ef7a55f 100644
--- a/block.c
+++ b/block.c
@@ -49,6 +49,11 @@
  #include windows.h
  #endif
  
+typedef struct BdrvDirtyBitmap {

+HBitmap *bitmap;
+QLIST_ENTRY(BdrvDirtyBitmap) list;
+} BdrvDirtyBitmap;
+
  #define NOT_DONE 0x7fff /* used while emulated sync operation in progress 
*/
  
  typedef enum {

@@ -323,6 +328,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);
@@ -1615,7 +1621,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;
@@ -1648,7 +1654,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);
+