From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([140.186.70.92]:59368)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <mdroth@linux.vnet.ibm.com>) id 1RskG6-0005Lx-C9
	for qemu-devel@nongnu.org; Wed, 01 Feb 2012 19:10:40 -0500
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <mdroth@linux.vnet.ibm.com>) id 1RskG5-0007P3-36
	for qemu-devel@nongnu.org; Wed, 01 Feb 2012 19:10:38 -0500
Received: from e32.co.us.ibm.com ([32.97.110.150]:43140)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <mdroth@linux.vnet.ibm.com>) id 1RskG4-0007OE-L6
	for qemu-devel@nongnu.org; Wed, 01 Feb 2012 19:10:37 -0500
Received: from /spool/local
	by e32.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only!
	Violators will be prosecuted
	for <qemu-devel@nongnu.org> from <mdroth@linux.vnet.ibm.com>;
	Wed, 1 Feb 2012 17:10:20 -0700
Received: from d03relay02.boulder.ibm.com (d03relay02.boulder.ibm.com
	[9.17.195.227])
	by d03dlp03.boulder.ibm.com (Postfix) with ESMTP id 3624519D804F
	for <qemu-devel@nongnu.org>; Wed,  1 Feb 2012 17:09:17 -0700 (MST)
Received: from d03av02.boulder.ibm.com (d03av02.boulder.ibm.com [9.17.195.168])
	by d03relay02.boulder.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id
	q1209JEF161670
	for <qemu-devel@nongnu.org>; Wed, 1 Feb 2012 17:09:19 -0700
Received: from d03av02.boulder.ibm.com (loopback [127.0.0.1])
	by d03av02.boulder.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP
	id q1209Icu005557
	for <qemu-devel@nongnu.org>; Wed, 1 Feb 2012 17:09:19 -0700
Message-ID: <4F29D42C.8090203@linux.vnet.ibm.com>
Date: Wed, 01 Feb 2012 18:09:16 -0600
From: Michael Roth <mdroth@linux.vnet.ibm.com>
MIME-Version: 1.0
References: <20120201030557.2990.74150.sendpatchset@skannery.in.ibm.com>
	<20120201030641.2990.46503.sendpatchset@skannery.in.ibm.com>
In-Reply-To: <20120201030641.2990.46503.sendpatchset@skannery.in.ibm.com>
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit
Subject: Re: [Qemu-devel] [RFC Patch 3/7]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>, Stefan Hajnoczi <stefanha@gmail.com>, Luiz Capitulino <lcapitulino@redhat.com>, qemu-devel@nongnu.org, Christoph Hellwig <hch@lst.de>

