Re: [Qemu-devel] [PATCH v9 3/4] block: Add replaces argument to drive-mirror

[Max Reitz <mreitz@redhat.com>]

On 11.06.2014 15:24, Benoît Canet wrote:
> drive-mirror will bdrv_swap the new BDS named node-name with the one
> pointed by replaces when the mirroring is finished.
>
> Signed-off-by: Benoit Canet <benoit@irqsave.net>
> ---
>   block.c                   | 17 ++++++++++++++
>   block/mirror.c            | 60 +++++++++++++++++++++++++++++++++++++----------
>   blockdev.c                | 30 +++++++++++++++++++++++-
>   hmp.c                     |  2 +-
>   include/block/block.h     |  4 ++++
>   include/block/block_int.h |  4 +++-
>   qapi/block-core.json      |  6 ++++-
>   qmp-commands.hx           |  4 +++-
>   8 files changed, 109 insertions(+), 18 deletions(-)
>
> diff --git a/block.c b/block.c
> index 17f763d..318f1e6 100644
> --- a/block.c
> +++ b/block.c
> @@ -5795,3 +5795,20 @@ bool bdrv_is_first_non_filter(BlockDriverState *candidate)
>   
>       return false;
>   }
> +
> +BlockDriverState *check_to_replace_node(const char *node_name, Error **errp)
> +{
> +    BlockDriverState *to_replace_bs = bdrv_find_node(node_name);
> +    if (!to_replace_bs) {
> +        error_setg(errp, "Node name '%s' not found",
> +                   node_name);
> +        return NULL;
> +    }
> +
> +    if (bdrv_op_is_blocked(to_replace_bs, BLOCK_OP_TYPE_REPLACE, errp)) {
> +        return NULL;
> +    }
> +
> +    return to_replace_bs;
> +}
> +
> diff --git a/block/mirror.c b/block/mirror.c
> index 94c8661..b00365f 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -32,6 +32,12 @@ typedef struct MirrorBlockJob {
>       RateLimit limit;
>       BlockDriverState *target;
>       BlockDriverState *base;
> +    /* The name of the graph node to replace */
> +    char *replaces;
> +    /* The block BDS to replace */

Maybe s/block //

> +    BlockDriverState *to_replace;
> +    /* Used to block operations on the drive-mirror-replace target */
> +    Error *replace_blocker;
>       bool is_none_mode;
>       BlockdevOnError on_source_error, on_target_error;
>       bool synced;
> @@ -490,10 +496,14 @@ immediate_exit:
>       bdrv_release_dirty_bitmap(bs, s->dirty_bitmap);
>       bdrv_iostatus_disable(s->target);
>       if (s->should_complete && ret == 0) {
> -        if (bdrv_get_flags(s->target) != bdrv_get_flags(s->common.bs)) {
> -            bdrv_reopen(s->target, bdrv_get_flags(s->common.bs), NULL);
> +        BlockDriverState *to_replace = s->common.bs;
> +        if (s->to_replace) {
> +            to_replace = s->to_replace;
>           }
> -        bdrv_swap(s->target, s->common.bs);
> +        if (bdrv_get_flags(s->target) != bdrv_get_flags(to_replace)) {
> +            bdrv_reopen(s->target, bdrv_get_flags(to_replace), NULL);
> +        }
> +        bdrv_swap(s->target, to_replace);
>           if (s->common.driver->job_type == BLOCK_JOB_TYPE_COMMIT) {
>               /* drop the bs loop chain formed by the swap: break the loop then
>                * trigger the unref from the top one */
> @@ -502,6 +512,12 @@ immediate_exit:
>               bdrv_unref(p);
>           }
>       }
> +    if (s->to_replace) {
> +        bdrv_op_unblock_all(s->to_replace, s->replace_blocker);
> +        error_free(s->replace_blocker);
> +        bdrv_unref(s->to_replace);
> +        g_free(s->replaces);

If we get here without mirror_complete() having been invoked (which is 
possible, as far as I know), s->to_replace will still be NULL while 
s->replaces may be not. In that case, s->replaces will be leaked.

> +    }
>       bdrv_unref(s->target);
>       block_job_completed(&s->common, ret);
>   }
> @@ -540,6 +556,20 @@ static void mirror_complete(BlockJob *job, Error **errp)
>           return;
>       }
>   
> +    /* check the target bs is not block and block all operations on it */

