From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46356) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1btd8T-0006yy-8q for qemu-devel@nongnu.org; Mon, 10 Oct 2016 12:09:08 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1btd8R-0002DJ-E9 for qemu-devel@nongnu.org; Mon, 10 Oct 2016 12:09:04 -0400 References: <1475232808-4852-1-git-send-email-vsementsov@virtuozzo.com> <1475232808-4852-17-git-send-email-vsementsov@virtuozzo.com> From: Max Reitz Message-ID: Date: Mon, 10 Oct 2016 18:08:54 +0200 MIME-Version: 1.0 In-Reply-To: <1475232808-4852-17-git-send-email-vsementsov@virtuozzo.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="wJnpH500uG4HL3JTGmsr7Xml98iG9rGHk" Subject: Re: [Qemu-devel] [PATCH 16/22] qmp: add persistent flag to block-dirty-bitmap-add List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladimir Sementsov-Ogievskiy , qemu-block@nongnu.org, qemu-devel@nongnu.org Cc: kwolf@redhat.com, armbru@redhat.com, eblake@redhat.com, jsnow@redhat.com, famz@redhat.com, den@openvz.org, stefanha@redhat.com, pbonzini@redhat.com This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --wJnpH500uG4HL3JTGmsr7Xml98iG9rGHk From: Max Reitz To: Vladimir Sementsov-Ogievskiy , qemu-block@nongnu.org, qemu-devel@nongnu.org Cc: kwolf@redhat.com, armbru@redhat.com, eblake@redhat.com, jsnow@redhat.com, famz@redhat.com, den@openvz.org, stefanha@redhat.com, pbonzini@redhat.com Message-ID: Subject: Re: [PATCH 16/22] qmp: add persistent flag to block-dirty-bitmap-add References: <1475232808-4852-1-git-send-email-vsementsov@virtuozzo.com> <1475232808-4852-17-git-send-email-vsementsov@virtuozzo.com> In-Reply-To: <1475232808-4852-17-git-send-email-vsementsov@virtuozzo.com> Content-Type: text/plain; charset=iso-8859-15 Content-Transfer-Encoding: quoted-printable On 30.09.2016 12:53, Vladimir Sementsov-Ogievskiy wrote: > Add optional 'persistent' flag to qmp command block-dirty-bitmap-add. > Default is false. >=20 > Signed-off-by: Vladimir Sementsov-Ogievskiy > Signed-off-by: Denis V. Lunev > --- > blockdev.c | 12 +++++++++++- > qapi/block-core.json | 7 ++++++- > qmp-commands.hx | 5 ++++- > 3 files changed, 21 insertions(+), 3 deletions(-) >=20 > diff --git a/blockdev.c b/blockdev.c > index 97062e3..ec0ec75 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -1991,6 +1991,7 @@ static void block_dirty_bitmap_add_prepare(BlkAct= ionState *common, > /* AIO context taken and released within qmp_block_dirty_bitmap_ad= d */ > qmp_block_dirty_bitmap_add(action->node, action->name, > action->has_granularity, action->granul= arity, > + action->has_persistent, action->persist= ent, > &local_err); > =20 > if (!local_err) { > @@ -2694,10 +2695,12 @@ out: > =20 > void qmp_block_dirty_bitmap_add(const char *node, const char *name, > bool has_granularity, uint32_t granula= rity, > + bool has_persistent, bool persistent, > Error **errp) > { > AioContext *aio_context; > BlockDriverState *bs; > + BdrvDirtyBitmap *bitmap; > =20 > if (!name || name[0] =3D=3D '\0') { > error_setg(errp, "Bitmap name cannot be empty"); > @@ -2723,7 +2726,14 @@ void qmp_block_dirty_bitmap_add(const char *node= , const char *name, > granularity =3D bdrv_get_default_bitmap_granularity(bs); > } > =20 > - bdrv_create_dirty_bitmap(bs, granularity, name, errp); > + if (!has_persistent) { > + persistent =3D false; > + } > + > + bitmap =3D bdrv_create_dirty_bitmap(bs, granularity, name, errp); > + if (bitmap !=3D NULL) { > + bdrv_dirty_bitmap_set_persistance(bitmap, persistent); As I said before, I think this command should be able to return errors and make use of that to reject making bitmaps persistent when the respective block driver cannot handle them. > + } > =20 > out: > aio_context_release(aio_context); > diff --git a/qapi/block-core.json b/qapi/block-core.json > index 31f9990..2bf56cd 100644 > --- a/qapi/block-core.json > +++ b/qapi/block-core.json > @@ -1235,10 +1235,15 @@ > # @granularity: #optional the bitmap granularity, default is 64k for > # block-dirty-bitmap-add > # > +# @persistent: #optional the bitmap is persistent, i.e. it will be sav= ed to > +# corresponding block device on it's close. Default is fa= lse. > +# For block-dirty-bitmap-add. (Since 2.8) I'm not sure what the "For block-dirty-bitmap-add." is supposed to mean, because this whole struct is for block-dirty-bitmap-add (and for block-dirty-bitmap-add transactions, to be exact, but @persistent will surely work there, too, won't it?). Also, I'd say "will be saved to the corresponding block device image file" instead of just "block device", because in my understanding a block device and its image file are two separate things. > +# > # Since 2.4 > ## > { 'struct': 'BlockDirtyBitmapAdd', > - 'data': { 'node': 'str', 'name': 'str', '*granularity': 'uint32' } }= > + 'data': { 'node': 'str', 'name': 'str', '*granularity': 'uint32', > + '*persistent': 'bool' } } I think normally we'd align the new line so that the opening ' of '*persistent' is under the opening ' of 'node'. > =20 > ## > # @block-dirty-bitmap-add > diff --git a/qmp-commands.hx b/qmp-commands.hx > index ba2a916..434b418 100644 > --- a/qmp-commands.hx > +++ b/qmp-commands.hx > @@ -1441,7 +1441,7 @@ EQMP > =20 > { > .name =3D "block-dirty-bitmap-add", > - .args_type =3D "node:B,name:s,granularity:i?", > + .args_type =3D "node:B,name:s,granularity:i?,persistent:b?", > .mhandler.cmd_new =3D qmp_marshal_block_dirty_bitmap_add, > }, > =20 > @@ -1458,6 +1458,9 @@ 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) > +- "persistent": bitmap will be saved to corresponding block device > + on it's close. Block driver should maintain persistent= bitmaps > + (json-bool, optional, default false) (Since 2.8) And I don't know what the user is supposed to make of the information that block drivers will take care of maintaining persistent bitmaps. All they care about is that it will be stored in the corresponding image file, so in my opinion it would be better to just omit the last sentence here. Max > =20 > Example: > =20 >=20 --wJnpH500uG4HL3JTGmsr7Xml98iG9rGHk Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- iQEvBAEBCAAZBQJX+70WEhxtcmVpdHpAcmVkaGF0LmNvbQAKCRD0B9sAYdXPQDaR CACCGzICNFc3PLQNhpH2O0hxF2aqZ63ETuMhPM0QPaEnp/P59p1D1xUvtma3Bnz0 R8qXGRH35eSAeZXHmGBUtVbYxmZz6MlsgLFxMX7oYTiKEGQZTS6VwLWiG1dZbUTG IDiajGnhauJplUIt57Xt+j1oXTHANk3KApEXjKfioTJzyxZHPIxID050naCkohFA GhiSVNrjOAsEqOuLPZHkRANhkQjigq5t33j5YRd1mo8xt0hY8GNqWh0oI/IUBjmG z9arKHqaUNWGm2yYxenMvMsd//ljjuta17jy0mXZQsmt83PZOjs8rrL7ILXpJKQM aZboZLAeuV+njwq2hwU/HSDU =Ka9Y -----END PGP SIGNATURE----- --wJnpH500uG4HL3JTGmsr7Xml98iG9rGHk--