Re: [Qemu-devel] [PATCH v9 07/10] qmp: Add support of "dirty-bitmap" sync mode for drive-backup

[Stefan Hajnoczi]

On Mon, Dec 01, 2014 at 03:30:13PM -0500, John Snow wrote:
> From: Fam Zheng <address@hidden>
> 
> For "dirty-bitmap" sync mode, the block job will iterate through the
> given dirty bitmap to decide if a sector needs backup (backup all the
> dirty clusters and skip clean ones), just as allocation conditions of
> "top" sync mode.
> 
> There are two bitmap use modes for sync=dirty-bitmap:
> 
>  - reset: backup job makes a copy of bitmap and resets the original
>    one.
>  - consume: backup job makes the original anonymous (invisible to user)
>    and releases it after use.

It's not obvious to me that we need both modes.  Can you explain why the
choice between reset and consume is necessary?

> Signed-off-by: Fam Zheng <address@hidden>
> Signed-off-by: John Snow <address@hidden>
> Reviewed-by: Max Reitz <address@hidden>
> ---
>  block.c                   |   5 ++
>  block/backup.c            | 130 
> ++++++++++++++++++++++++++++++++++++++--------
>  block/mirror.c            |   4 ++
>  blockdev.c                |  17 +++++-
>  hmp.c                     |   4 +-
>  include/block/block.h     |   1 +
>  include/block/block_int.h |   6 +++
>  qapi/block-core.json      |  30 +++++++++--
>  qmp-commands.hx           |   7 +--
>  9 files changed, 174 insertions(+), 30 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 85215b3..42244f6 100644
> --- a/block.c
> +++ b/block.c
> @@ -5489,6 +5489,11 @@ void bdrv_dirty_iter_init(BlockDriverState *bs,
>      hbitmap_iter_init(hbi, bitmap->bitmap, 0);
>  }
>  
> +void bdrv_dirty_iter_set(HBitmapIter *hbi, int64_t offset)
> +{
> +    hbitmap_iter_init(hbi, hbi->hb, offset);
> +}
> +
>  void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector,
>                      int nr_sectors)
>  {
> diff --git a/block/backup.c b/block/backup.c
> index 792e655..2aab68f 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -37,6 +37,10 @@ typedef struct CowRequest {
>  typedef struct BackupBlockJob {
>      BlockJob common;
>      BlockDriverState *target;
> +    /* bitmap for sync=dirty-bitmap */
> +    BdrvDirtyBitmap *sync_bitmap;
> +    /* dirty bitmap granularity */
> +    int64_t sync_bitmap_gran;

What is the point of this field?

I think we've duplicated granularity twice now (originally stored in
HBitmap, once in BdrvDirtyBitmap, and now again).

Maybe the one user of this field should just call
bdrv_dirty_bitmap_granularity().

>      MirrorSyncMode sync_mode;
>      RateLimit limit;
>      BlockdevOnError on_source_error;
> @@ -242,6 +246,31 @@ static void backup_complete(BlockJob *job, void *opaque)
>      g_free(data);
>  }
>  
> +static bool yield_and_check(BackupBlockJob *job)

missing coroutine_fn annotation.  coroutine_fn tells the reader that
this function may only be called from coroutine context.

(In this case the function and contains "yield" so that's a big hint but
please still always mark coroutine functions coroutine_fn so that we can
enforce static checking in the future.)

pgp2rsIL4d_jf.pgp
Description: PGP signature