Patchwork [v1,3/10] Qemu: Cmd "block_set_hostcache" for dynamic cache change

login
register
mail settings
Submitter Supriya Kannery
Date June 15, 2012, 8:47 p.m.
Message ID <20120615204730.9853.61858.sendpatchset@skannery.in.ibm.com>
Download mbox | patch
Permalink /patch/165220/
State New
Headers show

Comments

Supriya Kannery - June 15, 2012, 8:47 p.m.
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(+)
Eric Blake - June 15, 2012, 9:56 p.m.
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)?
Jeff Cody - June 20, 2012, 6:18 p.m.
On 06/15/2012 04: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(+)
> 
> 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);
> +    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();
> +        }
> +    }
> +
> +    return ret;
> +}
> +
>  void bdrv_close(BlockDriverState *bs)
>  {
>      bdrv_flush(bs);
> @@ -953,6 +982,34 @@ void bdrv_drain_all(void)
>      }
>  }
>  
> +int bdrv_change_hostcache(BlockDriverState *bs, bool enable)
> +{
> +    int bdrv_flags = bs->open_flags;
> +
> +    /* set hostcache flags (without changing WCE/flush bits) */
> +    if (enable) {
> +        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) {
> +        printf("no need to change\n");
> +        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);
> +    } 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
> @@ -128,6 +128,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);
> @@ -177,6 +178,7 @@ int 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
> @@ -1198,3 +1198,27 @@ 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.
> +*/
> +void qmp_block_set_hostcache(const char *device, bool enable,
> +                             Error **errp)
> +{
> +    BlockDriverState *bs = NULL;
> +
> +    /* Validate device */
> +    bs = bdrv_find(device);
> +    if (!bs) {
> +        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
> +        return;
> +    }
> +
> +    if (bdrv_change_hostcache(bs, enable)) {
> +        error_set(errp, QERR_HOSTCACHE_NOT_CHANGED, device);
> +    }
> +
> +    return;
> +}
> +
> Index: qemu/hmp-commands.hx
> ===================================================================
> --- qemu.orig/hmp-commands.hx
> +++ qemu/hmp-commands.hx
> @@ -808,7 +808,6 @@ ETEXI
>          .mhandler.cmd = hmp_migrate,
>      },
>  
> -
>  STEXI
>  @item migrate [-d] [-b] [-i] @var{uri}
>  @findex migrate
> @@ -1314,6 +1313,21 @@ client.  @var{keep} changes the password
>  ETEXI
>  
>      {
> +        .name       = "block_set_hostcache",
> +        .args_type  = "device:B,option:b",
> +        .params     = "device on|off",
> +        .help       = "Change setting of host pagecache",
> +        .mhandler.cmd = hmp_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
> +
> +
> +    {
>          .name       = "expire_password",
>          .args_type  = "protocol:s,time:s",
>          .params     = "protocol time",
> Index: qemu/qmp-commands.hx
> ===================================================================
> --- qemu.orig/qmp-commands.hx
> +++ qemu/qmp-commands.hx
> @@ -821,7 +821,31 @@ Example:
>  
>  EQMP
>  
> +
>      {
> +        .name       = "block_set_hostcache",
> +        .args_type  = "device:B,option:b",
> +        .mhandler.cmd_new = qmp_block_set_hostcache,

The QAPI commands need to go through the marshaller - this needs to be:
           .mhandler.cmd_new = qmp_marshal_input_block_set_hostcache,

(qmp_marshal_input_block_set_hostcache is automatically generated, and
it will call qmp_block_set_hostcache)

> +    },
> +
> +SQMP
> +block_set_hostcache
> +-------------------
> +
> +Change host pagecache setting of a block device
> +
> +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,
> 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
> +#
> +# Since: 1.2
> +##
> +{ 'command': 'block_set_hostcache',
> +  'data': { 'device': 'str', 'option': 'bool' } }
>  
>  ##
>  # @block-stream:
> Index: qemu/hmp.c
> ===================================================================
> --- qemu.orig/hmp.c
> +++ qemu/hmp.c
> @@ -836,6 +836,15 @@ void hmp_block_set_io_throttle(Monitor *
>      hmp_handle_error(mon, &err);
>  }
>  
> +void hmp_block_set_hostcache(Monitor *mon, const QDict *qdict)
> +{
> +    Error *err = NULL;
> +
> +    qmp_block_set_hostcache(qdict_get_str(qdict, "device"),
> +                              qdict_get_bool(qdict, "option"), &err);
> +    hmp_handle_error(mon, &err);
> +}
> +
>  void hmp_block_stream(Monitor *mon, const QDict *qdict)
>  {
>      Error *error = NULL;
> Index: qemu/hmp.h
> ===================================================================
> --- qemu.orig/hmp.h
> +++ qemu/hmp.h
> @@ -64,5 +64,6 @@ void hmp_device_del(Monitor *mon, const 
>  void hmp_dump_guest_memory(Monitor *mon, const QDict *qdict);
>  void hmp_netdev_add(Monitor *mon, const QDict *qdict);
>  void hmp_netdev_del(Monitor *mon, const QDict *qdict);
> +void hmp_block_set_hostcache(Monitor *mon, const QDict *qdict);
>  
>  #endif
> 
>
Kevin Wolf - July 4, 2012, 6:30 a.m.
Am 04.07.2012 07:10, schrieb Shrinidhi Joshi:
> Updated patch to auto generate qmp_marshal_input_block_set_hostcache
> 
> --------------------------------------------------------------------
> 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>
> 
> Signed-off-by: Shrinidhi Joshi <spjoshi31@gmail.com>

Please use git-send-email for sending patches. A line-wrapped
HTML-formatted patch like this cannot be applied and is useless.

Kevin
Kevin Wolf - July 9, 2012, 2:52 p.m.
Am 15.06.2012 22:47, schrieb Supriya Kannery:
> 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
> @@ -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();

bdrv_drain_all() should be used instead.

> +    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_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();

Like Eric said, committing a broken version and then fixing it later in
the series isn't really nice. At least bs->drv = NULL; instead of
abort() is required. Starting with the real thing is probably even better.

Kevin
Luiz Capitulino - July 11, 2012, 2:16 p.m.
On Sat, 16 Jun 2012 02:17:30 +0530
Supriya Kannery <supriyak@linux.vnet.ibm.com> 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
> @@ -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);

Please, use error_set() instead of qerror_report() and as Kevin said IOError
seems enough here.

> +        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_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();
> +        }
> +    }
> +
> +    return ret;
> +}
> +
>  void bdrv_close(BlockDriverState *bs)
>  {
>      bdrv_flush(bs);
> @@ -953,6 +982,34 @@ void bdrv_drain_all(void)
>      }
>  }
>  
> +int bdrv_change_hostcache(BlockDriverState *bs, bool enable)
> +{
> +    int bdrv_flags = bs->open_flags;
> +
> +    /* set hostcache flags (without changing WCE/flush bits) */
> +    if (enable) {
> +        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) {
> +        printf("no need to change\n");
> +        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);
> +    } 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
> @@ -128,6 +128,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);
> @@ -177,6 +178,7 @@ int 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
> @@ -1198,3 +1198,27 @@ 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.
> +*/
> +void qmp_block_set_hostcache(const char *device, bool enable,
> +                             Error **errp)
> +{
> +    BlockDriverState *bs = NULL;
> +
> +    /* Validate device */
> +    bs = bdrv_find(device);
> +    if (!bs) {
> +        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
> +        return;
> +    }
> +
> +    if (bdrv_change_hostcache(bs, enable)) {
> +        error_set(errp, QERR_HOSTCACHE_NOT_CHANGED, device);
> +    }
> +
> +    return;
> +}
> +
> Index: qemu/hmp-commands.hx
> ===================================================================
> --- qemu.orig/hmp-commands.hx
> +++ qemu/hmp-commands.hx
> @@ -808,7 +808,6 @@ ETEXI
>          .mhandler.cmd = hmp_migrate,
>      },
>  
> -
>  STEXI
>  @item migrate [-d] [-b] [-i] @var{uri}
>  @findex migrate
> @@ -1314,6 +1313,21 @@ client.  @var{keep} changes the password
>  ETEXI
>  
>      {
> +        .name       = "block_set_hostcache",
> +        .args_type  = "device:B,option:b",
> +        .params     = "device on|off",
> +        .help       = "Change setting of host pagecache",
> +        .mhandler.cmd = hmp_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
> +
> +
> +    {
>          .name       = "expire_password",
>          .args_type  = "protocol:s,time:s",
>          .params     = "protocol time",
> Index: qemu/qmp-commands.hx
> ===================================================================
> --- qemu.orig/qmp-commands.hx
> +++ qemu/qmp-commands.hx
> @@ -821,7 +821,31 @@ Example:
>  
>  EQMP
>  
> +
>      {
> +        .name       = "block_set_hostcache",
> +        .args_type  = "device:B,option:b",
> +        .mhandler.cmd_new = qmp_block_set_hostcache,
> +    },
> +
> +SQMP
> +block_set_hostcache
> +-------------------
> +
> +Change host pagecache setting of a block device
> +
> +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,
> 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
> +#
> +# Since: 1.2
> +##
> +{ 'command': 'block_set_hostcache',
> +  'data': { 'device': 'str', 'option': 'bool' } }
>  
>  ##
>  # @block-stream:
> Index: qemu/hmp.c
> ===================================================================
> --- qemu.orig/hmp.c
> +++ qemu/hmp.c
> @@ -836,6 +836,15 @@ void hmp_block_set_io_throttle(Monitor *
>      hmp_handle_error(mon, &err);
>  }
>  
> +void hmp_block_set_hostcache(Monitor *mon, const QDict *qdict)
> +{
> +    Error *err = NULL;
> +
> +    qmp_block_set_hostcache(qdict_get_str(qdict, "device"),
> +                              qdict_get_bool(qdict, "option"), &err);
> +    hmp_handle_error(mon, &err);
> +}
> +
>  void hmp_block_stream(Monitor *mon, const QDict *qdict)
>  {
>      Error *error = NULL;
> Index: qemu/hmp.h
> ===================================================================
> --- qemu.orig/hmp.h
> +++ qemu/hmp.h
> @@ -64,5 +64,6 @@ void hmp_device_del(Monitor *mon, const 
>  void hmp_dump_guest_memory(Monitor *mon, const QDict *qdict);
>  void hmp_netdev_add(Monitor *mon, const QDict *qdict);
>  void hmp_netdev_del(Monitor *mon, const QDict *qdict);
> +void hmp_block_set_hostcache(Monitor *mon, const QDict *qdict);
>  
>  #endif
>
Supriya Kannery - July 29, 2012, 7:33 a.m.
On 06/16/2012 03:26 AM, Eric Blake wrote:
> 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.
>
>

  Yes, will reorder the patches to have safe reopen done first and
then block_set_hostcache use it.

-thanks, Supriya
Supriya Kannery - July 29, 2012, 7:56 a.m.
On 07/11/2012 07:46 PM, Luiz Capitulino wrote:
> On Sat, 16 Jun 2012 02:17:30 +0530
> Supriya Kannery<supriyak@linux.vnet.ibm.com>  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
>> @@ -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);
>
> Please, use error_set() instead of qerror_report() and as Kevin said IOError
> seems enough here.
>

Sure, will replace qemu_aio_flush() with bdrv_drain_all()
and qerror_report() with error_set().

Combination of bdrv_drain_all() and bdrv_flush(), should
help in flushing out the specific disk.

-thanks, Supriya

Patch

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);
+    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();
+        }
+    }
+
+    return ret;
+}
+
 void bdrv_close(BlockDriverState *bs)
 {
     bdrv_flush(bs);
@@ -953,6 +982,34 @@  void bdrv_drain_all(void)
     }
 }
 
