From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59954) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WLIq4-00009F-M2 for qemu-devel@nongnu.org; Wed, 05 Mar 2014 15:54:57 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WLIq0-0004MS-6J for qemu-devel@nongnu.org; Wed, 05 Mar 2014 15:54:52 -0500 Received: from mx1.redhat.com ([209.132.183.28]:28768) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WLIpz-0004MH-TV for qemu-devel@nongnu.org; Wed, 05 Mar 2014 15:54:48 -0500 Message-ID: <53178F14.80402@redhat.com> Date: Wed, 05 Mar 2014 13:54:44 -0700 From: Eric Blake MIME-Version: 1.0 References: <1394032700-1642-1-git-send-email-benoit.canet@irqsave.net> <1394032700-1642-2-git-send-email-benoit.canet@irqsave.net> In-Reply-To: <1394032700-1642-2-git-send-email-benoit.canet@irqsave.net> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="KLsmlkCWanRkNGV466GeOP5VbvHC0LLlQ" Subject: Re: [Qemu-devel] [PATCH 1/2] block: Add node-name and to-replace-node-name arguments to drive-mirror. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?UTF-8?B?QmVub8OudCBDYW5ldA==?= , qemu-devel@nongnu.org Cc: kwolf@redhat.com, Fam Zheng , Benoit Canet , mreitz@redhat.com, stefanha@redhat.com, pbonzini@redhat.com This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --KLsmlkCWanRkNGV466GeOP5VbvHC0LLlQ Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 03/05/2014 08:18 AM, Beno=C3=AEt Canet wrote: > node-name give a name to the created BDS and register it in the node gr= aph. s/give/gives/ s/register/registers/ >=20 > to-replace-node-name can be used when drive-mirror is called with sync=3D= full. >=20 > 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. >=20 > 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. >=20 > Signed-off-by: Benoit Canet > --- > @@ -312,6 +313,10 @@ static void coroutine_fn mirror_run(void *opaque) > s->common.len =3D bdrv_getlength(bs); > if (s->common.len <=3D 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". > =20 > static void mirror_start_job(BlockDriverState *bs, BlockDriverState *t= arget, > + 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; > } > =20 > + /* 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 !=3D MIRROR_SYNC_MODE_FULL) {= > + error_setg(errp, > + "to-replace-node-name can only be used with sync=3D= 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. > +++ 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 th= e 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=3Dfull graph node name to= be > +# replaced by the new image when a whole image = copy is > +# done. This can be used to repair broken Quoru= m 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' comma= nd? > ## > { '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? --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --KLsmlkCWanRkNGV466GeOP5VbvHC0LLlQ Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJTF48UAAoJEKeha0olJ0NqbTEH/0sCQO7yeXViqygsjfUTFiQ3 MRITbgWwhiLvAsUeTk+mTKCl1SSFsLSQvkjnOSyPWc0jGur5+pqBeMjjDqKNFMJW Jch82y0P84Du85UAC3W6QRPpFPMUirTtufnqgL6KHjuxxjsxyCsL2lf/FFi+sDjS q7/qcgX3jva5NTnzrcVHdSzgEbc3Ogwzox5a+6SYouBFcnBaeBsyMsIWjwM7OWoL 5fwkW0kiIPu9cTL2Tc2j7udtRiA6eBePkxFXqvTTwTl98kERDgf/bhVsw5kJMk93 r/WfEm5bU+AbxpTw1JkpEOkRy6RxVKFuhnuAQYhevhO2yZAZNHybHt4jHo5VKiw= =X1lJ -----END PGP SIGNATURE----- --KLsmlkCWanRkNGV466GeOP5VbvHC0LLlQ--