| Message ID | 20120615204730.9853.61858.sendpatchset@skannery.in.ibm.com |
|---|---|
| State | New |
| Headers | show |
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)?
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 > >
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
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
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 >
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
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
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
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(+)