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

[Eric Blake]

On 06/09/2014 02:46 PM, Benoît Canet wrote:
> node-name gives a name to the created BDS and registers it in the node graph.
> 
> to-replace-node-name can be used when drive-mirror is called with sync=full.

Why can't it work with other modes?  That is, if I have:

base1 <- snap1 \
base2 <- snap2  > quorum
base3 <- snap3 /

and want to replace the 'base3 <- snap3' arm of the quorum with 'base4
<- snap4', where base3 and base4 are identical, the fact that you are
forcing sync=full will not let me do so.  There's a lot of things where
if management does something stupid, then the guest will see data
instantly corrupted; but that doesn't mean that we necessarily have to
cripple the power of the command.

> 
> The purpose of these fields is to be able to reconstruct and replace a broken
> quorum file.
> 
> 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>
> ---

> +
> +        if (size < bdrv_getlength(to_replace_bs)) {
> +            error_setg(errp, "cannot replace to-replace-node-name image with 
> a "
> +                             "mirror image that would be smaller in size");

Should this be enforcing equality in size, rather than just prohibiting
shrinking?  Doesn't the quorum code already require that all quorum
members have equal size?


>  
> +    /* 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;
> +    }

Again, I'm not sure if this restriction makes sense.

> +++ b/qapi/block-core.json
> @@ -769,6 +769,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)

Is it worth splitting this patch into two? The ability to name the new
node of a drive-mirror makes sense as an independent patch, which might
be applied sooner even while worrying about the semantics of how
replacement will work.

> +#
> +# @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)

So if I understand correctly, the point of this command is that if I
have a quorum with three backing named nodes, and want to hotswap out
one of those modes, then I create a drive-mirror that names which of the
three nodes is the victim, and on completion, the quorum now has the
remaining two nodes and my new mirror as its new three node setup.

Am I correct that to-replace-node-name can only be used on a node that
is already composed of other nodes, and that the replacement must be one
of those nodes?

What if I have a 3/5 quorum - can I replace 2 nodes at once?

> +#
>  # @mode: #optional whether and how QEMU should create a new image, default is
>  #        'absolute-paths'.
>  #
> @@ -801,6 +809,7 @@
>  ##
>  { 'command': 'drive-mirror',
>    'data': { 'device': 'str', 'target': 'str', '*format': 'str',
> +            '*new-node-name': 'str', '*to-replace-node-name': 'str',

Bikeshedding: those are some long names.  Is it sufficient to go with
something shorter, '*node-name' for what to name the new mirror (again,
might be worth splitting that into its own patch), and '*replaces' for
the name of a node to be replaced?

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

signature.asc
Description: OpenPGP digital signature