+int bdrv_change_hostcache(BlockDriverState *bs, bool enable)
+{
+    int bdrv_flags = bs->open_flags;
+
+    /* set hostcache flags (without changing WCE/flush bits) */
+    if (enable) {
+        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) {
+        printf("no need to change\n");
+        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);
+    } 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
@@ -128,6 +128,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);
@@ -177,6 +178,7 @@  int 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
@@ -1198,3 +1198,27 @@  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.
+*/
+void qmp_block_set_hostcache(const char *device, bool enable,
+                             Error **errp)
+{
+    BlockDriverState *bs = NULL;
+
+    /* Validate device */
+    bs = bdrv_find(device);
+    if (!bs) {
+        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
+        return;
+    }
+
+    if (bdrv_change_hostcache(bs, enable)) {
+        error_set(errp, QERR_HOSTCACHE_NOT_CHANGED, device);
+    }
+
+    return;
+}
+
Index: qemu/hmp-commands.hx
===================================================================
--- qemu.orig/hmp-commands.hx
+++ qemu/hmp-commands.hx
@@ -808,7 +808,6 @@  ETEXI
         .mhandler.cmd = hmp_migrate,
     },
 
-
 STEXI
 @item migrate [-d] [-b] [-i] @var{uri}
 @findex migrate
