Re: [Qemu-devel] [PATCH v2 8/9] qmp: Add support of "dirty-bitmap" sync mode for drive-backup

[Fam Zheng]

On Thu, 03/13 15:32, Benoît Canet wrote:
> The Wednesday 12 Mar 2014 à 14:31:03 (+0800), Fam Zheng wrote :
> > 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: back job makes the original anonymous (invisible to user)
> s/back job/backup job/ ? 

Yes, thanks.

> 
> >    and releases it after use.
> > 
> > Signed-off-by: Fam Zheng <address@hidden>
> > ---
> >  block/backup.c            | 51 
> > ++++++++++++++++++++++++++++++++++++++++++++++-
> >  block/mirror.c            |  4 ++++
> >  blockdev.c                |  9 ++++++++-
> >  hmp.c                     |  4 +++-
> >  include/block/block_int.h |  3 +++
> >  qapi-schema.json          | 30 ++++++++++++++++++++++++----
> >  qmp-commands.hx           |  7 ++++---
> >  7 files changed, 98 insertions(+), 10 deletions(-)
> > 
> > diff --git a/block/backup.c b/block/backup.c
> > index 15a2e55..6b05429 100644
> > --- a/block/backup.c
> > +++ b/block/backup.c
> > @@ -37,6 +37,8 @@ typedef struct CowRequest {
> >  typedef struct BackupBlockJob {
> >      BlockJob common;
> >      BlockDriverState *target;
> > +    BdrvDirtyBitmap *sync_bitmap;
> > +    int sync_bitmap_gran;
> 
> Maybe a comment would help.
> Reading this variable name for the first time I wonder what the _gran suffix 
> mean.

OK.

> 
> >      MirrorSyncMode sync_mode;
> >      RateLimit limit;
> >      BlockdevOnError on_source_error;
> > @@ -263,7 +265,7 @@ static void coroutine_fn backup_run(void *opaque)
> >              job->common.busy = true;
> >          }
> >      } else {
> > -        /* Both FULL and TOP SYNC_MODE's require copying.. */
> > +        /* FULL, TOP and DIRTY_BITMAP SYNC_MODE's require copying.. */
> >          for (; start < end; start++) {
> >              bool error_is_read;
> >  
> > @@ -317,7 +319,21 @@ static void coroutine_fn backup_run(void *opaque)
> >                  if (alloced == 0) {
> >                      continue;
> >                  }
> > +            } else if (job->sync_mode == MIRROR_SYNC_MODE_DIRTY_BITMAP) {
> > +                int i, dirty = 0;
> > +                for (i = 0; i < BACKUP_SECTORS_PER_CLUSTER;
> > +                     i += job->sync_bitmap_gran) {
> 
> Ok it's _gran for _granularity.
> 
> > +                    if (bdrv_get_dirty(bs, job->sync_bitmap,
> > +                            start * BACKUP_SECTORS_PER_CLUSTER + i)) {
> > +                        dirty = 1;
> > +                        break;
> > +                    }
> > +                }
> > +                if (!dirty) {
> > +                    continue;
> > +                }
> >              }
> > +
> >              /* FULL sync mode we copy the whole drive. */
> >              ret = backup_do_cow(bs, start * BACKUP_SECTORS_PER_CLUSTER,
> >                      BACKUP_SECTORS_PER_CLUSTER, &error_is_read);
> > @@ -341,6 +357,9 @@ static void coroutine_fn backup_run(void *opaque)
> >      qemu_co_rwlock_wrlock(&job->flush_rwlock);
> >      qemu_co_rwlock_unlock(&job->flush_rwlock);
> >  
> > +    if (job->sync_bitmap) {
> > +        bdrv_release_dirty_bitmap(bs, job->sync_bitmap);
> > +    }
> >      hbitmap_free(job->bitmap);
> >  
> >      bdrv_iostatus_disable(target);
> > @@ -351,12 +370,15 @@ static void coroutine_fn backup_run(void *opaque)
> >  
> >  void backup_start(BlockDriverState *bs, BlockDriverState *target,
> >                    int64_t speed, MirrorSyncMode sync_mode,
> > +                  BdrvDirtyBitmap *sync_bitmap,
> > +                  BitmapUseMode bitmap_mode,
> >                    BlockdevOnError on_source_error,
> >                    BlockdevOnError on_target_error,
> >                    BlockDriverCompletionFunc *cb, void *opaque,
> >                    Error **errp)
> >  {
> >      int64_t len;
> > +    BdrvDirtyBitmap *original;
> >  
> >      assert(bs);
> >      assert(target);
> > @@ -369,6 +391,28 @@ void backup_start(BlockDriverState *bs, 
> > BlockDriverState *target,
> >          return;
> >      }
> >  
> > +    if (sync_mode == MIRROR_SYNC_MODE_DIRTY_BITMAP && !sync_bitmap) {
> > +        error_setg(errp, "must provide a valid bitmap name for 
> > \"dirty-bitmap\""
> > +                         "sync mode");
> > +        return;
> > +    }
> > +
> > +    if (sync_bitmap) {
> > +        switch (bitmap_mode) {
> > +        case BITMAP_USE_MODE_RESET:
> > +            original = sync_bitmap;
> > +            sync_bitmap = bdrv_copy_dirty_bitmap(bs, sync_bitmap, NULL);
> > +            bdrv_reset_dirty_bitmap(bs, original);
> > +            break;
> > +        case BITMAP_USE_MODE_CONSUME:
> > +            bdrv_dirty_bitmap_make_anon(bs, sync_bitmap);
> > +            break;
> > +        default:
> > +            assert(0);
> > +        }
> > +        bdrv_disable_dirty_bitmap(bs, sync_bitmap);
> > +    }
> > +
> >      len = bdrv_getlength(bs);
> >      if (len < 0) {
> >          error_setg_errno(errp, -len, "unable to get length for '%s'",
> > @@ -386,6 +430,11 @@ void backup_start(BlockDriverState *bs, 
> > BlockDriverState *target,
> >      job->on_target_error = on_target_error;
> >      job->target = target;
> >      job->sync_mode = sync_mode;
> > +    job->sync_bitmap = sync_bitmap;
> > +    if (sync_bitmap) {
> > +        job->sync_bitmap_gran =
> > +            bdrv_dirty_bitmap_granularity(bs, job->sync_bitmap);
> > +    }
> >      job->common.len = len;
> >      job->common.co = qemu_coroutine_create(backup_run);
> >      qemu_coroutine_enter(job->common.co, job);
> > diff --git a/block/mirror.c b/block/mirror.c
> > index be8b2a1..6e4f3f3 100644
> > --- a/block/mirror.c
> > +++ b/block/mirror.c
> > @@ -615,6 +615,10 @@ void mirror_start(BlockDriverState *bs, 
> > BlockDriverState *target,
> >      bool is_none_mode;
> >      BlockDriverState *base;
> >  
> > +    if (mode == MIRROR_SYNC_MODE_DIRTY_BITMAP) {
> > +        error_setg(errp, "Sync mode 'dirty-bitmap' not supported");
> > +        return;
> > +    }
> >      is_none_mode = mode == MIRROR_SYNC_MODE_NONE;
> >      base = mode == MIRROR_SYNC_MODE_TOP ? bs->backing_hd : NULL;
> >      mirror_start_job(bs, target, speed, granularity, buf_size,
> > diff --git a/blockdev.c b/blockdev.c
> > index aa3ee55..4120dee 100644
> > --- a/blockdev.c
> > +++ b/blockdev.c
> > @@ -1384,6 +1384,8 @@ static void drive_backup_prepare(BlkTransactionState 
> > *common, Error **errp)
> >                       backup->sync,
> >                       backup->has_mode, backup->mode,
> >                       backup->has_speed, backup->speed,
> > +                     backup->has_bitmap, backup->bitmap,
> > +                     backup->has_bitmap_use_mode, backup->bitmap_use_mode,
> >                       backup->has_on_source_error, backup->on_source_error,
> >                       backup->has_on_target_error, backup->on_target_error,
> >                       &local_err);
> > @@ -2032,6 +2034,8 @@ void qmp_drive_backup(const char *device, const char 
> > *target,
> >                        enum MirrorSyncMode sync,
> >                        bool has_mode, enum NewImageMode mode,
> >                        bool has_speed, int64_t speed,
> > +                      bool has_bitmap, const char *bitmap,
> > +                      bool has_bitmap_use_mode, enum BitmapUseMode 
> > bitmap_mode,
> >                        bool has_on_source_error, BlockdevOnError 
> > on_source_error,
> >                        bool has_on_target_error, BlockdevOnError 
> > on_target_error,
> >                        Error **errp)
> > @@ -2129,7 +2133,10 @@ void qmp_drive_backup(const char *device, const char 
> > *target,
> >          return;
> >      }
> >  
> > -    backup_start(bs, target_bs, speed, sync, on_source_error, 
> > on_target_error,
> > +    backup_start(bs, target_bs, speed, sync,
> > +                 has_bitmap ? bdrv_find_dirty_bitmap(bs, bitmap) : NULL,
> > +                 has_bitmap_use_mode ? bitmap_mode : BITMAP_USE_MODE_RESET,
> > +                 on_source_error, on_target_error,
> >                   block_job_cb, bs, &local_err);
> >      if (local_err != NULL) {
> >          bdrv_unref(target_bs);
> > diff --git a/hmp.c b/hmp.c
> > index 2f279c4..2c84377 100644
> > --- a/hmp.c
> > +++ b/hmp.c
> > @@ -950,7 +950,9 @@ void hmp_drive_backup(Monitor *mon, const QDict *qdict)
> >  
> >      qmp_drive_backup(device, filename, !!format, format,
> >                       full ? MIRROR_SYNC_MODE_FULL : MIRROR_SYNC_MODE_TOP,
> > -                     true, mode, false, 0, false, 0, false, 0, &errp);
> > +                     true, mode, false, 0, false, NULL,
> > +                     false, 0,
> > +                     false, 0, false, 0, &errp);
> >      hmp_handle_error(mon, &errp);
> >  }
> >  
> > diff --git a/include/block/block_int.h b/include/block/block_int.h
> > index 0bcf1c9..d02c090 100644
> > --- a/include/block/block_int.h
> > +++ b/include/block/block_int.h
> > @@ -484,6 +484,7 @@ void mirror_start(BlockDriverState *bs, 
> > BlockDriverState *target,
> >   * @target: Block device to write to.
> >   * @speed: The maximum speed, in bytes per second, or 0 for unlimited.
> >   * @sync_mode: What parts of the disk image should be copied to the 
> > destination.
> > + * @sync_bitmap: The dirty bitmap if sync_mode is 
> > MIRROR_SYNC_MODE_DIRTY_BITMAP.
> >   * @on_source_error: The action to take upon error reading from the source.
> >   * @on_target_error: The action to take upon error writing to the target.
> >   * @cb: Completion function for the job.
> > @@ -494,6 +495,8 @@ void mirror_start(BlockDriverState *bs, 
> > BlockDriverState *target,
> >   */
> >  void backup_start(BlockDriverState *bs, BlockDriverState *target,
> >                    int64_t speed, MirrorSyncMode sync_mode,
> > +                  BdrvDirtyBitmap *sync_bitmap,
> > +                  BitmapUseMode bitmap_mode,
> >                    BlockdevOnError on_source_error,
> >                    BlockdevOnError on_target_error,
> >                    BlockDriverCompletionFunc *cb, void *opaque,
> > diff --git a/qapi-schema.json b/qapi-schema.json
> > index f8cd6b8..e4e66cb 100644
> > --- a/qapi-schema.json
> > +++ b/qapi-schema.json
> > @@ -1503,7 +1503,7 @@
> >  # Since: 1.3
> >  ##
> >  { 'enum': 'MirrorSyncMode',
> > -  'data': ['top', 'full', 'none'] }
> > +  'data': ['top', 'full', 'none', 'dirty-bitmap'] }
> >  
> >  ##
> >  # @BlockJobType:
> > @@ -1879,6 +1879,22 @@
> >    'data': { 'device': 'str', 'name': 'str' } }
> >  
> >  ##
> > +# @BitmapUseMode
> > +#
> > +# An enumeration that tells QEMU what operation to take when consuming a 
> > bitmap
> > +# a new image file.
> > +#
> > +# @consume: QEMU should just consume the bitmap and release it after using
> > +#
> > +# @reset: QEMU should keep an internal copy of the bitmap and reset the
> > +#         dirty record from original one
> > +#
> > +# Since: 2.1
> > +##
> > +{ 'enum': 'BitmapUseMode',
> > +  'data': [ 'consume', 'reset' ] }
> > +
> > +##
> >  # @DriveBackup
> >  #
> >  # @device: the name of the device which should be copied.
> > @@ -1891,14 +1907,19 @@
> >  #          probe if @mode is 'existing', else the format of the source
> >  #
> >  # @sync: what parts of the disk image should be copied to the destination
> > -#        (all the disk, only the sectors allocated in the topmost image, or
> > -#        only new I/O).
> > +#        (all the disk, only the sectors allocated in the topmost image, 
> > from a
> > +#        dirty bitmap, or only new I/O).
> >  #
> >  # @mode: #optional whether and how QEMU should create a new image, default 
> > is
> >  #        'absolute-paths'.
> >  #
> >  # @speed: #optional the maximum speed, in bytes per second
> >  #
> > +# @bitmap: #optional the name of dirty bitmap if sync is "dirty-bitmap"
> (Since 2.1)

OK.

Thanks,
Fam