From: Eric Blake <eblake@redhat.com>
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>
Subject: Re: [Qemu-devel] [v1 Patch 3/10]Qemu: Cmd "block_set_hostcache" for dynamic cache change
Date: Fri, 15 Jun 2012 15:56:54 -0600 [thread overview]
Message-ID: <4FDBAFA6.7060601@redhat.com> (raw)
In-Reply-To: <20120615204730.9853.61858.sendpatchset@skannery.in.ibm.com>
[-- Attachment #1: Type: text/plain, Size: 3548 bytes --]
On 06/15/2012 02:47 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(+)
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.
>
> Index: qemu/block.c
> ===================================================================
> --- qemu.orig/block.c
> +++ qemu/block.c
> @@ -858,6 +858,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);
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 = 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 = hmp_migrate,
> },
>
> -
> STEXI
Spurious whitespace change.
> +
> +SQMP
> +block_set_hostcache
QMP commands favor '-' over '_'; this should be 'block-set-hostcache'.
> Index: qemu/qapi-schema.json
> ===================================================================
> --- 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)?
--
Eric Blake eblake@redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 620 bytes --]
next prev parent reply other threads:[~2012-06-15 21:57 UTC|newest]
Thread overview: 33+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-06-15 20:46 [Qemu-devel] [v1 Patch 0/10]Qemu: Dynamic host pagecache change and image file reopen Supriya Kannery
2012-06-15 20:47 ` [Qemu-devel] [v1 Patch 1/10]Qemu: Enhance "info block" to display host cache setting Supriya Kannery
2012-06-15 21:07 ` Eric Blake
2012-07-09 14:43 ` Kevin Wolf
2012-07-11 14:03 ` Luiz Capitulino
2012-07-29 6:21 ` Supriya Kannery
2012-07-05 16:38 ` Jeff Cody
2012-07-29 6:54 ` Supriya Kannery
2012-06-15 20:47 ` [Qemu-devel] [v1 Patch 2/10]Qemu: Error classes for hostcache setting and data sync failures Supriya Kannery
2012-07-09 14:47 ` Kevin Wolf
2012-07-29 6:58 ` Supriya Kannery
2012-06-15 20:47 ` [Qemu-devel] [v1 Patch 3/10]Qemu: Cmd "block_set_hostcache" for dynamic cache change Supriya Kannery
2012-06-15 21:56 ` Eric Blake [this message]
2012-07-29 7:33 ` Supriya Kannery
2012-06-20 18:18 ` Jeff Cody
2012-07-04 5:10 ` Shrinidhi Joshi
2012-07-04 6:30 ` Kevin Wolf
2012-07-09 14:52 ` Kevin Wolf
2012-07-11 14:16 ` Luiz Capitulino
2012-07-29 7:56 ` Supriya Kannery
2012-06-15 20:47 ` [Qemu-devel] [v1 Patch 4/10]Qemu: Framework for reopening image files safely Supriya Kannery
2012-06-15 22:02 ` Eric Blake
2012-07-09 15:06 ` Kevin Wolf
2012-06-15 20:48 ` [Qemu-devel] [v1 Patch 5/10]Qemu: raw-posix image file reopen Supriya Kannery
2012-06-15 22:11 ` Eric Blake
2012-07-04 5:15 ` Shrinidhi Joshi
2012-07-04 11:32 ` Eric Blake
2012-06-15 20:48 ` [Qemu-devel] [v1 Patch 6/10]Qemu: raw-win32 " Supriya Kannery
2012-06-15 20:48 ` [Qemu-devel] [v1 Patch 7/10]Qemu: vmdk " Supriya Kannery
2012-06-15 20:48 ` [Qemu-devel] [v1 Patch 8/10]Qemu: qcow2 " Supriya Kannery
2012-06-15 20:48 ` [Qemu-devel] [v1 Patch 9/10]Qemu: qcow " Supriya Kannery
2012-06-15 20:49 ` [Qemu-devel] [v1 Patch 10/10]Qemu: qed " Supriya Kannery
2012-07-09 17:51 ` [Qemu-devel] [v1 Patch 0/10]Qemu: Dynamic host pagecache change and " Stefan Weil
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4FDBAFA6.7060601@redhat.com \
--to=eblake@redhat.com \
--cc=hch@lst.de \
--cc=jcody@redhat.com \
--cc=kwolf@redhat.com \
--cc=lcapitulino@redhat.com \
--cc=qemu-devel@nongnu.org \
--cc=spjoshi31@gmail.com \
--cc=stefanha@gmail.com \
--cc=supriyak@linux.vnet.ibm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.