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

[Benoît Canet]

The Monday 09 Jun 2014 à 15:08:29 (-0600), Eric Blake wrote :
> 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.

I am affraid that the user could form loop in the graph.

> 
> > 
> > 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?

I though that quorum was still returning the smallest image size for getlength
and that it would be a way to grow the quorum by replacing with bigger image.
But it's not the case I will enforce equality in 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.

ok

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

No you can't.
The first drive-mirror will lock the quorum device.

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

ok

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