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

["Benoît Canet" <benoit.canet@irqsave.net>]

The Wednesday 05 Mar 2014 à 22:13:13 (+0100), Benoît Canet wrote :
> 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 <benoit@irqsave.net>
> > > ---
> > 
> > > @@ -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.
> 
> I would really like to get this merged first before 2.0 reach hard freeze
> since quorum is not very usable in its current state.
> 
> This particular comment was here to inform reviewer of the work I plan to do
> once Fam's series is merged.
> 
> I would do the work in 2.1.
> 
> > 
> > > +        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".
> 
> Thanks for the rule I just used it above :)
> 
> > 
> > 
> > >  
> > >  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.
> 
> I choose to hook into drive-mirror because it is supposed to do the swap at the
> very moment the two files converge.
> 
> I though it would be harder to implement with a separate command because new
> writes could obsolete the mirror after drive-mirror complete and before the
> swap command is launched.
> 
> > 
> > > +++ 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.
> 
> I remembered after sending the series that 2.0 was not in hard freeze yet and
> that we have a small chance of shipping quorum in an usable state.
> 
> > 
> > > +#
> > > +# @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?
> 
> I think that we should be able to launch multiple separate drive-mirror
> operation. I don't know about the transaction.

In fact no. The first drive-mirror will set the quorum device as "in use".
No parallelisation is possible.

Best regards

Benoît

> 
> > 
> > >  ##
> > >  { '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?
> 
> The first modified line was exceeding the 80 characters limit.
> 
> > 
> > -- 
> > Eric Blake   eblake redhat com    +1-919-301-3266
> > Libvirt virtualization library http://libvirt.org
> > 
> 
> 
>