s/block/blocked/

> +    if (s->replaces) {
> +        s->to_replace = check_to_replace_node(s->replaces, errp);
> +
> +        if (!s->to_replace) {
> +            return;
> +        }
> +
> +        error_setg(&s->replace_blocker,
> +                   "block device is in use by block-job-complete");
> +        bdrv_op_block_all(s->to_replace, s->replace_blocker);
> +        bdrv_ref(s->to_replace);
> +    }
> +
>       s->should_complete = true;
>       block_job_resume(job);
>   }
> @@ -562,14 +592,15 @@ static const BlockJobDriver commit_active_job_driver = {
>   };
>   
>   static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target,
> -                            int64_t speed, int64_t granularity,
> -                            int64_t buf_size,
> -                            BlockdevOnError on_source_error,
> -                            BlockdevOnError on_target_error,
> -                            BlockDriverCompletionFunc *cb,
> -                            void *opaque, Error **errp,
> -                            const BlockJobDriver *driver,
> -                            bool is_none_mode, BlockDriverState *base)
> +                             const char *replaces,
> +                             int64_t speed, int64_t granularity,
> +                             int64_t buf_size,
> +                             BlockdevOnError on_source_error,
> +                             BlockdevOnError on_target_error,
> +                             BlockDriverCompletionFunc *cb,
> +                             void *opaque, Error **errp,
> +                             const BlockJobDriver *driver,
> +                             bool is_none_mode, BlockDriverState *base)
>   {
>       MirrorBlockJob *s;
>   
> @@ -600,6 +631,7 @@ static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target,
>           return;
>       }
>   
> +    s->replaces = g_strdup(replaces);
>       s->on_source_error = on_source_error;
>       s->on_target_error = on_target_error;
>       s->target = target;
> @@ -621,6 +653,7 @@ static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target,
>   }
>   
>   void mirror_start(BlockDriverState *bs, BlockDriverState *target,
> +                  const char *replaces,
>                     int64_t speed, int64_t granularity, int64_t buf_size,
>                     MirrorSyncMode mode, BlockdevOnError on_source_error,
>                     BlockdevOnError on_target_error,
> @@ -632,7 +665,8 @@ void mirror_start(BlockDriverState *bs, BlockDriverState *target,
>   
>       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,
> +    mirror_start_job(bs, target, replaces,
> +                     speed, granularity, buf_size,
>                        on_source_error, on_target_error, cb, opaque, errp,
>                        &mirror_job_driver, is_none_mode, base);
>   }
> @@ -680,7 +714,7 @@ void commit_active_start(BlockDriverState *bs, BlockDriverState *base,
>       }
>   
>       bdrv_ref(base);
> -    mirror_start_job(bs, base, speed, 0, 0,
> +    mirror_start_job(bs, base, NULL, speed, 0, 0,
>                        on_error, on_error, cb, opaque, &local_err,
>                        &commit_active_job_driver, false, base);
>       if (local_err) {
> diff --git a/blockdev.c b/blockdev.c
> index 06b14f2..237a548 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2107,6 +2107,7 @@ BlockDeviceInfoList *qmp_query_named_block_nodes(Error **errp)
>   void qmp_drive_mirror(const char *device, const char *target,
>                         bool has_format, const char *format,
>                         bool has_node_name, const char *node_name,
> +                      bool has_replaces, const char *replaces,
>                         enum MirrorSyncMode sync,
>                         bool has_mode, enum NewImageMode mode,
>                         bool has_speed, int64_t speed,
> @@ -2194,6 +2195,28 @@ void qmp_drive_mirror(const char *device, const char *target,
>           return;
>       }
>   
> +    if (has_replaces) {
> +        BlockDriverState *to_replace_bs;
> +
> +        if (!has_node_name) {
> +            error_setg(errp, "a node-name must be provided when replacing a"
> +                             " named node of the graph");
> +            return;
> +        }
> +
> +        to_replace_bs = check_to_replace_node(replaces, errp);
> +
> +        if (!to_replace_bs) {
> +            return;
> +        }
> +
> +        if (size != bdrv_getlength(to_replace_bs)) {
> +            error_setg(errp, "cannot replace image with a mirror image of "
> +                             "different size");
> +            return;
> +        }
> +    }
> +
>       if ((sync == MIRROR_SYNC_MODE_FULL || !source)
>           && mode != NEW_IMAGE_MODE_EXISTING)
>       {
> @@ -2238,7 +2261,12 @@ void qmp_drive_mirror(const char *device, const char *target,
>           return;
>       }
>   
> -    mirror_start(bs, target_bs, speed, granularity, buf_size, sync,
> +    /* pass the node name to replace to mirror start since it's loose coupling
> +     * and will allow to check whether the node still exist at mirror completion
> +     */
> +    mirror_start(bs, target_bs,
> +                 has_replaces ? replaces : NULL,
> +                 speed, granularity, buf_size, sync,
>                    on_source_error, on_target_error,
>                    block_job_cb, bs, &local_err);
>       if (local_err != NULL) {
> diff --git a/hmp.c b/hmp.c
> index ef0d583..9a4b8da 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -930,7 +930,7 @@ void hmp_drive_mirror(Monitor *mon, const QDict *qdict)
>       }
>   
>       qmp_drive_mirror(device, filename, !!format, format,
> -                     false, NULL,
> +                     false, NULL, false, NULL,
>                        full ? MIRROR_SYNC_MODE_FULL : MIRROR_SYNC_MODE_TOP,
>                        true, mode, false, 0, false, 0, false, 0,
>                        false, 0, false, 0, &err);
> diff --git a/include/block/block.h b/include/block/block.h
> index 7d86e29..b0ed701 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -179,6 +179,7 @@ typedef enum BlockOpType {
>       BLOCK_OP_TYPE_MIRROR,
>       BLOCK_OP_TYPE_RESIZE,
>       BLOCK_OP_TYPE_STREAM,
> +    BLOCK_OP_TYPE_REPLACE,
>       BLOCK_OP_TYPE_MAX,
>   } BlockOpType;
>   
> @@ -319,6 +320,9 @@ bool bdrv_recurse_is_first_non_filter(BlockDriverState *bs,
>                                         BlockDriverState *candidate);
>   bool bdrv_is_first_non_filter(BlockDriverState *candidate);
>   
> +/* check if a named node can be replaced when doing drive-mirror */
> +BlockDriverState *check_to_replace_node(const char *node_name, Error **errp);
> +
>   /* async block I/O */
>   typedef void BlockDriverDirtyHandler(BlockDriverState *bs, int64_t sector,
>                                        int sector_num);
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 8d58334..fdf3ed2 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -86,7 +86,6 @@ struct BlockDriver {
>        */
>       bool (*bdrv_recurse_is_first_non_filter)(BlockDriverState *bs,
>                                                BlockDriverState *candidate);
> -
>       int (*bdrv_probe)(const uint8_t *buf, int buf_size, const char *filename);
>       int (*bdrv_probe_device)(const char *filename);
>   

This hunk doesn't seem intended...?


Other than these, the patch looks fine to me. If you fix the leakage of 
s->replaces (or explain why mirror_complete() is always invoked before 
immediate_exit):

Reviewed-by: Max Reitz <mreitz@redhat.com>