@@ -1314,6 +1313,21 @@  client.  @var{keep} changes the password
 ETEXI
 
     {
+        .name       = "block_set_hostcache",
+        .args_type  = "device:B,option:b",
+        .params     = "device on|off",
+        .help       = "Change setting of host pagecache",
+        .mhandler.cmd = hmp_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
+
+
+    {
         .name       = "expire_password",
         .args_type  = "protocol:s,time:s",
         .params     = "protocol time",
Index: qemu/qmp-commands.hx
===================================================================
--- qemu.orig/qmp-commands.hx
+++ qemu/qmp-commands.hx
@@ -821,7 +821,31 @@  Example:
 
 EQMP
 
+
     {
+        .name       = "block_set_hostcache",
+        .args_type  = "device:B,option:b",
+        .mhandler.cmd_new = qmp_block_set_hostcache,
+    },
+
+SQMP
+block_set_hostcache
+-------------------
+
+Change host pagecache setting of a block device
+
+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,
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
+#
+# Since: 1.2
+##
+{ 'command': 'block_set_hostcache',
+  'data': { 'device': 'str', 'option': 'bool' } }
 
 ##
 # @block-stream:
Index: qemu/hmp.c
===================================================================
--- qemu.orig/hmp.c
+++ qemu/hmp.c
@@ -836,6 +836,15 @@  void hmp_block_set_io_throttle(Monitor *
     hmp_handle_error(mon, &err);
 }
 
+void hmp_block_set_hostcache(Monitor *mon, const QDict *qdict)
+{
+    Error *err = NULL;
+
+    qmp_block_set_hostcache(qdict_get_str(qdict, "device"),
+                              qdict_get_bool(qdict, "option"), &err);
+    hmp_handle_error(mon, &err);
+}
+
 void hmp_block_stream(Monitor *mon, const QDict *qdict)
 {
     Error *error = NULL;
Index: qemu/hmp.h
===================================================================
--- qemu.orig/hmp.h
+++ qemu/hmp.h
@@ -64,5 +64,6 @@  void hmp_device_del(Monitor *mon, const 
 void hmp_dump_guest_memory(Monitor *mon, const QDict *qdict);
 void hmp_netdev_add(Monitor *mon, const QDict *qdict);
 void hmp_netdev_del(Monitor *mon, const QDict *qdict);
+void hmp_block_set_hostcache(Monitor *mon, const QDict *qdict);
 
 #endif