* [Qemu-devel] [RFC Patch 0/7]Qemu: Dynamic host pagecache change
@ 2012-02-01 3:05 Supriya Kannery
2012-02-01 3:06 ` [Qemu-devel] [RFC Patch 1/7]Qemu: Enhance "info block" to display host cache setting Supriya Kannery
` (7 more replies)
0 siblings, 8 replies; 30+ messages in thread
From: Supriya Kannery @ 2012-02-01 3:05 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Stefan Hajnoczi, Christoph Hellwig, Luiz Capitulino
For changing host pagecache setting of a running VM, it is
important to have a safe way of reopening its image file.
Following patchset introduces:
* a generic way to reopen image files safely.
In this approach, before reopening an image, for each
block driver, its state will be stashed. Incase preparation
(bdrv_reopen_prepare) for reopening returns success, the stashed
state will be cleared (bdrv_reopen_commit) and reopened state will
be used further. Incase preparation of reopening returns failure,
the state of the driver will be rolled back (bdrv_reopen_abort)
to the stashed state.
This approach is extended to raw-posix, raw-win32 and vmdk block
drivers in this patchset. Once this is reviewed and finalised, I will
extend the implementation to other drivers like qcow2, qed etc..
* qmp and hmp command 'block_set_hostcache' using which host
pagecache setting for a block device can be changed
when the VM is running.
* BDRVReopenState, a generic structure which can be
extended by each of the block drivers to reopen
respective image files.
ToDo:
* memcpy is used to save driver state. Replace this with copying
individual fields of driver state (?)
* Extend this implementation to other block drivers.
* Build and verify raw-win32 driver changes in windows
Earlier discussions related to dynamic change of host pagecache can be found
at: http://lists.gnu.org/archive/html/qemu-devel/2011-11/msg01482.html
New block command added:
"block_set_hostcache"
-- Sets hostcache parameter for block device while guest is running.
Usage:
block_set_hostcache <device> <option>
<device> = block device
<option> = on/off
qemu/block.c | 112 +++++++++++++++++++++++++++++++++++++++++++++----
qemu/block.h | 5 +
qemu/block/raw-posix.c | 74 ++++++++++++++++++++++++++++++++
qemu/block/raw-win32.c | 95 +++++++++++++++++++++++++++++++++++++++++
qemu/block/raw.c | 20 ++++++++
qemu/block/vmdk.c | 80 +++++++++++++++++++++++++++++++++--
qemu/block_int.h | 11 ++++
qemu/blockdev.c | 26 +++++++++++
qemu/blockdev.h | 2
qemu/hmp-commands.hx | 14 ++++++
qemu/hmp.c | 2
qemu/qapi-schema.json | 4 +
qemu/qemu-common.h | 1
qemu/qerror.c | 8 +++
qemu/qerror.h | 6 ++
qemu/qmp-commands.hx | 27 +++++++++++
18 files changed, 474 insertions(+), 13 deletions(-)
~
~
~
~
~
~
~
~
~
-- INSERT --
^ permalink raw reply [flat|nested] 30+ messages in thread
* [Qemu-devel] [RFC Patch 1/7]Qemu: Enhance "info block" to display host cache setting
2012-02-01 3:05 [Qemu-devel] [RFC Patch 0/7]Qemu: Dynamic host pagecache change Supriya Kannery
@ 2012-02-01 3:06 ` Supriya Kannery
2012-02-08 12:00 ` Luiz Capitulino
2012-02-01 3:06 ` [Qemu-devel] [RFC Patch 2/7]Qemu: Error classes for file reopen and data sync failure Supriya Kannery
` (6 subsequent siblings)
7 siblings, 1 reply; 30+ messages in thread
From: Supriya Kannery @ 2012-02-01 3:06 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Stefan Hajnoczi, Christoph Hellwig, Luiz Capitulino
Enhance "info block" to display hostcache setting for each
block device.
Example:
(qemu) info block
ide0-hd0: removable=0 file=../rhel6-32.raw ro=0 drv=raw encrypted=0
Enhanced to display "hostcache" setting:
(qemu) info block
ide0-hd0: removable=0 hostcache=1 file=../rhel6-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/qapi-schema.json
===================================================================
--- qemu.orig/qapi-schema.json
+++ qemu/qapi-schema.json
@@ -423,6 +423,8 @@
# @locked: True if the guest has locked this device from having its media
# removed
#
+# @hostcache: True if host pagecache is enabled.
+#
# @tray_open: #optional True if the device has a tray and it is open
# (only present if removable is true)
#
@@ -436,7 +438,7 @@
##
{ 'type': 'BlockInfo',
'data': {'device': 'str', 'type': 'str', 'removable': 'bool',
- 'locked': 'bool', '*inserted': 'BlockDeviceInfo',
+ 'locked': 'bool','hostcache': 'bool', '*inserted': 'BlockDeviceInfo',
'*tray_open': 'bool', '*io-status': 'BlockDeviceIoStatus'} }
##
Index: qemu/block.c
===================================================================
--- qemu.orig/block.c
+++ qemu/block.c
@@ -2285,6 +2285,7 @@ BlockInfoList *qmp_query_block(Error **e
info->value->device = g_strdup(bs->device_name);
info->value->type = g_strdup("unknown");
info->value->locked = bdrv_dev_is_medium_locked(bs);
+ info->value->hostcache = !(bs->open_flags & BDRV_O_NOCACHE);
info->value->removable = bdrv_dev_has_removable_media(bs);
if (bdrv_dev_has_removable_media(bs)) {
Index: qemu/hmp.c
===================================================================
--- qemu.orig/hmp.c
+++ qemu/hmp.c
@@ -209,6 +209,8 @@ void hmp_info_block(Monitor *mon)
monitor_printf(mon, " tray-open=%d", info->value->tray_open);
}
+ monitor_printf(mon, " hostcache=%d", info->value->hostcache);
+
if (info->value->has_io_status) {
monitor_printf(mon, " io-status=%s",
BlockDeviceIoStatus_lookup[info->value->io_status]);
^ permalink raw reply [flat|nested] 30+ messages in thread
* [Qemu-devel] [RFC Patch 2/7]Qemu: Error classes for file reopen and data sync failure
2012-02-01 3:05 [Qemu-devel] [RFC Patch 0/7]Qemu: Dynamic host pagecache change Supriya Kannery
2012-02-01 3:06 ` [Qemu-devel] [RFC Patch 1/7]Qemu: Enhance "info block" to display host cache setting Supriya Kannery
@ 2012-02-01 3:06 ` Supriya Kannery
2012-02-07 7:56 ` Stefan Hajnoczi
2012-02-01 3:06 ` [Qemu-devel] [RFC Patch 3/7]Qemu: Cmd "block_set_hostcache" for dynamic cache change Supriya Kannery
` (5 subsequent siblings)
7 siblings, 1 reply; 30+ messages in thread
From: Supriya Kannery @ 2012-02-01 3:06 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Stefan Hajnoczi, Christoph Hellwig, Luiz Capitulino
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
@@ -108,6 +108,14 @@ static const QErrorStringTable qerror_ta
.desc = "Device '%(device)' has multiple child busses",
},
{
+ .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
@@ -117,6 +117,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 } }"
@@ -180,6 +183,9 @@ QError *qobject_to_qerror(const QObject
#define QERR_PERMISSION_DENIED \
"{ 'class': 'PermissionDenied', 'data': {} }"
+#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] 30+ messages in thread
* [Qemu-devel] [RFC Patch 3/7]Qemu: Cmd "block_set_hostcache" for dynamic cache change
2012-02-01 3:05 [Qemu-devel] [RFC Patch 0/7]Qemu: Dynamic host pagecache change Supriya Kannery
2012-02-01 3:06 ` [Qemu-devel] [RFC Patch 1/7]Qemu: Enhance "info block" to display host cache setting Supriya Kannery
2012-02-01 3:06 ` [Qemu-devel] [RFC Patch 2/7]Qemu: Error classes for file reopen and data sync failure Supriya Kannery
@ 2012-02-01 3:06 ` Supriya Kannery
2012-02-02 0:09 ` Michael Roth
2012-02-08 12:07 ` Luiz Capitulino
2012-02-01 3:06 ` [Qemu-devel] [RFC Patch 4/7]Qemu: Framework for reopening image files safely Supriya Kannery
` (4 subsequent siblings)
7 siblings, 2 replies; 30+ messages in thread
From: Supriya Kannery @ 2012-02-01 3:06 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Stefan Hajnoczi, Christoph Hellwig, Luiz Capitulino
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
@@ -808,6 +808,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_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) {
@@ -870,6 +899,33 @@ void bdrv_drain_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 */
+ 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
@@ -119,6 +119,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);
@@ -160,6 +161,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
@@ -1080,3 +1080,29 @@ 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.
+*/
+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
@@ -62,4 +62,6 @@ void qmp_change_blockdev(const char *dev
bool has_format, const char *format, Error **errp);
void do_commit(Monitor *mon, const QDict *qdict);
int do_drive_del(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
@@ -796,6 +796,20 @@ ETEXI
.mhandler.cmd_new = do_migrate,
},
+ {
+ .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{option}
+@findex block_set_hostcache
+Change host pagecache setting of a block device while guest is running.
+ETEXI
+
STEXI
@item migrate [-d] [-b] [-i] @var{uri}
Index: qemu/qmp-commands.hx
===================================================================
--- qemu.orig/qmp-commands.hx
+++ qemu/qmp-commands.hx
@@ -698,7 +698,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",
.mhandler.cmd_new = qmp_marshal_input_balloon,
^ permalink raw reply [flat|nested] 30+ messages in thread
* [Qemu-devel] [RFC Patch 4/7]Qemu: Framework for reopening image files safely
2012-02-01 3:05 [Qemu-devel] [RFC Patch 0/7]Qemu: Dynamic host pagecache change Supriya Kannery
` (2 preceding siblings ...)
2012-02-01 3:06 ` [Qemu-devel] [RFC Patch 3/7]Qemu: Cmd "block_set_hostcache" for dynamic cache change Supriya Kannery
@ 2012-02-01 3:06 ` Supriya Kannery
2012-02-07 10:08 ` Stefan Hajnoczi
2012-02-08 15:07 ` Kevin Wolf
2012-02-01 3:07 ` [Qemu-devel] [RFC Patch 5/7]Qemu: raw-posix image file reopen Supriya Kannery
` (3 subsequent siblings)
7 siblings, 2 replies; 30+ messages in thread
From: Supriya Kannery @ 2012-02-01 3:06 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Stefan Hajnoczi, Christoph Hellwig, Luiz Capitulino
Struct BDRVReopenState along with three reopen related functions
introduced for handling reopening of images safely. This can be
extended by each of the block drivers to reopen respective
image files.
Signed-off-by: Supriya Kannery <supriyak@linux.vnet.ibm.com>
Index: qemu/block.c
===================================================================
--- qemu.orig/block.c
+++ qemu/block.c
@@ -808,10 +808,32 @@ unlink_and_fail:
return ret;
}
+int bdrv_reopen_prepare(BlockDriverState *bs, BDRVReopenState **prs, int flags)
+{
+ BlockDriver *drv = bs->drv;
+
+ return drv->bdrv_reopen_prepare(bs, prs, flags);
+}
+
+void bdrv_reopen_commit(BlockDriverState *bs, BDRVReopenState *rs)
+{
+ BlockDriver *drv = bs->drv;
+
+ drv->bdrv_reopen_commit(bs, rs);
+}
+
+void bdrv_reopen_abort(BlockDriverState *bs, BDRVReopenState *rs)
+{
+ BlockDriver *drv = bs->drv;
+
+ drv->bdrv_reopen_abort(bs, rs);
+}
+
int bdrv_reopen(BlockDriverState *bs, int bdrv_flags)
{
BlockDriver *drv = bs->drv;
int ret = 0, open_flags;
+ BDRVReopenState *reopen_state = NULL;
/* Quiesce IO for the given block device */
qemu_aio_flush();
@@ -820,17 +842,32 @@ 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 = bdrv_reopen_prepare(bs, &reopen_state, bdrv_flags);
+ if (ret < 0) {
+ bdrv_reopen_abort(bs, reopen_state);
+ qerror_report(QERR_REOPEN_FILE_FAILED, bs->filename);
+ return ret;
+ }
+
+ bdrv_reopen_commit(bs, reopen_state);
+ bs->open_flags = bdrv_flags;
+
+ } else {
+ open_flags = bs->open_flags;
+ bdrv_close(bs);
+
+ ret = bdrv_open(bs, bs->filename, bdrv_flags, drv);
if (ret < 0) {
- /* Reopen failed with orig and modified flags */
- abort();
+ /* 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 */
+ bs->drv = NULL;
+ }
}
}
Index: qemu/block_int.h
===================================================================
--- qemu.orig/block_int.h
+++ qemu/block_int.h
@@ -105,6 +105,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 **prs,
+ int flags);
+ void (*bdrv_reopen_commit)(BlockDriverState *bs, BDRVReopenState *rs);
+ void (*bdrv_reopen_abort)(BlockDriverState *bs, 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);
@@ -299,6 +306,10 @@ struct BlockDriverState {
BlockJob *job;
};
+struct BDRVReopenState {
+ BlockDriverState *bs;
+};
+
struct BlockDriverAIOCB {
AIOPool *pool;
BlockDriverState *bs;
Index: qemu/qemu-common.h
===================================================================
--- qemu.orig/qemu-common.h
+++ qemu/qemu-common.h
@@ -210,6 +210,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.h
===================================================================
--- qemu.orig/block.h
+++ qemu/block.h
@@ -120,6 +120,9 @@ int bdrv_file_open(BlockDriverState **pb
int bdrv_open(BlockDriverState *bs, const char *filename, int flags,
BlockDriver *drv);
int bdrv_reopen(BlockDriverState *bs, int bdrv_flags);
+int bdrv_reopen_prepare(BlockDriverState *bs, BDRVReopenState **prs, int flags);
+void bdrv_reopen_commit(BlockDriverState *bs, BDRVReopenState *rs);
+void bdrv_reopen_abort(BlockDriverState *bs, BDRVReopenState *rs);
void bdrv_close(BlockDriverState *bs);
int bdrv_attach_dev(BlockDriverState *bs, void *dev);
void bdrv_attach_dev_nofail(BlockDriverState *bs, void *dev);
^ permalink raw reply [flat|nested] 30+ messages in thread
* [Qemu-devel] [RFC Patch 5/7]Qemu: raw-posix image file reopen
2012-02-01 3:05 [Qemu-devel] [RFC Patch 0/7]Qemu: Dynamic host pagecache change Supriya Kannery
` (3 preceding siblings ...)
2012-02-01 3:06 ` [Qemu-devel] [RFC Patch 4/7]Qemu: Framework for reopening image files safely Supriya Kannery
@ 2012-02-01 3:07 ` Supriya Kannery
2012-02-02 0:15 ` Michael Roth
` (2 more replies)
2012-02-01 3:07 ` [Qemu-devel] [RFC Patch 6/7]Qemu: raw-win32 " Supriya Kannery
` (2 subsequent siblings)
7 siblings, 3 replies; 30+ messages in thread
From: Supriya Kannery @ 2012-02-01 3:07 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Stefan Hajnoczi, Christoph Hellwig, Luiz Capitulino
raw-posix driver changes for bdrv_reopen_xx functions to
safely reopen image files. Reopening of image files while
changing hostcache dynamically is handled here.
Signed-off-by: Supriya Kannery <supriyak@linux.vnet.ibm.com>
Index: qemu/block/raw.c
===================================================================
--- qemu.orig/block/raw.c
+++ qemu/block/raw.c
@@ -9,6 +9,22 @@ static int raw_open(BlockDriverState *bs
return 0;
}
+static int raw_reopen_prepare(BlockDriverState *bs, BDRVReopenState **prs,
+ int flags)
+{
+ return bdrv_reopen_prepare(bs->file, prs, flags);
+}
+
+static void raw_reopen_commit(BlockDriverState *bs, BDRVReopenState *rs)
+{
+ bdrv_reopen_commit(bs->file, rs);
+}
+
+static void raw_reopen_abort(BlockDriverState *bs, BDRVReopenState *rs)
+{
+ bdrv_reopen_abort(bs->file, rs);
+}
+
static int coroutine_fn raw_co_readv(BlockDriverState *bs, int64_t sector_num,
int nb_sectors, QEMUIOVector *qiov)
{
@@ -109,6 +125,10 @@ static BlockDriver bdrv_raw = {
.instance_size = 1,
.bdrv_open = raw_open,
+ .bdrv_reopen_prepare
+ = raw_reopen_prepare,
+ .bdrv_reopen_commit = raw_reopen_commit,
+ .bdrv_reopen_abort = raw_reopen_abort,
.bdrv_close = raw_close,
.bdrv_co_readv = raw_co_readv,
Index: qemu/block/raw-posix.c
===================================================================
--- qemu.orig/block/raw-posix.c
+++ qemu/block/raw-posix.c
@@ -136,6 +136,11 @@ typedef struct BDRVRawState {
#endif
} BDRVRawState;
+typedef struct BDRVRawReopenState {
+ BDRVReopenState reopen_state;
+ BDRVRawState *stash_s;
+} BDRVRawReopenState;
+
static int fd_open(BlockDriverState *bs);
static int64_t raw_getlength(BlockDriverState *bs);
@@ -279,6 +284,71 @@ static int raw_open(BlockDriverState *bs
return raw_open_common(bs, filename, flags, 0);
}
+static int raw_reopen_prepare(BlockDriverState *bs, BDRVReopenState **prs,
+ int flags)
+{
+ BDRVRawReopenState *raw_rs = g_malloc0(sizeof(BDRVRawReopenState));
+ BDRVRawState *s = bs->opaque;
+ int ret = 0;
+
+ raw_rs->reopen_state.bs = bs;
+
+ /* stash state before reopen */
+ raw_rs->stash_s = g_malloc0(sizeof(BDRVRawState));
+ memcpy(raw_rs->stash_s, s, sizeof(BDRVRawState));
+ s->fd = dup(raw_rs->stash_s->fd);
+
+ *prs = &(raw_rs->reopen_state);
+
+ /* Flags that can be set using fcntl */
+ int fcntl_flags = BDRV_O_NOCACHE;
+
+ if ((bs->open_flags & ~fcntl_flags) == (flags & ~fcntl_flags)) {
+ if ((flags & BDRV_O_NOCACHE)) {
+ s->open_flags |= O_DIRECT;
+ } else {
+ s->open_flags &= ~O_DIRECT;
+ }
+ printf("O_DIRECT flag\n");
+ ret = fcntl_setfl(s->fd, s->open_flags);
+ } else {
+
+ printf("close and open with new flags\n");
+ /* close and reopen using new flags */
+ bs->drv->bdrv_close(bs);
+ ret = bs->drv->bdrv_file_open(bs, bs->filename, flags);
+ }
+ return ret;
+}
+
+static void raw_reopen_commit(BlockDriverState *bs, BDRVReopenState *rs)
+{
+ BDRVRawReopenState *raw_rs;
+
+ raw_rs = container_of(rs, BDRVRawReopenState, reopen_state);
+
+ /* clean up stashed state */
+ close(raw_rs->stash_s->fd);
+ g_free(raw_rs->stash_s);
+ g_free(raw_rs);
+}
+
+static void raw_reopen_abort(BlockDriverState *bs, BDRVReopenState *rs)
+{
+ BDRVRawReopenState *raw_rs;
+ BDRVRawState *s = bs->opaque;
+
+ raw_rs = container_of(rs, BDRVRawReopenState, reopen_state);
+
+ /* revert to stashed state */
+ if (s->fd != -1) {
+ close(s->fd);
+ }
+ memcpy(s, raw_rs->stash_s, sizeof(BDRVRawState));
+ g_free(raw_rs->stash_s);
+ g_free(raw_rs);
+}
+
/* XXX: use host sector size if necessary with:
#ifdef DIOCGSECTORSIZE
{
@@ -631,6 +701,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_close = raw_close,
.bdrv_create = raw_create,
.bdrv_co_discard = raw_co_discard,
^ permalink raw reply [flat|nested] 30+ messages in thread
* [Qemu-devel] [RFC Patch 6/7]Qemu: raw-win32 image file reopen
2012-02-01 3:05 [Qemu-devel] [RFC Patch 0/7]Qemu: Dynamic host pagecache change Supriya Kannery
` (4 preceding siblings ...)
2012-02-01 3:07 ` [Qemu-devel] [RFC Patch 5/7]Qemu: raw-posix image file reopen Supriya Kannery
@ 2012-02-01 3:07 ` Supriya Kannery
2012-02-08 15:02 ` Kevin Wolf
2012-02-01 3:07 ` [Qemu-devel] [RFC Patch 7/7]Qemu: vmdk " Supriya Kannery
2012-02-01 22:41 ` [Qemu-devel] [RFC Patch 0/7]Qemu: Dynamic host pagecache change Eric Blake
7 siblings, 1 reply; 30+ messages in thread
From: Supriya Kannery @ 2012-02-01 3:07 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Stefan Hajnoczi, Christoph Hellwig, Luiz Capitulino
win32 driver changes for bdrv_reopen_xx functions to
safely reopen image files. Reopening of image files while
changing hostcache dynamically is handled here.
Signed-off-by: Supriya Kannery <supriyak@linux.vnet.ibm.com>
Index: qemu/block/raw-win32.c
===================================================================
--- qemu.orig/block/raw-win32.c
+++ qemu/block/raw-win32.c
@@ -26,18 +26,27 @@
#include "block_int.h"
#include "module.h"
#include <windows.h>
+#include <winbase.h>
#include <winioctl.h>
#define FTYPE_FILE 0
#define FTYPE_CD 1
#define FTYPE_HARDDISK 2
+#define WINDOWS_VISTA 6
typedef struct BDRVRawState {
HANDLE hfile;
int type;
char drive_path[16]; /* format: "d:\" */
+ DWORD overlapped;
} BDRVRawState;
+typedef struct BDRVRawReopenState {
+ BDRVReopenState reopen_state;
+ HANDLE stash_hfile;
+ DWORD stash_overlapped;
+} BDRVRawReopenState;
+
int qemu_ftruncate64(int fd, int64_t length)
{
LARGE_INTEGER li;
@@ -106,9 +115,96 @@ static int raw_open(BlockDriverState *bs
return -EACCES;
return -1;
}
+ s->overlapped = overlapped;
return 0;
}
+static int raw_reopen_prepare(BlockDriverState *bs, BDRVReopenState **prs,
+ int flags)
+{
+ BDRVRawReopenState *raw_rs = g_malloc0(sizeof(BDRVRawReopenState));
+ BDRVRawState *s = bs->opaque;
+ int ret = 0;
+ OSVERSIONINFO osvi;
+ BOOL bIsWindowsVistaorLater;
+
+ raw_rs->bs = bs;
+ raw_rs->stash_hfile = s->hfile;
+ raw_rs->stash_overlapped = s->overlapped;
+ *prs = raw_rs;
+
+ if (flags & BDRV_O_NOCACHE) {
+ s->overlapped |= FILE_FLAG_NO_BUFFERING;
+ } else {
+ s->overlapped &= ~FILE_FLAG_NO_BUFFERING;
+ }
+
+ if (!(flags & BDRV_O_CACHE_WB)) {
+ s->overlapped |= FILE_FLAG_WRITE_THROUGH;
+ } else {
+ s->overlapped &= ~FILE_FLAG_WRITE_THROUGH;
+ }
+
+ ZeroMemory(&osvi, sizeof(OSVERSIONINFO));
+ osvi.dwOSVersionInfoSize = sizeof(OSVERSIONINFO);
+
+ GetVersionEx(&osvi);
+
+ if (osvi.dwMajorVersion >= WINDOWS_VISTA) {
+ s->hfile = ReOpenFile(raw_rs->stash_hfile, 0, FILE_SHARE_READ,
+ overlapped);
+ if (s->hfile == INVALID_HANDLE_VALUE) {
+ int err = GetLastError();
+ if (err == ERROR_ACCESS_DENIED) {
+ ret = -EACCES;
+ } else {
+ ret = -1;
+ }
+ }
+ } else {
+
+ DuplicateHandle(GetCurrentProcess(),
+ raw_rs->stash_hfile,
+ GetCurrentProcess(),
+ &s->hfile,
+ 0,
+ FALSE,
+ DUPLICATE_SAME_ACCESS);
+ bs->drv->bdrv_close(bs);
+ ret = bs->drv->bdrv_open(bs, bs->filename, flags);
+ }
+ return ret;
+}
+
+
+static void raw_reopen_commit(BlockDriverState *bs, BDRVReopenState *rs)
+{
+ BDRVRawReopenState *raw_rs;
+
+ raw_rs = container_of(rs, BDRVRawReopenState, reopen_state);
+
+ /* clean up stashed handle */
+ CloseHandle(raw_rs->stash_hfile);
+ g_free(raw_rs);
+
+}
+
+static void raw_reopen_abort(BlockDriverState *bs, BDRVReopenState *rs)
+{
+
+ BDRVRawReopenState *raw_rs;
+ BDRVRawState *s = bs->opaque;
+
+ raw_rs = container_of(rs, BDRVRawReopenState, reopen_state);
+
+ if (s->hfile && (s->hfile != INVALID_HANDLE_VALUE)) {
+ CloseHandle(s->hfile);
+ }
+ s->hfile = raw_rs->stash_hfile;
+ s->overlapped = raw_rs->stash_overlapped;
+ g_free(raw_rs);
+}
+
static int raw_read(BlockDriverState *bs, int64_t sector_num,
uint8_t *buf, int nb_sectors)
{
^ permalink raw reply [flat|nested] 30+ messages in thread
* [Qemu-devel] [RFC Patch 7/7]Qemu: vmdk image file reopen
2012-02-01 3:05 [Qemu-devel] [RFC Patch 0/7]Qemu: Dynamic host pagecache change Supriya Kannery
` (5 preceding siblings ...)
2012-02-01 3:07 ` [Qemu-devel] [RFC Patch 6/7]Qemu: raw-win32 " Supriya Kannery
@ 2012-02-01 3:07 ` Supriya Kannery
2012-02-01 22:41 ` [Qemu-devel] [RFC Patch 0/7]Qemu: Dynamic host pagecache change Eric Blake
7 siblings, 0 replies; 30+ messages in thread
From: Supriya Kannery @ 2012-02-01 3:07 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Stefan Hajnoczi, Christoph Hellwig, Luiz Capitulino
vmdk driver changes for bdrv_reopen_xx functions to
safely reopen image files. Reopening of image files while
changing hostcache flag dynamically is handled here.
Signed-off-by: Supriya Kannery <supriyak@linux.vnet.ibm.com>
Index: qemu/block/vmdk.c
===================================================================
--- qemu.orig/block/vmdk.c
+++ qemu/block/vmdk.c
@@ -115,6 +115,11 @@ typedef struct VmdkGrainMarker {
uint8_t data[0];
} VmdkGrainMarker;
+typedef struct BDRVVmdkReopenState {
+ BDRVReopenState reopen_state;
+ BDRVVmdkState *stash_s;
+} BDRVVmdkReopenState;
+
static int vmdk_probe(const uint8_t *buf, int buf_size, const char *filename)
{
uint32_t magic;
@@ -588,7 +593,6 @@ static int vmdk_parse_extents(const char
if (!strcmp(type, "FLAT")) {
/* FLAT extent */
VmdkExtent *extent;
-
extent = vmdk_add_extent(bs, extent_file, true, sectors,
0, 0, 0, 0, sectors);
extent->flat_start_offset = flat_offset << 9;
@@ -675,6 +679,71 @@ fail:
return ret;
}
+static int vmdk_reopen_prepare(BlockDriverState *bs, BDRVReopenState **prs,
+ int flags)
+{
+ BDRVVmdkReopenState *vmdk_rs = g_malloc0(sizeof(BDRVVmdkReopenState));
+ int ret = 0;
+ BDRVVmdkState *s = bs->opaque;
+
+ vmdk_rs->reopen_state.bs = bs;
+
+ /* save state before reopen */
+ vmdk_rs->stash_s = g_malloc0(sizeof(BDRVVmdkState));
+ memcpy(vmdk_rs->stash_s, s, sizeof(BDRVVmdkState));
+ s->num_extents = 0;
+ s->extents = NULL ;
+ *prs = &(vmdk_rs->reopen_state);
+
+ /* create extents afresh with new flags */
+ ret = vmdk_open(bs, flags);
+ return ret;
+}
+
+static void vmdk_reopen_commit(BlockDriverState *bs, BDRVReopenState *rs)
+{
+ BDRVVmdkReopenState *vmdk_rs;
+ BDRVVmdkState *stashed_s;
+ VmdkExtent *e;
+ int i;
+
+ vmdk_rs = container_of(rs, BDRVVmdkReopenState, reopen_state);
+ stashed_s = vmdk_rs->stash_s;
+
+ /* clean up stashed state */
+ for (i = 0; i < stashed_s->num_extents; i++) {
+ e = &stashed_s->extents[i];
+ g_free(e->l1_table);
+ g_free(e->l2_cache);
+ g_free(e->l1_backup_table);
+ }
+ g_free(stashed_s->extents);
+ g_free(vmdk_rs->stash_s);
+ g_free(vmdk_rs);
+}
+
+static void vmdk_reopen_abort(BlockDriverState *bs, BDRVReopenState *rs)
+{
+ BDRVVmdkReopenState *vmdk_rs;
+ BDRVVmdkState *s = bs->opaque;
+ VmdkExtent *e;
+ int i;
+
+ vmdk_rs = container_of(rs, BDRVVmdkReopenState, reopen_state);
+
+ /* revert to stashed state */
+ for (i = 0; i < s->num_extents; i++) {
+ e = &s->extents[i];
+ g_free(e->l1_table);
+ g_free(e->l2_cache);
+ g_free(e->l1_backup_table);
+ }
+ g_free(s->extents);
+ memcpy(s, vmdk_rs->stash_s, sizeof(BDRVVmdkState));
+ g_free(vmdk_rs->stash_s);
+ g_free(vmdk_rs);
+}
+
static int get_whole_cluster(BlockDriverState *bs,
VmdkExtent *extent,
uint64_t cluster_offset,
@@ -1382,7 +1451,6 @@ static int vmdk_create(const char *filen
if (filename_decompose(filename, path, prefix, postfix, PATH_MAX)) {
return -EINVAL;
}
- printf("vmdk_create\n");
/* Read out options */
while (options && options->name) {
if (!strcmp(options->name, BLOCK_OPT_SIZE)) {
@@ -1517,8 +1585,8 @@ exit:
static void vmdk_close(BlockDriverState *bs)
{
BDRVVmdkState *s = bs->opaque;
-
printf("vmdk_close\n");
+
vmdk_free_extents(bs);
migrate_del_blocker(s->migration_blocker);
@@ -1595,6 +1663,12 @@ static BlockDriver bdrv_vmdk = {
.instance_size = sizeof(BDRVVmdkState),
.bdrv_probe = vmdk_probe,
.bdrv_open = vmdk_open,
+ .bdrv_reopen_prepare
+ = vmdk_reopen_prepare,
+ .bdrv_reopen_commit
+ = vmdk_reopen_commit,
+ .bdrv_reopen_abort
+ = vmdk_reopen_abort,
.bdrv_read = vmdk_co_read,
.bdrv_write = vmdk_co_write,
.bdrv_close = vmdk_close,
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [RFC Patch 0/7]Qemu: Dynamic host pagecache change
2012-02-01 3:05 [Qemu-devel] [RFC Patch 0/7]Qemu: Dynamic host pagecache change Supriya Kannery
` (6 preceding siblings ...)
2012-02-01 3:07 ` [Qemu-devel] [RFC Patch 7/7]Qemu: vmdk " Supriya Kannery
@ 2012-02-01 22:41 ` Eric Blake
2012-02-02 9:12 ` Kevin Wolf
7 siblings, 1 reply; 30+ messages in thread
From: Eric Blake @ 2012-02-01 22:41 UTC (permalink / raw)
To: Supriya Kannery
Cc: Kevin Wolf, Stefan Hajnoczi, Luiz Capitulino, qemu-devel,
Christoph Hellwig
[-- Attachment #1: Type: text/plain, Size: 1508 bytes --]
On 01/31/2012 08:05 PM, Supriya Kannery wrote:
> For changing host pagecache setting of a running VM, it is
> important to have a safe way of reopening its image file.
>
> Following patchset introduces:
> * a generic way to reopen image files safely.
> In this approach, before reopening an image, for each
> block driver, its state will be stashed. Incase preparation
> (bdrv_reopen_prepare) for reopening returns success, the stashed
> state will be cleared (bdrv_reopen_commit) and reopened state will
> be used further. Incase preparation of reopening returns failure,
> the state of the driver will be rolled back (bdrv_reopen_abort)
> to the stashed state.
> This approach is extended to raw-posix, raw-win32 and vmdk block
> drivers in this patchset. Once this is reviewed and finalised, I will
> extend the implementation to other drivers like qcow2, qed etc..
How will this interplay with the goal of passing images in by fd rather
than by name? I'd really like to start thinking about how we plan on
coordinating situations where an fd has to be reopened in order to
switch flags (such as O_RDONLY becoming O_RDWR, or adding or subtracting
O_DIRECT), but where SELinux or other isolation means that the
management app (such as libvirt) has to do the open and pass the fd via
'getfd' monitor command.
--
Eric Blake eblake@redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 620 bytes --]
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [RFC Patch 3/7]Qemu: Cmd "block_set_hostcache" for dynamic cache change
2012-02-01 3:06 ` [Qemu-devel] [RFC Patch 3/7]Qemu: Cmd "block_set_hostcache" for dynamic cache change Supriya Kannery
@ 2012-02-02 0:09 ` Michael Roth
2012-02-02 10:14 ` Kevin Wolf
2012-02-08 12:07 ` Luiz Capitulino
1 sibling, 1 reply; 30+ messages in thread
From: Michael Roth @ 2012-02-02 0:09 UTC (permalink / raw)
To: Supriya Kannery
Cc: Kevin Wolf, Stefan Hajnoczi, Luiz Capitulino, qemu-devel,
Christoph Hellwig
On 01/31/2012 09:06 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
> @@ -808,6 +808,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_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) {
> @@ -870,6 +899,33 @@ void bdrv_drain_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 */
> + bdrv_flags&= ~BDRV_O_CACHE_WB;
> + return bdrv_reopen(bs, bdrv_flags);
It seems like the real interface we're wanting here is bdrv_set_flags(),
or something along that line, with the re-opening being more of an
implementation detail.
For instance, with raw-posix.c:raw_reopen_prepare() we'll end up
skipping the re-opening completely if fcntl() is sufficient.
> + } 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
> @@ -119,6 +119,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);
> @@ -160,6 +161,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
> @@ -1080,3 +1080,29 @@ 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.
> +*/
> +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
> @@ -62,4 +62,6 @@ void qmp_change_blockdev(const char *dev
> bool has_format, const char *format, Error **errp);
> void do_commit(Monitor *mon, const QDict *qdict);
> int do_drive_del(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
> @@ -796,6 +796,20 @@ ETEXI
> .mhandler.cmd_new = do_migrate,
> },
>
> + {
> + .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{option}
> +@findex block_set_hostcache
> +Change host pagecache setting of a block device while guest is running.
> +ETEXI
> +
>
> STEXI
> @item migrate [-d] [-b] [-i] @var{uri}
> Index: qemu/qmp-commands.hx
> ===================================================================
> --- qemu.orig/qmp-commands.hx
> +++ qemu/qmp-commands.hx
> @@ -698,7 +698,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",
> .mhandler.cmd_new = qmp_marshal_input_balloon,
>
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [RFC Patch 5/7]Qemu: raw-posix image file reopen
2012-02-01 3:07 ` [Qemu-devel] [RFC Patch 5/7]Qemu: raw-posix image file reopen Supriya Kannery
@ 2012-02-02 0:15 ` Michael Roth
2012-02-13 13:12 ` Supriya Kannery
2012-02-07 10:17 ` Stefan Hajnoczi
2012-02-08 14:54 ` Kevin Wolf
2 siblings, 1 reply; 30+ messages in thread
From: Michael Roth @ 2012-02-02 0:15 UTC (permalink / raw)
To: Supriya Kannery
Cc: Kevin Wolf, Stefan Hajnoczi, Luiz Capitulino, qemu-devel,
Christoph Hellwig
On 01/31/2012 09:07 PM, Supriya Kannery wrote:
> raw-posix driver changes for bdrv_reopen_xx functions to
> safely reopen image files. Reopening of image files while
> changing hostcache dynamically is handled here.
>
> Signed-off-by: Supriya Kannery<supriyak@linux.vnet.ibm.com>
>
> Index: qemu/block/raw.c
> ===================================================================
> --- qemu.orig/block/raw.c
> +++ qemu/block/raw.c
> @@ -9,6 +9,22 @@ static int raw_open(BlockDriverState *bs
> return 0;
> }
>
> +static int raw_reopen_prepare(BlockDriverState *bs, BDRVReopenState **prs,
> + int flags)
> +{
> + return bdrv_reopen_prepare(bs->file, prs, flags);
> +}
> +
> +static void raw_reopen_commit(BlockDriverState *bs, BDRVReopenState *rs)
> +{
> + bdrv_reopen_commit(bs->file, rs);
> +}
> +
> +static void raw_reopen_abort(BlockDriverState *bs, BDRVReopenState *rs)
> +{
> + bdrv_reopen_abort(bs->file, rs);
> +}
> +
> static int coroutine_fn raw_co_readv(BlockDriverState *bs, int64_t sector_num,
> int nb_sectors, QEMUIOVector *qiov)
> {
> @@ -109,6 +125,10 @@ static BlockDriver bdrv_raw = {
> .instance_size = 1,
>
> .bdrv_open = raw_open,
> + .bdrv_reopen_prepare
> + = raw_reopen_prepare,
> + .bdrv_reopen_commit = raw_reopen_commit,
> + .bdrv_reopen_abort = raw_reopen_abort,
> .bdrv_close = raw_close,
>
> .bdrv_co_readv = raw_co_readv,
> Index: qemu/block/raw-posix.c
> ===================================================================
> --- qemu.orig/block/raw-posix.c
> +++ qemu/block/raw-posix.c
> @@ -136,6 +136,11 @@ typedef struct BDRVRawState {
> #endif
> } BDRVRawState;
>
> +typedef struct BDRVRawReopenState {
> + BDRVReopenState reopen_state;
> + BDRVRawState *stash_s;
> +} BDRVRawReopenState;
> +
> static int fd_open(BlockDriverState *bs);
> static int64_t raw_getlength(BlockDriverState *bs);
>
> @@ -279,6 +284,71 @@ static int raw_open(BlockDriverState *bs
> return raw_open_common(bs, filename, flags, 0);
> }
>
> +static int raw_reopen_prepare(BlockDriverState *bs, BDRVReopenState **prs,
> + int flags)
> +{
> + BDRVRawReopenState *raw_rs = g_malloc0(sizeof(BDRVRawReopenState));
> + BDRVRawState *s = bs->opaque;
> + int ret = 0;
> +
> + raw_rs->reopen_state.bs = bs;
> +
> + /* stash state before reopen */
> + raw_rs->stash_s = g_malloc0(sizeof(BDRVRawState));
> + memcpy(raw_rs->stash_s, s, sizeof(BDRVRawState));
> + s->fd = dup(raw_rs->stash_s->fd);
> +
> + *prs =&(raw_rs->reopen_state);
> +
> + /* Flags that can be set using fcntl */
> + int fcntl_flags = BDRV_O_NOCACHE;
> +
> + if ((bs->open_flags& ~fcntl_flags) == (flags& ~fcntl_flags)) {
> + if ((flags& BDRV_O_NOCACHE)) {
> + s->open_flags |= O_DIRECT;
> + } else {
> + s->open_flags&= ~O_DIRECT;
> + }
> + printf("O_DIRECT flag\n");
> + ret = fcntl_setfl(s->fd, s->open_flags);
raw-posix.c:raw_aio_submit() does some extra alignment work if
s->aligned_buf was set due to the image being opened O_DIRECT initially,
not sure what the impact is but probably want to clean that up here.
> + } else {
> +
> + printf("close and open with new flags\n");
> + /* close and reopen using new flags */
> + bs->drv->bdrv_close(bs);
> + ret = bs->drv->bdrv_file_open(bs, bs->filename, flags);
> + }
> + return ret;
> +}
> +
> +static void raw_reopen_commit(BlockDriverState *bs, BDRVReopenState *rs)
> +{
> + BDRVRawReopenState *raw_rs;
> +
> + raw_rs = container_of(rs, BDRVRawReopenState, reopen_state);
> +
> + /* clean up stashed state */
> + close(raw_rs->stash_s->fd);
> + g_free(raw_rs->stash_s);
> + g_free(raw_rs);
> +}
> +
> +static void raw_reopen_abort(BlockDriverState *bs, BDRVReopenState *rs)
> +{
> + BDRVRawReopenState *raw_rs;
> + BDRVRawState *s = bs->opaque;
> +
> + raw_rs = container_of(rs, BDRVRawReopenState, reopen_state);
> +
> + /* revert to stashed state */
> + if (s->fd != -1) {
> + close(s->fd);
> + }
> + memcpy(s, raw_rs->stash_s, sizeof(BDRVRawState));
> + g_free(raw_rs->stash_s);
> + g_free(raw_rs);
> +}
> +
> /* XXX: use host sector size if necessary with:
> #ifdef DIOCGSECTORSIZE
> {
> @@ -631,6 +701,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_close = raw_close,
> .bdrv_create = raw_create,
> .bdrv_co_discard = raw_co_discard,
>
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [RFC Patch 0/7]Qemu: Dynamic host pagecache change
2012-02-01 22:41 ` [Qemu-devel] [RFC Patch 0/7]Qemu: Dynamic host pagecache change Eric Blake
@ 2012-02-02 9:12 ` Kevin Wolf
0 siblings, 0 replies; 30+ messages in thread
From: Kevin Wolf @ 2012-02-02 9:12 UTC (permalink / raw)
To: Eric Blake
Cc: Supriya Kannery, Luiz Capitulino, Christoph Hellwig, qemu-devel,
Stefan Hajnoczi
Am 01.02.2012 23:41, schrieb Eric Blake:
> On 01/31/2012 08:05 PM, Supriya Kannery wrote:
>> For changing host pagecache setting of a running VM, it is
>> important to have a safe way of reopening its image file.
>>
>> Following patchset introduces:
>> * a generic way to reopen image files safely.
>> In this approach, before reopening an image, for each
>> block driver, its state will be stashed. Incase preparation
>> (bdrv_reopen_prepare) for reopening returns success, the stashed
>> state will be cleared (bdrv_reopen_commit) and reopened state will
>> be used further. Incase preparation of reopening returns failure,
>> the state of the driver will be rolled back (bdrv_reopen_abort)
>> to the stashed state.
>> This approach is extended to raw-posix, raw-win32 and vmdk block
>> drivers in this patchset. Once this is reviewed and finalised, I will
>> extend the implementation to other drivers like qcow2, qed etc..
>
> How will this interplay with the goal of passing images in by fd rather
> than by name? I'd really like to start thinking about how we plan on
> coordinating situations where an fd has to be reopened in order to
> switch flags (such as O_RDONLY becoming O_RDWR, or adding or subtracting
> O_DIRECT), but where SELinux or other isolation means that the
> management app (such as libvirt) has to do the open and pass the fd via
> 'getfd' monitor command.
That's easy: Either your host OS allows to change the respective flag
using fcntl(), which I believe is true for O_DIRECT/O_DSYNC and recent
Linux kernels, or you're out of luck.
Is there any reason for switching between rw/ro other than modifying the
backing file chain (i.e. creating or deleting a snapshot)?
Kevin
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [RFC Patch 3/7]Qemu: Cmd "block_set_hostcache" for dynamic cache change
2012-02-02 0:09 ` Michael Roth
@ 2012-02-02 10:14 ` Kevin Wolf
0 siblings, 0 replies; 30+ messages in thread
From: Kevin Wolf @ 2012-02-02 10:14 UTC (permalink / raw)
To: Michael Roth
Cc: Supriya Kannery, Stefan Hajnoczi, Jeff Cody, qemu-devel,
Luiz Capitulino, Christoph Hellwig
Am 02.02.2012 01:09, schrieb Michael Roth:
> On 01/31/2012 09:06 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
>> @@ -808,6 +808,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_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) {
>> @@ -870,6 +899,33 @@ void bdrv_drain_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 */
>> + bdrv_flags&= ~BDRV_O_CACHE_WB;
>> + return bdrv_reopen(bs, bdrv_flags);
>
> It seems like the real interface we're wanting here is bdrv_set_flags(),
> or something along that line, with the re-opening being more of an
> implementation detail.
>
> For instance, with raw-posix.c:raw_reopen_prepare() we'll end up
> skipping the re-opening completely if fcntl() is sufficient.
It's reopening a BlockDriverState, not necessarily reopening the image
file that backs it.
bdrv_set_flags() would be good name for what this series is doing, but
I've been thinking about adding a way to actually switch the image file
as well. We could need this for implementing external snapshots of
multiple images atomically.
Kevin
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [RFC Patch 2/7]Qemu: Error classes for file reopen and data sync failure
2012-02-01 3:06 ` [Qemu-devel] [RFC Patch 2/7]Qemu: Error classes for file reopen and data sync failure Supriya Kannery
@ 2012-02-07 7:56 ` Stefan Hajnoczi
2012-02-13 13:13 ` Supriya Kannery
0 siblings, 1 reply; 30+ messages in thread
From: Stefan Hajnoczi @ 2012-02-07 7:56 UTC (permalink / raw)
To: Supriya Kannery
Cc: Kevin Wolf, Christoph Hellwig, qemu-devel, Luiz Capitulino
On Wed, Feb 01, 2012 at 08:36:28AM +0530, Supriya Kannery wrote:
> Index: qemu/qerror.c
> ===================================================================
> --- qemu.orig/qerror.c
> +++ qemu/qerror.c
> @@ -108,6 +108,14 @@ static const QErrorStringTable qerror_ta
> .desc = "Device '%(device)' has multiple child busses",
> },
> {
> + .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)'",
> + },
The comment in qerror.c says:
"Please keep the entries in alphabetical order.
Use scripts/check-qerror.sh to check."
> + {
> .error_fmt = QERR_DEVICE_NO_BUS,
> .desc = "Device '%(device)' has no child bus",
> },
> Index: qemu/qerror.h
> ===================================================================
> --- qemu.orig/qerror.h
> +++ qemu/qerror.h
> @@ -117,6 +117,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 } }"
> +
Same here:
/*
* QError class list
* Please keep the definitions in alphabetical order.
* Use scripts/check-qerror.sh to check.
*/
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [RFC Patch 4/7]Qemu: Framework for reopening image files safely
2012-02-01 3:06 ` [Qemu-devel] [RFC Patch 4/7]Qemu: Framework for reopening image files safely Supriya Kannery
@ 2012-02-07 10:08 ` Stefan Hajnoczi
2012-02-14 13:34 ` Supriya Kannery
2012-02-08 15:07 ` Kevin Wolf
1 sibling, 1 reply; 30+ messages in thread
From: Stefan Hajnoczi @ 2012-02-07 10:08 UTC (permalink / raw)
To: Supriya Kannery
Cc: Kevin Wolf, Christoph Hellwig, qemu-devel, Luiz Capitulino
On Wed, Feb 01, 2012 at 08:36:58AM +0530, Supriya Kannery wrote:
> Struct BDRVReopenState along with three reopen related functions
> introduced for handling reopening of images safely. This can be
> extended by each of the block drivers to reopen respective
> image files.
>
> Signed-off-by: Supriya Kannery <supriyak@linux.vnet.ibm.com>
>
> Index: qemu/block.c
> ===================================================================
> --- qemu.orig/block.c
> +++ qemu/block.c
> @@ -808,10 +808,32 @@ unlink_and_fail:
> return ret;
> }
>
> +int bdrv_reopen_prepare(BlockDriverState *bs, BDRVReopenState **prs, int flags)
> +{
> + BlockDriver *drv = bs->drv;
> +
> + return drv->bdrv_reopen_prepare(bs, prs, flags);
Indentation should be 4 spaces. I suggest configuring your editor so
this is always correct and done automatically.
> +}
> +
> +void bdrv_reopen_commit(BlockDriverState *bs, BDRVReopenState *rs)
> +{
> + BlockDriver *drv = bs->drv;
> +
> + drv->bdrv_reopen_commit(bs, rs);
> +}
> +
> +void bdrv_reopen_abort(BlockDriverState *bs, BDRVReopenState *rs)
> +{
> + BlockDriver *drv = bs->drv;
> +
> + drv->bdrv_reopen_abort(bs, rs);
> +}
> +
> int bdrv_reopen(BlockDriverState *bs, int bdrv_flags)
> {
> BlockDriver *drv = bs->drv;
> int ret = 0, open_flags;
> + BDRVReopenState *reopen_state = NULL;
>
> /* Quiesce IO for the given block device */
> qemu_aio_flush();
> @@ -820,17 +842,32 @@ 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 = bdrv_reopen_prepare(bs, &reopen_state, bdrv_flags);
> + if (ret < 0) {
Indentation
> + bdrv_reopen_abort(bs, reopen_state);
> + qerror_report(QERR_REOPEN_FILE_FAILED, bs->filename);
> + return ret;
> + }
> +
> + bdrv_reopen_commit(bs, reopen_state);
> + bs->open_flags = bdrv_flags;
Is bs->open_flags not assigned inside bdrv_reopen_*()?
> +
> + } else {
> + open_flags = bs->open_flags;
> + bdrv_close(bs);
> +
> + ret = bdrv_open(bs, bs->filename, bdrv_flags, drv);
> if (ret < 0) {
> - /* Reopen failed with orig and modified flags */
> - abort();
> + /* 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 */
> + bs->drv = NULL;
This should be a post-condition of bdrv_open(). If you have found a
case where bs->drv != NULL after bdrv_open() fails, then please fix that
code path instead of assigning NULL here.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [RFC Patch 5/7]Qemu: raw-posix image file reopen
2012-02-01 3:07 ` [Qemu-devel] [RFC Patch 5/7]Qemu: raw-posix image file reopen Supriya Kannery
2012-02-02 0:15 ` Michael Roth
@ 2012-02-07 10:17 ` Stefan Hajnoczi
2012-02-14 13:36 ` Supriya Kannery
2012-02-08 14:54 ` Kevin Wolf
2 siblings, 1 reply; 30+ messages in thread
From: Stefan Hajnoczi @ 2012-02-07 10:17 UTC (permalink / raw)
To: Supriya Kannery
Cc: Kevin Wolf, Christoph Hellwig, qemu-devel, Luiz Capitulino
On Wed, Feb 01, 2012 at 08:37:12AM +0530, Supriya Kannery wrote:
> + /* stash state before reopen */
> + raw_rs->stash_s = g_malloc0(sizeof(BDRVRawState));
> + memcpy(raw_rs->stash_s, s, sizeof(BDRVRawState));
Copying a struct is fragile, Mike Roth pointed out the potential issue
with aligned_buf.
If raw-posix could open from a given file descriptor as an alternative
to opening a filename, then it would be clean and natural to simply
re-initialize from the dup'd file descriptor in the abort case. That's
the approach I would try instead of stashing the whole struct.
Stefan
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [RFC Patch 1/7]Qemu: Enhance "info block" to display host cache setting
2012-02-01 3:06 ` [Qemu-devel] [RFC Patch 1/7]Qemu: Enhance "info block" to display host cache setting Supriya Kannery
@ 2012-02-08 12:00 ` Luiz Capitulino
2012-02-13 13:19 ` Supriya Kannery
0 siblings, 1 reply; 30+ messages in thread
From: Luiz Capitulino @ 2012-02-08 12:00 UTC (permalink / raw)
To: Supriya Kannery
Cc: Kevin Wolf, Stefan Hajnoczi, qemu-devel, Christoph Hellwig
On Wed, 01 Feb 2012 08:36:14 +0530
Supriya Kannery <supriyak@linux.vnet.ibm.com> wrote:
> Enhance "info block" to display hostcache setting for each
> block device.
>
> Example:
> (qemu) info block
> ide0-hd0: removable=0 file=../rhel6-32.raw ro=0 drv=raw encrypted=0
>
> Enhanced to display "hostcache" setting:
> (qemu) info block
> ide0-hd0: removable=0 hostcache=1 file=../rhel6-32.raw ro=0 drv=raw encrypted=0
The day we'll want to refactor 'info block' output is coming...
> 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/qapi-schema.json
> ===================================================================
> --- qemu.orig/qapi-schema.json
> +++ qemu/qapi-schema.json
> @@ -423,6 +423,8 @@
> # @locked: True if the guest has locked this device from having its media
> # removed
> #
> +# @hostcache: True if host pagecache is enabled.
> +#
> # @tray_open: #optional True if the device has a tray and it is open
> # (only present if removable is true)
> #
> @@ -436,7 +438,7 @@
> ##
> { 'type': 'BlockInfo',
> 'data': {'device': 'str', 'type': 'str', 'removable': 'bool',
> - 'locked': 'bool', '*inserted': 'BlockDeviceInfo',
> + 'locked': 'bool','hostcache': 'bool', '*inserted': 'BlockDeviceInfo',
> '*tray_open': 'bool', '*io-status': 'BlockDeviceIoStatus'} }
>
> ##
> Index: qemu/block.c
> ===================================================================
> --- qemu.orig/block.c
> +++ qemu/block.c
> @@ -2285,6 +2285,7 @@ BlockInfoList *qmp_query_block(Error **e
> info->value->device = g_strdup(bs->device_name);
> info->value->type = g_strdup("unknown");
> info->value->locked = bdrv_dev_is_medium_locked(bs);
> + info->value->hostcache = !(bs->open_flags & BDRV_O_NOCACHE);
> info->value->removable = bdrv_dev_has_removable_media(bs);
>
> if (bdrv_dev_has_removable_media(bs)) {
> Index: qemu/hmp.c
> ===================================================================
> --- qemu.orig/hmp.c
> +++ qemu/hmp.c
> @@ -209,6 +209,8 @@ void hmp_info_block(Monitor *mon)
> monitor_printf(mon, " tray-open=%d", info->value->tray_open);
> }
>
> + monitor_printf(mon, " hostcache=%d", info->value->hostcache);
> +
> if (info->value->has_io_status) {
> monitor_printf(mon, " io-status=%s",
> BlockDeviceIoStatus_lookup[info->value->io_status]);
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [RFC Patch 3/7]Qemu: Cmd "block_set_hostcache" for dynamic cache change
2012-02-01 3:06 ` [Qemu-devel] [RFC Patch 3/7]Qemu: Cmd "block_set_hostcache" for dynamic cache change Supriya Kannery
2012-02-02 0:09 ` Michael Roth
@ 2012-02-08 12:07 ` Luiz Capitulino
2012-02-13 13:21 ` Supriya Kannery
1 sibling, 1 reply; 30+ messages in thread
From: Luiz Capitulino @ 2012-02-08 12:07 UTC (permalink / raw)
To: Supriya Kannery
Cc: Kevin Wolf, Stefan Hajnoczi, qemu-devel, Christoph Hellwig
On Wed, 01 Feb 2012 08:36:41 +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
> @@ -808,6 +808,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_REOPEN_FILE_FAILED, bs->filename);
OPEN_FILE_FAILED is fine.
> + 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) {
> @@ -870,6 +899,33 @@ void bdrv_drain_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 */
> + 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
> @@ -119,6 +119,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);
> @@ -160,6 +161,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
> @@ -1080,3 +1080,29 @@ 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.
> +*/
> +int do_block_set_hostcache(Monitor *mon, const QDict *qdict,
> + QObject **ret_data)
This is not a QAPI command, please read docs/writing-qmp-commands.txt to know
how to write QMP commands using the QAPI.
> +{
> + 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
> @@ -62,4 +62,6 @@ void qmp_change_blockdev(const char *dev
> bool has_format, const char *format, Error **errp);
> void do_commit(Monitor *mon, const QDict *qdict);
> int do_drive_del(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
> @@ -796,6 +796,20 @@ ETEXI
> .mhandler.cmd_new = do_migrate,
> },
>
> + {
> + .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{option}
> +@findex block_set_hostcache
> +Change host pagecache setting of a block device while guest is running.
> +ETEXI
> +
>
> STEXI
> @item migrate [-d] [-b] [-i] @var{uri}
> Index: qemu/qmp-commands.hx
> ===================================================================
> --- qemu.orig/qmp-commands.hx
> +++ qemu/qmp-commands.hx
> @@ -698,7 +698,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",
> .mhandler.cmd_new = qmp_marshal_input_balloon,
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [RFC Patch 5/7]Qemu: raw-posix image file reopen
2012-02-01 3:07 ` [Qemu-devel] [RFC Patch 5/7]Qemu: raw-posix image file reopen Supriya Kannery
2012-02-02 0:15 ` Michael Roth
2012-02-07 10:17 ` Stefan Hajnoczi
@ 2012-02-08 14:54 ` Kevin Wolf
2012-02-13 13:28 ` Supriya Kannery
2 siblings, 1 reply; 30+ messages in thread
From: Kevin Wolf @ 2012-02-08 14:54 UTC (permalink / raw)
To: Supriya Kannery
Cc: Stefan Hajnoczi, Christoph Hellwig, qemu-devel, Luiz Capitulino
Am 01.02.2012 04:07, schrieb Supriya Kannery:
> raw-posix driver changes for bdrv_reopen_xx functions to
> safely reopen image files. Reopening of image files while
> changing hostcache dynamically is handled here.
>
> Signed-off-by: Supriya Kannery <supriyak@linux.vnet.ibm.com>
>
> Index: qemu/block/raw.c
> ===================================================================
> --- qemu.orig/block/raw.c
> +++ qemu/block/raw.c
> @@ -9,6 +9,22 @@ static int raw_open(BlockDriverState *bs
> return 0;
> }
>
> +static int raw_reopen_prepare(BlockDriverState *bs, BDRVReopenState **prs,
> + int flags)
> +{
> + return bdrv_reopen_prepare(bs->file, prs, flags);
> +}
> +
> +static void raw_reopen_commit(BlockDriverState *bs, BDRVReopenState *rs)
> +{
> + bdrv_reopen_commit(bs->file, rs);
> +}
> +
> +static void raw_reopen_abort(BlockDriverState *bs, BDRVReopenState *rs)
> +{
> + bdrv_reopen_abort(bs->file, rs);
> +}
> +
> static int coroutine_fn raw_co_readv(BlockDriverState *bs, int64_t sector_num,
> int nb_sectors, QEMUIOVector *qiov)
> {
> @@ -109,6 +125,10 @@ static BlockDriver bdrv_raw = {
> .instance_size = 1,
>
> .bdrv_open = raw_open,
> + .bdrv_reopen_prepare
> + = raw_reopen_prepare,
You can just indent to the next level instead of line wrapping.
> + .bdrv_reopen_commit = raw_reopen_commit,
> + .bdrv_reopen_abort = raw_reopen_abort,
> .bdrv_close = raw_close,
>
> .bdrv_co_readv = raw_co_readv,
> Index: qemu/block/raw-posix.c
> ===================================================================
> --- qemu.orig/block/raw-posix.c
> +++ qemu/block/raw-posix.c
> @@ -136,6 +136,11 @@ typedef struct BDRVRawState {
> #endif
> } BDRVRawState;
>
> +typedef struct BDRVRawReopenState {
> + BDRVReopenState reopen_state;
> + BDRVRawState *stash_s;
> +} BDRVRawReopenState;
See Stefan's comment. If it's possible to save only the fd and maybe one
or two other fields, then we should do that.
> static int fd_open(BlockDriverState *bs);
> static int64_t raw_getlength(BlockDriverState *bs);
>
> @@ -279,6 +284,71 @@ static int raw_open(BlockDriverState *bs
> return raw_open_common(bs, filename, flags, 0);
> }
>
> +static int raw_reopen_prepare(BlockDriverState *bs, BDRVReopenState **prs,
> + int flags)
> +{
> + BDRVRawReopenState *raw_rs = g_malloc0(sizeof(BDRVRawReopenState));
> + BDRVRawState *s = bs->opaque;
> + int ret = 0;
> +
> + raw_rs->reopen_state.bs = bs;
> +
> + /* stash state before reopen */
> + raw_rs->stash_s = g_malloc0(sizeof(BDRVRawState));
> + memcpy(raw_rs->stash_s, s, sizeof(BDRVRawState));
> + s->fd = dup(raw_rs->stash_s->fd);
> +
> + *prs = &(raw_rs->reopen_state);
> +
> + /* Flags that can be set using fcntl */
> + int fcntl_flags = BDRV_O_NOCACHE;
> +
> + if ((bs->open_flags & ~fcntl_flags) == (flags & ~fcntl_flags)) {
> + if ((flags & BDRV_O_NOCACHE)) {
> + s->open_flags |= O_DIRECT;
> + } else {
> + s->open_flags &= ~O_DIRECT;
> + }
> + printf("O_DIRECT flag\n");
Debugging leftover?
> + ret = fcntl_setfl(s->fd, s->open_flags);
> + } else {
> +
> + printf("close and open with new flags\n");
Same here.
Kevin
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [RFC Patch 6/7]Qemu: raw-win32 image file reopen
2012-02-01 3:07 ` [Qemu-devel] [RFC Patch 6/7]Qemu: raw-win32 " Supriya Kannery
@ 2012-02-08 15:02 ` Kevin Wolf
2012-02-13 13:29 ` Supriya Kannery
0 siblings, 1 reply; 30+ messages in thread
From: Kevin Wolf @ 2012-02-08 15:02 UTC (permalink / raw)
To: Supriya Kannery
Cc: Stefan Hajnoczi, Christoph Hellwig, qemu-devel, Luiz Capitulino
Am 01.02.2012 04:07, schrieb Supriya Kannery:
> win32 driver changes for bdrv_reopen_xx functions to
> safely reopen image files. Reopening of image files while
> changing hostcache dynamically is handled here.
>
> Signed-off-by: Supriya Kannery <supriyak@linux.vnet.ibm.com>
>
> Index: qemu/block/raw-win32.c
> ===================================================================
> --- qemu.orig/block/raw-win32.c
> +++ qemu/block/raw-win32.c
> @@ -26,18 +26,27 @@
> #include "block_int.h"
> #include "module.h"
> #include <windows.h>
> +#include <winbase.h>
> #include <winioctl.h>
>
> #define FTYPE_FILE 0
> #define FTYPE_CD 1
> #define FTYPE_HARDDISK 2
> +#define WINDOWS_VISTA 6
>
> typedef struct BDRVRawState {
> HANDLE hfile;
> int type;
> char drive_path[16]; /* format: "d:\" */
> + DWORD overlapped;
> } BDRVRawState;
>
> +typedef struct BDRVRawReopenState {
> + BDRVReopenState reopen_state;
> + HANDLE stash_hfile;
> + DWORD stash_overlapped;
> +} BDRVRawReopenState;
> +
> int qemu_ftruncate64(int fd, int64_t length)
> {
> LARGE_INTEGER li;
> @@ -106,9 +115,96 @@ static int raw_open(BlockDriverState *bs
> return -EACCES;
> return -1;
> }
> + s->overlapped = overlapped;
> return 0;
> }
>
> +static int raw_reopen_prepare(BlockDriverState *bs, BDRVReopenState **prs,
> + int flags)
> +{
> + BDRVRawReopenState *raw_rs = g_malloc0(sizeof(BDRVRawReopenState));
> + BDRVRawState *s = bs->opaque;
> + int ret = 0;
> + OSVERSIONINFO osvi;
> + BOOL bIsWindowsVistaorLater;
> +
> + raw_rs->bs = bs;
> + raw_rs->stash_hfile = s->hfile;
> + raw_rs->stash_overlapped = s->overlapped;
> + *prs = raw_rs;
> +
> + if (flags & BDRV_O_NOCACHE) {
> + s->overlapped |= FILE_FLAG_NO_BUFFERING;
> + } else {
> + s->overlapped &= ~FILE_FLAG_NO_BUFFERING;
> + }
> +
> + if (!(flags & BDRV_O_CACHE_WB)) {
> + s->overlapped |= FILE_FLAG_WRITE_THROUGH;
> + } else {
> + s->overlapped &= ~FILE_FLAG_WRITE_THROUGH;
> + }
> +
> + ZeroMemory(&osvi, sizeof(OSVERSIONINFO));
> + osvi.dwOSVersionInfoSize = sizeof(OSVERSIONINFO);
> +
> + GetVersionEx(&osvi);
> +
> + if (osvi.dwMajorVersion >= WINDOWS_VISTA) {
> + s->hfile = ReOpenFile(raw_rs->stash_hfile, 0, FILE_SHARE_READ,
> + overlapped);
> + if (s->hfile == INVALID_HANDLE_VALUE) {
> + int err = GetLastError();
> + if (err == ERROR_ACCESS_DENIED) {
> + ret = -EACCES;
> + } else {
> + ret = -1;
Returning -1 where -errno is expected is bad (turns out as -EPERM on
Linux, which is misleading). Maybe -EIO here.
Kevin
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [RFC Patch 4/7]Qemu: Framework for reopening image files safely
2012-02-01 3:06 ` [Qemu-devel] [RFC Patch 4/7]Qemu: Framework for reopening image files safely Supriya Kannery
2012-02-07 10:08 ` Stefan Hajnoczi
@ 2012-02-08 15:07 ` Kevin Wolf
2012-02-13 13:49 ` Supriya Kannery
1 sibling, 1 reply; 30+ messages in thread
From: Kevin Wolf @ 2012-02-08 15:07 UTC (permalink / raw)
To: Supriya Kannery
Cc: Stefan Hajnoczi, Christoph Hellwig, qemu-devel, Luiz Capitulino
Am 01.02.2012 04:06, schrieb Supriya Kannery:
> Struct BDRVReopenState along with three reopen related functions
> introduced for handling reopening of images safely. This can be
> extended by each of the block drivers to reopen respective
> image files.
>
> Signed-off-by: Supriya Kannery <supriyak@linux.vnet.ibm.com>
>
> Index: qemu/block.c
> ===================================================================
> --- qemu.orig/block.c
> +++ qemu/block.c
> @@ -808,10 +808,32 @@ unlink_and_fail:
> return ret;
> }
>
> +int bdrv_reopen_prepare(BlockDriverState *bs, BDRVReopenState **prs, int flags)
> +{
> + BlockDriver *drv = bs->drv;
> +
> + return drv->bdrv_reopen_prepare(bs, prs, flags);
> +}
> +
> +void bdrv_reopen_commit(BlockDriverState *bs, BDRVReopenState *rs)
> +{
> + BlockDriver *drv = bs->drv;
> +
> + drv->bdrv_reopen_commit(bs, rs);
> +}
> +
> +void bdrv_reopen_abort(BlockDriverState *bs, BDRVReopenState *rs)
> +{
> + BlockDriver *drv = bs->drv;
> +
> + drv->bdrv_reopen_abort(bs, rs);
> +}
> +
> int bdrv_reopen(BlockDriverState *bs, int bdrv_flags)
> {
> BlockDriver *drv = bs->drv;
> int ret = 0, open_flags;
> + BDRVReopenState *reopen_state = NULL;
>
> /* Quiesce IO for the given block device */
> qemu_aio_flush();
> @@ -820,17 +842,32 @@ 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 = bdrv_reopen_prepare(bs, &reopen_state, bdrv_flags);
> + if (ret < 0) {
> + bdrv_reopen_abort(bs, reopen_state);
> + qerror_report(QERR_REOPEN_FILE_FAILED, bs->filename);
> + return ret;
> + }
> +
> + bdrv_reopen_commit(bs, reopen_state);
> + bs->open_flags = bdrv_flags;
> +
> + } else {
> + open_flags = bs->open_flags;
> + bdrv_close(bs);
> +
> + ret = bdrv_open(bs, bs->filename, bdrv_flags, drv);
> if (ret < 0) {
> - /* Reopen failed with orig and modified flags */
> - abort();
> + /* 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 */
> + bs->drv = NULL;
> + }
> }
Most image formats don't have a bdrv_reopen_* implementation after this
series, so usually you'll have something like qcow2 on top of file. This
code uses bdrv_close/open for the whole stack, even though the file
layer could actually make use of a bdrv_reopen_* implementation and the
qcow2 open isn't likely to fail if the image file could be opened.
I think we can use drv->bdrv_close/open to reopen only one layer and try
using bdrv_reopen_* for the lower layer again.
This is an improvement that can be done in a separate patch, though.
Kevin
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [RFC Patch 5/7]Qemu: raw-posix image file reopen
2012-02-02 0:15 ` Michael Roth
@ 2012-02-13 13:12 ` Supriya Kannery
0 siblings, 0 replies; 30+ messages in thread
From: Supriya Kannery @ 2012-02-13 13:12 UTC (permalink / raw)
To: Michael Roth
Cc: Kevin Wolf, Stefan Hajnoczi, Luiz Capitulino, qemu-devel,
Christoph Hellwig
On 02/02/2012 05:45 AM, Michael Roth wrote:
> On 01/31/2012 09:07 PM, Supriya Kannery wrote:
>> raw-posix driver changes for bdrv_reopen_xx functions to
>> safely reopen image files. Reopening of image files while
>> changing hostcache dynamically is handled here.
>>
>> +
>> + /* Flags that can be set using fcntl */
>> + int fcntl_flags = BDRV_O_NOCACHE;
>> +
>> + if ((bs->open_flags& ~fcntl_flags) == (flags& ~fcntl_flags)) {
>> + if ((flags& BDRV_O_NOCACHE)) {
>> + s->open_flags |= O_DIRECT;
>> + } else {
>> + s->open_flags&= ~O_DIRECT;
>> + }
>> + printf("O_DIRECT flag\n");
>> + ret = fcntl_setfl(s->fd, s->open_flags);
>
> raw-posix.c:raw_aio_submit() does some extra alignment work if
> s->aligned_buf was set due to the image being opened O_DIRECT initially,
> not sure what the impact is but probably want to clean that up here.
>
ok, will check on this
thanks! for reviewing
Supriya
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [RFC Patch 2/7]Qemu: Error classes for file reopen and data sync failure
2012-02-07 7:56 ` Stefan Hajnoczi
@ 2012-02-13 13:13 ` Supriya Kannery
0 siblings, 0 replies; 30+ messages in thread
From: Supriya Kannery @ 2012-02-13 13:13 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Kevin Wolf, Luiz Capitulino, Christoph Hellwig, qemu-devel
On 02/07/2012 01:26 PM, Stefan Hajnoczi wrote:
> On Wed, Feb 01, 2012 at 08:36:28AM +0530, Supriya Kannery wrote:
>> Index: qemu/qerror.c
>> ===================================================================
>> --- qemu.orig/qerror.c
>> +++ qemu/qerror.c
>> @@ -108,6 +108,14 @@ static const QErrorStringTable qerror_ta
>> .desc = "Device '%(device)' has multiple child busses",
>> },
>> {
>> + .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)'",
>> + },
>
> The comment in qerror.c says:
>
> "Please keep the entries in alphabetical order.
> Use scripts/check-qerror.sh to check."
>
ok
>> + {
>> .error_fmt = QERR_DEVICE_NO_BUS,
>> .desc = "Device '%(device)' has no child bus",
>> },
>> Index: qemu/qerror.h
>> ===================================================================
>> --- qemu.orig/qerror.h
>> +++ qemu/qerror.h
>> @@ -117,6 +117,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 } }"
>> +
>
> Same here:
>
> /*
> * QError class list
> * Please keep the definitions in alphabetical order.
> * Use scripts/check-qerror.sh to check.
> */
>
ok
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [RFC Patch 1/7]Qemu: Enhance "info block" to display host cache setting
2012-02-08 12:00 ` Luiz Capitulino
@ 2012-02-13 13:19 ` Supriya Kannery
0 siblings, 0 replies; 30+ messages in thread
From: Supriya Kannery @ 2012-02-13 13:19 UTC (permalink / raw)
To: Luiz Capitulino
Cc: Kevin Wolf, Stefan Hajnoczi, qemu-devel, Christoph Hellwig
On 02/08/2012 05:30 PM, Luiz Capitulino wrote:
> On Wed, 01 Feb 2012 08:36:14 +0530
> Supriya Kannery<supriyak@linux.vnet.ibm.com> wrote:
>
>> Enhance "info block" to display hostcache setting for each
>> block device.
>>
>> Example:
>> (qemu) info block
>> ide0-hd0: removable=0 file=../rhel6-32.raw ro=0 drv=raw encrypted=0
>>
>> Enhanced to display "hostcache" setting:
>> (qemu) info block
>> ide0-hd0: removable=0 hostcache=1 file=../rhel6-32.raw ro=0 drv=raw encrypted=0
>
> The day we'll want to refactor 'info block' output is coming...
>
ok :-)
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [RFC Patch 3/7]Qemu: Cmd "block_set_hostcache" for dynamic cache change
2012-02-08 12:07 ` Luiz Capitulino
@ 2012-02-13 13:21 ` Supriya Kannery
0 siblings, 0 replies; 30+ messages in thread
From: Supriya Kannery @ 2012-02-13 13:21 UTC (permalink / raw)
To: Luiz Capitulino
Cc: Kevin Wolf, Stefan Hajnoczi, qemu-devel, Christoph Hellwig
On 02/08/2012 05:37 PM, Luiz Capitulino wrote:
> On Wed, 01 Feb 2012 08:36:41 +0530
> Supriya Kannery<supriyak@linux.vnet.ibm.com> wrote:
>
>> + 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);
>
> OPEN_FILE_FAILED is fine.
>
ok
>> +/*
>> + * Change host page cache setting while guest is running.
>> +*/
>> +int do_block_set_hostcache(Monitor *mon, const QDict *qdict,
>> + QObject **ret_data)
>
> This is not a QAPI command, please read docs/writing-qmp-commands.txt to know
> how to write QMP commands using the QAPI.
>
fine, will check the doc
-thanks, Supriya
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [RFC Patch 5/7]Qemu: raw-posix image file reopen
2012-02-08 14:54 ` Kevin Wolf
@ 2012-02-13 13:28 ` Supriya Kannery
0 siblings, 0 replies; 30+ messages in thread
From: Supriya Kannery @ 2012-02-13 13:28 UTC (permalink / raw)
To: Kevin Wolf
Cc: Stefan Hajnoczi, Luiz Capitulino, Christoph Hellwig, qemu-devel
On 02/08/2012 08:24 PM, Kevin Wolf wrote:
> Am 01.02.2012 04:07, schrieb Supriya Kannery:
>> raw-posix driver changes for bdrv_reopen_xx functions to
>> safely reopen image files. Reopening of image files while
>> changing hostcache dynamically is handled here.
>>
>>
>> +typedef struct BDRVRawReopenState {
>> + BDRVReopenState reopen_state;
>> + BDRVRawState *stash_s;
>> +} BDRVRawReopenState;
>
> See Stefan's comment. If it's possible to save only the fd and maybe one
> or two other fields, then we should do that.
>
Yes, for V1 of this patchset, will look for stashing only those relevant
fields of a driver state wherever possible
>> +
>> + if ((bs->open_flags& ~fcntl_flags) == (flags& ~fcntl_flags)) {
>> + if ((flags& BDRV_O_NOCACHE)) {
>> + s->open_flags |= O_DIRECT;
>> + } else {
>> + s->open_flags&= ~O_DIRECT;
>> + }
>> + printf("O_DIRECT flag\n");
>
> Debugging leftover?
>
yes :-), didn't do a proper cleanup as this is RFC
for the stashing approach.
>> + ret = fcntl_setfl(s->fd, s->open_flags);
>> + } else {
>> +
>> + printf("close and open with new flags\n");
>
> Same here.
>
V1 will be a clean one !
> Kevin
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [RFC Patch 6/7]Qemu: raw-win32 image file reopen
2012-02-08 15:02 ` Kevin Wolf
@ 2012-02-13 13:29 ` Supriya Kannery
0 siblings, 0 replies; 30+ messages in thread
From: Supriya Kannery @ 2012-02-13 13:29 UTC (permalink / raw)
To: Kevin Wolf
Cc: Stefan Hajnoczi, Luiz Capitulino, Christoph Hellwig, qemu-devel
On 02/08/2012 08:32 PM, Kevin Wolf wrote:
> Am 01.02.2012 04:07, schrieb Supriya Kannery:
>> win32 driver changes for bdrv_reopen_xx functions to
>> safely reopen image files. Reopening of image files while
>> changing hostcache dynamically is handled here.
>>
>> +
>> + if (osvi.dwMajorVersion>= WINDOWS_VISTA) {
>> + s->hfile = ReOpenFile(raw_rs->stash_hfile, 0, FILE_SHARE_READ,
>> + overlapped);
>> + if (s->hfile == INVALID_HANDLE_VALUE) {
>> + int err = GetLastError();
>> + if (err == ERROR_ACCESS_DENIED) {
>> + ret = -EACCES;
>> + } else {
>> + ret = -1;
>
> Returning -1 where -errno is expected is bad (turns out as -EPERM on
> Linux, which is misleading). Maybe -EIO here.
ok
-thanks, Supriya
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [RFC Patch 4/7]Qemu: Framework for reopening image files safely
2012-02-08 15:07 ` Kevin Wolf
@ 2012-02-13 13:49 ` Supriya Kannery
0 siblings, 0 replies; 30+ messages in thread
From: Supriya Kannery @ 2012-02-13 13:49 UTC (permalink / raw)
To: Kevin Wolf
Cc: Stefan Hajnoczi, Luiz Capitulino, Christoph Hellwig, qemu-devel
On 02/08/2012 08:37 PM, Kevin Wolf wrote:
> Am 01.02.2012 04:06, schrieb Supriya Kannery:
>> Struct BDRVReopenState along with three reopen related functions
>> introduced for handling reopening of images safely. This can be
>> extended by each of the block drivers to reopen respective
>> image files.
>> + } else {
>> + open_flags = bs->open_flags;
>> + bdrv_close(bs);
>> +
>> + ret = bdrv_open(bs, bs->filename, bdrv_flags, drv);
>> if (ret< 0) {
>> - /* Reopen failed with orig and modified flags */
>> - abort();
>> + /* 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 */
>> + bs->drv = NULL;
>> + }
>> }
>
> Most image formats don't have a bdrv_reopen_* implementation after this
> series, so usually you'll have something like qcow2 on top of file. This
> code uses bdrv_close/open for the whole stack, even though the file
> layer could actually make use of a bdrv_reopen_* implementation and the
> qcow2 open isn't likely to fail if the image file could be opened.
>
> I think we can use drv->bdrv_close/open to reopen only one layer and try
> using bdrv_reopen_* for the lower layer again.
>
> This is an improvement that can be done in a separate patch, though.
What I understood is, in the enhancement patch, we will have something
like (taking qcow2 as an example)
Implement bdrv_reopen_qcow2(image file) which reopens only the qcow2
image file
Then, drv->bdrv_open(qcow2 driver) will reopen qcow2 driver
=> calls bdrv_reopen_qcow2(qcow2 image file) if image file has
to be reopen
Can you please explain a bit more, it this is not what you meant.
-thanks, Supriya
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [RFC Patch 4/7]Qemu: Framework for reopening image files safely
2012-02-07 10:08 ` Stefan Hajnoczi
@ 2012-02-14 13:34 ` Supriya Kannery
0 siblings, 0 replies; 30+ messages in thread
From: Supriya Kannery @ 2012-02-14 13:34 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Kevin Wolf, Christoph Hellwig, qemu-devel, Luiz Capitulino
On 02/07/2012 03:38 PM, Stefan Hajnoczi wrote:
> On Wed, Feb 01, 2012 at 08:36:58AM +0530, Supriya Kannery wrote:
>> Struct BDRVReopenState along with three reopen related functions
>> introduced for handling reopening of images safely. This can be
>> extended by each of the block drivers to reopen respective
>> image files.
>>
>> +int bdrv_reopen_prepare(BlockDriverState *bs, BDRVReopenState **prs, int flags)
>> +{
>> + BlockDriver *drv = bs->drv;
>> +
>> + return drv->bdrv_reopen_prepare(bs, prs, flags);
>
> Indentation should be 4 spaces. I suggest configuring your editor so
> this is always correct and done automatically.
>
ok
>> - 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 = bdrv_reopen_prepare(bs,&reopen_state, bdrv_flags);
>> + if (ret< 0) {
>
> Indentation
>
>> + bdrv_reopen_abort(bs, reopen_state);
>> + qerror_report(QERR_REOPEN_FILE_FAILED, bs->filename);
>> + return ret;
>> + }
>> +
>> + bdrv_reopen_commit(bs, reopen_state);
>> + bs->open_flags = bdrv_flags;
>
> Is bs->open_flags not assigned inside bdrv_reopen_*()?
>
No, Since it is generic for all the drivers, placed it here.
>> +
>> + } else {
>> + open_flags = bs->open_flags;
>> + bdrv_close(bs);
>> +
>> + ret = bdrv_open(bs, bs->filename, bdrv_flags, drv);
>> if (ret< 0) {
>> - /* Reopen failed with orig and modified flags */
>> - abort();
>> + /* 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 */
>> + bs->drv = NULL;
>
> This should be a post-condition of bdrv_open(). If you have found a
> case where bs->drv != NULL after bdrv_open() fails, then please fix that
> code path instead of assigning NULL here.
>
ok, will check on this
-thanks for reviewing, Supriya
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [RFC Patch 5/7]Qemu: raw-posix image file reopen
2012-02-07 10:17 ` Stefan Hajnoczi
@ 2012-02-14 13:36 ` Supriya Kannery
0 siblings, 0 replies; 30+ messages in thread
From: Supriya Kannery @ 2012-02-14 13:36 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Kevin Wolf, Christoph Hellwig, qemu-devel, Luiz Capitulino
On 02/07/2012 03:47 PM, Stefan Hajnoczi wrote:
> On Wed, Feb 01, 2012 at 08:37:12AM +0530, Supriya Kannery wrote:
>> + /* stash state before reopen */
>> + raw_rs->stash_s = g_malloc0(sizeof(BDRVRawState));
>> + memcpy(raw_rs->stash_s, s, sizeof(BDRVRawState));
>
> Copying a struct is fragile, Mike Roth pointed out the potential issue
> with aligned_buf.
>
> If raw-posix could open from a given file descriptor as an alternative
> to opening a filename, then it would be clean and natural to simply
> re-initialize from the dup'd file descriptor in the abort case. That's
> the approach I would try instead of stashing the whole struct.
>
> Stefan
>
fine, will get V1 with stashing of only required fields wherever
possible instead of stashing the full struct.
^ permalink raw reply [flat|nested] 30+ messages in thread
end of thread, other threads:[~2012-02-14 13:37 UTC | newest]
Thread overview: 30+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-02-01 3:05 [Qemu-devel] [RFC Patch 0/7]Qemu: Dynamic host pagecache change Supriya Kannery
2012-02-01 3:06 ` [Qemu-devel] [RFC Patch 1/7]Qemu: Enhance "info block" to display host cache setting Supriya Kannery
2012-02-08 12:00 ` Luiz Capitulino
2012-02-13 13:19 ` Supriya Kannery
2012-02-01 3:06 ` [Qemu-devel] [RFC Patch 2/7]Qemu: Error classes for file reopen and data sync failure Supriya Kannery
2012-02-07 7:56 ` Stefan Hajnoczi
2012-02-13 13:13 ` Supriya Kannery
2012-02-01 3:06 ` [Qemu-devel] [RFC Patch 3/7]Qemu: Cmd "block_set_hostcache" for dynamic cache change Supriya Kannery
2012-02-02 0:09 ` Michael Roth
2012-02-02 10:14 ` Kevin Wolf
2012-02-08 12:07 ` Luiz Capitulino
2012-02-13 13:21 ` Supriya Kannery
2012-02-01 3:06 ` [Qemu-devel] [RFC Patch 4/7]Qemu: Framework for reopening image files safely Supriya Kannery
2012-02-07 10:08 ` Stefan Hajnoczi
2012-02-14 13:34 ` Supriya Kannery
2012-02-08 15:07 ` Kevin Wolf
2012-02-13 13:49 ` Supriya Kannery
2012-02-01 3:07 ` [Qemu-devel] [RFC Patch 5/7]Qemu: raw-posix image file reopen Supriya Kannery
2012-02-02 0:15 ` Michael Roth
2012-02-13 13:12 ` Supriya Kannery
2012-02-07 10:17 ` Stefan Hajnoczi
2012-02-14 13:36 ` Supriya Kannery
2012-02-08 14:54 ` Kevin Wolf
2012-02-13 13:28 ` Supriya Kannery
2012-02-01 3:07 ` [Qemu-devel] [RFC Patch 6/7]Qemu: raw-win32 " Supriya Kannery
2012-02-08 15:02 ` Kevin Wolf
2012-02-13 13:29 ` Supriya Kannery
2012-02-01 3:07 ` [Qemu-devel] [RFC Patch 7/7]Qemu: vmdk " Supriya Kannery
2012-02-01 22:41 ` [Qemu-devel] [RFC Patch 0/7]Qemu: Dynamic host pagecache change Eric Blake
2012-02-02 9:12 ` Kevin Wolf
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.