Re: [Qemu-devel] [PATCH] mirror: add sync mode incremental to drive-mirror and blockdev-mirror

[Stefan Hajnoczi <stefanha@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3162 bytes --]

On Thu, May 04, 2017 at 12:54:40PM +0200, Daniel Kucera wrote:

Seems like a logical extension along the same lines as the backup block
job's dirty bitmap sync mode.

> parameter bitmap chooses existing dirtymap instead of newly created
> in mirror_start_job
> 
> Signed-off-by: Daniel Kucera <daniel.kucera@gmail.com>
> ---
>  block/mirror.c            | 47 ++++++++++++++++++++++++++++++-----------------
>  blockdev.c                | 16 +++++++++++++++-
>  include/block/block_int.h |  4 +++-
>  qapi/block-core.json      | 12 ++++++++++--
>  4 files changed, 58 insertions(+), 21 deletions(-)

This patch modifies the user's bitmap in-place and does not
differentiate successful completion from failure.  I suggest following
the same bitmap lifecycle as the backup block job so that
cancellation/failure offers useful behavior.  (If the job fails due to
intermittent errors on the target it should be possible to start the job
again - the bitmap must not be lost!).

> diff --git a/block/mirror.c b/block/mirror.c
> index 9f5eb69..1bfbe2e 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -49,7 +49,7 @@ typedef struct MirrorBlockJob {
>      BlockDriverState *to_replace;
>      /* Used to block operations on the drive-mirror-replace target */
>      Error *replace_blocker;
> -    bool is_none_mode;
> +    MirrorSyncMode sync_mode;
>      BlockMirrorBackingMode backing_mode;
>      BlockdevOnError on_source_error, on_target_error;
>      bool synced;
> @@ -523,7 +523,9 @@ static void mirror_exit(BlockJob *job, void *opaque)
>      bdrv_child_try_set_perm(mirror_top_bs->backing, 0, BLK_PERM_ALL,
>                              &error_abort);
>      if (s->backing_mode == MIRROR_SOURCE_BACKING_CHAIN) {
> -        BlockDriverState *backing = s->is_none_mode ? src : s->base;
> +        BlockDriverState *backing =
> +        (s->sync_mode == MIRROR_SYNC_MODE_INCREMENTAL) ||
> +        (s->sync_mode == MIRROR_SYNC_MODE_NONE) ? src : s->base;

Please update the comment in include/block/block_int.h:typedef enum
BlockMirrorBackingMode.

> @@ -1213,9 +1217,21 @@ static void mirror_start_job(const char *job_id, BlockDriverState *bs,
>          s->should_complete = true;
>      }
>  
> -    s->dirty_bitmap = bdrv_create_dirty_bitmap(bs, granularity, NULL, errp);
> -    if (!s->dirty_bitmap) {
> -        goto fail;
> +    if (s->sync_mode == MIRROR_SYNC_MODE_INCREMENTAL) {
> +        if (bitmap == NULL) {
> +            error_setg(errp, "Mode incremental requires parameter 'bitmap'");
> +            goto fail;
> +        }
> +        s->dirty_bitmap = bdrv_find_dirty_bitmap(bs, bitmap);
> +        if (!s->dirty_bitmap) {
> +            error_setg(errp, "Bitmap '%s' not found", bitmap);
> +            goto fail;
> +        }
> +    } else {
> +        s->dirty_bitmap = bdrv_create_dirty_bitmap(bs, granularity, NULL, errp);
> +        if (!s->dirty_bitmap) {
> +            goto fail;
> +        }
>      }

Do we need to check that granularity is compatible with the pre-existing
dirty bitmap's granularity when sync_mode is
MIRROR_SYNC_MODE_INCREMENTAL?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]