[v5,2/3] block: Add drive-mirror-replace command

When a quorum file is totally destroyed (broken NAS or SAN) the user can start a
drive-mirror job on the quorum block backend and then replace the broken
quorum file with drive-mirror-replace given it has a node-name.

Signed-off-by: Benoit Canet <benoit@irqsave.net>
---
 block.c                   |   8 ++--
 block/mirror.c            | 118 ++++++++++++++++++++++++++++++++++++++++++++--
 blockdev.c                |  27 +++++++++++
 include/block/block.h     |   3 ++
 include/block/block_int.h |  15 ++++++
 qapi-schema.json          |  33 +++++++++++++
 qmp-commands.hx           |   5 ++
 trace-events              |   1 +
 8 files changed, 204 insertions(+), 6 deletions(-)

On 30.05.2014 13:18, Benoît Canet wrote:
> When a quorum file is totally destroyed (broken NAS or SAN) the user can start a
> drive-mirror job on the quorum block backend and then replace the broken
> quorum file with drive-mirror-replace given it has a node-name.
>
> Signed-off-by: Benoit Canet <benoit@irqsave.net>
> ---
>   block.c                   |   8 ++--
>   block/mirror.c            | 118 ++++++++++++++++++++++++++++++++++++++++++++--
>   blockdev.c                |  27 +++++++++++
>   include/block/block.h     |   3 ++
>   include/block/block_int.h |  15 ++++++
>   qapi-schema.json          |  33 +++++++++++++
>   qmp-commands.hx           |   5 ++
>   trace-events              |   1 +
>   8 files changed, 204 insertions(+), 6 deletions(-)

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

