Re: [Qemu-devel] [PATCH 1/2] block: Add node-name and to-replace-node-name arguments to drive-mirror.

[Eric Blake]

On 03/05/2014 08:18 AM, Benoît Canet wrote:
> node-name give a name to the created BDS and register it in the node graph.

s/give/gives/ s/register/registers/

> 
> to-replace-node-name can be used when drive-mirror is called with sync=full.
> 
> The purpose of these fields is to be able to reconstruct and replace a broken
> quorum file.

There may be other uses possible from this, but the idea makes sense.

> 
> drive-mirror will bdrv_swap the new BDS named node-name with the one
> pointed by to-replace-node-name when the mirroring is finished.
> 
> Signed-off-by: Benoit Canet <address@hidden>
> ---

> @@ -312,6 +313,10 @@ static void coroutine_fn mirror_run(void *opaque)
>      s->common.len = bdrv_getlength(bs);
>      if (s->common.len <= 0) {
>          block_job_completed(&s->common, s->common.len);
> +        /* Fam's new blocker API should be used here. */
> +        if (s->to_replace) {

Who is getting merged first?  It seems like this should be fixed before
taking this patch, if Fam's work is indeed closer to inclusion.  At any
rate, the comment seems odd - a year from now, Fam's work won't be new.

> +        BlockDriverState *to_replace;
> +        /* if a to-replace-node-name was specified use it's bs */

s/it's/its/ - the rule is anywhere that you see "it's", re-read the
sentence with "it is" and see if it still makes sense; if not, you meant
"its".


>  
>  static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target,
> +                             BlockDriverState *to_replace,
>                              int64_t speed, int64_t granularity,

Pre-existing, but as long as you are touching this, you might as well
fix indentation of the other lines in the same signature.

> @@ -2158,19 +2195,33 @@ void qmp_drive_mirror(const char *device, const char 
> *target,
>          return;
>      }
>  
> +    /* if we are planning to replace a graph node name the code should do a 
> full
> +     * mirror of the source image
> +     */
> +    if (has_to_replace_node_name && sync != MIRROR_SYNC_MODE_FULL) {
> +        error_setg(errp,
> +                   "to-replace-node-name can only be used with sync=full");
> +        return;
> +    }

I'm not sure I follow this restriction.  What's to prevent me from doing
a shallow mirror coupled with the mode of reusing an existing file that
already points to a sane backing file, rather than forcing a full sync?
 That is, why not let this command be a fully-generic swap command,
where the semantics are that as long as my old and new image have the
same contents from the guest's perspective (or I'm replacing a broken
file out of a quorum, and the new image has the same contents as the
quorum majority), then we are just updating qemu to point to a new BDS.

On the other hand, back around the 1.5 timeframe, downstream RHEL tried
to add a 'drive-reopen' command that did just that - replaced the
backing file of a guest's disk with an arbitrary other file.  But it was
so powerful and risky that at the time upstream finally added
'transaction' support, we decided to go with the simpler
'drive-mirror/block-job-complete' sequence as the only supported way to
cause qemu to associate a different BDS with a guest image.  Of course,
things have advanced since then, so maybe we finally are at a point
where we want to expose a generic reopen command that can swap out
arbitrary named nodes without interrupting guest services, but now I'm
starting to wonder if it should be a new command instead of adding
optional arguments to the existing drive-mirror.

> +++ b/qapi-schema.json
> @@ -2140,6 +2140,14 @@
>  # @format: #optional the format of the new destination, default is to
>  #          probe if @mode is 'existing', else the format of the source
>  #
> +# @new-node-name: #optional the new block driver state node name in the graph
> +#                 (Since 2.1)

Ah, so you're not trying to get this in before 2.0 freeze - which means
we have more time to think about the implications.

> +#
> +# @to-replace-node-name: #optional with sync=full graph node name to be
> +#                        replaced by the new image when a whole image copy is
> +#                        done. This can be used to repair broken Quorum 
> files.
> +#                        (Since 2.1)

This naming feels long, but I'm not sure if I have a better suggestion.
 It looks like you only allow swapping out one quorum file per
drive-mirror - but what if I have a 3/5 quorum and want to swap out two
files at once?  Also, how does this interact with the 'transaction' command?

>  ##
>  { 'command': 'drive-mirror',
>    'data': { 'device': 'str', 'target': 'str', '*format': 'str',
> -            'sync': 'MirrorSyncMode', '*mode': 'NewImageMode',
> -            '*speed': 'int', '*granularity': 'uint32',
> -            '*buf-size': 'int', '*on-source-error': 'BlockdevOnError',
> +            '*new-node-name': 'str', '*to-replace-node-name': 'str',
> +            'sync': 'MirrorSyncMode', '*mode': 'NewImageMode', '*speed': 
> 'int',
> +            '*granularity': 'uint32', '*buf-size': 'int',
> +            '*on-source-error': 'BlockdevOnError',

Why the reindent of existing options?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature