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 <ben...@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
> >
>
>
>