The Friday 30 May 2014 à 13:18:43 (+0200), Benoît Canet wrote :
> When a quorum file is totally destroyed (broken NAS or SAN) the user can start a
> drive-mirror job on the quorum block backend and then replace the broken
> quorum file with drive-mirror-replace given it has a node-name.
> 
> Signed-off-by: Benoit Canet <benoit@irqsave.net>
> ---
>  block.c                   |   8 ++--
>  block/mirror.c            | 118 ++++++++++++++++++++++++++++++++++++++++++++--
>  blockdev.c                |  27 +++++++++++
>  include/block/block.h     |   3 ++
>  include/block/block_int.h |  15 ++++++
>  qapi-schema.json          |  33 +++++++++++++
>  qmp-commands.hx           |   5 ++
>  trace-events              |   1 +
>  8 files changed, 204 insertions(+), 6 deletions(-)
> 
> diff --git a/block.c b/block.c
> index a517d72..f8be55f 100644
> --- a/block.c
> +++ b/block.c
> @@ -842,10 +842,12 @@ static int bdrv_open_flags(BlockDriverState *bs, int flags)
>      return open_flags;
>  }
>  
> -static void bdrv_assign_node_name(BlockDriverState *bs,
> -                                  const char *node_name,
> -                                  Error **errp)
> +void bdrv_assign_node_name(BlockDriverState *bs,
> +                           const char *node_name,
> +                           Error **errp)
>  {
> +    assert(bs->node_name[0] == '\0');
> +
>      if (!node_name) {
>          return;
>      }
> diff --git a/block/mirror.c b/block/mirror.c
> index 94c8661..2d4c0ed 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -49,6 +49,15 @@ typedef struct MirrorBlockJob {
>      unsigned long *in_flight_bitmap;
>      int in_flight;
>      int ret;
> +    /* these four fields are used by drive-mirror-replace */
> +    /* The code must replace a target with the new mirror */
> +    bool must_replace;
> +    /* The block BDS to replace */
> +    BlockDriverState *to_replace;
> +    /* the node-name of the new mirror BDS */
> +    char *new_node_name;
> +    /* Used to block operations on the drive-mirror-replace target. */
> +    Error *replace_blocker;
>  } MirrorBlockJob;
>  
>  typedef struct MirrorOp {
> @@ -313,6 +322,7 @@ static void coroutine_fn mirror_run(void *opaque)
>      MirrorBlockJob *s = opaque;
>      BlockDriverState *bs = s->common.bs;
>      int64_t sector_num, end, sectors_per_chunk, length;
> +    BlockDriverState *to_replace;
>      uint64_t last_pause_ns;
>      BlockDriverInfo bdi;
>      char backing_filename[1024];
> @@ -489,11 +499,17 @@ immediate_exit:
>      g_free(s->in_flight_bitmap);
>      bdrv_release_dirty_bitmap(bs, s->dirty_bitmap);
>      bdrv_iostatus_disable(s->target);
> +    /* Here we handle the drive-mirror-replace QMP command */
> +    if (s->must_replace) {
> +        to_replace = s->to_replace;
> +    } else {
> +        to_replace = s->common.bs;
> +    }
>      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);
> +        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, s->common.bs);
> +        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 +518,11 @@ immediate_exit:
>              bdrv_unref(p);
>          }
>      }
> +    if (s->must_replace) {
> +        bdrv_op_unblock_all(s->to_replace, s->replace_blocker);
> +        error_free(s->replace_blocker);
> +        bdrv_unref(s->to_replace);
> +    }
>      bdrv_unref(s->target);
>      block_job_completed(&s->common, ret);
>  }
> @@ -524,6 +545,90 @@ static void mirror_iostatus_reset(BlockJob *job)
>      bdrv_iostatus_reset(s->target);
>  }
>  
> +bool mirror_set_replace_target(BlockJob *job, const char *reference,
> +                               bool has_new_node_name,
> +                               const char *new_node_name, Error **errp)
> +{
> +    MirrorBlockJob *s = container_of(job, MirrorBlockJob, common);
> +    BlockDriverState *to_replace;
> +
> +    /* We don't want to give too much power to the user as this could result in
> +     * BlockDriverState loops if used with something other than sync=full.
> +     */
> +    if (s->is_none_mode || s->base) {
> +        error_setg(errp, "Can only be used on a mirror done with sync=full");
> +        return false;
> +    }
> +
> +    /* check that the target reference is not an empty string */
> +    if (!reference[0]) {
> +        error_setg(errp, "target-reference is an empty string");
> +        return false;
> +    }
> +
> +    /* Get the block driver state to be replaced */
> +    to_replace = bdrv_lookup_bs(reference, reference, errp);
> +    if (!to_replace) {
> +        return false;
> +    }
> +
> +    /* If the BDS to be replaced is a regular node we need a new node name */
> +    if (to_replace->node_name[0] && !has_new_node_name) {
> +        error_setg(errp, "A new-node-name must be provided");
> +        return false;
> +    }
> +
> +    /* Can only replace something else than the source of the mirror */
> +    if (to_replace == job->bs) {
> +        error_setg(errp, "Cannot replace the mirror source");
> +        return false;
> +    }
> +
> +    /* is this bs replace operation blocked */
> +    if (bdrv_op_is_blocked(to_replace, BLOCK_OP_TYPE_REPLACE, errp)) {
> +        return false;
> +    }
> +
> +    /* save useful infos for later */
> +    s->to_replace = to_replace;
> +    /* don't make any assumption on new_node_name state if !has_new_node_name */
> +    s->new_node_name = has_new_node_name ? g_strdup(new_node_name) : NULL;
> +
> +    return true;
> +}
> +
> +static void mirror_activate_replace_target(BlockJob *job, Error **errp)
> +{
> +    MirrorBlockJob *s = container_of(job, MirrorBlockJob, common);
> +    Error *local_err = NULL;
> +
> +    /* Set the new node name if the BDS to replace is a regular node
> +     * of the graph.
> +     */
> +    if (s->to_replace->node_name[0]) {
> +        assert(s->new_node_name);
> +        bdrv_assign_node_name(s->target, s->new_node_name, &local_err);
> +    }
> +
> +    g_free(s->new_node_name);
> +
> +    if (local_err) {
> +        s->to_replace = NULL;
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +
> +    /* block all operations on the target bs */
> +    error_setg(&s->replace_blocker,
> +               "block device is in use by drive-mirror-replace");
> +    bdrv_op_block_all(s->to_replace, s->replace_blocker);
> +
> +    bdrv_ref(s->to_replace);
> +
> +    /* activate the replacement operation */
> +    s->must_replace = true;
> +}
> +
>  static void mirror_complete(BlockJob *job, Error **errp)
>  {
>      MirrorBlockJob *s = container_of(job, MirrorBlockJob, common);
> @@ -540,6 +645,13 @@ static void mirror_complete(BlockJob *job, Error **errp)
>          return;
>      }
>  
> +    /* drive-mirror-replace is being called on this job so activate the
> +     * replacement target
> +     */
> +    if (s->to_replace) {
> +        mirror_activate_replace_target(job, errp);
> +    }
> +
>      s->should_complete = true;
>      block_job_resume(job);
>  }
> diff --git a/blockdev.c b/blockdev.c
> index 8cc42fb..74e9174 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2317,6 +2317,33 @@ void qmp_block_job_complete(const char *device, Error **errp)
>      block_job_complete(job, errp);
>  }
>  
> +void qmp_drive_mirror_replace(const char *device, const char *target_reference,
> +                              bool has_new_node_name,
> +                              const char *new_node_name,
> +                              Error **errp)
> +{
> +    BlockJob *job = find_block_job(device);
> +
> +    if (!job) {
> +        error_set(errp, QERR_BLOCK_JOB_NOT_ACTIVE, device);
> +        return;
> +    }
> +
> +    if (!job->driver || job->driver->job_type != BLOCK_JOB_TYPE_MIRROR) {
> +        error_setg(errp, "Can only be used on a drive-mirror block job");
> +        return;
> +    }
> +
> +    if (!mirror_set_replace_target(job, target_reference, has_new_node_name,
> +                                   new_node_name, errp)) {
> +        return;
> +    }
> +
> +    trace_qmp_drive_mirror_replace(job, target_reference,
> +                                   has_new_node_name ? new_node_name : "");
> +    block_job_complete(job, errp);
> +}
> +
>  void qmp_blockdev_add(BlockdevOptions *options, Error **errp)
>  {
>      QmpOutputVisitor *ov = qmp_output_visitor_new();
> diff --git a/include/block/block.h b/include/block/block.h
> index faee3aa..2765bc9 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;
>  
> @@ -217,6 +218,8 @@ int bdrv_open_image(BlockDriverState **pbs, const char *filename,
>                      QDict *options, const char *bdref_key, int flags,
>                      bool allow_none, Error **errp);
>  void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd);
> +void bdrv_assign_node_name(BlockDriverState *bs, const char *node_name,
> +                           Error **errp);
>  int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp);
>  void bdrv_append_temp_snapshot(BlockDriverState *bs, int flags, Error **errp);
>  int bdrv_open(BlockDriverState **pbs, const char *filename,
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index f2e753f..27f0443 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -486,6 +486,21 @@ void mirror_start(BlockDriverState *bs, BlockDriverState *target,
>                    void *opaque, Error **errp);
>  
>  /*
> + * mirror_set_replace_target:
> + * @job: An active mirroring block job started with sync=full.
> + * @reference: id or node-name of the BDS to replace when the mirror is done.
> + * @has_new_node_name: Set to true if new_node_name if provided
> + * @new_node_name: The optional new node name of the new mirror.
> + * @errp: Error object.
> + *
> + * Prepare a mirroring operation to replace a BDS pointed to by reference with
> + * the new mirror.
> + */
> +bool mirror_set_replace_target(BlockJob *job, const char *reference,
> +                               bool has_new_node_name,
> +                               const char *new_node_name, Error **errp);
> +
> +/*
>   * backup_start:
>   * @bs: Block device to operate on.
>   * @target: Block device to write to.
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 7bb1d47..0fd10b6 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -2703,6 +2703,39 @@
>  { 'command': 'block-job-complete', 'data': { 'device': 'str' } }
>  
>  ##
> +# @drive-mirror-replace:
> +#
> +# Manually trigger completion of an active background drive-mirror operation
> +# and replace the target reference with the new mirror.
> +# This switches the device to write to the target path only.
> +# The ability to complete is signaled with a BLOCK_JOB_READY event.
> +#
> +# This command completes an active drive-mirror background operation
> +# synchronously and replaces the target reference with the mirror.
> +# The ordering of this command's return with the BLOCK_JOB_COMPLETED event
> +# is not defined.  Note that if an I/O error occurs during the processing of
> +# this command: 1) the command itself will fail; 2) the error will be processed
> +# according to the rerror/werror arguments that were specified when starting
> +# the operation.
> +#
> +# A cancelled or paused drive-mirror job cannot be completed.
> +#
> +# @device:           the device name
> +# @target-reference: the id or node name of the block driver state to replace
> +# @new-node-name:    #optional set the node-name of the new block driver state
> +#                    needed if the target reference points to a regular node of
> +#                    the graph
> +#
> +# Returns: Nothing on success
> +#          If no background operation is active on this device, DeviceNotActive
> +#
> +# Since: 2.1
> +##
> +{ 'command': 'drive-mirror-replace',
> +  'data': { 'device': 'str', 'target-reference': 'str',
> +            '*new-node-name': 'str' } }
> +
> +##
>  # @ObjectTypeInfo:
>  #
>  # This structure describes a search result from @qom-list-types
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index d8aa4ed..3421d90 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -1106,6 +1106,11 @@ EQMP
>          .mhandler.cmd_new = qmp_marshal_input_block_job_complete,
>      },
>      {
> +        .name       = "drive-mirror-replace",
> +        .args_type  = "device:B,target-reference:s,new-node-name:s?",
> +        .mhandler.cmd_new = qmp_marshal_input_drive_mirror_replace,
> +    },
> +    {
>          .name       = "transaction",
>          .args_type  = "actions:q",
>          .mhandler.cmd_new = qmp_marshal_input_transaction,
> diff --git a/trace-events b/trace-events
> index ea56d52..54d84db 100644
> --- a/trace-events
> +++ b/trace-events
> @@ -103,6 +103,7 @@ qmp_block_job_cancel(void *job) "job %p"
>  qmp_block_job_pause(void *job) "job %p"
>  qmp_block_job_resume(void *job) "job %p"
>  qmp_block_job_complete(void *job) "job %p"
> +qmp_drive_mirror_replace(void *job, const char *target_reference, const char *new_node_name) "job %p target_reference %s new_node_name %s"
>  block_job_cb(void *bs, void *job, int ret) "bs %p job %p ret %d"
>  qmp_block_stream(void *bs, void *job) "bs %p job %p"
>  
> -- 
> 1.9.1
> 
> 

Gentle ping

On Fri, May 30, 2014 at 01:18:43PM +0200, Benoît Canet wrote:
> When a quorum file is totally destroyed (broken NAS or SAN) the user can start a
> drive-mirror job on the quorum block backend and then replace the broken
> quorum file with drive-mirror-replace given it has a node-name.
> 
> Signed-off-by: Benoit Canet <benoit@irqsave.net>
> ---
>  block.c                   |   8 ++--
>  block/mirror.c            | 118 ++++++++++++++++++++++++++++++++++++++++++++--
>  blockdev.c                |  27 +++++++++++
>  include/block/block.h     |   3 ++
>  include/block/block_int.h |  15 ++++++
>  qapi-schema.json          |  33 +++++++++++++
>  qmp-commands.hx           |   5 ++
>  trace-events              |   1 +
>  8 files changed, 204 insertions(+), 6 deletions(-)

The drive-mirror-replace command is just block-job-complete plus
stashing arguments away.  I think the approach you took in v1 was
simpler: add an argument to drive-mirror and don't introduce new QAPI
commands.

There was discussion of a generic swap command, but drive-mirror-replace
is not generic.

Please revert to the drive-mirror argument approach since the
drive-mirror + block-job-complete API is sufficient to implement this
behavior.

> @@ -502,6 +518,11 @@ immediate_exit:
>              bdrv_unref(p);
>          }
>      }
> +    if (s->must_replace) {
> +        bdrv_op_unblock_all(s->to_replace, s->replace_blocker);
> +        error_free(s->replace_blocker);
> +        bdrv_unref(s->to_replace);
> +    }
>      bdrv_unref(s->target);
>      block_job_completed(&s->common, ret);
>  }

Can we do this unconditionally on the to_replace target (s->common.bs or
user-defined BDS)?  This code is already fairly complex and it's nice to
avoid more conditionals.

The swap operation is really the same whether to_replace == s->common.bs
or a user-defined BDS.  The op blocker cleanup and unref could be
unconditional as long as we op block and ref s->common.bs when no
user-defined BDS was given.

Stefan