From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60646) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YMOfX-0004RY-7I for qemu-devel@nongnu.org; Fri, 13 Feb 2015 17:25:04 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YMOfS-0000iI-3b for qemu-devel@nongnu.org; Fri, 13 Feb 2015 17:25:03 -0500 Received: from mx1.redhat.com ([209.132.183.28]:37153) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YMOfR-0000iA-RW for qemu-devel@nongnu.org; Fri, 13 Feb 2015 17:24:58 -0500 Message-ID: <54DE79B7.3030802@redhat.com> Date: Fri, 13 Feb 2015 15:24:55 -0700 From: Eric Blake MIME-Version: 1.0 References: <1423532117-14490-1-git-send-email-jsnow@redhat.com> <1423532117-14490-3-git-send-email-jsnow@redhat.com> In-Reply-To: <1423532117-14490-3-git-send-email-jsnow@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="AbE9NUDosRnMO8HGx6CMqCNTLSjjMaFVx" Subject: Re: [Qemu-devel] [PATCH v12 02/17] qmp: Add block-dirty-bitmap-add and block-dirty-bitmap-remove List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: John Snow , qemu-devel@nongnu.org Cc: kwolf@redhat.com, famz@redhat.com, armbru@redhat.com, mreitz@redhat.com, vsementsov@parallels.com, stefanha@redhat.com This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --AbE9NUDosRnMO8HGx6CMqCNTLSjjMaFVx Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 02/09/2015 06:35 PM, John Snow wrote: > The new command pair is added to manage user created dirty bitmap. The > dirty bitmap's name is mandatory and must be unique for the same device= , > but different devices can have bitmaps with the same names. I haven't been following this series closely (I know I should be doing that, though). Is the bitmap associated with the BDS (a host resource, independent of which device(s) are currently viewing that content) or with the BlockBackend (only one bitmap namespace per device)? I'm a bit worried that we will WANT to have bitmaps associated with BDS (if we don't already) because of image fleecing. That is, if we start with: base <- mid <- active and request an image fleecing operation, we want: base <- mid <- active \- overlay where overlay serves the NBD that sees the point in time. If we then allow a block-commit, then writes to 'mid' containing the content from 'active' will trigger another write to 'overlay' with the pre-modified contents, so that the NBD fleecing operation doesn't see any changes. If we then migrate, it means we need multiple bitmaps: the map for the commit of active into mid (how much remains to be committed), and the map for mid to overlay (how much of mid has been changed since the point-in-time overlay was created). By associating bitmaps with a device (a BB), rather than a BDS, you may be artificially limiting which operations can be performed. On the other hand, if you associate with a BDS, and we improve things to allow arbitrary refactoring relationships where a BDS can be in more than one tree at once, it starts to be hard to prove that bitmap names won't be duplicated. Am I overthinking something here, or are we okay limiting bitmap names to just the BB device, rather than a BDS? >=20 > The granularity is an optional field. If it is not specified, we will > choose a default granularity based on the cluster size if available, > clamped to between 4K and 64K to mirror how the 'mirror' code was > already choosing granularity. If we do not have cluster size info > available, we choose 64K. This code has been factored out into a helper= > shared with block/mirror. >=20 > This patch also introduces the 'block_dirty_bitmap_lookup' helper, > which takes a device name and a dirty bitmap name and validates the > lookup, returning NULL and setting errp if there is a problem with > either field. This helper will be re-used in future patches in this > series. >=20 > The types added to block-core.json will be re-used in future patches > in this series, see: > 'qapi: Add transaction support to block-dirty-bitmap-{add, enable, disa= ble}' >=20 > Signed-off-by: Fam Zheng > Signed-off-by: John Snow > --- > block.c | 20 ++++++++++ > block/mirror.c | 10 +---- > blockdev.c | 100 ++++++++++++++++++++++++++++++++++++++++++= ++++++++ > include/block/block.h | 1 + > qapi/block-core.json | 55 +++++++++++++++++++++++++++ > qmp-commands.hx | 51 +++++++++++++++++++++++++ > 6 files changed, 228 insertions(+), 9 deletions(-) >=20 > +++ b/blockdev.c > @@ -1173,6 +1173,48 @@ out_aio_context: > return NULL; > } > =20 > +/** > + * Return a dirty bitmap (if present), after validating > + * the node reference and bitmap names. Returns NULL on error, > + * including when the BDS and/or bitmap is not found. > + */ > +static BdrvDirtyBitmap *block_dirty_bitmap_lookup(const char *node, > + const char *name, > + BlockDriverState **p= bs, > + Error **errp) > +{ > + BlockDriverState *bs; > + BdrvDirtyBitmap *bitmap; > + > + if (!node) { > + error_setg(errp, "Node cannot be NULL"); > + return NULL; > + } Node tends to be the term we use for BDS, rather than for device BB. > + if (!name) { > + error_setg(errp, "Bitmap name cannot be NULL"); > + return NULL; > + } > + > + bs =3D bdrv_lookup_bs(node, node, NULL); > + if (!bs) { > + error_setg(errp, "Node '%s' not found", node); > + return NULL; > + } > + > + /* If caller provided a BDS*, provide the result of that lookup, t= oo. */ > + if (pbs) { > + *pbs =3D bs; > + } > + > + bitmap =3D bdrv_find_dirty_bitmap(bs, name); and this seems to say that bitmap names are per-BDS, not per-device. > + if (!bitmap) { > + error_setg(errp, "Dirty bitmap '%s' not found", name); > + return NULL; > + } > + > + return bitmap; > +} > + > /* New and old BlockDriverState structs for atomic group operations */= > +++ b/qapi/block-core.json > @@ -959,6 +959,61 @@ > '*on-target-error': 'BlockdevOnError' } } > =20 > ## > +# @BlockDirtyBitmap > +# > +# @node: name of device/node which the bitmap is tracking > +# > +# @name: name of the dirty bitmap > +# > +# Since 2.3 > +## > +{ 'type': 'BlockDirtyBitmap', > + 'data': { 'node': 'str', 'name': 'str' } } This naming implies that bitmap is a per-BDS option, but that as a convenience, we allow a device name (BB) as shorthand for the top-level BDS associated with the BB. I can live with that. > +++ b/qmp-commands.hx > @@ -1244,6 +1244,57 @@ Example: > EQMP > =20 > { > + .name =3D "block-dirty-bitmap-add", > + .args_type =3D "node:B,name:s,granularity:i?", > + .mhandler.cmd_new =3D qmp_marshal_input_block_dirty_bitmap_add= , > + }, > + { > + .name =3D "block-dirty-bitmap-remove", > + .args_type =3D "node:B,name:s", > + .mhandler.cmd_new =3D qmp_marshal_input_block_dirty_bitmap_rem= ove, > + }, I don't know if it is worth interleaving these declarations... > + > +SQMP > + > +block-dirty-bitmap-add > +---------------------- > +Since 2.3 > + > +Create a dirty bitmap with a name on the device, and start tracking th= e writes. > + > +Arguments: > + > +- "node": device/node on which to create dirty bitmap (json-string) > +- "name": name of the new dirty bitmap (json-string) > +- "granularity": granularity to track writes with (int, optional) > + > +Example: > + > +-> { "execute": "block-dirty-bitmap-add", "arguments": { "node": "driv= e0", > + "name": "bitmap0" }= } > +<- { "return": {} } > + > +block-dirty-bitmap-remove > +------------------------- > +Since 2.3 =2E..to align with their examples. I don't see much else grouping in this= file. > + > +Stop write tracking and remove the dirty bitmap that was created with > +block-dirty-bitmap-add. > + > +Arguments: > + > +- "node": device/node on which to remove dirty bitmap (json-string) > +- "name": name of the dirty bitmap to remove (json-string) > + > +Example: > + > +-> { "execute": "block-dirty-bitmap-remove", "arguments": { "node": "d= rive0", > + "name": "bitmap0= " } } > +<- { "return": {} } > + > +EQMP > + > + { > .name =3D "blockdev-snapshot-sync", > .args_type =3D "device:s?,node-name:s?,snapshot-file:s,snapsh= ot-node-name:s?,format:s?,mode:s?", > .mhandler.cmd_new =3D qmp_marshal_input_blockdev_snapshot_sync= , >=20 The interface seems okay; I'm not sure if there are any tweaks we need to the commit message or documentation. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --AbE9NUDosRnMO8HGx6CMqCNTLSjjMaFVx 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/ iQEcBAEBCAAGBQJU3nm3AAoJEKeha0olJ0Nq7nsIAKo0f7Qpqj1TdGunGwwd9EE3 ApRBWe88F9cP85qaTJreN603XWIG4XCXVTEDioRusOw3xkUs5T2X7cMdN3yOMpHQ 9xWKrGgRawhE7tZ4X4YWi7KgMHIVcfOqnJxe2HizZTA1HWQgcHd6+BGOUz3TiRIZ Z7p3ZU8yHsQvkSTFfjpu1p/EFJzw7XoaXvtB4jGRlcbD0YcxdzPrKYCagHP+03nF sIo2+//Ngcq2v8YTg+MqKKO99kpcrPX8+Udqsm3djlilUwoqL4P+/AjDoOZg/AHm OIV33cCgDNDmSWiAQ26+kCetv18m9qCFxB3LfEaAecE659YWg+ZtTMIEHVGa3ws= =47av -----END PGP SIGNATURE----- --AbE9NUDosRnMO8HGx6CMqCNTLSjjMaFVx--