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

[Max Reitz]

On 2014-10-30 at 04:22, 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: backup job makes the original anonymous (invisible to user)
>     and releases it after use.
>
> Signed-off-by: Fam Zheng <address@hidden>
> ---
>   block/backup.c            | 54 ++++++++++++++++++++++++++++++++++++++++++++++-
>   block/mirror.c            |  4 ++++
>   blockdev.c                |  9 +++++++-
>   hmp.c                     |  4 +++-
>   include/block/block_int.h |  4 ++++
>   qapi/block-core.json      | 30 ++++++++++++++++++++++----
>   qmp-commands.hx           |  7 +++---
>   7 files changed, 102 insertions(+), 10 deletions(-)
>
> diff --git a/block/backup.c b/block/backup.c
> index e334740..4870694 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 */
> +    int sync_bitmap_gran;
>       MirrorSyncMode sync_mode;
>       RateLimit limit;
>       BlockdevOnError on_source_error;
> @@ -263,7 +267,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 modes require copying.. */
>           for (; start < end; start++) {
>               bool error_is_read;
>   
> @@ -317,7 +321,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) {

->sync_bitmap_gran should be a sector count here... [1]

> +                    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 +359,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 +372,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,
>                     BlockCompletionFunc *cb, void *opaque,
>                     Error **errp)
>   {
>       int64_t len;
> +    BdrvDirtyBitmap *original;
>   
>       assert(bs);
>       assert(target);
> @@ -369,6 +393,28 @@ void backup_start(BlockDriverState *bs, BlockDriverState 
> *target,
>           return;
>       }
>   
> +    if (sync_mode == MIRROR_SYNC_MODE_DIRTY_BITMAP) {
> +        if (!sync_bitmap) {
> +            error_setg(errp, "must provide a valid bitmap name for "
> +                             "\"dirty-bitmap\" sync mode");
> +            return;
> +        }

How about an error when sync_mode != MIRROR_SYNC_MODE_DIRTY_BITMAP and 
sync_bitmap != NULL? It's not really fatal, but I wouldn't call it a 
valid combination either.

> +
> +        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);

Better use abort() directly (or g_assert_not_reached(), which however 
suffers from the same problem as assert(0) which is NDEBUG or the 
equivalent for glib).

> +        }
> +        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 +432,12 @@ 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_mode == MIRROR_SYNC_MODE_DIRTY_BITMAP ?
> +                       sync_bitmap : NULL;
> +    if (sync_bitmap) {
> +        job->sync_bitmap_gran =
> +            bdrv_dirty_bitmap_granularity(bs, job->sync_bitmap);

[1] so this should return a sector count (relevant for patch 3); 
although I'd still personally prefer it to return a byte count which is 
converted to sectors here (but it doesn't matter from a technical 
perspective).

> +    }
>       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 cecd448..8dfd908 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -679,6 +679,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, replaces,
> diff --git a/blockdev.c b/blockdev.c
> index c7e98ad..fca909b 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1463,6 +1463,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);
> @@ -2186,6 +2188,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)
> @@ -2282,7 +2286,10 @@ void qmp_drive_backup(const char *device, const char 
> *target,
>           return;
>       }
>   

This conflicts with Stefan's blockjob/dataplane series (can be easily 
fixed, though).

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

Shouldn't we be checking the return value of bdrv_find_dirty_bitmap() here?

I know backup_start() does a check itself, but I'd prefer a check here, 
too, nonetheless.

> +                 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 63d7686..bc5a2d2 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -966,7 +966,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, &err);
> +                     true, mode, false, 0, false, NULL,
> +                     false, 0,
> +                     false, 0, false, 0, &err);
>       hmp_handle_error(mon, &err);
>   }
>   
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 8898c6c..0671c89 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -567,6 +567,8 @@ 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 name of dirty bitmap if sync_mode is
> + *               MIRROR_SYNC_MODE_DIRTY_BITMAP.

It's not the name, it's the bitmap itself. Also, documentation for 
bitmap_mode is missing.

>    * @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.
> @@ -577,6 +579,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,
>                     BlockCompletionFunc *cb, void *opaque,
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 97c31bf..e225220 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -473,7 +473,7 @@
>   # Since: 1.3
>   ##
>   { 'enum': 'MirrorSyncMode',
> -  'data': ['top', 'full', 'none'] }
> +  'data': ['top', 'full', 'none', 'dirty-bitmap'] }
>   
>   ##
>   # @BlockJobType:
> @@ -634,6 +634,21 @@
>               '*format': 'str', '*mode': 'NewImageMode' } }
>   
>   ##
> +# @BitmapUseMode
> +#
> +# An enumeration that tells QEMU what operation to take when using a bitmap
> +# in drive backup sync mode.

Did you mean "when using a bitmap in drive backup sync mode 
dirty-bitmap"? (or just "when using a bitmap for drive-backup")

> +#
> +# @consume: QEMU should just consume the bitmap and release it after using
> +#
> +# @reset: QEMU should reset the dirty bitmap
> +#
> +# Since: 2.3
> +##
> +{ 'enum': 'BitmapUseMode',
> +'data': [ 'consume', 'reset' ] }
> +
> +##
>   # @DriveBackup
>   #
>   # @device: the name of the device which should be copied.
> @@ -646,14 +661,20 @@
>   #          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.3)
> +#
> +# @bitmap-use-mode: #optional which operation to take when consuming @bitmap,
> +#                   default is reset. (Since 2.1)

*2.3

Max

> +#
>   # @on-source-error: #optional the action to take on an error on the source,
>   #                   default 'report'.  'stop' and 'enospc' can only be used
>   #                   if the block device supports io-status (see BlockInfo).
> @@ -671,7 +692,8 @@
>   { 'type': 'DriveBackup',
>     'data': { 'device': 'str', 'target': 'str', '*format': 'str',
>               'sync': 'MirrorSyncMode', '*mode': 'NewImageMode',
> -            '*speed': 'int',
> +            '*speed': 'int', '*bitmap': 'str',
> +            '*bitmap-use-mode': 'BitmapUseMode',
>               '*on-source-error': 'BlockdevOnError',
>               '*on-target-error': 'BlockdevOnError' } }
>   
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 6d0c944..75035e1 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -1048,7 +1048,7 @@ EQMP
>       {
>           .name       = "drive-backup",
>           .args_type  = "sync:s,device:B,target:s,speed:i?,mode:s?,format:s?,"
> -                      "on-source-error:s?,on-target-error:s?",
> +                      "bitmap:s?,on-source-error:s?,on-target-error:s?",
>           .mhandler.cmd_new = qmp_marshal_input_drive_backup,
>       },
>   
> @@ -1075,8 +1075,9 @@ Arguments:
>               (json-string, optional)
>   - "sync": what parts of the disk image should be copied to the destination;
>     possibilities include "full" for all the disk, "top" for only the sectors
> -  allocated in the topmost image, or "none" to only replicate new I/O
> -  (MirrorSyncMode).
> +  allocated in the topmost image, "dirty-bitmap" for only the dirty sectors in
> +  the bitmap, or "none" to only replicate new I/O (MirrorSyncMode).
> +- "bitmap": dirty bitmap name for sync==dirty-bitmap
>   - "mode": whether and how QEMU should create a new image
>             (NewImageMode, optional, default 'absolute-paths')
>   - "speed": the maximum speed, in bytes per second (json-int, optional)