qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [V2 2/2]Qemu: Add commands "hostcache_set" and "hostcac


From: Stefan Hajnoczi
Subject: Re: [Qemu-devel] [V2 2/2]Qemu: Add commands "hostcache_set" and "hostcache_get"
Date: Fri, 20 May 2011 09:20:33 +0100
User-agent: Mutt/1.5.21 (2010-09-15)

On Thu, May 19, 2011 at 10:38:03PM +0530, Supriya Kannery wrote:
> Monitor commands "hostcache_set" and "hostcache_get" added for dynamic
> host cache change and display of host cache setting respectively.

A generic command for changing block device options would be nice,
althought I don't see other options where it makes sense to change them
at runtime.

The alternative would be:

block_set hostcache on

"block_set", {"device": "ide1-cd0", "name": "hostcache", "enable": true}

The hostcache_get information would be part of query-block output:
         {
            "device":"ide0-hd0",
            "locked":false,
            "removable":false,
            "inserted":{
               "ro":false,
               "drv":"qcow2",
               "encrypted":false,
               "file":"disks/test.img"
               "hostcache":true,
            },
            "type":"hd"
         },

This approach is extensible if more options need to be exposed.

> Signed-off-by: Supriya Kannery <address@hidden>
> 
> ---
>  block.c         |   48 ++++++++++++++++++++++++++++++++++++++++++++++++
>  block.h         |    2 ++
>  blockdev.c      |   48 ++++++++++++++++++++++++++++++++++++++++++++++++
>  blockdev.h      |    2 ++
>  hmp-commands.hx |   29 +++++++++++++++++++++++++++++
>  qmp-commands.hx |   55 
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  6 files changed, 184 insertions(+)
> 
> Index: qemu/hmp-commands.hx
> ===================================================================
> --- qemu.orig/hmp-commands.hx
> +++ qemu/hmp-commands.hx
> @@ -70,6 +70,35 @@ but should be used with extreme caution.
>  resizes image files, it can not resize block devices like LVM volumes.
>  ETEXI
>  
> +    {
> +        .name       = "hostcache_get",
> +        .args_type  = "device:B",
> +        .params     = "device",
> +        .help       = "retrieve host cache settings for device",

Please make it clear these operations affect block devices:
"for block device"

> +        .user_print = monitor_user_noop,
> +        .mhandler.cmd_new = do_hostcache_get,
> +    },
> +
> +STEXI
> address@hidden hostcache_get
> address@hidden hostcache_get
> +Display host cache settings for a block device while guest is running.
> +ETEXI
> +
> +    {
> +        .name       = "hostcache_set",
> +        .args_type  = "device:B,hostcache:s",
> +        .params     = "device hostcache",
> +        .help       = "change host cache setting for device",
> +        .user_print = monitor_user_noop,
> +        .mhandler.cmd_new = do_hostcache_set,
> +    },
> +
> +STEXI
> address@hidden hostcache_set
> address@hidden hostcache_set
> +Change host cache options for a block device while guest is running.
> +ETEXI
>  
>      {
>          .name       = "eject",
> Index: qemu/block.c
> ===================================================================
> --- qemu.orig/block.c
> +++ qemu/block.c
> @@ -657,6 +657,34 @@ unlink_and_fail:
>      return ret;
>  }
>  
> +int bdrv_reopen(BlockDriverState *bs, int bdrv_flags)
> +{
> +    BlockDriver *drv = bs->drv;
> +    int ret = 0;
> +
> +    /* No need to reopen as no change in flags */
> +    if (bdrv_flags == bs->open_flags) {
> +        return 0;
> +    }
> +
> +    /* Quiesce IO for the given block device */
> +    qemu_aio_flush();
> +    bdrv_flush(bs);
> +
> +    bdrv_close(bs);
> +    ret = bdrv_open(bs, bs->filename, bdrv_flags, drv);
> +
> +    /*
> +     * A failed attempt to reopen the image file must lead to 'abort()'
> +     */
> +    if (ret != 0) {
> +        qerror_report(QERR_REOPEN_FILE_FAILED, bs->filename);
> +        abort();

The error is never reported on a QMP monitor because qerror_report()
simply stashes away the qerror.  The QMP client doesn't have a chance to
read the error before QEMU terminates.

> +    }
> +
> +    return ret;
> +}
> +
>  void bdrv_close(BlockDriverState *bs)
>  {
>      if (bs->drv) {
> @@ -3049,3 +3077,23 @@ out:
>  
>      return ret;
>  }
> +
> +int bdrv_change_hostcache(BlockDriverState *bs, bool enable_host_cache)

Consistently using "hostcache" or "host_cache" would be nice.

> +{
> +    int bdrv_flags = bs->open_flags;
> +
> +    /* No change in existing hostcache setting */
> +    if(!enable_host_cache == (bdrv_flags & BDRV_O_NOCACHE)) {

This expression doesn't work as expected.  bool has a lower rank than
int.  That means !enable_host_cache is converted to an int and compared
against bdrv_flags & BDRV_O_NOCACHE.  This expression is always false
because a bool is 0 or 1 and BDRV_O_NOCACHE is 0x0020.

> +        return -1;

This shouldn't be a failure and please don't use -1 when a negative
errno indicates failure.  -1 == -EPERM.  The return value should be 0
here.

> +    }

Anyway, this whole check is unnecessary since bdrv_reopen() already
performs it.

> +
> +    /* set hostcache flags (without changing WCE/flush bits) */
> +    if(!enable_host_cache) {
> +        bdrv_flags |= BDRV_O_NOCACHE;
> +    } else {
> +        bdrv_flags &= ~BDRV_O_NOCACHE;
> +    }
> +
> +    /* Reopen file with changed set of flags */
> +    return(bdrv_reopen(bs, bdrv_flags));

Please run scripts/checkpatch.pl before submitting patches.

> +}
> Index: qemu/blockdev.c
> ===================================================================
> --- qemu.orig/blockdev.c
> +++ qemu/blockdev.c
> @@ -796,3 +796,51 @@ int do_block_resize(Monitor *mon, const 
>  
>      return 0;
>  }
> +
> +int do_hostcache_get(Monitor *mon, const QDict *qdict, QObject **ret_data)
> +{
> +    const char *device = qdict_get_str(qdict, "device");
> +    BlockDriverState *bs;
> +
> +    bs = bdrv_find(device);
> +    if (!bs) {
> +        qerror_report(QERR_DEVICE_NOT_FOUND, device);
> +        return -1;
> +    }
> +
> +    monitor_printf(mon, "%s:", device);
> +
> +    if (bs->open_flags & BDRV_O_NOCACHE) {
> +        monitor_printf(mon, " hostcache=off\n");
> +    } else {
> +        monitor_printf(mon, " hostcache=on\n");
> +    }
> +
> +    return 0;
> +}
> +
> +int do_hostcache_set(Monitor *mon, const QDict *qdict, QObject **ret_data)
> +{
> +    const char *device = qdict_get_str(qdict, "device");
> +    const char *hostcache = qdict_get_str(qdict, "hostcache");
> +    BlockDriverState *bs;
> +    bool enable_host_cache = 0;
> +
> +    bs = bdrv_find(device);
> +    if (!bs) {
> +        qerror_report(QERR_DEVICE_NOT_FOUND, device);
> +        return -1;
> +    }
> +
> +    /* cache change applicable only if device inserted */
> +    if (bdrv_is_inserted(bs)) {
> +        if (!strcmp(hostcache,"on"))
> +            enable_host_cache = 1;
> +        else
> +            enable_host_cache = 0;

Coding style, please use {} or remove the if:
enable_host_cache = !strcmp(hostcache, "on");

> +        return(bdrv_change_hostcache(bs, enable_host_cache));
> +    } else {
> +       qerror_report(QERR_DEVICE_NOT_INSERTED, device);
> +       return -1;
> +    }
> +}
> Index: qemu/block.h
> ===================================================================
> --- qemu.orig/block.h
> +++ qemu/block.h
> @@ -71,6 +71,7 @@ void bdrv_delete(BlockDriverState *bs);
>  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(BlockDriverState *bs, DeviceState *qdev);
>  void bdrv_detach(BlockDriverState *bs, DeviceState *qdev);
> @@ -96,6 +97,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.h
> ===================================================================
> --- qemu.orig/blockdev.h
> +++ qemu/blockdev.h
> @@ -64,5 +64,7 @@ int do_change_block(Monitor *mon, const 
>  int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data);
>  int do_snapshot_blkdev(Monitor *mon, const QDict *qdict, QObject **ret_data);
>  int do_block_resize(Monitor *mon, const QDict *qdict, QObject **ret_data);
> +int do_hostcache_get(Monitor *mon, const QDict *qdict, QObject **ret_data);
> +int do_hostcache_set(Monitor *mon, const QDict *qdict, QObject **ret_data);
>  
>  #endif
> Index: qemu/qmp-commands.hx
> ===================================================================
> --- qemu.orig/qmp-commands.hx
> +++ qemu/qmp-commands.hx
> @@ -664,6 +664,61 @@ Example:
>  -> { "execute": "block_resize", "arguments": { "device": "scratch", "size": 
> 1073741824 } }
>  <- { "return": {} }
>  
> +
> +EQMP
> +
> +    {
> +        .name       = "hostcache_get",
> +        .args_type  = "device:B",
> +        .params     = "device",
> +        .help       = "retrieve host cache settings for device",
> +        .user_print = monitor_user_noop,
> +        .mhandler.cmd_new = do_hostcache_get,
> +    },
> +SQMP
> +cache_get

hostcache_get

> +---------
> +
> +Retrieve host page cache setting while a guest is running.

Please make it clear that this is an operation on block devices.

> +
> +Arguments:
> +
> +- "device": the device's ID, must be unique (json-string)
> +
> +Example:
> +
> +-> { "execute": "hostcache_set", "arguments": { "device": "ide0-hd0" } }
> +<- { "return": {} }
> +
> +
> +EQMP
> +
> +    {
> +        .name       = "hostcache_set",
> +        .args_type  = "device:B,cache:s",
> +        .params     = "device cache",
> +        .help       = "change hostcache setting for device",
> +        .user_print = monitor_user_noop,
> +        .mhandler.cmd_new = do_hostcache_set,
> +    },
> +
> +SQMP
> +hostcache_set
> +-------------
> +
> +Change host page cache setting while a guest is running.
> +
> +Arguments:
> +
> +- "device": the device's ID, must be unique (json-string)
> +- "cache": host cache value "off" or "on" (json-string)

There is a boolean value that can be used instead of string on|off.  See
the set_link command.

Stefan



reply via email to

[Prev in Thread] Current Thread [Next in Thread]