Re: [Qemu-devel] [PATCH v5 2/3] block: Add drive-mirror-replace command

[Stefan Hajnoczi]

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 <address@hidden>
> ---
>  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

pgp4_4OUNWlO_.pgp
Description: PGP signature