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

[Benoît Canet]

The Wednesday 05 Mar 2014 à 13:54:44 (-0700), Eric Blake wrote :
> 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.

Ok this feature won't go in QEMU 2.0.

What about implementing as an additional
drive-mirror-complete target-node-name=no_name_to_replace 
new-node-name=node_name_of_the_new_bs

The command would do the same work as block-job-complete plus some extras
work and would be used after starting a regular drive-mirror command.

Best regards

Benoît

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