Re: [Qemu-devel] [v1 Patch 3/10]Qemu: Cmd "block_set_hostcache" for dynamic cache change

[Eric Blake]

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 <address@hidden>
> 
> ---
>  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   address@hidden    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature