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

[Kevin Wolf]

Am 16.06.2014 um 12:00 hat Benoît Canet geschrieben:
> 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 <address@hidden>
> Reviewed-by: Max Reitz <address@hidden>
> ---
>  block.c                   | 17 ++++++++++++++
>  block/mirror.c            | 60 
> +++++++++++++++++++++++++++++++++++++----------
>  blockdev.c                | 30 +++++++++++++++++++++++-
>  hmp.c                     |  2 +-
>  include/block/block.h     |  4 ++++
>  include/block/block_int.h |  3 +++
>  qapi/block-core.json      |  6 ++++-
>  qmp-commands.hx           |  4 +++-
>  8 files changed, 109 insertions(+), 17 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);

Unnecessary line break.

> +        return NULL;
> +    }
> +
> +    if (bdrv_op_is_blocked(to_replace_bs, BLOCK_OP_TYPE_REPLACE, errp)) {
> +        return NULL;
> +    }
> +
> +    return to_replace_bs;
> +}
> +

Empty line before EOF.

> diff --git a/block/mirror.c b/block/mirror.c
> index 94c8661..151167e 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 BDS to replace */
> +    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);
>      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 blocked and block all operations on it */
> +    if (s->replaces) {
> +        s->to_replace = check_to_replace_node(s->replaces, errp);
> +

This empty line looks unusual.

> +        if (!s->to_replace) {
> +            return;
> +        }

So here is the thing that I really wanted to comment on. In the case of
a REPLACE blocker being set, this is a silent failure. The completion
command will return success, but s->should_complete won't actually be
set, so the completion doesn't happen. The only thing that actually
happens is the bdrv_open_backing_file(s->target) (which looks somewhat
questionable, too...)

Now I would expect that the REPLACE blocker is actually set for any
backing file, because that is what bdrv_set_backing_hd() does. For
quorum it does work as expected because quorum children don't get any
backing_blocker (we need to check whether they should get something
similar from the quorum BDS), so this is probably why it escaped your
testing. We'll need a test case that tries replacing some ordinary
backing file.

Now I think the (accidental?) restriction to only replacing quorum nodes
actually makes this patch pretty safe, so maybe it would be nice to keep
this behaviour; but we need to fix it to not fail silently but return an
explicit error.

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

One design question that isn't quite clear to me yet is why you resolve
the device name only in mirror_complete() and not here. This means that
the drive-mirror QMP command can refer to one BDS with node-name foo,
which then gets removed and another BDS with node-name foo is added, and
then it would refer to the new BDS on completion time.

I would find it less surprising if we took a reference to the old BDS
here so that you can't remove it. Perhaps setting the replace_blocker
here already would be safer, too.

> @@ -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;
> +        }

We may want to loosen some of these restrictions later, but it's good to
start with more restrictions if in doubt.

> +    }
> +
>      if ((sync == MIRROR_SYNC_MODE_FULL || !source)
>          && mode != NEW_IMAGE_MODE_EXISTING)
>      {

Kevin