From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([208.118.235.92]:37203)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <eblake@redhat.com>) id 1SfeVr-0003L9-W5
	for qemu-devel@nongnu.org; Fri, 15 Jun 2012 17:57:05 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <eblake@redhat.com>) id 1SfeVq-0001oy-34
	for qemu-devel@nongnu.org; Fri, 15 Jun 2012 17:57:03 -0400
Received: from mx1.redhat.com ([209.132.183.28]:55266)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <eblake@redhat.com>) id 1SfeVp-0001ol-Qf
	for qemu-devel@nongnu.org; Fri, 15 Jun 2012 17:57:02 -0400
Message-ID: <4FDBAFA6.7060601@redhat.com>
Date: Fri, 15 Jun 2012 15:56:54 -0600
From: Eric Blake <eblake@redhat.com>
MIME-Version: 1.0
References: <20120615204648.9853.1225.sendpatchset@skannery.in.ibm.com>
	<20120615204730.9853.61858.sendpatchset@skannery.in.ibm.com>
In-Reply-To: <20120615204730.9853.61858.sendpatchset@skannery.in.ibm.com>
Content-Type: multipart/signed; micalg=pgp-sha256;
	protocol="application/pgp-signature";
	boundary="------------enig0FA1829989F3460F53A63853"
Subject: Re: [Qemu-devel] [v1 Patch 3/10]Qemu: Cmd "block_set_hostcache" for
 dynamic cache change
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Supriya Kannery <supriyak@linux.vnet.ibm.com>
Cc: Kevin Wolf <kwolf@redhat.com>, Shrinidhi Joshi <spjoshi31@gmail.com>, Stefan Hajnoczi <stefanha@gmail.com>, Jeff Cody <jcody@redhat.com>, qemu-devel@nongnu.org, Luiz Capitulino <lcapitulino@redhat.com>, Christoph Hellwig <hch@lst.de>

This is an OpenPGP/MIME signed message (RFC 2440 and 3156)
--------------enig0FA1829989F3460F53A63853
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

On 06/15/2012 02:47 PM, Supriya Kannery wrote:
> New command "block_set_hostcache" added for dynamically changing=20
> host pagecache setting of a block device.
>=20
> Usage:=20
>  block_set_hostcache  <device> <option>
>    <device> =3D block device
>    <option> =3D on/off
>=20
> Example:
>  (qemu) block_set_hostcache ide0-hd0 off
>=20
> Signed-off-by: Supriya Kannery <supriyak@linux.vnet.ibm.com>
>=20
> ---
>  block.c         |   54 +++++++++++++++++++++++++++++++++++++++++++++++=
+++++++
>  block.h         |    2 ++
>  blockdev.c      |   26 ++++++++++++++++++++++++++
>  blockdev.h      |    2 ++
>  hmp-commands.hx |   14 ++++++++++++++
>  qmp-commands.hx |   27 +++++++++++++++++++++++++++
>  6 files changed, 125 insertions(+)

Doesn't this also need to touch qapi-schema.json?
[/me reads full patch]
Oh, you did - but your diffstat is stale.  It might be worth figuring
out what in your workflow leads to stale diffstats.

>=20
> Index: qemu/block.c
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> --- qemu.orig/block.c
> +++ qemu/block.c
> @@ -858,6 +858,35 @@ unlink_and_fail:
>      return ret;
>  }
> =20
> +int bdrv_reopen(BlockDriverState *bs, int bdrv_flags)
> +{
> +    BlockDriver *drv =3D bs->drv;
> +    int ret =3D 0, open_flags;
> +
> +    /* Quiesce IO for the given block device */
> +    qemu_aio_flush();
> +    ret =3D bdrv_flush(bs);
> +    if (ret !=3D 0) {
> +        qerror_report(QERR_DATA_SYNC_FAILED, bs->device_name);
> +        return ret;
> +    }
> +    open_flags =3D bs->open_flags;
> +    bdrv_close(bs);
> +
> +    ret =3D bdrv_open(bs, bs->filename, bdrv_flags, drv);

Yuck.  This is bad, and why 'transaction' was invented.  Any time you
close() before open() you risk completely losing the file...

> +    if (ret < 0) {
> +        /* Reopen failed. Try to open with original flags */
> +        qerror_report(QERR_OPEN_FILE_FAILED, bs->filename);
> +        ret =3D bdrv_open(bs, bs->filename, open_flags, drv);
> +        if (ret < 0) {
> +            /* Reopen failed with orig and modified flags */
> +            abort();

and an abort() is not a nice reaction to that.

I think we should rebase the series to do the safe reopen prior to
adding this command (at least, just judging by the title of 4/10), to
avoid intermediate bad code.


> @@ -808,7 +808,6 @@ ETEXI
>          .mhandler.cmd =3D hmp_migrate,
>      },
> =20
> -
>  STEXI

Spurious whitespace change.


> +
> +SQMP
> +block_set_hostcache

QMP commands favor '-' over '_'; this should be 'block-set-hostcache'.


> Index: qemu/qapi-schema.json
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> --- qemu.orig/qapi-schema.json
> +++ qemu/qapi-schema.json
> @@ -1598,6 +1598,22 @@
>  { 'command': 'block_set_io_throttle',
>    'data': { 'device': 'str', 'bps': 'int', 'bps_rd': 'int', 'bps_wr': =
'int',
>              'iops': 'int', 'iops_rd': 'int', 'iops_wr': 'int' } }
> +##
> +# @block_set_hostcache:
> +#
> +# Change host pagecache setting of a block device
> +#
> +# @device: name of the block device
> +#
> +# @option: hostcache setting (true/false)
> +#
> +# Returns: Nothing on success
> +#          If @device is not a valid block device, DeviceNotFound

What happens if the device is valid, but the setting cannot be changed
(perhaps because reopen has not been implemented for that driver)?

--=20
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


--------------enig0FA1829989F3460F53A63853
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.4.12 (GNU/Linux)
Comment: Public key at http://people.redhat.com/eblake/eblake.gpg
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iQEcBAEBCAAGBQJP26+mAAoJEKeha0olJ0NqQncH/i62lVlHnCIqdlzBMWu8fFpP
fzNdO6J3yOgioRfZF+IsVx9ZyMvPdaDx2/CZk9p5oRpSL1av+BjMCVaC9BktKlii
feKw9pP9POu7TwRMLMkNAZFI1XpLpZBkFqDRpQG2psGhQkuB7beZFaFbEOR+W9Vr
KOIHbDivTJ92FCwZUz7XLZUwiURezQpUBn+VUWQaJMbipn0E8/+62FgFdit2vhq4
dCNzuZK2lj1Pcv8ib8w2/kV1+5p/3wJ8ApH10VEu0L531l1bKMMldEseT/AW2IyG
8BbgV8xtnf2tn7tYHqDj8NaqleKLY0cQJi98p7mgLL556twhVJfv/p9MzCHZiA4=
=AGM/
-----END PGP SIGNATURE-----

--------------enig0FA1829989F3460F53A63853--