From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46718) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XzNr4-0002iE-FJ for qemu-devel@nongnu.org; Fri, 12 Dec 2014 05:53:59 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XzNqv-0006my-Bh for qemu-devel@nongnu.org; Fri, 12 Dec 2014 05:53:50 -0500 Received: from mail-wg0-x232.google.com ([2a00:1450:400c:c00::232]:60777) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XzNqv-0006mk-0X for qemu-devel@nongnu.org; Fri, 12 Dec 2014 05:53:41 -0500 Received: by mail-wg0-f50.google.com with SMTP id a1so8776343wgh.37 for ; Fri, 12 Dec 2014 02:53:40 -0800 (PST) Date: Fri, 12 Dec 2014 10:53:36 +0000 From: Stefan Hajnoczi Message-ID: <20141212105336.GA22222@stefanha-thinkpad.redhat.com> References: <1417465816-19345-1-git-send-email-jsnow@redhat.com> <1417465816-19345-3-git-send-email-jsnow@redhat.com> <20141209160059.GD27053@stefanha-thinkpad.redhat.com> <87y4qfsmik.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="qDbXVdCdHGoSgWSk" Content-Disposition: inline In-Reply-To: <87y4qfsmik.fsf@blackfin.pond.sub.org> Subject: Re: [Qemu-devel] [PATCH v9 02/10] qmp: Add block-dirty-bitmap-add and block-dirty-bitmap-remove List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: kwolf@redhat.com, Fam Zheng , qemu-devel@nongnu.org, mreitz@redhat.com, vsementsov@parallels.com, stefanha@redhat.com, pbonzini@redhat.com, John Snow --qDbXVdCdHGoSgWSk Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Dec 10, 2014 at 01:43:47PM +0100, Markus Armbruster wrote: > Stefan Hajnoczi writes: >=20 > > On Mon, Dec 01, 2014 at 03:30:08PM -0500, John Snow wrote: > >> diff --git a/block.c b/block.c > >> index e5c6ccf..3f27519 100644 > >> --- a/block.c > >> +++ b/block.c > >> @@ -5420,6 +5420,25 @@ int bdrv_get_dirty(BlockDriverState *bs, BdrvDi= rtyBitmap *bitmap, int64_t sector > >> } > >> } > >> =20 > >> +#define BDB_MIN_DEF_GRANULARITY 4096 > >> +#define BDB_MAX_DEF_GRANULARITY 65536 > >> +#define BDB_DEFAULT_GRANULARITY BDB_MAX_DEF_GRANULARITY > >> + > >> +uint64_t bdrv_dbm_calc_def_granularity(BlockDriverState *bs) > > > > Long names are unwieldy but introducing multiple abbreviations is not a > > good solution, it makes the code more confusing (BDB vs dbm). > > > > I would call the function bdrv_get_default_bitmap_granularity(). > > > > The constants weren't necessary since the point of this function is to > > capture the default value. No one else should use the constants - > > otherwise there is a high probability that they are reimplementing this > > logic. I would just leave them as literals in the code. > > > >> diff --git a/blockdev.c b/blockdev.c > >> index 5651a8e..4d30b09 100644 > >> --- a/blockdev.c > >> +++ b/blockdev.c > >> @@ -1894,6 +1894,60 @@ void qmp_block_set_io_throttle(const char *devi= ce, int64_t bps, int64_t bps_rd, > >> aio_context_release(aio_context); > >> } > >> =20 > >> +void qmp_block_dirty_bitmap_add(const char *device, const char *name, > >> + bool has_granularity, int64_t granula= rity, > >> + Error **errp) > >> +{ > >> + BlockDriverState *bs; > >> + > >> + bs =3D bdrv_lookup_bs(device, NULL, errp); > > > > Markus: I think we need to support node-name here so dirty bitmaps can > > be applied at any node in the graph? >=20 > We need to consider node names for all new QMP commands. Whenever we > add a backend name parameter, like we do for the two new commands in > this patch, we need to decide whether nodes make sense, too. Do they? >=20 > I'm afraid we haven't quite made up our mind what to do when nodes make > sense. >=20 > Existing patterns of backend / node name parameters: >=20 > 1. Backend name only >=20 > Parameter is commonly called @device. Examples: eject, > block_set_io_throttle. >=20 > Code uses blk_by_name() or bdrv_find(). The latter needs to be > converted to the former. >=20 > 2. Backend or node name >=20 > 2a. Two optional parameters, commonly called @device and @node-name, > of which exactly one must be given. Example: block_passwd. >=20 > Code uses >=20 > bs =3D bdrv_lookup_bs(has_device ? device : NULL, > has_node_name ? node_name : NULL, > &local_err); >=20 > which is a roundabout way to say >=20 > bs =3D bdrv_lookup_bs(device, node_name, &local_err); >=20 > 2b. Single parameter. The single example is anonymous union > BlockdevRef. >=20 > Code uses >=20 > bs =3D bdrv_lookup_bs(reference, reference, errp); >=20 > If we want to adopt "single parameter" going forward, we need to > decide on a naming convention. Existing commands are probably stuck > with @device for compatibility. Do we care for names enough to > improve on that? >=20 > A convenience wrapper around bdrv_lookup_bs() to avoid stuttering > name argument could make sense. Initially only the backend needs dirty bitmap support (this satisfies the incremental backup use case). It is possible that future use cases will require node-name. I'm happy with just allowing the device parameter today and adding the node-name parameter if needed later. This conservative approach seems good because IMO we shouldn't add unused features to the API since they need to be tested and maintained forever. So maybe use a device argument with blk_by_name() for now. In the future switch to bdrv_lookup_bs() with has_device/has_node_name. If anyone strongly feels we should support node-name from day 1, I'm okay with that too but there needs to be a test case which actually exercises that code! Stefan --qDbXVdCdHGoSgWSk Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQEcBAEBAgAGBQJUiskwAAoJEJykq7OBq3PISV4IALfYvNhr0OWtR/nUfatNX2gt jpjWd+l6lDzsp/HXeypq3NzU9WLx9jBLQW9eWZcPE3uifIY7JIaJIv5FQqloni0D Q908w+iNvN+xt7pp+qH3z/KuM1q3FXvOPLmyYveOV7LmsVoET2k2srygp0cVEo8q tpY67vtmuRP179pZKsxFt3t8lIwvjKKTTQDYMBRiVi9+7bfg4KPCaHOt1+GywWP3 qKRavVTQUVVFRlDYEVt4vVA45VPUZec7TQLtbJ9gvo2F+QQWdlk0DeCVjlq9BDmG 3zH2GS8dj6J2JD2GbKmZIR46QO67Sm/9wqkVeoEOSrLZa8NvRC33KDqpwe5bgMM= =TRNQ -----END PGP SIGNATURE----- --qDbXVdCdHGoSgWSk--