* [Qemu-devel] [v7 Patch 1/5]Qemu: Enhance "info block" to display host cache setting
2011-10-11 3:10 [Qemu-devel] [v7 Patch 0/5]Qemu: Host pagecache setting from cmdline and monitor Supriya Kannery
@ 2011-10-11 3:10 ` Supriya Kannery
2011-10-12 14:17 ` Kevin Wolf
2011-10-11 3:11 ` [Qemu-devel] [v7 Patch 2/5]Qemu: Error classes for file reopen and data sync failure Supriya Kannery
` (3 subsequent siblings)
4 siblings, 1 reply; 16+ messages in thread
From: Supriya Kannery @ 2011-10-11 3:10 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, Stefan Hajnoczi, Christoph Hellwig
Enhance "info block" to display hostcache setting for each
block device.
Example:
(qemu) info block
ide0-hd0: removable=0 file=../sles11-32.raw ro=0 drv=raw encrypted=0
Enhanced to display "hostcache" setting:
(qemu) info block
ide0-hd0: removable=0 hostcache=1 file=../sles11-32.raw ro=0 drv=raw encrypted=0
Signed-off-by: Supriya Kannery <supriyak@linux.vnet.ibm.com>
---
block.c | 20 ++++++++++++++++----
qmp-commands.hx | 2 ++
2 files changed, 18 insertions(+), 4 deletions(-)
Index: qemu/qmp-commands.hx
===================================================================
--- qemu.orig/qmp-commands.hx
+++ qemu/qmp-commands.hx
@@ -1142,6 +1142,7 @@ Each json-object contain the following:
- "locked": true if the device is locked, false otherwise (json-bool)
- "tray-open": only present if removable, true if the device has a tray,
and it is open (json-bool)
+- "hostcache": true if host pagecache enabled, false otherwise (json-bool)
- "inserted": only present if the device is inserted, it is a json-object
containing the following:
- "file": device file name (json-string)
@@ -1163,6 +1164,7 @@ Example:
{
"device":"ide0-hd0",
"locked":false,
+ "hostcache":false,
"removable":false,
"inserted":{
"ro":false,
Index: qemu/block.c
===================================================================
--- qemu.orig/block.c
+++ qemu/block.c
@@ -1866,6 +1866,15 @@ static void bdrv_print_dict(QObject *obj
monitor_printf(mon, " tray-open=%d",
qdict_get_bool(bs_dict, "tray-open"));
}
+
+ if (qdict_haskey(bs_dict, "open_flags")) {
+ int open_flags = qdict_get_int(bs_dict, "open_flags");
+ if (open_flags & BDRV_O_NOCACHE)
+ monitor_printf(mon, " hostcache=0");
+ else
+ monitor_printf(mon, " hostcache=1");
+ }
+
if (qdict_haskey(bs_dict, "inserted")) {
QDict *qdict = qobject_to_qdict(qdict_get(bs_dict, "inserted"));
@@ -1903,11 +1912,14 @@ void bdrv_info(Monitor *mon, QObject **r
QDict *bs_dict;
bs_obj = qobject_from_jsonf("{ 'device': %s, 'type': 'unknown', "
- "'removable': %i, 'locked': %i }",
+ "'removable': %i, 'locked': %i, "
+ "'hostcache': %i }",
bs->device_name,
bdrv_dev_has_removable_media(bs),
- bdrv_dev_is_medium_locked(bs));
+ bdrv_dev_is_medium_locked(bs),
+ !(bs->open_flags & BDRV_O_NOCACHE));
bs_dict = qobject_to_qdict(bs_obj);
+ qdict_put(bs_dict, "open_flags", qint_from_int(bs->open_flags));
if (bdrv_dev_has_removable_media(bs)) {
qdict_put(bs_dict, "tray-open",
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [Qemu-devel] [v7 Patch 1/5]Qemu: Enhance "info block" to display host cache setting
2011-10-11 3:10 ` [Qemu-devel] [v7 Patch 1/5]Qemu: Enhance "info block" to display host cache setting Supriya Kannery
@ 2011-10-12 14:17 ` Kevin Wolf
2011-10-14 10:47 ` Supriya Kannery
0 siblings, 1 reply; 16+ messages in thread
From: Kevin Wolf @ 2011-10-12 14:17 UTC (permalink / raw)
To: Supriya Kannery; +Cc: Stefan Hajnoczi, qemu-devel, Christoph Hellwig
Am 11.10.2011 05:10, schrieb Supriya Kannery:
> Enhance "info block" to display hostcache setting for each
> block device.
>
> Example:
> (qemu) info block
> ide0-hd0: removable=0 file=../sles11-32.raw ro=0 drv=raw encrypted=0
>
> Enhanced to display "hostcache" setting:
> (qemu) info block
> ide0-hd0: removable=0 hostcache=1 file=../sles11-32.raw ro=0 drv=raw encrypted=0
>
> Signed-off-by: Supriya Kannery <supriyak@linux.vnet.ibm.com>
>
> ---
> block.c | 20 ++++++++++++++++----
> qmp-commands.hx | 2 ++
> 2 files changed, 18 insertions(+), 4 deletions(-)
>
> Index: qemu/qmp-commands.hx
> ===================================================================
> --- qemu.orig/qmp-commands.hx
> +++ qemu/qmp-commands.hx
> @@ -1142,6 +1142,7 @@ Each json-object contain the following:
> - "locked": true if the device is locked, false otherwise (json-bool)
> - "tray-open": only present if removable, true if the device has a tray,
> and it is open (json-bool)
> +- "hostcache": true if host pagecache enabled, false otherwise (json-bool)
> - "inserted": only present if the device is inserted, it is a json-object
> containing the following:
> - "file": device file name (json-string)
> @@ -1163,6 +1164,7 @@ Example:
> {
> "device":"ide0-hd0",
> "locked":false,
> + "hostcache":false,
> "removable":false,
> "inserted":{
> "ro":false,
> Index: qemu/block.c
> ===================================================================
> --- qemu.orig/block.c
> +++ qemu/block.c
> @@ -1866,6 +1866,15 @@ static void bdrv_print_dict(QObject *obj
> monitor_printf(mon, " tray-open=%d",
> qdict_get_bool(bs_dict, "tray-open"));
> }
> +
> + if (qdict_haskey(bs_dict, "open_flags")) {
> + int open_flags = qdict_get_int(bs_dict, "open_flags");
> + if (open_flags & BDRV_O_NOCACHE)
> + monitor_printf(mon, " hostcache=0");
> + else
> + monitor_printf(mon, " hostcache=1");
Coding style requires braces.
> + }
> +
> if (qdict_haskey(bs_dict, "inserted")) {
> QDict *qdict = qobject_to_qdict(qdict_get(bs_dict, "inserted"));
>
> @@ -1903,11 +1912,14 @@ void bdrv_info(Monitor *mon, QObject **r
> QDict *bs_dict;
>
> bs_obj = qobject_from_jsonf("{ 'device': %s, 'type': 'unknown', "
> - "'removable': %i, 'locked': %i }",
> + "'removable': %i, 'locked': %i, "
> + "'hostcache': %i }",
> bs->device_name,
> bdrv_dev_has_removable_media(bs),
> - bdrv_dev_is_medium_locked(bs));
> + bdrv_dev_is_medium_locked(bs),
> + !(bs->open_flags & BDRV_O_NOCACHE));
> bs_dict = qobject_to_qdict(bs_obj);
> + qdict_put(bs_dict, "open_flags", qint_from_int(bs->open_flags));
No. This adds a open_flags field to the QMP structure that is
transferred to clients. This is wrong, open_flags is an internal thing
that should never be visible on an interface.
In bdrv_print_dict, access the hostcache field that you introduced, it
provides the same information.
Kevin
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [Qemu-devel] [v7 Patch 1/5]Qemu: Enhance "info block" to display host cache setting
2011-10-12 14:17 ` Kevin Wolf
@ 2011-10-14 10:47 ` Supriya Kannery
0 siblings, 0 replies; 16+ messages in thread
From: Supriya Kannery @ 2011-10-14 10:47 UTC (permalink / raw)
To: Kevin Wolf; +Cc: Stefan Hajnoczi, qemu-devel, Christoph Hellwig
On 10/12/2011 07:47 PM, Kevin Wolf wrote:
> Am 11.10.2011 05:10, schrieb Supriya Kannery:
>> Enhance "info block" to display hostcache setting for each
>> block device.
>> + if (qdict_haskey(bs_dict, "open_flags")) {
>> + int open_flags = qdict_get_int(bs_dict, "open_flags");
>> + if (open_flags& BDRV_O_NOCACHE)
>> + monitor_printf(mon, " hostcache=0");
>> + else
>> + monitor_printf(mon, " hostcache=1");
>
> Coding style requires braces.
>
ok..will add. checkpatch.pl didn't catch this!
>>
>> bs_obj = qobject_from_jsonf("{ 'device': %s, 'type': 'unknown', "
>> - "'removable': %i, 'locked': %i }",
>> + "'removable': %i, 'locked': %i, "
>> + "'hostcache': %i }",
>> bs->device_name,
>> bdrv_dev_has_removable_media(bs),
>> - bdrv_dev_is_medium_locked(bs));
>> + bdrv_dev_is_medium_locked(bs),
>> + !(bs->open_flags& BDRV_O_NOCACHE));
>> bs_dict = qobject_to_qdict(bs_obj);
>> + qdict_put(bs_dict, "open_flags", qint_from_int(bs->open_flags));
>
> No. This adds a open_flags field to the QMP structure that is
> transferred to clients. This is wrong, open_flags is an internal thing
> that should never be visible on an interface.
>
> In bdrv_print_dict, access the hostcache field that you introduced, it
> provides the same information.
>
Will replace "open_flags" with "hostcache" field.
thanks, Supriya
^ permalink raw reply [flat|nested] 16+ messages in thread
* [Qemu-devel] [v7 Patch 2/5]Qemu: Error classes for file reopen and data sync failure
2011-10-11 3:10 [Qemu-devel] [v7 Patch 0/5]Qemu: Host pagecache setting from cmdline and monitor Supriya Kannery
2011-10-11 3:10 ` [Qemu-devel] [v7 Patch 1/5]Qemu: Enhance "info block" to display host cache setting Supriya Kannery
@ 2011-10-11 3:11 ` Supriya Kannery
2011-10-11 3:11 ` [Qemu-devel] [v7 Patch 3/5]Qemu: Cmd "block_set_hostcache" for dynamic cache change Supriya Kannery
` (2 subsequent siblings)
4 siblings, 0 replies; 16+ messages in thread
From: Supriya Kannery @ 2011-10-11 3:11 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, Stefan Hajnoczi, Christoph Hellwig
New error classes defined for file reopen failure and data
sync error
Signed-off-by: Supriya Kannery <supriyak@linux.vnet.ibm.com>
---
qerror.c | 8 ++++++++
qerror.h | 6 ++++++
2 files changed, 14 insertions(+)
Index: qemu/qerror.c
===================================================================
--- qemu.orig/qerror.c
+++ qemu/qerror.c
@@ -97,6 +97,14 @@ static const QErrorStringTable qerror_ta
.desc = "Device '%(device)' is not removable",
},
{
+ .error_fmt = QERR_DATA_SYNC_FAILED,
+ .desc = "Syncing of data failed for device '%(device)'",
+ },
+ {
+ .error_fmt = QERR_REOPEN_FILE_FAILED,
+ .desc = "Could not reopen '%(filename)'",
+ },
+ {
.error_fmt = QERR_DEVICE_NO_BUS,
.desc = "Device '%(device)' has no child bus",
},
Index: qemu/qerror.h
===================================================================
--- qemu.orig/qerror.h
+++ qemu/qerror.h
@@ -87,6 +87,9 @@ QError *qobject_to_qerror(const QObject
#define QERR_DEVICE_NOT_FOUND \
"{ 'class': 'DeviceNotFound', 'data': { 'device': %s } }"
+#define QERR_DATA_SYNC_FAILED \
+ "{ 'class': 'DataSyncFailed', 'data': { 'device': %s } }"
+
#define QERR_DEVICE_NOT_REMOVABLE \
"{ 'class': 'DeviceNotRemovable', 'data': { 'device': %s } }"
@@ -144,6 +147,9 @@ QError *qobject_to_qerror(const QObject
#define QERR_OPEN_FILE_FAILED \
"{ 'class': 'OpenFileFailed', 'data': { 'filename': %s } }"
+#define QERR_REOPEN_FILE_FAILED \
+ "{ 'class': 'ReopenFileFailed', 'data': { 'filename': %s } }"
+
#define QERR_PROPERTY_NOT_FOUND \
"{ 'class': 'PropertyNotFound', 'data': { 'device': %s, 'property': %s } }"
^ permalink raw reply [flat|nested] 16+ messages in thread* [Qemu-devel] [v7 Patch 3/5]Qemu: Cmd "block_set_hostcache" for dynamic cache change
2011-10-11 3:10 [Qemu-devel] [v7 Patch 0/5]Qemu: Host pagecache setting from cmdline and monitor Supriya Kannery
2011-10-11 3:10 ` [Qemu-devel] [v7 Patch 1/5]Qemu: Enhance "info block" to display host cache setting Supriya Kannery
2011-10-11 3:11 ` [Qemu-devel] [v7 Patch 2/5]Qemu: Error classes for file reopen and data sync failure Supriya Kannery
@ 2011-10-11 3:11 ` Supriya Kannery
2011-10-11 3:11 ` [Qemu-devel] [v7 Patch 4/5]Qemu: Add commandline -drive option 'hostcache' Supriya Kannery
2011-10-11 3:11 ` [Qemu-devel] [v7 Patch 5/5]Qemu: New struct 'BDRVReopenState' for image files reopen Supriya Kannery
4 siblings, 0 replies; 16+ messages in thread
From: Supriya Kannery @ 2011-10-11 3:11 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, Stefan Hajnoczi, Christoph Hellwig
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
@@ -702,6 +702,34 @@ 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();
+ if (bdrv_flush(bs)) {
+ 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_REOPEN_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)
{
if (bs->drv) {
@@ -739,6 +767,32 @@ void bdrv_close_all(void)
}
}
+int bdrv_change_hostcache(BlockDriverState *bs, bool enable_host_cache)
+{
+ int bdrv_flags = bs->open_flags;
+
+ /* set hostcache flags (without changing WCE/flush bits) */
+ if (enable_host_cache) {
+ 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) {
+ return 0;
+ }
+
+ if (bdrv_is_inserted(bs)) {
+ /* Reopen file with changed set of flags */
+ 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
@@ -99,6 +99,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);
@@ -133,6 +134,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.c
===================================================================
--- qemu.orig/blockdev.c
+++ qemu/blockdev.c
@@ -776,3 +776,29 @@ int do_block_resize(Monitor *mon, const
return 0;
}
+
+
+/*
+ * Change host page cache setting while guest is running.
+*/
+int do_block_set_hostcache(Monitor *mon, const QDict *qdict,
+ QObject **ret_data)
+{
+ BlockDriverState *bs = NULL;
+ int enable;
+ const char *device;
+
+ /* Validate device */
+ device = qdict_get_str(qdict, "device");
+ bs = bdrv_find(device);
+ if (!bs) {
+ qerror_report(QERR_DEVICE_NOT_FOUND, device);
+ return -1;
+ }
+
+ /* Read hostcache setting */
+ enable = qdict_get_bool(qdict, "option");
+ return bdrv_change_hostcache(bs, enable);
+
+}
+
Index: qemu/blockdev.h
===================================================================
--- qemu.orig/blockdev.h
+++ qemu/blockdev.h
@@ -65,5 +65,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_block_set_hostcache(Monitor *mon, const QDict *qdict,
+ QObject **ret_data);
#endif
Index: qemu/hmp-commands.hx
===================================================================
--- qemu.orig/hmp-commands.hx
+++ qemu/hmp-commands.hx
@@ -70,6 +70,20 @@ but should be used with extreme caution.
resizes image files, it can not resize block devices like LVM volumes.
ETEXI
+ {
+ .name = "block_set_hostcache",
+ .args_type = "device:B,option:b",
+ .params = "device on|off",
+ .help = "Change setting of host pagecache",
+ .user_print = monitor_user_noop,
+ .mhandler.cmd_new = do_block_set_hostcache,
+ },
+STEXI
+@item block_set_hostcache @var{device} @var{setting}
+@findex block_set_hostcache
+Change host pagecache setting of a block device while guest is running.
+ETEXI
+
{
.name = "eject",
Index: qemu/qmp-commands.hx
===================================================================
--- qemu.orig/qmp-commands.hx
+++ qemu/qmp-commands.hx
@@ -718,7 +718,34 @@ Example:
EQMP
+
{
+ .name = "block_set_hostcache",
+ .args_type = "device:B,option:b",
+ .params = "device on|off",
+ .help = "Change setting of host pagecache (true|false)",
+ .user_print = monitor_user_noop,
+ .mhandler.cmd_new = do_block_set_hostcache,
+ },
+
+SQMP
+block_set_hostcache
+-------------------
+
+Change host pagecache setting of a block device (on|off)
+
+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",
.params = "target",
^ permalink raw reply [flat|nested] 16+ messages in thread* [Qemu-devel] [v7 Patch 4/5]Qemu: Add commandline -drive option 'hostcache'
2011-10-11 3:10 [Qemu-devel] [v7 Patch 0/5]Qemu: Host pagecache setting from cmdline and monitor Supriya Kannery
` (2 preceding siblings ...)
2011-10-11 3:11 ` [Qemu-devel] [v7 Patch 3/5]Qemu: Cmd "block_set_hostcache" for dynamic cache change Supriya Kannery
@ 2011-10-11 3:11 ` Supriya Kannery
2011-10-12 14:30 ` Kevin Wolf
2011-10-11 3:11 ` [Qemu-devel] [v7 Patch 5/5]Qemu: New struct 'BDRVReopenState' for image files reopen Supriya Kannery
4 siblings, 1 reply; 16+ messages in thread
From: Supriya Kannery @ 2011-10-11 3:11 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, Stefan Hajnoczi, Christoph Hellwig
qemu command option 'hostcache' added to -drive for block devices.
While starting a VM from qemu commandline, this option can be used
for setting host cache usage for block data access. Simultaneous
use of 'hostcache' and 'cache' options not allowed.
Signed-off-by: Supriya Kannery <supriyak@linux.vnet.ibm.com>
---
blockdev.c | 13 +++++++++++++
qemu-config.c | 4 ++++
qemu-options.hx | 2 +-
3 files changed, 18 insertions(+), 1 deletion(-)
Index: qemu/blockdev.c
===================================================================
--- qemu.orig/blockdev.c
+++ qemu/blockdev.c
@@ -237,6 +237,7 @@ DriveInfo *drive_init(QemuOpts *opts, in
DriveInfo *dinfo;
int snapshot = 0;
int ret;
+ int hostcache = 0;
translation = BIOS_ATA_TRANSLATION_AUTO;
media = MEDIA_DISK;
@@ -319,7 +320,19 @@ DriveInfo *drive_init(QemuOpts *opts, in
}
}
+ if ((hostcache = qemu_opt_get_bool(opts, "hostcache", -1)) != -1) {
+ if (!hostcache) {
+ bdrv_flags |= BDRV_O_NOCACHE;
+ } else {
+ bdrv_flags &= ~BDRV_O_NOCACHE;
+ }
+ }
+
if ((buf = qemu_opt_get(opts, "cache")) != NULL) {
+ if (hostcache != -1) {
+ error_report("'hostcache' and 'cache' cannot co-exist");
+ return NULL;
+ }
if (bdrv_parse_cache_flags(buf, &bdrv_flags) != 0) {
error_report("invalid cache option");
return NULL;
Index: qemu/qemu-options.hx
===================================================================
--- qemu.orig/qemu-options.hx
+++ qemu/qemu-options.hx
@@ -135,7 +135,7 @@ DEF("drive", HAS_ARG, QEMU_OPTION_drive,
" [,cyls=c,heads=h,secs=s[,trans=t]][,snapshot=on|off]\n"
" [,cache=writethrough|writeback|none|directsync|unsafe][,format=f]\n"
" [,serial=s][,addr=A][,id=name][,aio=threads|native]\n"
- " [,readonly=on|off]\n"
+ " [,readonly=on|off][,hostcache=on|off]\n"
" use 'file' as a drive image\n", QEMU_ARCH_ALL)
STEXI
@item -drive @var{option}[,@var{option}[,@var{option}[,...]]]
Index: qemu/qemu-config.c
===================================================================
--- qemu.orig/qemu-config.c
+++ qemu/qemu-config.c
@@ -85,6 +85,10 @@ static QemuOptsList qemu_drive_opts = {
.name = "readonly",
.type = QEMU_OPT_BOOL,
.help = "open drive file as read-only",
+ },{
+ .name = "hostcache",
+ .type = QEMU_OPT_BOOL,
+ .help = "set or reset hostcache (on/off)"
},
{ /* end of list */ }
},
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [Qemu-devel] [v7 Patch 4/5]Qemu: Add commandline -drive option 'hostcache'
2011-10-11 3:11 ` [Qemu-devel] [v7 Patch 4/5]Qemu: Add commandline -drive option 'hostcache' Supriya Kannery
@ 2011-10-12 14:30 ` Kevin Wolf
2011-10-14 11:19 ` Supriya Kannery
0 siblings, 1 reply; 16+ messages in thread
From: Kevin Wolf @ 2011-10-12 14:30 UTC (permalink / raw)
To: Supriya Kannery; +Cc: Stefan Hajnoczi, qemu-devel, Christoph Hellwig
Am 11.10.2011 05:11, schrieb Supriya Kannery:
> qemu command option 'hostcache' added to -drive for block devices.
> While starting a VM from qemu commandline, this option can be used
> for setting host cache usage for block data access. Simultaneous
> use of 'hostcache' and 'cache' options not allowed.
>
> Signed-off-by: Supriya Kannery <supriyak@linux.vnet.ibm.com>
I'm not sure if introducing this alone makes sense. I think I would only
do it when we introduce more options that allow replacing all cache=...
options by other parameters.
>
> ---
> blockdev.c | 13 +++++++++++++
> qemu-config.c | 4 ++++
> qemu-options.hx | 2 +-
> 3 files changed, 18 insertions(+), 1 deletion(-)
>
> Index: qemu/blockdev.c
> ===================================================================
> --- qemu.orig/blockdev.c
> +++ qemu/blockdev.c
> @@ -237,6 +237,7 @@ DriveInfo *drive_init(QemuOpts *opts, in
> DriveInfo *dinfo;
> int snapshot = 0;
> int ret;
> + int hostcache = 0;
>
> translation = BIOS_ATA_TRANSLATION_AUTO;
> media = MEDIA_DISK;
> @@ -319,7 +320,19 @@ DriveInfo *drive_init(QemuOpts *opts, in
> }
> }
>
> + if ((hostcache = qemu_opt_get_bool(opts, "hostcache", -1)) != -1) {
> + if (!hostcache) {
> + bdrv_flags |= BDRV_O_NOCACHE;
> + } else {
> + bdrv_flags &= ~BDRV_O_NOCACHE;
> + }
bdrv_flags is initialised to 0, so the else branch is unnecessary.
Kevin
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [Qemu-devel] [v7 Patch 4/5]Qemu: Add commandline -drive option 'hostcache'
2011-10-12 14:30 ` Kevin Wolf
@ 2011-10-14 11:19 ` Supriya Kannery
2011-10-14 11:26 ` Kevin Wolf
0 siblings, 1 reply; 16+ messages in thread
From: Supriya Kannery @ 2011-10-14 11:19 UTC (permalink / raw)
To: Kevin Wolf; +Cc: Stefan Hajnoczi, qemu-devel, Christoph Hellwig
On 10/12/2011 08:00 PM, Kevin Wolf wrote:
> Am 11.10.2011 05:11, schrieb Supriya Kannery:
>> qemu command option 'hostcache' added to -drive for block devices.
>> While starting a VM from qemu commandline, this option can be used
>> for setting host cache usage for block data access. Simultaneous
>> use of 'hostcache' and 'cache' options not allowed.
>>
>> Signed-off-by: Supriya Kannery<supriyak@linux.vnet.ibm.com>
>
> I'm not sure if introducing this alone makes sense. I think I would only
> do it when we introduce more options that allow replacing all cache=...
> options by other parameters.
>
Can we do transition to alternatives for 'cache=' in a phased manner?
Until all other params are ready, we can allow hostcache (as well
as other params as and when they are ready) in cmdline with the
condition that 'cache=x', if specified, overrides these params.
Once we have all other params ready, 'cache=' can be replaced completely.
thanks, Supriya
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [v7 Patch 4/5]Qemu: Add commandline -drive option 'hostcache'
2011-10-14 11:19 ` Supriya Kannery
@ 2011-10-14 11:26 ` Kevin Wolf
0 siblings, 0 replies; 16+ messages in thread
From: Kevin Wolf @ 2011-10-14 11:26 UTC (permalink / raw)
To: supriyak; +Cc: Stefan Hajnoczi, qemu-devel, Christoph Hellwig
Am 14.10.2011 13:19, schrieb Supriya Kannery:
> On 10/12/2011 08:00 PM, Kevin Wolf wrote:
>> Am 11.10.2011 05:11, schrieb Supriya Kannery:
>>> qemu command option 'hostcache' added to -drive for block devices.
>>> While starting a VM from qemu commandline, this option can be used
>>> for setting host cache usage for block data access. Simultaneous
>>> use of 'hostcache' and 'cache' options not allowed.
>>>
>>> Signed-off-by: Supriya Kannery<supriyak@linux.vnet.ibm.com>
>>
>> I'm not sure if introducing this alone makes sense. I think I would only
>> do it when we introduce more options that allow replacing all cache=...
>> options by other parameters.
>>
>
> Can we do transition to alternatives for 'cache=' in a phased manner?
> Until all other params are ready, we can allow hostcache (as well
> as other params as and when they are ready) in cmdline with the
> condition that 'cache=x', if specified, overrides these params.
> Once we have all other params ready, 'cache=' can be replaced completely.
I guess that would be good enough. There's still not much use in
specifying hostcache=... at the same time as cache=... but at least it
doesn't take away other options then.
Kevin
^ permalink raw reply [flat|nested] 16+ messages in thread
* [Qemu-devel] [v7 Patch 5/5]Qemu: New struct 'BDRVReopenState' for image files reopen
2011-10-11 3:10 [Qemu-devel] [v7 Patch 0/5]Qemu: Host pagecache setting from cmdline and monitor Supriya Kannery
` (3 preceding siblings ...)
2011-10-11 3:11 ` [Qemu-devel] [v7 Patch 4/5]Qemu: Add commandline -drive option 'hostcache' Supriya Kannery
@ 2011-10-11 3:11 ` Supriya Kannery
2011-10-12 14:55 ` Kevin Wolf
2011-10-14 11:13 ` Stefan Hajnoczi
4 siblings, 2 replies; 16+ messages in thread
From: Supriya Kannery @ 2011-10-11 3:11 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, Stefan Hajnoczi, Christoph Hellwig
Struct BDRVReopenState introduced for handling reopen state of images.
This can be extended by each of the block drivers to reopen respective
image files. Implementation for raw-posix is done here.
Signed-off-by: Supriya Kannery <supriyak@linux.vnet.ibm.com>
---
block.c | 46 ++++++++++++++++++++++++++++++++--------
block/raw-posix.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
block_int.h | 15 +++++++++++++
3 files changed, 114 insertions(+), 9 deletions(-)
Index: qemu/block/raw-posix.c
===================================================================
--- qemu.orig/block/raw-posix.c
+++ qemu/block/raw-posix.c
@@ -279,6 +279,66 @@ static int raw_open(BlockDriverState *bs
return raw_open_common(bs, filename, flags, 0);
}
+static int raw_reopen_prepare(BlockDriverState *bs, BDRVReopenState **rs,
+ int flags)
+{
+ BDRVReopenState *raw_rs = g_malloc0(sizeof(BDRVReopenState));
+ BDRVRawState *s = bs->opaque;
+
+ raw_rs->bs = bs;
+ raw_rs->reopen_flags = flags;
+ raw_rs->reopen_fd = -1;
+
+ /* If only O_DIRECT to be toggled, use fcntl */
+ if (!((bs->open_flags & ~BDRV_O_NOCACHE) ^
+ (flags & ~BDRV_O_NOCACHE))) {
+ raw_rs->reopen_fd = dup(s->fd);
+ if (raw_rs->reopen_fd <= 0) {
+ return -1;
+ }
+ }
+
+ /* TBD: Handle O_DSYNC and other flags */
+ *rs = raw_rs;
+ return 0;
+}
+
+static int raw_reopen_commit(BDRVReopenState *rs)
+{
+ BlockDriverState *bs = rs->bs;
+ BDRVRawState *s = bs->opaque;
+
+ if (!rs->reopen_fd) {
+ return -1;
+ }
+
+ int ret = fcntl_setfl(rs->reopen_fd, rs->reopen_flags);
+ if (ret < 0) {
+ return ret;
+ }
+
+ /* Set new flags, so replace old fd with new one */
+ close(s->fd);
+ s->fd = rs->reopen_fd;
+ s->open_flags = bs->open_flags = rs->reopen_flags;
+ g_free(rs);
+ rs = NULL;
+
+ return 0;
+}
+
+static int raw_reopen_abort(BDRVReopenState *rs)
+{
+ if (rs->reopen_fd != -1) {
+ close(rs->reopen_fd);
+ rs->reopen_fd = -1;
+ rs->reopen_flags = 0;
+ }
+ g_free(rs);
+ rs = NULL;
+ return 0;
+}
+
/* XXX: use host sector size if necessary with:
#ifdef DIOCGSECTORSIZE
{
@@ -896,6 +956,9 @@ static BlockDriver bdrv_file = {
.instance_size = sizeof(BDRVRawState),
.bdrv_probe = NULL, /* no probe for protocols */
.bdrv_file_open = raw_open,
+ .bdrv_reopen_prepare = raw_reopen_prepare,
+ .bdrv_reopen_commit = raw_reopen_commit,
+ .bdrv_reopen_abort = raw_reopen_abort,
.bdrv_read = raw_read,
.bdrv_write = raw_write,
.bdrv_close = raw_close,
@@ -1166,6 +1229,10 @@ static BlockDriver bdrv_host_device = {
.instance_size = sizeof(BDRVRawState),
.bdrv_probe_device = hdev_probe_device,
.bdrv_file_open = hdev_open,
+ .bdrv_reopen_prepare
+ = raw_reopen_prepare,
+ .bdrv_reopen_commit = raw_reopen_commit,
+ .bdrv_reopen_abort = raw_reopen_abort,
.bdrv_close = raw_close,
.bdrv_create = hdev_create,
.create_options = raw_create_options,
Index: qemu/block.c
===================================================================
--- qemu.orig/block.c
+++ qemu/block.c
@@ -706,6 +706,7 @@ int bdrv_reopen(BlockDriverState *bs, in
{
BlockDriver *drv = bs->drv;
int ret = 0, open_flags;
+ BDRVReopenState *rs;
/* Quiesce IO for the given block device */
qemu_aio_flush();
@@ -713,20 +714,48 @@ int bdrv_reopen(BlockDriverState *bs, in
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_REOPEN_FILE_FAILED, bs->filename);
- ret = bdrv_open(bs, bs->filename, open_flags, drv);
+ /* Use driver specific reopen() if available */
+ if (drv->bdrv_reopen_prepare) {
+ ret = drv->bdrv_reopen_prepare(bs, &rs, bdrv_flags);
if (ret < 0) {
- /* Reopen failed with orig and modified flags */
- abort();
+ goto fail;
}
- }
+ if (drv->bdrv_reopen_commit) {
+ ret = drv->bdrv_reopen_commit(rs);
+ if (ret < 0) {
+ goto fail;
+ }
+ return 0;
+ }
+fail:
+ if (drv->bdrv_reopen_abort) {
+ ret = drv->bdrv_reopen_abort(rs);
+ qerror_report(QERR_REOPEN_FILE_FAILED, bs->filename);
+ return ret;
+ }
+
+ } else {
+
+ /* Use reopen procedure common for all drivers */
+ 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_REOPEN_FILE_FAILED, bs->filename);
+ ret = bdrv_open(bs, bs->filename, open_flags, drv);
+ if (ret < 0) {
+ /*
+ * Reopen with orig and modified flags failed
+ */
+ abort();
+ }
+ }
+ }
return ret;
}
Index: qemu/block_int.h
===================================================================
--- qemu.orig/block_int.h
+++ qemu/block_int.h
@@ -55,6 +55,13 @@ struct BlockDriver {
int (*bdrv_probe)(const uint8_t *buf, int buf_size, const char *filename);
int (*bdrv_probe_device)(const char *filename);
int (*bdrv_open)(BlockDriverState *bs, int flags);
+
+ /* For handling image reopen for split or non-split files */
+ int (*bdrv_reopen_prepare)(BlockDriverState *bs, BDRVReopenState **rs,
+ int flags);
+ int (*bdrv_reopen_commit)(BDRVReopenState *rs);
+ int (*bdrv_reopen_abort)(BDRVReopenState *rs);
+
int (*bdrv_file_open)(BlockDriverState *bs, const char *filename, int flags);
int (*bdrv_read)(BlockDriverState *bs, int64_t sector_num,
uint8_t *buf, int nb_sectors);
@@ -207,6 +214,14 @@ struct BlockDriverState {
void *private;
};
+struct BDRVReopenState {
+ BlockDriverState *bs;
+ int reopen_flags;
+
+ /* For raw-posix */
+ int reopen_fd;
+};
+
struct BlockDriverAIOCB {
AIOPool *pool;
BlockDriverState *bs;
Index: qemu/qemu-common.h
===================================================================
--- qemu.orig/qemu-common.h
+++ qemu/qemu-common.h
@@ -228,6 +228,7 @@ typedef struct NICInfo NICInfo;
typedef struct HCIInfo HCIInfo;
typedef struct AudioState AudioState;
typedef struct BlockDriverState BlockDriverState;
+typedef struct BDRVReopenState BDRVReopenState;
typedef struct DriveInfo DriveInfo;
typedef struct DisplayState DisplayState;
typedef struct DisplayChangeListener DisplayChangeListener;
Index: qemu/block/raw.c
===================================================================
--- qemu.orig/block/raw.c
+++ qemu/block/raw.c
@@ -9,6 +9,15 @@ static int raw_open(BlockDriverState *bs
return 0;
}
+static int raw_reopen(BlockDriverState *bs, BDRVReopenState **prs, int flags)
+{
+ int ret = bdrv_reopen(bs->file, flags);
+ if (!ret) {
+ bs->open_flags = bs->file->open_flags;
+ }
+ return ret;
+}
+
static int raw_read(BlockDriverState *bs, int64_t sector_num,
uint8_t *buf, int nb_sectors)
{
@@ -128,6 +137,8 @@ static BlockDriver bdrv_raw = {
.instance_size = 1,
.bdrv_open = raw_open,
+ .bdrv_reopen_prepare
+ = raw_reopen,
.bdrv_close = raw_close,
.bdrv_read = raw_read,
.bdrv_write = raw_write,
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [Qemu-devel] [v7 Patch 5/5]Qemu: New struct 'BDRVReopenState' for image files reopen
2011-10-11 3:11 ` [Qemu-devel] [v7 Patch 5/5]Qemu: New struct 'BDRVReopenState' for image files reopen Supriya Kannery
@ 2011-10-12 14:55 ` Kevin Wolf
2011-10-14 13:42 ` Supriya Kannery
2011-10-14 11:13 ` Stefan Hajnoczi
1 sibling, 1 reply; 16+ messages in thread
From: Kevin Wolf @ 2011-10-12 14:55 UTC (permalink / raw)
To: Supriya Kannery; +Cc: Stefan Hajnoczi, qemu-devel, Christoph Hellwig
Am 11.10.2011 05:11, schrieb Supriya Kannery:
> Struct BDRVReopenState introduced for handling reopen state of images.
> This can be extended by each of the block drivers to reopen respective
> image files. Implementation for raw-posix is done here.
>
> Signed-off-by: Supriya Kannery <supriyak@linux.vnet.ibm.com>
Maybe it would make sense to split this into two patches, one for the
block.c infrastructure and another one for adding the callbacks in drivers.
> ---
> block.c | 46 ++++++++++++++++++++++++++++++++--------
> block/raw-posix.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> block_int.h | 15 +++++++++++++
> 3 files changed, 114 insertions(+), 9 deletions(-)
>
> Index: qemu/block/raw-posix.c
> ===================================================================
> --- qemu.orig/block/raw-posix.c
> +++ qemu/block/raw-posix.c
> @@ -279,6 +279,66 @@ static int raw_open(BlockDriverState *bs
> return raw_open_common(bs, filename, flags, 0);
> }
>
> +static int raw_reopen_prepare(BlockDriverState *bs, BDRVReopenState **rs,
> + int flags)
> +{
> + BDRVReopenState *raw_rs = g_malloc0(sizeof(BDRVReopenState));
> + BDRVRawState *s = bs->opaque;
> +
> + raw_rs->bs = bs;
> + raw_rs->reopen_flags = flags;
> + raw_rs->reopen_fd = -1;
> +
> + /* If only O_DIRECT to be toggled, use fcntl */
> + if (!((bs->open_flags & ~BDRV_O_NOCACHE) ^
> + (flags & ~BDRV_O_NOCACHE))) {
> + raw_rs->reopen_fd = dup(s->fd);
> + if (raw_rs->reopen_fd <= 0) {
> + return -1;
This leaks raw_rs.
> + }
> + }
> +
> + /* TBD: Handle O_DSYNC and other flags */
> + *rs = raw_rs;
> + return 0;
> +}
> +
> +static int raw_reopen_commit(BDRVReopenState *rs)
bdrv_reopen_commit must never fail. Any error that can happen must
already be handled in bdrv_reopen_prepare.
The commit function should really only do s->fd = rs->reopen_fd (besides
cleanup), everything else should already be prepared.
> +{
> + BlockDriverState *bs = rs->bs;
> + BDRVRawState *s = bs->opaque;
> +
> + if (!rs->reopen_fd) {
> + return -1;
> + }
> +
> + int ret = fcntl_setfl(rs->reopen_fd, rs->reopen_flags);
reopen_flags is BDRV_O_*, not O_*, so it needs to be translated.
> + if (ret < 0) {
> + return ret;
> + }
> +
> + /* Set new flags, so replace old fd with new one */
> + close(s->fd);
> + s->fd = rs->reopen_fd;
> + s->open_flags = bs->open_flags = rs->reopen_flags;
> + g_free(rs);
> + rs = NULL;
Setting to NULL has no effect, rs isn't read afterwards.
> +
> + return 0;
> +}
> +
> +static int raw_reopen_abort(BDRVReopenState *rs)
This must not fail either, so it can be void, too.
> +{
> + if (rs->reopen_fd != -1) {
> + close(rs->reopen_fd);
> + rs->reopen_fd = -1;
> + rs->reopen_flags = 0;
> + }
> + g_free(rs);
> + rs = NULL;
> + return 0;
> +}
> +
> /* XXX: use host sector size if necessary with:
> #ifdef DIOCGSECTORSIZE
> {
> @@ -896,6 +956,9 @@ static BlockDriver bdrv_file = {
> .instance_size = sizeof(BDRVRawState),
> .bdrv_probe = NULL, /* no probe for protocols */
> .bdrv_file_open = raw_open,
> + .bdrv_reopen_prepare = raw_reopen_prepare,
> + .bdrv_reopen_commit = raw_reopen_commit,
> + .bdrv_reopen_abort = raw_reopen_abort,
> .bdrv_read = raw_read,
> .bdrv_write = raw_write,
> .bdrv_close = raw_close,
> @@ -1166,6 +1229,10 @@ static BlockDriver bdrv_host_device = {
> .instance_size = sizeof(BDRVRawState),
> .bdrv_probe_device = hdev_probe_device,
> .bdrv_file_open = hdev_open,
> + .bdrv_reopen_prepare
> + = raw_reopen_prepare,
> + .bdrv_reopen_commit = raw_reopen_commit,
> + .bdrv_reopen_abort = raw_reopen_abort,
> .bdrv_close = raw_close,
> .bdrv_create = hdev_create,
> .create_options = raw_create_options,
> Index: qemu/block.c
> ===================================================================
> --- qemu.orig/block.c
> +++ qemu/block.c
> @@ -706,6 +706,7 @@ int bdrv_reopen(BlockDriverState *bs, in
> {
> BlockDriver *drv = bs->drv;
> int ret = 0, open_flags;
> + BDRVReopenState *rs;
>
> /* Quiesce IO for the given block device */
> qemu_aio_flush();
> @@ -713,20 +714,48 @@ int bdrv_reopen(BlockDriverState *bs, in
> 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_REOPEN_FILE_FAILED, bs->filename);
> - ret = bdrv_open(bs, bs->filename, open_flags, drv);
> + /* Use driver specific reopen() if available */
> + if (drv->bdrv_reopen_prepare) {
> + ret = drv->bdrv_reopen_prepare(bs, &rs, bdrv_flags);
> if (ret < 0) {
> - /* Reopen failed with orig and modified flags */
> - abort();
> + goto fail;
> }
> - }
> + if (drv->bdrv_reopen_commit) {
> + ret = drv->bdrv_reopen_commit(rs);
> + if (ret < 0) {
> + goto fail;
> + }
> + return 0;
> + }
Pull the return 0; out one level. It would be really strange if we
turned a successful prepare into reopen_abort just because the driver
doesn't need a commit function.
(The other consistent way would be to require that if a driver
implements one reopen function, it has to implement all three of them.
I'm fine either way.)
> +fail:
> + if (drv->bdrv_reopen_abort) {
> + ret = drv->bdrv_reopen_abort(rs);
> + qerror_report(QERR_REOPEN_FILE_FAILED, bs->filename);
> + return ret;
> + }
> +
> + } else {
> +
> + /* Use reopen procedure common for all drivers */
> + 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_REOPEN_FILE_FAILED, bs->filename);
> + ret = bdrv_open(bs, bs->filename, open_flags, drv);
> + if (ret < 0) {
> + /*
> + * Reopen with orig and modified flags failed
> + */
> + abort();
Make this bs->drv = NULL, so that trying to access to image will fail,
but at least not the whole VM crashes.
> + }
> + }
> + }
> return ret;
> }
>
> Index: qemu/block_int.h
> ===================================================================
> --- qemu.orig/block_int.h
> +++ qemu/block_int.h
> @@ -55,6 +55,13 @@ struct BlockDriver {
> int (*bdrv_probe)(const uint8_t *buf, int buf_size, const char *filename);
> int (*bdrv_probe_device)(const char *filename);
> int (*bdrv_open)(BlockDriverState *bs, int flags);
> +
> + /* For handling image reopen for split or non-split files */
> + int (*bdrv_reopen_prepare)(BlockDriverState *bs, BDRVReopenState **rs,
> + int flags);
> + int (*bdrv_reopen_commit)(BDRVReopenState *rs);
> + int (*bdrv_reopen_abort)(BDRVReopenState *rs);
> +
> int (*bdrv_file_open)(BlockDriverState *bs, const char *filename, int flags);
> int (*bdrv_read)(BlockDriverState *bs, int64_t sector_num,
> uint8_t *buf, int nb_sectors);
> @@ -207,6 +214,14 @@ struct BlockDriverState {
> void *private;
> };
>
> +struct BDRVReopenState {
> + BlockDriverState *bs;
> + int reopen_flags;
> +
> + /* For raw-posix */
> + int reopen_fd;
> +};
> +
> struct BlockDriverAIOCB {
> AIOPool *pool;
> BlockDriverState *bs;
> Index: qemu/qemu-common.h
> ===================================================================
> --- qemu.orig/qemu-common.h
> +++ qemu/qemu-common.h
> @@ -228,6 +228,7 @@ typedef struct NICInfo NICInfo;
> typedef struct HCIInfo HCIInfo;
> typedef struct AudioState AudioState;
> typedef struct BlockDriverState BlockDriverState;
> +typedef struct BDRVReopenState BDRVReopenState;
> typedef struct DriveInfo DriveInfo;
> typedef struct DisplayState DisplayState;
> typedef struct DisplayChangeListener DisplayChangeListener;
> Index: qemu/block/raw.c
> ===================================================================
> --- qemu.orig/block/raw.c
> +++ qemu/block/raw.c
> @@ -9,6 +9,15 @@ static int raw_open(BlockDriverState *bs
> return 0;
> }
>
> +static int raw_reopen(BlockDriverState *bs, BDRVReopenState **prs, int flags)
> +{
> + int ret = bdrv_reopen(bs->file, flags);
> + if (!ret) {
> + bs->open_flags = bs->file->open_flags;
> + }
> + return ret;
> +}
> +
> static int raw_read(BlockDriverState *bs, int64_t sector_num,
> uint8_t *buf, int nb_sectors)
> {
> @@ -128,6 +137,8 @@ static BlockDriver bdrv_raw = {
> .instance_size = 1,
>
> .bdrv_open = raw_open,
> + .bdrv_reopen_prepare
> + = raw_reopen,
> .bdrv_close = raw_close,
> .bdrv_read = raw_read,
> .bdrv_write = raw_write,
I think raw must pass through all three functions. Otherwise it could
happen that we need to abort, but the image has already been reopened.
Kevin
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [Qemu-devel] [v7 Patch 5/5]Qemu: New struct 'BDRVReopenState' for image files reopen
2011-10-12 14:55 ` Kevin Wolf
@ 2011-10-14 13:42 ` Supriya Kannery
2011-10-14 13:52 ` Kevin Wolf
0 siblings, 1 reply; 16+ messages in thread
From: Supriya Kannery @ 2011-10-14 13:42 UTC (permalink / raw)
To: Kevin Wolf; +Cc: Stefan Hajnoczi, qemu-devel, Christoph Hellwig
On 10/12/2011 08:25 PM, Kevin Wolf wrote:
> Am 11.10.2011 05:11, schrieb Supriya Kannery:
>> Struct BDRVReopenState introduced for handling reopen state of images.
>> This can be extended by each of the block drivers to reopen respective
>> image files. Implementation for raw-posix is done here.
>>
>> Signed-off-by: Supriya Kannery<supriyak@linux.vnet.ibm.com>
>
> Maybe it would make sense to split this into two patches, one for the
> block.c infrastructure and another one for adding the callbacks in drivers.
>
ok..will split in v8.
>> +
>> + /* If only O_DIRECT to be toggled, use fcntl */
>> + if (!((bs->open_flags& ~BDRV_O_NOCACHE) ^
>> + (flags& ~BDRV_O_NOCACHE))) {
>> + raw_rs->reopen_fd = dup(s->fd);
>> + if (raw_rs->reopen_fd<= 0) {
>> + return -1;
>
> This leaks raw_rs.
>
will handle
>> + }
>> + }
>> +
>> + /* TBD: Handle O_DSYNC and other flags */
>> + *rs = raw_rs;
>> + return 0;
>> +}
>> +
>> +static int raw_reopen_commit(BDRVReopenState *rs)
>
> bdrv_reopen_commit must never fail. Any error that can happen must
> already be handled in bdrv_reopen_prepare.
>
> The commit function should really only do s->fd = rs->reopen_fd (besides
> cleanup), everything else should already be prepared.
>
will move call to fcntl to bdrv_reopen_prepare.
>> +{
>> + BlockDriverState *bs = rs->bs;
>> + BDRVRawState *s = bs->opaque;
>> +
>> + if (!rs->reopen_fd) {
>> + return -1;
>> + }
>> +
>> + int ret = fcntl_setfl(rs->reopen_fd, rs->reopen_flags);
>
> reopen_flags is BDRV_O_*, not O_*, so it needs to be translated.
>
ok
>> + /* Use driver specific reopen() if available */
>> + if (drv->bdrv_reopen_prepare) {
>> + ret = drv->bdrv_reopen_prepare(bs,&rs, bdrv_flags);
>> if (ret< 0) {
>> - /* Reopen failed with orig and modified flags */
>> - abort();
>> + goto fail;
>> }
>> - }
>> + if (drv->bdrv_reopen_commit) {
>> + ret = drv->bdrv_reopen_commit(rs);
>> + if (ret< 0) {
>> + goto fail;
>> + }
>> + return 0;
>> + }
>
> Pull the return 0; out one level. It would be really strange if we
> turned a successful prepare into reopen_abort just because the driver
> doesn't need a commit function.
>
> (The other consistent way would be to require that if a driver
> implements one reopen function, it has to implement all three of them.
> I'm fine either way.)
>
Will give flexibility to drivers, not mandating all the three functions.
>> + ret = bdrv_open(bs, bs->filename, open_flags, drv);
>> + if (ret< 0) {
>> + /*
>> + * Reopen with orig and modified flags failed
>> + */
>> + abort();
>
> Make this bs->drv = NULL, so that trying to access to image will fail,
> but at least not the whole VM crashes.
>
ok
>> static int raw_read(BlockDriverState *bs, int64_t sector_num,
>> uint8_t *buf, int nb_sectors)
>> {
>> @@ -128,6 +137,8 @@ static BlockDriver bdrv_raw = {
>> .instance_size = 1,
>>
>> .bdrv_open = raw_open,
>> + .bdrv_reopen_prepare
>> + = raw_reopen,
>> .bdrv_close = raw_close,
>> .bdrv_read = raw_read,
>> .bdrv_write = raw_write,
>
> I think raw must pass through all three functions. Otherwise it could
> happen that we need to abort, but the image has already been reopened.
>
ok..got it..will have three separate functions to avoid unnecessary
dependencies.
Got a question..
In raw-posix, the three functions are implemented for file_reopen
for now. Should this be extended to hdev, cdrom and floppy?
> Kevin
>
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [Qemu-devel] [v7 Patch 5/5]Qemu: New struct 'BDRVReopenState' for image files reopen
2011-10-14 13:42 ` Supriya Kannery
@ 2011-10-14 13:52 ` Kevin Wolf
0 siblings, 0 replies; 16+ messages in thread
From: Kevin Wolf @ 2011-10-14 13:52 UTC (permalink / raw)
To: supriyak; +Cc: Stefan Hajnoczi, qemu-devel, Christoph Hellwig
Am 14.10.2011 15:42, schrieb Supriya Kannery:
>>> + /* Use driver specific reopen() if available */
>>> + if (drv->bdrv_reopen_prepare) {
>>> + ret = drv->bdrv_reopen_prepare(bs,&rs, bdrv_flags);
>>> if (ret< 0) {
>>> - /* Reopen failed with orig and modified flags */
>>> - abort();
>>> + goto fail;
>>> }
>>> - }
>>> + if (drv->bdrv_reopen_commit) {
>>> + ret = drv->bdrv_reopen_commit(rs);
>>> + if (ret< 0) {
>>> + goto fail;
>>> + }
>>> + return 0;
>>> + }
>>
>> Pull the return 0; out one level. It would be really strange if we
>> turned a successful prepare into reopen_abort just because the driver
>> doesn't need a commit function.
>>
>> (The other consistent way would be to require that if a driver
>> implements one reopen function, it has to implement all three of them.
>> I'm fine either way.)
>>
>
> Will give flexibility to drivers, not mandating all the three functions.
Do we have a use case where it is actually possible to implement less
functions without introducing bugs?
If yes, let's keep it as it is.
> Got a question..
> In raw-posix, the three functions are implemented for file_reopen
> for now. Should this be extended to hdev, cdrom and floppy?
Yes, that would be good. And I think the same implementation can be used
for all of them.
Kevin
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [v7 Patch 5/5]Qemu: New struct 'BDRVReopenState' for image files reopen
2011-10-11 3:11 ` [Qemu-devel] [v7 Patch 5/5]Qemu: New struct 'BDRVReopenState' for image files reopen Supriya Kannery
2011-10-12 14:55 ` Kevin Wolf
@ 2011-10-14 11:13 ` Stefan Hajnoczi
2011-10-14 13:45 ` Supriya Kannery
1 sibling, 1 reply; 16+ messages in thread
From: Stefan Hajnoczi @ 2011-10-14 11:13 UTC (permalink / raw)
To: Supriya Kannery; +Cc: Kevin Wolf, qemu-devel, Christoph Hellwig
On Tue, Oct 11, 2011 at 08:41:59AM +0530, Supriya Kannery wrote:
> Index: qemu/block.c
> ===================================================================
> --- qemu.orig/block.c
> +++ qemu/block.c
> @@ -706,6 +706,7 @@ int bdrv_reopen(BlockDriverState *bs, in
> {
> BlockDriver *drv = bs->drv;
> int ret = 0, open_flags;
> + BDRVReopenState *rs;
BDRVReopenState *rs = NULL;
If the abort path is taken we need to make sure rs has a defined value.
Note that the abort path currently doesn't handle rs == NULL and will
segfault in raw_reopen_abort().
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [Qemu-devel] [v7 Patch 5/5]Qemu: New struct 'BDRVReopenState' for image files reopen
2011-10-14 11:13 ` Stefan Hajnoczi
@ 2011-10-14 13:45 ` Supriya Kannery
0 siblings, 0 replies; 16+ messages in thread
From: Supriya Kannery @ 2011-10-14 13:45 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: Kevin Wolf, qemu-devel, Christoph Hellwig
On 10/14/2011 04:43 PM, Stefan Hajnoczi wrote:
> On Tue, Oct 11, 2011 at 08:41:59AM +0530, Supriya Kannery wrote:
>> Index: qemu/block.c
>> ===================================================================
>> --- qemu.orig/block.c
>> +++ qemu/block.c
>> @@ -706,6 +706,7 @@ int bdrv_reopen(BlockDriverState *bs, in
>> {
>> BlockDriver *drv = bs->drv;
>> int ret = 0, open_flags;
>> + BDRVReopenState *rs;
>
> BDRVReopenState *rs = NULL;
>
> If the abort path is taken we need to make sure rs has a defined value.
> Note that the abort path currently doesn't handle rs == NULL and will
> segfault in raw_reopen_abort().
>
sure..will check on this.
thanks, Supriya
^ permalink raw reply [flat|nested] 16+ messages in thread