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