On 01/31/2012 09:06 PM, Supriya Kannery wrote:
> New command "block_set_hostcache" added for dynamically changing
> host pagecache setting of a block device.
>
> Usage:
>   block_set_hostcache<device>  <option>
>     <device>  = block device
>     <option>  = on/off
>
> Example:
>   (qemu) block_set_hostcache ide0-hd0 off
>
> Signed-off-by: Supriya Kannery<supriyak@linux.vnet.ibm.com>
>
> ---
>   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(+)
>
> Index: qemu/block.c
> ===================================================================
> --- qemu.orig/block.c
> +++ qemu/block.c
> @@ -808,6 +808,35 @@ unlink_and_fail:
>       return ret;
>   }
>
> +int bdrv_reopen(BlockDriverState *bs, int bdrv_flags)
> +{
> +    BlockDriver *drv = bs->drv;
> +    int ret = 0, open_flags;
> +
> +    /* Quiesce IO for the given block device */
> +    qemu_aio_flush();
> +    ret = bdrv_flush(bs);
> +    if (ret != 0) {
> +        qerror_report(QERR_DATA_SYNC_FAILED, bs->device_name);
> +        return ret;
> +    }
> +    open_flags = bs->open_flags;
> +    bdrv_close(bs);
> +
> +    ret = bdrv_open(bs, bs->filename, bdrv_flags, drv);
> +    if (ret<  0) {
> +        /* Reopen failed. Try to open with original flags */
> +        qerror_report(QERR_REOPEN_FILE_FAILED, bs->filename);
> +        ret = bdrv_open(bs, bs->filename, open_flags, drv);
> +        if (ret<  0) {
> +            /* Reopen failed with orig and modified flags */
> +            abort();
> +        }
> +    }
> +
> +    return ret;
> +}
> +
>   void bdrv_close(BlockDriverState *bs)
>   {
>       if (bs->drv) {
> @@ -870,6 +899,33 @@ void bdrv_drain_all(void)
>       }
>   }
>
> +int bdrv_change_hostcache(BlockDriverState *bs, bool enable_host_cache)
> +{
> +    int bdrv_flags = bs->open_flags;
> +
> +    /* set hostcache flags (without changing WCE/flush bits) */
> +    if (enable_host_cache) {
> +        bdrv_flags&= ~BDRV_O_NOCACHE;
> +    } else {
> +        bdrv_flags |= BDRV_O_NOCACHE;
> +    }
> +
> +    /* If no change in flags, no need to reopen */
> +    if (bdrv_flags == bs->open_flags) {
> +        return 0;
> +    }
> +
> +    if (bdrv_is_inserted(bs)) {
> +        /* Reopen file with changed set of flags */
> +        bdrv_flags&= ~BDRV_O_CACHE_WB;
> +        return bdrv_reopen(bs, bdrv_flags);

It seems like the real interface we're wanting here is bdrv_set_flags(), 
or something along that line, with the re-opening being more of an 
implementation detail.

For instance, with raw-posix.c:raw_reopen_prepare() we'll end up 
skipping the re-opening completely if fcntl() is sufficient.

> +    } else {
> +        /* Save hostcache change for future use */
> +        bs->open_flags = bdrv_flags;
> +        return 0;
> +    }
> +}
> +
>   /* make a BlockDriverState anonymous by removing from bdrv_state list.
>      Also, NULL terminate the device_name to prevent double remove */
>   void bdrv_make_anon(BlockDriverState *bs)
> Index: qemu/block.h
> ===================================================================
> --- qemu.orig/block.h
> +++ qemu/block.h
> @@ -119,6 +119,7 @@ int bdrv_parse_cache_flags(const char *m
>   int bdrv_file_open(BlockDriverState **pbs, const char *filename, int flags);
>   int bdrv_open(BlockDriverState *bs, const char *filename, int flags,
>                 BlockDriver *drv);
> +int bdrv_reopen(BlockDriverState *bs, int bdrv_flags);
>   void bdrv_close(BlockDriverState *bs);
>   int bdrv_attach_dev(BlockDriverState *bs, void *dev);
>   void bdrv_attach_dev_nofail(BlockDriverState *bs, void *dev);
> @@ -160,6 +161,7 @@ void bdrv_commit_all(void);
>   int bdrv_change_backing_file(BlockDriverState *bs,
>       const char *backing_file, const char *backing_fmt);
>   void bdrv_register(BlockDriver *bdrv);
> +int bdrv_change_hostcache(BlockDriverState *bs, bool enable_host_cache);
>
>
>   typedef struct BdrvCheckResult {
> Index: qemu/blockdev.c
> ===================================================================
> --- qemu.orig/blockdev.c
> +++ qemu/blockdev.c
> @@ -1080,3 +1080,29 @@ BlockJobInfoList *qmp_query_block_jobs(E
>       bdrv_iterate(do_qmp_query_block_jobs_one,&prev);
>       return dummy.next;
>   }
> +
> +
> +/*
> + * Change host page cache setting while guest is running.
> +*/
> +int do_block_set_hostcache(Monitor *mon, const QDict *qdict,
> +                           QObject **ret_data)
> +{
> +    BlockDriverState *bs = NULL;
> +    int enable;
> +    const char *device;
> +
> +    /* Validate device */
> +    device = qdict_get_str(qdict, "device");
> +    bs = bdrv_find(device);
> +    if (!bs) {
> +        qerror_report(QERR_DEVICE_NOT_FOUND, device);
> +        return -1;
> +    }
> +
> +    /* Read hostcache setting */
> +    enable = qdict_get_bool(qdict, "option");
> +    return bdrv_change_hostcache(bs, enable);
> +
> +}
> +
> Index: qemu/blockdev.h
> ===================================================================
> --- qemu.orig/blockdev.h
> +++ qemu/blockdev.h
> @@ -62,4 +62,6 @@ void qmp_change_blockdev(const char *dev
>                            bool has_format, const char *format, Error **errp);
>   void do_commit(Monitor *mon, const QDict *qdict);
>   int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data);
> +int do_block_set_hostcache(Monitor *mon, const QDict *qdict,
> +                           QObject **ret_data);
>   #endif
> Index: qemu/hmp-commands.hx
> ===================================================================
> --- qemu.orig/hmp-commands.hx
> +++ qemu/hmp-commands.hx
> @@ -796,6 +796,20 @@ ETEXI
>   	.mhandler.cmd_new = do_migrate,
>       },
>
> +    {
> +        .name       = "block_set_hostcache",
> +        .args_type  = "device:B,option:b",
> +        .params     = "device on|off",
> +        .help       = "Change setting of host pagecache",
> +        .user_print = monitor_user_noop,
> +        .mhandler.cmd_new = do_block_set_hostcache,
> +    },
> +STEXI
> +@item block_set_hostcache @var{device} @var{option}
> +@findex block_set_hostcache
> +Change host pagecache setting of a block device while guest is running.
> +ETEXI
> +
>
>   STEXI
>   @item migrate [-d] [-b] [-i] @var{uri}
> Index: qemu/qmp-commands.hx
> ===================================================================
> --- qemu.orig/qmp-commands.hx
> +++ qemu/qmp-commands.hx
> @@ -698,7 +698,34 @@ Example:
>
>   EQMP
>
> +
>       {
> +        .name       = "block_set_hostcache",
> +        .args_type  = "device:B,option:b",
> +        .params     = "device on|off",
> +        .help       = "Change setting of host pagecache (true|false)",
> +        .user_print = monitor_user_noop,
> +        .mhandler.cmd_new = do_block_set_hostcache,
> +    },
> +
> +SQMP
> +block_set_hostcache
> +-------------------
> +
> +Change host pagecache setting of a block device (on|off)
> +
> +Arguments:
> +
> +- "device": the device's ID, must be unique (json-string)
> +- "option": hostcache setting (json-bool)
> +
> +Example:
> +->  { "execute": "block_set_hostcache", "arguments": { "device": "ide0-hd0", "option": false } }
> +<- { "return": {} }
> +
> +EQMP
> +
> +	{
>           .name       = "balloon",
>           .args_type  = "value:M",
>           .mhandler.cmd_new = qmp_marshal_input_balloon,
>
>