block: bdrv_reopen() patches
v2 cover block: bdrv_reopen() patches
These patches are based off Supriya Kannery's original bdrv_reopen() patches as part of the hostcache series.
This provides support for safe reopen of a single image, or transactional reopening of multiple images atomically.
Changes from v1 -> v2:
Patch 01/16: None
Patch 02/16: New patch
Patch 03/16: [Kevin] Use QSIMPLEQ_FOREACH_SAFE
[Kevin] Use BDRV_O_ALLOW_RDWR flag instead of keep_read_only [Kevin] Preserve error on failure of bdrv_flush() [Kevin] bdrv_reopen_prepare() no longer calls bdrv_reopen_abort() [Kevin] Directly embed BDRVReopenState field in
BlockReopenQueueEntry, rather than by reference
[Jeff] Add BlockReopenQueue field to the BDRVReopenState struct
Feedback not incorporated:
--------------------------
[Kevin] Sharing 3 of the BDS flag setting lines with bdrv_open().
I didn't see a great way of doing this.
Patch 04/16: New patch, aio init code motion
Patch 05/16: [Kevin] New patch, raw_parse_flags code motion
Patch 06/16: [Kevin] New patch. Do not parse BDRV_O_CACHE_WB in
raw_posix/raw_win32
Patch 07/16: [Kevin] New patch. Code motion - move aligned_buf allocation
to helper function.
Patch 08/16: [Kevin] See patches 4-7
[Kevin] Init AIO, if appropriate [Kevin] Fallback to qemu_open, if fcntl fails [Eric] Remove spurious mode in qemu_open() [Corey] Use qemu_close() instead of qemu_open()
Feedback not incorporated:
--------------------------
Moving from fcntl + fcntl_setfl to qemu_dup_flags or equivalent. Ideally I think a separate patch series creating a wrapper function for fcntl, and then update this code with the new wrapper.
Patch 09/16: None
Patch 10/16: None
Patch 11/16: None
Patch 12/16: None
Patch 13/16: New patch, VMDK driver for reopen
Patch 14/16: New patch, raw-win32 driver for reopen. Note, not necessarily
safe. If anyone knows a better way to do it under win32, without potentially having to close the handle first, please let me know.
Patch 15/16: New patch, move bdrv_commit to use bdrv_reopen()
Patch 16/16: [Kevin] Get rid of keep_read_only flag completely
Jeff Cody (16):
block: correctly set the keep_read_only flag
block: make bdrv_set_enable_write_cache() modify open_flags
block: Framework for reopening files safely
block: move aio initialization into a helper function
block: move open flag parsing in raw block drivers to helper
functions
block: do not parse BDRV_O_CACHE_WB in raw block drivers
block: move allocating aligned_buf into a helper function in
raw_posix.c
block: raw-posix image file reopen
block: raw image file reopen
block: qed image file reopen
block: qcow2 image file reopen
block: qcow image file reopen
block: vmdk image file reopen
block: raw-win32 driver reopen support
block: convert bdrv_commit() to use bdrv_reopen()
block: remove keep_read_only flag from BlockDriverState struct
block.c | 292 ++++++++++++++++++++++++++++++++++++++++++++---------- block.h | 24 +++++ block/qcow.c | 23 +++++ block/qcow2.c | 22 ++++ block/qed.c | 20 ++++ block/raw-posix.c | 243 +++++++++++++++++++++++++++++++++++++-------- block/raw-win32.c | 144 +++++++++++++++++++++++---- block/raw.c | 22 ++++ block/vmdk.c | 47 +++++++++ block_int.h | 8 +- qemu-common.h | 1 + 11 files changed, 732 insertions(+), 114 deletions(-)
Signature · 1 line
1.7.11.4
PATCH v2 1/16 block: correctly set the keep_read_only flag
I believe the bs->keep_read_only flag is supposed to reflect the initial open state of the device. If the device is initially opened R/O, then commit operations, or reopen operations changing to R/W, are prohibited.
Currently, the keep_read_only flag is only accurate for the active layer, and its backing file. Subsequent images end up always having the keep_read_only flag set.
For instance, what happens now:
[ base ] kro = 1, ro = 1
|
v
[ snap-1 ] kro = 1, ro = 1
|
v
[ snap-2 ] kro = 0, ro = 1
|
v[ active ] kro = 0, ro = 0
What we want:
[ base ] kro = 0, ro = 1
|
v
[ snap-1 ] kro = 0, ro = 1
|
v
[ snap-2 ] kro = 0, ro = 1
|
v[ active ] kro = 0, ro = 0
--- block.c | 14 +++++++------- block.h | 1 + 2 files changed, 8 insertions(+), 7 deletions(-)
block.c +7 -7
@@ -664,7 +664,7 @@ static int bdrv_open_common(BlockDriverState *bs, const char *filename, open_flags |=BDRV_O_RDWR ; } - bs->keep_read_only = bs->read_only = !(open_flags &BDRV_O_RDWR ); + bs->read_only = !(open_flags &BDRV_O_RDWR );/* Open the image, either directly or using a protocol */ if (drv->bdrv_file_open) {
@@ -804,6 +804,12 @@ int bdrv_open(BlockDriverState *bs, const char *filename, int flags, goto unlink_and_fail; } +if (flags &BDRV_O_RDWR ) { + flags |=BDRV_O_ALLOW_RDWR ; + } + + bs->keep_read_only = !(flags &BDRV_O_ALLOW_RDWR ); +/* Open the image */ ret =bdrv_open_common (bs, filename, flags, drv);if (ret <0 ) {
@@ -833,12 +839,6 @@ int bdrv_open(BlockDriverState *bs, const char *filename, int flags,bdrv_close (bs);return ret; } -if (bs->is_temporary) { - bs->backing_hd->keep_read_only = !(flags &BDRV_O_RDWR ); - }else { -/* base image inherits from "parent" */ - bs->backing_hd->keep_read_only = bs->keep_read_only; - } }if (!bdrv_key_required (bs)) {
block.h +1 -0
@@ -80,6 +80,7 @@ typedef struct BlockDevOps {#define BDRV_O_COPY_ON_READ 0x0400/* copy read backing sectors into image */ #define BDRV_O_INCOMING 0x0800/* consistency hint for incoming migration */ #define BDRV_O_CHECK 0x1000/* open solely for consistency check */ +#define BDRV_O_ALLOW_RDWR 0x2000/* allow reopen to change from r/o to r/w */ #define BDRV_O_CACHE_MASK (BDRV_O_NOCACHE | BDRV_O_CACHE_WB | BDRV_O_NO_FLUSH)
Signature · 1 line
1.7.11.4
PATCH v2 2/16 block: make bdrv_set_enable_write_cache() modify open_flags
In reply to Jeff Codybdrv_set_enable_write_cache() sets the bs->enable_write_cache flag, but without the flag recorded in bs->open_flags, then next time a reopen() is performed the enable_write_cache setting may be inadvertently lost.
This will set the flag in open_flags, so it is preserved across reopens.
--- block.c | 7 +++++++ 1 file changed, 7 insertions(+)
block.c +7 -0
@@ -2164,6 +2164,13 @@ int bdrv_enable_write_cache(BlockDriverState *bs)void bdrv_set_enable_write_cache (BlockDriverState *bs,bool wce) { bs->enable_write_cache = wce; + +/* so a reopen() will preserve wce */ +if (wce) { + bs->open_flags |=BDRV_O_CACHE_WB ; + }else { + bs->open_flags &= ~BDRV_O_CACHE_WB ; + } }int bdrv_is_encrypted (BlockDriverState *bs)
Signature · 1 line
1.7.11.4
PATCH v2 3/16 block: Framework for reopening files safely
In reply to Jeff CodyThis is based on Supriya Kannery's bdrv_reopen() patch series.
This provides a transactional method to reopen multiple images files safely.
Image files are queue for reopen via bdrv_reopen_queue(), and the reopen occurs when bdrv_reopen_multiple() is called. Changes are staged in bdrv_reopen_prepare() and in the equivalent driver level functions. If any of the staged images fails a prepare, then all of the images left untouched, and the staged changes for each image abandoned.
Block drivers are passed a reopen state structure, that contains:
* BDS to reopen
* flags for the reopen
* opaque pointer for any driver-specific data that needs to be persistent from _prepare to _commit/_abort
* reopen queue pointer, if the driver needs to queue additional BDS for a reopen
--- block.c | 225 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ block.h | 23 ++++++ block_int.h | 7 ++ qemu-common.h | 1 + 4 files changed, 256 insertions(+)
block.c +225 -0
@@ -859,6 +859,231 @@ unlink_and_fail:return ret; } +/* +* Adds a BlockDriverState to a simple queue for an atomic, transactional +* reopen of multiple devices. +* +* bs_queue can either be an existing BlockReopenQueue that has had QSIMPLE_INIT +* already performed, or alternatively may be NULL a new BlockReopenQueue will +* be created and initialized. This newly created BlockReopenQueue should be +* passed back in for subsequent calls that are intended to be of the same +* atomic 'set'. +* +* bs is the BlockDriverState to add to the reopen queue. +* +* flags contains the open flags for the associated bs +* +* returns a pointer to bs_queue, which is either the newly allocated +* bs_queue, or the existing bs_queue being used. +* +*/ +BlockReopenQueue *bdrv_reopen_queue (BlockReopenQueue *bs_queue, +BlockDriverState *bs,int flags) +{ +assert (bs !=NULL ); + +BlockReopenQueueEntry *bs_entry; +if (bs_queue ==NULL ) { + bs_queue =g_new0 (BlockReopenQueue,1 ); +QSIMPLEQ_INIT (bs_queue); + } + +if (bs->file) { +bdrv_reopen_queue (bs_queue, bs->file, flags); + } + + bs_entry =g_new0 (BlockReopenQueueEntry,1 ); +QSIMPLEQ_INSERT_TAIL (bs_queue, bs_entry, entry); + + bs_entry->state.bs = bs; + bs_entry->state.flags = flags; + bs_entry->state.queue = bs_queue; + +return bs_queue; +} + +/* +* Reopen multiple BlockDriverStates atomically & transactionally. +* +* The queue passed in (bs_queue) must have been built up previous +* via bdrv_reopen_queue(). +* +* Reopens all BDS specified in the queue, with the appropriate +* flags. All devices are prepared for reopen, and failure of any +* device will cause all device changes to be abandonded, and intermediate +* data cleaned up. +* +* If all devices prepare successfully, then the changes are committed +* to all devices. +* +*/ +int bdrv_reopen_multiple (BlockReopenQueue *bs_queue,Error **errp) +{ +int ret =-1 ; +BlockReopenQueueEntry *bs_entry, *next; +Error *local_err =NULL ; + +assert (bs_queue !=NULL ); + +bdrv_drain_all (); + + QSIMPLEQ_FOREACH(bs_entry, bs_queue, entry) { +if (bdrv_reopen_prepare (&bs_entry->state, &local_err)) { +error_propagate (errp, local_err); + goto cleanup; + } + bs_entry->prepared = true; + } + +/* If we reach this point, we have success and just need to apply the +* changes +*/ + QSIMPLEQ_FOREACH(bs_entry, bs_queue, entry) { +bdrv_reopen_commit (&bs_entry->state); + } + + ret =0 ; + +cleanup: + QSIMPLEQ_FOREACH_SAFE(bs_entry, bs_queue, entry, next) { +if (ret && bs_entry->prepared) { +bdrv_reopen_abort (&bs_entry->state); + } +g_free (bs_entry); + } +g_free (bs_queue); +return ret; +} + + +/* Reopen a single BlockDriverState with the specified flags. */ +int bdrv_reopen (BlockDriverState *bs,int bdrv_flags,Error **errp) +{ +int ret =-1 ; +Error *local_err =NULL ; +BlockReopenQueue *queue =bdrv_reopen_queue (NULL , bs, bdrv_flags); + + ret =bdrv_reopen_multiple (queue, &local_err); +if (local_err !=NULL ) { +error_propagate (errp, local_err); + } +return ret; +} + + +/* +* Prepares a BlockDriverState for reopen. All changes are staged in the +* 'reopen_state' field of the BlockDriverState, which must be NULL when +* entering (all previous reopens must have completed for the BDS). +* +* bs is the BlockDriverState to reopen +* flags are the new open flags +* +* Returns 0 on success, non-zero on error. On error errp will be set +* as well. +* +* On failure, bdrv_reopen_abort() will be called to clean up any data. +* It is the responsibility of the caller to then call the abort() or +* commit() for any other BDS that have been left in a prepare() state +* +*/ +int bdrv_reopen_prepare (BDRVReopenState *reopen_state,Error **errp) +{ +int ret =-1 ; +Error *local_err =NULL ; +BlockDriver *drv; + +assert (reopen_state !=NULL ); +assert (reopen_state->bs->drv !=NULL ); + drv = reopen_state->bs->drv; + +/* if we are to stay read-only, do not allow permission change +* to r/w */ +if (!(reopen_state->bs->open_flags &BDRV_O_ALLOW_RDWR ) && + reopen_state->flags &BDRV_O_RDWR ) { +error_set (errp,QERR_DEVICE_IS_READ_ONLY , + reopen_state->bs->device_name); + goto error; + } + + + ret =bdrv_flush (reopen_state->bs); +if (ret) { +error_set (errp,ERROR_CLASS_GENERIC_ERROR ,"Error (%s) flushing drive" , +strerror (-ret)); + goto error; + } + +if (drv->bdrv_reopen_prepare) { + ret = drv->bdrv_reopen_prepare (reopen_state, &local_err); +if (ret) { +if (local_err !=NULL ) { +error_propagate (errp, local_err); + }else { +error_set (errp,QERR_OPEN_FILE_FAILED , + reopen_state->bs->filename); + } + goto error; + } + }else { +/* It is currently mandatory to have a bdrv_reopen_prepare() +* handler for each supported drv. */ +error_set (errp,QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED , + drv->format_name, reopen_state->bs->device_name, +"reopening of file" ); + ret =-1 ; + goto error; + } + + ret =0 ; + +error: +return ret; +} + +/* +* Takes the staged changes for the reopen from bdrv_reopen_prepare(), and +* makes them final by swapping the staging BlockDriverState contents into +* the active BlockDriverState contents. +*/ +void bdrv_reopen_commit (BDRVReopenState *reopen_state) +{ +BlockDriver *drv; + +assert (reopen_state !=NULL ); + drv = reopen_state->bs->drv; +assert (drv !=NULL ); + +/* If there are any driver level actions to take */ +if (drv->bdrv_reopen_commit) { + drv->bdrv_reopen_commit (reopen_state); + } + +/* set BDS specific flags now */ + reopen_state->bs->open_flags = reopen_state->flags; + reopen_state->bs->enable_write_cache = !!(reopen_state->flags & +BDRV_O_CACHE_WB ); + reopen_state->bs->read_only = !(reopen_state->flags &BDRV_O_RDWR ); +} + +/* +* Abort the reopen, and delete and free the staged changes in +* reopen_state +*/ +void bdrv_reopen_abort (BDRVReopenState *reopen_state) +{ +BlockDriver *drv; + +assert (reopen_state !=NULL ); + drv = reopen_state->bs->drv; +assert (drv !=NULL ); + +if (drv->bdrv_reopen_abort) { + drv->bdrv_reopen_abort (reopen_state); + } +} + +void bdrv_close (BlockDriverState *bs) {bdrv_flush (bs);
block.h +23 -0
@@ -97,6 +97,22 @@ typedef enum { BDRV_ACTION_REPORT, BDRV_ACTION_IGNORE, BDRV_ACTION_STOP } BlockQMPEventAction; +typedef QSIMPLEQ_HEAD(BlockReopenQueue, BlockReopenQueueEntry) BlockReopenQueue; + +typedef struct BDRVReopenState { +BlockDriverState *bs; +int flags; +void *opaque; +BlockReopenQueue *queue; +}BDRVReopenState ; + +typedef struct BlockReopenQueueEntry { +bool prepared; +BDRVReopenState state; +QSIMPLEQ_ENTRY (BlockReopenQueueEntry) entry; +}BlockReopenQueueEntry ; + +void bdrv_iostatus_enable (BlockDriverState *bs);void bdrv_iostatus_reset (BlockDriverState *bs);void bdrv_iostatus_disable (BlockDriverState *bs);
@@ -131,6 +147,13 @@ int bdrv_parse_cache_flags(const char *mode, int *flags);int bdrv_file_open (BlockDriverState **pbs,const char *filename,int flags);int bdrv_open (BlockDriverState *bs,const char *filename,int flags,BlockDriver *drv); +BlockReopenQueue *bdrv_reopen_queue (BlockReopenQueue *bs_queue, +BlockDriverState *bs,int flags); +int bdrv_reopen_multiple (BlockReopenQueue *bs_queue,Error **errp); +int bdrv_reopen (BlockDriverState *bs,int bdrv_flags,Error **errp); +int bdrv_reopen_prepare (BDRVReopenState *reopen_state,Error **errp); +void bdrv_reopen_commit (BDRVReopenState *reopen_state); +void bdrv_reopen_abort (BDRVReopenState *reopen_state);void bdrv_close (BlockDriverState *bs);int bdrv_attach_dev (BlockDriverState *bs,void *dev);void bdrv_attach_dev_nofail (BlockDriverState *bs,void *dev);
block_int.h +7 -0
@@ -139,6 +139,12 @@ struct BlockDriver {int instance_size;int (*bdrv_probe)(const uint8_t *buf,int buf_size,const char *filename);int (*bdrv_probe_device)(const char *filename); + +/* For handling image reopen for split or non-split files */ +int (*bdrv_reopen_prepare)(BDRVReopenState *reopen_state,Error **errp); +void (*bdrv_reopen_commit)(BDRVReopenState *reopen_state); +void (*bdrv_reopen_abort)(BDRVReopenState *reopen_state); +int (*bdrv_open)(BlockDriverState *bs,int flags);int (*bdrv_file_open)(BlockDriverState *bs,const char *filename,int flags); int (*bdrv_read)(BlockDriverState *bs, int64_t sector_num,
@@ -336,6 +342,7 @@ struct BlockDriverState {/* long-running background operation */ BlockJob *job; + };int get_tmp_filename (char *filename,int size);
qemu-common.h +1 -0
@@ -245,6 +245,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 ;
Signature · 1 line
1.7.11.4
PATCH v2 6/16 block: do not parse BDRV_O_CACHE_WB in raw block drivers
In reply to Jeff CodyBlock drivers should always open the files in writeback mode (see commit e1e9b0ac), so raw-posix/raw-win32 should not parse the BDRV_O_CACHE_WB flag.
--- block/raw-posix.c | 3 --- block/raw-win32.c | 3 --- 2 files changed, 6 deletions(-)
block/raw-posix.c +0 -3
@@ -202,9 +202,6 @@ static void raw_parse_flags(int bdrv_flags, int *open_flags)if ((bdrv_flags &BDRV_O_NOCACHE )) { *open_flags |=O_DIRECT ; } -if (!(bdrv_flags &BDRV_O_CACHE_WB )) { - *open_flags |=O_DSYNC ; - } }static int raw_set_aio (void **aio_ctx,int *use_aio,int bdrv_flags)
block/raw-win32.c +0 -3
@@ -92,9 +92,6 @@ static void raw_parse_flags(int flags, int *access_flags, DWORD *overlapped)if (flags &BDRV_O_NOCACHE ) { *overlapped |=FILE_FLAG_NO_BUFFERING ; } -if (!(flags &BDRV_O_CACHE_WB )) { - *overlapped |=FILE_FLAG_WRITE_THROUGH ; - } }static int raw_open (BlockDriverState *bs,const char *filename,int flags)
Signature · 1 line
1.7.11.4
PATCH v2 4/16 block: move aio initialization into a helper function
In reply to Jeff CodyMove AIO initialization for raw-posix block driver into a helper function.
In addition to just code motion, the aio_ctx pointer is checked for NULL, prior to calling laio_init(), to make sure laio_init() is only run once.
--- block/raw-posix.c | 55 +++++++++++++++++++++++++++++++++++-------------------- 1 file changed, 35 insertions(+), 20 deletions(-)
block/raw-posix.c +35 -20
@@ -185,6 +185,40 @@ static int raw_normalize_devicepath(const char **filename) }#endif +static int raw_set_aio (void **aio_ctx,int *use_aio,int bdrv_flags) +{ +#ifdef CONFIG_LINUX_AIO +int ret =-1 ; +assert (aio_ctx !=NULL ); +assert (use_aio !=NULL ); +/* +* Currently Linux do AIO only for files opened with O_DIRECT +* specified so check NOCACHE flag too +*/ +if ((bdrv_flags & (BDRV_O_NOCACHE |BDRV_O_NATIVE_AIO )) == + (BDRV_O_NOCACHE |BDRV_O_NATIVE_AIO )) { + +/* if non-NULL, laio_init() has already been run */ +if (*aio_ctx ==NULL ) { + *aio_ctx =laio_init (); +if (!*aio_ctx) { + goto error; + } + } + *use_aio =1 ; + }else { + *use_aio =0 ; + } + + ret =0 ; + +error: +return ret; +#else +return 0 ; +#endif +} +static int raw_open_common (BlockDriverState *bs,const char *filename,int bdrv_flags,int open_flags) {
@@ -239,26 +273,7 @@ static int raw_open_common(BlockDriverState *bs, const char *filename, goto out_free_buf; } -#ifdef CONFIG_LINUX_AIO -/* -* Currently Linux do AIO only for files opened with O_DIRECT -* specified so check NOCACHE flag too -*/ -if ((bdrv_flags & (BDRV_O_NOCACHE |BDRV_O_NATIVE_AIO )) == - (BDRV_O_NOCACHE |BDRV_O_NATIVE_AIO )) { - - s->aio_ctx =laio_init (); -if (!s->aio_ctx) { - goto out_free_buf; - } - s->use_aio =1 ; - } else -#endif - { -#ifdef CONFIG_LINUX_AIO - s->use_aio =0 ; -#endif - } +raw_set_aio (&s->aio_ctx, &s->use_aio, bdrv_flags);#ifdef CONFIG_XFS if (platform_test_xfs_fd (s->fd)) {
Signature · 1 line
1.7.11.4
PATCH v2 10/16 block: qed image file reopen
In reply to Jeff CodyThese are the stubs for the file reopen drivers for the qed format.
There is currently nothing that needs to be done by the qed driver in reopen.
--- block/qed.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)
block/qed.c +20 -0
@@ -505,6 +505,23 @@ out:return ret; } +/* We have nothing to do for QED reopen, stubs just return +* success */ +static int bdrv_qed_reopen_prepare (BDRVReopenState *state,Error **errp) +{ +return 0 ; +} + +static void bdrv_qed_reopen_commit (BDRVReopenState *state) +{ +return ; +} + +static void bdrv_qed_reopen_abort (BDRVReopenState *state) +{ +return ; +} +static void bdrv_qed_close (BlockDriverState *bs) {BDRVQEDState *s = bs->opaque;
@@ -1564,6 +1581,9 @@ static BlockDriver bdrv_qed = { .bdrv_rebind = bdrv_qed_rebind, .bdrv_open = bdrv_qed_open, .bdrv_close = bdrv_qed_close, + .bdrv_reopen_prepare = bdrv_qed_reopen_prepare, + .bdrv_reopen_commit = bdrv_qed_reopen_commit, + .bdrv_reopen_abort = bdrv_qed_reopen_abort, .bdrv_create = bdrv_qed_create, .bdrv_co_is_allocated = bdrv_qed_co_is_allocated, .bdrv_make_empty = bdrv_qed_make_empty,
Signature · 1 line
1.7.11.4
PATCH v2 9/16 block: raw image file reopen
In reply to Jeff CodyThese are the stubs for the file reopen drivers for the raw format.
There is currently nothing that needs to be done by the raw driver in reopen.
--- block/raw.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)
block/raw.c +22 -0
@@ -9,6 +9,24 @@ static int raw_open(BlockDriverState *bs, int flags)return 0 ; } +/* We have nothing to do for raw reopen, stubs just return +* success */ +static int raw_reopen_prepare (BDRVReopenState *state,Error **errp) +{ +return 0 ; +} + +static void raw_reopen_commit (BDRVReopenState *state) +{ +return ; +} + +static void raw_reopen_abort (BDRVReopenState *state) +{ +return ; +} + + static int coroutine_fn raw_co_readv(BlockDriverState *bs, int64_t sector_num,int nb_sectors,QEMUIOVector *qiov) {
@@ -115,6 +133,10 @@ static BlockDriver bdrv_raw = { .bdrv_open = raw_open, .bdrv_close = raw_close, + .bdrv_reopen_prepare = raw_reopen_prepare, + .bdrv_reopen_commit = raw_reopen_commit, + .bdrv_reopen_abort = raw_reopen_abort, + .bdrv_co_readv = raw_co_readv, .bdrv_co_writev = raw_co_writev, .bdrv_co_is_allocated = raw_co_is_allocated,
Signature · 1 line
1.7.11.4
PATCH v2 7/16 block: move allocating aligned_buf into a helper function in raw_posix.c
In reply to Jeff CodyCode motion, to move allocating aligned_buf and setting aligned_buf_size into a helper function.
--- block/raw-posix.c | 31 ++++++++++++++++++++++--------- 1 file changed, 22 insertions(+), 9 deletions(-)
block/raw-posix.c +22 -9
@@ -238,6 +238,26 @@ error:#endif } +static int raw_allocate_aligned_buf (uint8_t **aligned_buf, +unsigned *aligned_buf_size,int bdrv_flags) +{ +assert (aligned_buf !=NULL ); +assert (aligned_buf_size !=NULL ); + +if ((bdrv_flags &BDRV_O_NOCACHE )) { +/* +* Allocate a buffer for read/modify/write cycles. Chose the size +* pessimistically as we don't know the block size yet. +*/ + *aligned_buf_size =32 *MAX_BLOCKSIZE ; + *aligned_buf =qemu_memalign (MAX_BLOCKSIZE , *aligned_buf_size); +if (*aligned_buf ==NULL ) { +return -1 ; + } + } +return 0 ; +} +static int raw_open_common (BlockDriverState *bs,const char *filename,int bdrv_flags,int open_flags) {
@@ -263,16 +283,9 @@ static int raw_open_common(BlockDriverState *bs, const char *filename, s->fd = fd; s->aligned_buf =NULL ; -if ((bdrv_flags &BDRV_O_NOCACHE )) { -/* -* Allocate a buffer for read/modify/write cycles. Chose the size -* pessimistically as we don't know the block size yet. -*/ - s->aligned_buf_size =32 *MAX_BLOCKSIZE ; - s->aligned_buf =qemu_memalign (MAX_BLOCKSIZE , s->aligned_buf_size); -if (s->aligned_buf ==NULL ) { +if (raw_allocate_aligned_buf (&s->aligned_buf, &s->aligned_buf_size, + bdrv_flags)) { goto out_close; - } }/* We're falling back to POSIX AIO in some cases so init always */
Signature · 1 line
1.7.11.4
PATCH v2 8/16 block: raw-posix image file reopen
In reply to Jeff CodyThis is derived from the Supriya Kannery's reopen patches.
This contains the raw-posix driver changes for the bdrv_reopen_* functions. All changes are staged into a temporary scratch buffer during the prepare() stage, and copied over to the live structure during commit(). Upon abort(), all changes are abandoned, and the live structures are unmodified.
The _prepare() will create an extra fd - either by means of a dup, if possible, or opening a new fd if not (for instance, access control changes). Upon _commit(), the original fd is closed and the new fd is used. Upon _abort(), the duplicate/new fd is closed.
--- block/raw-posix.c | 123 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 123 insertions(+)
block/raw-posix.c +123 -0
@@ -140,6 +140,19 @@ typedef struct BDRVRawState {#endif } BDRVRawState; +typedef struct BDRVRawReopenState { +BDRVReopenState reopen_state; +int fd; +int open_flags; +#ifdef CONFIG_LINUX_AIO +int use_aio; +void *aio_ctx; +#endif +uint8_t *aligned_buf; +unsigned aligned_buf_size; +BDRVRawState *stash_s; +}BDRVRawReopenState ; +static int fd_open (BlockDriverState *bs);static int64_t raw_getlength (BlockDriverState *bs);
@@ -318,6 +331,113 @@ static int raw_open(BlockDriverState *bs, const char *filename, int flags)return raw_open_common (bs, filename, flags,0 ); } +static int raw_reopen_prepare (BDRVReopenState *state,Error **errp) +{ +BDRVRawState *s; +BDRVRawReopenState *raw_s; +int ret =0 ; + +assert (state !=NULL ); +assert (state->bs !=NULL ); + + s = state->bs->opaque; + + state->opaque =g_malloc0 (sizeof (BDRVRawReopenState)); + raw_s = state->opaque; + raw_s->use_aio = s->use_aio; + raw_s->aio_ctx = s->aio_ctx; + +raw_parse_flags (state->flags, &raw_s->open_flags); +raw_set_aio (&raw_s->aio_ctx, &raw_s->use_aio, state->flags); + +/* +* If we didn't have BDRV_O_NOCACHE set before, we may not have allocated +* aligned_buf +*/ + ret =raw_allocate_aligned_buf (&raw_s->aligned_buf, + &raw_s->aligned_buf_size, state->flags); +if (ret) { + goto error; + } + + raw_s->fd =-1 ; + +int fcntl_flags =O_APPEND |O_ASYNC |O_NONBLOCK ; +#ifdef O_NOATIME + fcntl_flags |=O_NOATIME ; +#endif +if ((raw_s->open_flags & ~fcntl_flags) == (s->open_flags & ~fcntl_flags)) { +/* dup the original fd */ +/* TODO: use qemu fcntl wrapper */ + raw_s->fd =fcntl (s->fd,F_DUPFD_CLOEXEC ,0 ); +if (raw_s->fd >=0 ) { + ret =fcntl_setfl (raw_s->fd, raw_s->open_flags); +if (ret) { +qemu_close (raw_s->fd); + raw_s->fd =-1 ; + } + } + } + +/* If we cannot use fctnl, or fcntl failed, fall back to qemu_open() */ +if (raw_s->fd ==-1 ) { +assert (!(raw_s->open_flags &O_CREAT )); + raw_s->fd =qemu_open (state->bs->filename, raw_s->open_flags); +if (raw_s->fd ==-1 ) { + ret =-1 ; + } + } +error: +return ret; +} + + +static void raw_reopen_commit (BDRVReopenState *state) +{ +BDRVRawReopenState *raw_s = state->opaque; +BDRVRawState *s = state->bs->opaque; + +if (raw_s->aligned_buf !=NULL ) { +if (s->aligned_buf) { +qemu_vfree (s->aligned_buf); + } + s->aligned_buf = raw_s->aligned_buf; + s->aligned_buf_size = raw_s->aligned_buf_size; + } + + s->open_flags = raw_s->open_flags; + +qemu_close (s->fd); + s->fd = raw_s->fd; + s->use_aio = raw_s->use_aio; + s->aio_ctx = raw_s->aio_ctx; + +g_free (state->opaque); + state->opaque =NULL ; +} + + +static void raw_reopen_abort (BDRVReopenState *state) +{ +BDRVRawReopenState *raw_s = state->opaque; + +/* nothing to do if NULL, we didn't get far enough */ +if (raw_s ==NULL ) { +return ; + } + +if (raw_s->fd >=0 ) { +qemu_close (raw_s->fd); + raw_s->fd =-1 ; +if (raw_s->aligned_buf !=NULL ) { +qemu_vfree (raw_s->aligned_buf); + } + } +g_free (state->opaque); + state->opaque =NULL ; +} + +/* XXX: use host sector size if necessary with: #ifdef DIOCGSECTORSIZE {
@@ -770,6 +890,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,
Signature · 1 line
1.7.11.4
PATCH v2 11/16 block: qcow2 image file reopen
In reply to Jeff CodyThese are the stubs for the file reopen drivers for the qcow2 format.
There is currently nothing that needs to be done by the qcow2 driver in reopen.
--- block/qcow2.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)
block/qcow2.c +22 -0
@@ -52,6 +52,7 @@ typedef struct {uint32_t magic;uint32_t len; } QCowExtension; +#define QCOW2_EXT_MAGIC_END 0#define QCOW2_EXT_MAGIC_BACKING_FORMAT 0xE2792ACA#define QCOW2_EXT_MAGIC_FEATURE_TABLE 0x6803f857
@@ -558,6 +559,24 @@ static int qcow2_set_key(BlockDriverState *bs, const char *key)return 0 ; } +/* We have nothing to do for QCOW2 reopen, stubs just return +* success */ +static int qcow2_reopen_prepare (BDRVReopenState *state,Error **errp) +{ +return 0 ; +} + +static void qcow2_reopen_commit (BDRVReopenState *state) +{ +return ; +} + +static void qcow2_reopen_abort (BDRVReopenState *state) +{ +return ; +} + + static int coroutine_fn qcow2_co_is_allocated(BlockDriverState *bs,int64_t sector_num,int nb_sectors,int *pnum) {
@@ -1679,6 +1698,9 @@ static BlockDriver bdrv_qcow2 = { .bdrv_probe = qcow2_probe, .bdrv_open = qcow2_open, .bdrv_close = qcow2_close, + .bdrv_reopen_prepare = qcow2_reopen_prepare, + .bdrv_reopen_commit = qcow2_reopen_commit, + .bdrv_reopen_abort = qcow2_reopen_abort, .bdrv_create = qcow2_create, .bdrv_co_is_allocated = qcow2_co_is_allocated, .bdrv_set_key = qcow2_set_key,
Signature · 1 line
1.7.11.4
PATCH v2 13/16 block: vmdk image file reopen
In reply to Jeff CodyThis patch supports reopen for VMDK image files. VMDK extents are added to the existing reopen queue, so that the transactional model of reopen is maintained with multiple image files.
--- block/vmdk.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+)
block/vmdk.c +47 -0
@@ -300,6 +300,50 @@ static int vmdk_is_cid_valid(BlockDriverState *bs)return 1 ; } +/* Queue extents, if any, for reopen() */ +static int vmdk_reopen_prepare (BDRVReopenState *state,Error **errp) +{ +BDRVVmdkState *s; +int ret =-1 ; +int i; +VmdkExtent *e; + +assert (state !=NULL ); +assert (state->bs !=NULL ); + +if (state->queue ==NULL ) { +error_set (errp,ERROR_CLASS_GENERIC_ERROR , +"No reopen queue for VMDK extents" ); + goto exit; + } + + s = state->bs->opaque; + +assert (s !=NULL ); + +for (i =0 ; i < s->num_extents; i++) { + e = &s->extents[i]; +if (e->file != state->bs->file) { +bdrv_reopen_queue (state->queue, e->file, state->flags); + } + } + ret =0 ; + +exit: +return ret; +} + +/* No action for commit, or abort. All VMDK has to do is +* queue the extents for reopen */ +static void vmdk_reopen_commit (BDRVReopenState *state) +{ +} + +static void vmdk_reopen_abort (BDRVReopenState *state) +{ +} + +static int vmdk_parent_open (BlockDriverState *bs) {char *p_name;
@@ -1646,6 +1690,9 @@ 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,
Signature · 1 line
1.7.11.4
PATCH v2 14/16 block: raw-win32 driver reopen support
In reply to Jeff CodyThis is the support for reopen, for win32. Note that there is a key difference in the win32 reopen, namely that it is not guaranteed safe like all the other drivers. Attempts are made to reopen the file, or open the file in a new handle prior to closing the old handle. However, this is not always feasible, and there are times when we must close the existing file and then open the new file, breaking the transactional nature of the reopen.
--- block/raw-posix.c | 1 - block/raw-win32.c | 104 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 104 insertions(+), 1 deletion(-)
block/raw-posix.c +0 -1
@@ -141,7 +141,6 @@ typedef struct BDRVRawState { } BDRVRawState; typedef struct BDRVRawReopenState { - BDRVReopenState reopen_state; int fd; int open_flags; #ifdef CONFIG_LINUX_AIO
block/raw-win32.c +104 -0
128 lines not on this page · raw message
Signature · 1 line
1.7.11.4
PATCH v2 12/16 block: qcow image file reopen
In reply to Jeff CodyThese are the stubs for the file reopen drivers for the qcow format.
There is currently nothing that needs to be done by the qcow driver in reopen.
--- block/qcow.c | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+)
block/qcow.c +23 -0
@@ -197,6 +197,26 @@ static int qcow_open(BlockDriverState *bs, int flags)return ret; } + +/* We have nothing to do for QCOW reopen, stubs just return +* success */ +static int qcow_reopen_prepare (BDRVReopenState *state,Error **errp) +{ +return 0 ; +} + +static void qcow_reopen_commit (BDRVReopenState *state) +{ +return ; +} + +static void qcow_reopen_abort (BDRVReopenState *state) +{ +return ; +} + + +static int qcow_set_key (BlockDriverState *bs,const char *key) {BDRVQcowState *s = bs->opaque;
@@ -868,6 +888,9 @@ static BlockDriver bdrv_qcow = { .bdrv_probe = qcow_probe, .bdrv_open = qcow_open, .bdrv_close = qcow_close, + .bdrv_reopen_prepare = qcow_reopen_prepare, + .bdrv_reopen_commit = qcow_reopen_commit, + .bdrv_reopen_abort = qcow_reopen_abort, .bdrv_create = qcow_create, .bdrv_co_readv = qcow_co_readv,
Signature · 1 line
1.7.11.4
PATCH v2 16/16 block: remove keep_read_only flag from BlockDriverState struct
In reply to Jeff CodyThe keep_read_only flag is no longer used, in favor of the bdrv flag BDRV_O_ALLOW_RDWR.
--- block.c | 2 -- block_int.h | 1 - 2 files changed, 3 deletions(-)
block.c +0 -2
@@ -808,8 +808,6 @@ int bdrv_open(BlockDriverState *bs, const char *filename, int flags, flags |=BDRV_O_ALLOW_RDWR ; } - bs->keep_read_only = !(flags &BDRV_O_ALLOW_RDWR ); -/* Open the image */ ret =bdrv_open_common (bs, filename, flags, drv);if (ret <0 ) {
block_int.h +0 -1
@@ -274,7 +274,6 @@ struct BlockDriverState {int64_t total_sectors;/* if we are reading a disk image, give its size in sectors */ int read_only;/* if true, the media is read only */ -int keep_read_only;/* if true, the media was requested to stay read only */ int open_flags;/* flags used to open the file, re-used for re-open */ int encrypted;/* if true, the media is encrypted */ int valid_key;/* if true, a valid encryption key has been set */
Signature · 1 line
1.7.11.4
PATCH v2 5/16 block: move open flag parsing in raw block drivers to helper functions
In reply to Jeff CodyCode motion, to move parsing of open flags into a helper function.
--- block/raw-posix.c | 38 ++++++++++++++++++++++++-------------- block/raw-win32.c | 43 +++++++++++++++++++++++-------------------- 2 files changed, 47 insertions(+), 34 deletions(-)
block/raw-posix.c +24 -14
@@ -185,6 +185,28 @@ static int raw_normalize_devicepath(const char **filename) }#endif +static void raw_parse_flags (int bdrv_flags,int *open_flags) +{ +assert (open_flags !=NULL ); + + *open_flags |=O_BINARY ; + *open_flags &= ~O_ACCMODE ; +if (bdrv_flags &BDRV_O_RDWR ) { + *open_flags |=O_RDWR ; + }else { + *open_flags |=O_RDONLY ; + } + +/* Use O_DSYNC for write-through caching, no flags for write-back caching, +* and O_DIRECT for no caching. */ +if ((bdrv_flags &BDRV_O_NOCACHE )) { + *open_flags |=O_DIRECT ; + } +if (!(bdrv_flags &BDRV_O_CACHE_WB )) { + *open_flags |=O_DSYNC ; + } +} +static int raw_set_aio (void **aio_ctx,int *use_aio,int bdrv_flags) {#ifdef CONFIG_LINUX_AIO
@@ -230,20 +252,8 @@ static int raw_open_common(BlockDriverState *bs, const char *filename,return ret; } - s->open_flags = open_flags |O_BINARY ; - s->open_flags &= ~O_ACCMODE ; -if (bdrv_flags &BDRV_O_RDWR ) { - s->open_flags |=O_RDWR ; - }else { - s->open_flags |=O_RDONLY ; - } - -/* Use O_DSYNC for write-through caching, no flags for write-back caching, -* and O_DIRECT for no caching. */ -if ((bdrv_flags &BDRV_O_NOCACHE )) - s->open_flags |=O_DIRECT ; -if (!(bdrv_flags &BDRV_O_CACHE_WB )) - s->open_flags |=O_DSYNC ; + s->open_flags = open_flags; +raw_parse_flags (bdrv_flags, &s->open_flags); s->fd =-1 ; fd =qemu_open (filename, s->open_flags,0644 );
block/raw-win32.c +23 -20
64 lines not on this page · raw message
Signature · 1 line
1.7.11.4
PATCH v2 15/16 block: convert bdrv_commit() to use bdrv_reopen()
In reply to Jeff CodyCurrently, bdrv_commit() reopens images r/w itself, via bdrv_delete() and bdrv_open() calls. Use the new safe method for drive reopen.
--- block.c | 48 +++++------------------------------------------- 1 file changed, 5 insertions(+), 43 deletions(-)
block.c +5 -43
@@ -1490,13 +1490,11 @@ int bdrv_check(BlockDriverState *bs, BdrvCheckResult *res, BdrvCheckMode fix)int bdrv_commit (BlockDriverState *bs) {BlockDriver *drv = bs->drv; -BlockDriver *backing_drv;int64_t sector, total_sectors;int n, ro, open_flags; -int ret =0 , rw_ret =0 ; +int ret =0 ;uint8_t *buf;char filename[1024 ]; -BlockDriverState *bs_rw, *bs_ro;if (!drv)return -ENOMEDIUM ;
@@ -1505,42 +1503,18 @@ int bdrv_commit(BlockDriverState *bs)return -ENOTSUP ; } -if (bs->backing_hd->keep_read_only) { -return -EACCES ; - } -if (bdrv_in_use (bs) ||bdrv_in_use (bs->backing_hd)) {return -EBUSY ; } - backing_drv = bs->backing_hd->drv; ro = bs->backing_hd->read_only;strncpy (filename, bs->backing_hd->filename,sizeof (filename)); open_flags = bs->backing_hd->open_flags;if (ro) { -/* re-open as RW */ -bdrv_delete (bs->backing_hd); - bs->backing_hd =NULL ; - bs_rw =bdrv_new ("" ); - rw_ret =bdrv_open (bs_rw, filename, open_flags |BDRV_O_RDWR , - backing_drv); -if (rw_ret <0 ) { -bdrv_delete (bs_rw); -/* try to re-open read-only */ - bs_ro =bdrv_new ("" ); - ret =bdrv_open (bs_ro, filename, open_flags & ~BDRV_O_RDWR , - backing_drv); -if (ret <0 ) { -bdrv_delete (bs_ro); -/* drive not functional anymore */ - bs->drv =NULL ; -return ret; - } - bs->backing_hd = bs_ro; -return rw_ret; +if (bdrv_reopen (bs->backing_hd, open_flags |BDRV_O_RDWR ,NULL )) { +return -EACCES ; } - bs->backing_hd = bs_rw; } total_sectors =bdrv_getlength (bs) >>BDRV_SECTOR_BITS ;
@@ -1577,20 +1551,8 @@ ro_cleanup:g_free (buf);if (ro) { -/* re-open as RO */ -bdrv_delete (bs->backing_hd); - bs->backing_hd =NULL ; - bs_ro =bdrv_new ("" ); - ret =bdrv_open (bs_ro, filename, open_flags & ~BDRV_O_RDWR , - backing_drv); -if (ret <0 ) { -bdrv_delete (bs_ro); -/* drive not functional anymore */ - bs->drv =NULL ; -return ret; - } - bs->backing_hd = bs_ro; - bs->backing_hd->keep_read_only =0 ; +/* ignoring error return here */ +bdrv_reopen (bs->backing_hd, open_flags & ~BDRV_O_RDWR ,NULL ); }return ret;
Signature · 1 line
1.7.11.4
Re: [Qemu-devel] [PATCH v2 08/16] block: raw-posix image file reopen
In reply to Jeff CodyIl 13/09/2012 17:49, Jeff Cody ha scritto:
block/raw-posix.c static int raw_open(BlockDriverState *bs, const char *filename, int flags) · line 358
Quoted 7 lines
block/raw-posix.c +7
-0+ +/* +* If we didn't have BDRV_O_NOCACHE set before, we may not have allocated +* aligned_buf +*/ + ret =raw_allocate_aligned_buf (&raw_s->aligned_buf, + &raw_s->aligned_buf_size, state->flags);
Aligned_buf is unused, except for checking if it exists here:
if (s->aligned_buf) {
if (!qiov_is_aligned(bs, qiov)) {
type |= QEMU_AIO_MISALIGNED;
#ifdef CONFIG_LINUX_AIO
} else if (s->use_aio) {
return laio_submit(bs, s->aio_ctx, s->fd, sector_num, qiov,
nb_sectors, cb, opaque, type);
#endif
}
}You can instead check BDRV_O_NOCACHE and kill aligned_buf completely.
Paolo
Re: [Qemu-devel] [PATCH v2 09/16] block: raw image file reopen
In reply to Jeff CodyIl 13/09/2012 17:49, Jeff Cody ha scritto:
Quoted 8 lines
block/raw.c +8
-0+/* We have nothing to do for raw reopen, stubs just return +* success */ +static int raw_reopen_prepare (BDRVReopenState *state,Error **errp) +{ +return 0 ; +} + +static void raw_reopen_commit (BDRVReopenState *state)
block/raw.c static int raw_open(BlockDriverState *bs, int flags) · line 29
Quoted 10 lines
block/raw.c +10
-0+{ +return ; +} + +static void raw_reopen_abort (BDRVReopenState *state) +{ +return ; +} + +
Commit and abort are optional, aren't they?
BTW, vdi and vpc should also support reopen.
Paolo
Re: [Qemu-devel] [PATCH v2 06/16] block: do not parse BDRV_O_CACHE_WB in raw block drivers
In reply to Jeff CodyIl 13/09/2012 17:49, Jeff Cody ha scritto:
Quoted 30 lines
Block drivers should always open the files in writeback mode (see commit
e1e9b0ac), so raw-posix/raw-win32 should not parse theBDRV_O_CACHE_WBflag.Signed-off-by Jeff Cody <[email protected]>--- block/raw-posix.c | 3 --- block/raw-win32.c | 3 --- 2 files changed, 6 deletions(-)block/raw-posix.c +0
-3@@ -202,9 +202,6 @@ static void raw_parse_flags(int bdrv_flags, int *open_flags)if ((bdrv_flags &BDRV_O_NOCACHE )) { *open_flags |=O_DIRECT ; } -if (!(bdrv_flags &BDRV_O_CACHE_WB )) { - *open_flags |=O_DSYNC ; - } }static int raw_set_aio (void **aio_ctx,int *use_aio,int bdrv_flags)block/raw-win32.c +0
-0
block/raw-win32.c static void raw_parse_flags(int flags, int *access_flags, DWORD *overlapped) · line 97
Quoted 9 lines
block/raw-win32.c +0
-3@@ -92,9 +92,6 @@ static void raw_parse_flags(int flags, int *access_flags, DWORD *overlapped)if (flags &BDRV_O_NOCACHE ) { *overlapped |=FILE_FLAG_NO_BUFFERING ; } -if (!(flags &BDRV_O_CACHE_WB )) { - *overlapped |=FILE_FLAG_WRITE_THROUGH ; - } }static int raw_open (BlockDriverState *bs,const char *filename,int flags)
Why does this matter? If raw-posix was opened directly (i.e. without the bs->file indirection) this would cause a writethrough file to be incorrectly opened as writeback.
Paolo
Re: [Qemu-devel] [PATCH v2 03/16] block: Framework for reopening files safely
In reply to Jeff CodyIl 13/09/2012 17:49, Jeff Cody ha scritto:
block.c unlink_and_fail: · line 1065
Quoted 3 lines
block.c +3
-0+ reopen_state->bs->open_flags = reopen_state->flags; + reopen_state->bs->enable_write_cache = !!(reopen_state->flags & +BDRV_O_CACHE_WB );
I think setting enable_write_cache is wrong here because this is really part of the guest state, not the host state (yes, it's a mess). Instead, you could proceed like this:
1) change bdrv_enable_write_cache like this:
int bdrv_enable_write_cache(BlockDriverState *bs)
{
return bs->enable_write_cache && (bs->open_flags & BDRV_O_CACHE_WB);
}and use it in block.c instead of checking bs->enable_write_cache directly;
2) always initialize bs->enable_write_cache to true, rather than to the value of BDRV_O_CACHE_WB;
This way bs->enable_write_cache is _really_ guest state rather than a mix.
3) never touch enable_write_cache in bdrv_reopen.
Paolo
Re: [Qemu-devel] [PATCH v2 08/16] block: raw-posix image file reopen
In reply to Paolo BonziniQuoted 22 lines · Paolo Bonzini
Il 13/09/2012 17:49, Jeff Cody ha scritto:
block/raw-posix.c static int raw_open(BlockDriverState *bs, const char *filename, int flags) · line 358
Quoted 7 lines
block/raw-posix.c +7
-0+ +/* +* If we didn't have BDRV_O_NOCACHE set before, we may not have allocated +* aligned_buf +*/ + ret =raw_allocate_aligned_buf (&raw_s->aligned_buf, + &raw_s->aligned_buf_size, state->flags);
Aligned_bufis unused, except for checking if it exists here:if (s->aligned_buf) { if (!qiov_is_aligned(bs, qiov)) { type |= QEMU_AIO_MISALIGNED; #ifdef CONFIG_LINUX_AIO } else if (s->use_aio) { return laio_submit(bs, s->aio_ctx, s->fd, sector_num, qiov, nb_sectors, cb, opaque, type); #endif } }
Quoted 4 lines
You can instead check
BDRV_O_NOCACHEand killaligned_bufcompletely.Paolo
Hmm - yes, it looks like that is so. This patch will transform into checking for BDRV_O_NOCACHE as you suggest, and killing aligned_buf.
Thanks,
Jeff
Re: [Qemu-devel] [PATCH v2 09/16] block: raw image file reopen
In reply to Paolo BonziniQuoted 22 lines · Paolo Bonzini
Il 13/09/2012 17:49, Jeff Cody ha scritto:
Quoted 8 lines
block/raw.c +8
-0+/* We have nothing to do for raw reopen, stubs just return +* success */ +static int raw_reopen_prepare (BDRVReopenState *state,Error **errp) +{ +return 0 ; +} + +static void raw_reopen_commit (BDRVReopenState *state)block/raw.c static int raw_open(BlockDriverState *bs, int flags) · line 29
Quoted 10 lines
block/raw.c +10
-0+{ +return ; +} + +static void raw_reopen_abort (BDRVReopenState *state) +{ +return ; +} + +Commit and abort are optional, aren't they?
Quoted 4 lines
BTW, vdi and vpc should also support reopen.
Paolo
The commit and abort block driver handlers are indeed optional, prepare is the only required one. I kept the stubs for them for completeness, however (I can remove them if that causes heartburn for others).
A quick look at the vdi and vpc block drivers seems to indicate that both of those can be supported via stubs as well; I'll add them to v3.
Thanks,
Jeff
Re: [Qemu-devel] [PATCH v2 06/16] block: do not parse BDRV_O_CACHE_WB in raw block drivers
In reply to Paolo BonziniQuoted 38 lines · Paolo Bonzini
Il 13/09/2012 17:49, Jeff Cody ha scritto:
Quoted 37 lines
Block drivers should always open the files in writeback mode (see commit
e1e9b0ac), so raw-posix/raw-win32 should not parse theBDRV_O_CACHE_WBflag.Signed-off-by Jeff Cody <[email protected]>--- block/raw-posix.c | 3 --- block/raw-win32.c | 3 --- 2 files changed, 6 deletions(-)block/raw-posix.c +0
-3@@ -202,9 +202,6 @@ static void raw_parse_flags(int bdrv_flags, int *open_flags)if ((bdrv_flags &BDRV_O_NOCACHE )) { *open_flags |=O_DIRECT ; } -if (!(bdrv_flags &BDRV_O_CACHE_WB )) { - *open_flags |=O_DSYNC ; - } }static int raw_set_aio (void **aio_ctx,int *use_aio,int bdrv_flags)block/raw-win32.c +0
-3@@ -92,9 +92,6 @@ static void raw_parse_flags(int flags, int *access_flags, DWORD *overlapped)if (flags &BDRV_O_NOCACHE ) { *overlapped |=FILE_FLAG_NO_BUFFERING ; } -if (!(flags &BDRV_O_CACHE_WB )) { - *overlapped |=FILE_FLAG_WRITE_THROUGH ; - } }
Quoted 9 lines
block/raw-win32.c static void raw_parse_flags(int flags, int *access_flags, DWORD *overlapped) · line 97
Quoted 2 lines
block/raw-win32.c +0
-0static int raw_open (BlockDriverState *bs,const char *filename,int flags)Why does this matter? If raw-posix was opened directly (i.e. without the
bs->fileindirection) this would cause a writethrough file to be incorrectly opened as writeback.Paolo
The problem this patch was trying to work around is that bdrv_open_common() forces BDRV_O_CACHE_WB (commit e1e9b0ac), but that setting is not preserved in bs->open_flags, so it is lost on a reopen.
Is there a scenario currently that has raw-posix opened directly as a writethrough file, or were you more concerned with future use?
Re: [Qemu-devel] [PATCH v2 07/16] block: move allocating aligned_buf into a helper function in raw_posix.c
In reply to Jeff Codyblock/raw-posix.c error: · line 249
Quoted 12 lines · Jeff Cody
Code motion, to move allocating
aligned_bufand settingaligned_buf_sizeinto a helper function.Signed-off-by Jeff Cody <[email protected]>--- block/raw-posix.c | 31 ++++++++++++++++++++++--------- 1 file changed, 22 insertions(+), 9 deletions(-)block/raw-posix.c +3
-0+if ((bdrv_flags &BDRV_O_NOCACHE )) { +/* +* Allocate a buffer for read/modify/write cycles. Chose the size
Pre-existing typo, but as long as you are moving it,
s/Chose/Choose/
Signature · 2 lines
Eric Blake [email protected] +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Re: [Qemu-devel] [PATCH v2 09/16] block: raw image file reopen
In reply to Jeff CodyIl 13/09/2012 19:02, Jeff Cody ha scritto:
Quoted 2 lines
I kept the stubs for them for completeness,
however (I can remove them if that causes heartburn for others).
The point of making them optional is to avoid stubs, isn't it? ;)
Paolo
Re: [Qemu-devel] [PATCH v2 06/16] block: do not parse BDRV_O_CACHE_WB in raw block drivers
In reply to Jeff CodyIl 13/09/2012 19:17, Jeff Cody ha scritto:
Quoted 10 lines
Quoted 7 lines
Quoted 7 lines
Why does this matter? If raw-posix was opened directly (i.e. without the
bs->fileindirection) this would cause a writethrough file to be incorrectly opened as writeback.Paolo
The problem this patch was trying to work around is that
bdrv_open_common()forcesBDRV_O_CACHE_WB(commite1e9b0ac), but that setting is not preserved inbs->open_flags, so it is lost on a reopen.
Perhaps we _should_ preserve that in bs->open_flags, while still using the initial value of BDRV_O_CACHE_WB to initialize bs->enable_write_cache.
Quoted 2 lines
Is there a scenario currently that has raw-posix opened directly as a writethrough file, or were you more concerned with future use?
Not for raw-posix, but IIRC some other protocol is opened directly without a format on top. rbd perhaps? I'm concerned of having to work around what seems like a bug elsewhere, in multiple protocols.
Paolo
Re: [Qemu-devel] [PATCH v2 06/16] block: do not parse BDRV_O_CACHE_WB in raw block drivers
Quoted 6 lines · Paolo Bonzini
Il 13/09/2012 19:17, Jeff Cody ha scritto:
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Why does this matter? If raw-posix was opened directly (i.e. without the
bs->fileindirection) this would cause a writethrough file to be incorrectly opened as writeback.
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
Quoted 2 lines
Paolo
The problem this patch was trying to work around is that
bdrv_open_common()forcesBDRV_O_CACHE_WB(commite1e9b0ac), but that setting is not preserved inbs->open_flags, so it is lost on a reopen.Perhaps we _should_ preserve that in
bs->open_flags, while still using the initial value ofBDRV_O_CACHE_WBto initializebs->enable_write_cache.
That would work. Part of the problem is that BDRV_O_CACHE_WB seems overloaded; maybe bdrv_parse_cache_flags() should set a new flag, called BDRV_O_CACHE_WCE, which can be used in lieu of enable_write_cache (similar to getting rid of keep_read_only in favor of
BDRV_O_ALLOW_RDWR). And then bdrv_parse_cache_flags() would just not
set BDRV_O_CACHE_WB, which can then be used exclusively by the lower layers for their parsing (and bdrv_open_common would just set bs->open_flags to always have BDRV_O_CACHE_WB).
Then patch 2/16 would change to having bdrv_set_enable_write_cache() toggle BDRV_O_CACHE_WCE.
Quoted 10 lines
Quoted 3 lines
Is there a scenario currently that has raw-posix opened directly as a writethrough file, or were you more concerned with future use?
Not for raw-posix, but IIRC some other protocol is opened directly without a format on top. rbd perhaps? I'm concerned of having to work around what seems like a bug elsewhere, in multiple protocols.
Paolo
Re: [Qemu-devel] [PATCH v2 09/16] block: raw image file reopen
In reply to Paolo BonziniQuoted 8 lines · Paolo Bonzini
Il 13/09/2012 19:02, Jeff Cody ha scritto:
Quoted 2 lines
I kept the stubs for them for completeness,
however (I can remove them if that causes heartburn for others).The point of making them optional is to avoid stubs, isn't it? ;)
Paolo
Fair point - consider them removed for v3. :)
Re: [Qemu-devel] [PATCH v2 06/16] block: do not parse BDRV_O_CACHE_WB in raw block drivers
In reply to Jeff CodyIl 13/09/2012 21:04, Jeff Cody ha scritto:
Quoted 11 lines
Quoted 2 lines
Quoted 2 lines
Perhaps we _should_ preserve that in
bs->open_flags, while still using the initial value ofBDRV_O_CACHE_WBto initializebs->enable_write_cache.That would work. Part of the problem is that
BDRV_O_CACHE_WBseems overloaded; maybebdrv_parse_cache_flags()should set a new flag, calledBDRV_O_CACHE_WCE, which can be used in lieu ofenable_write_cache(similar to getting rid ofkeep_read_onlyin favor of
BDRV_O_ALLOW_RDWR). And thenbdrv_parse_cache_flags()would just not
setBDRV_O_CACHE_WB, which can then be used exclusively by the lower layers for their parsing (andbdrv_open_commonwould just setbs->open_flagsto always haveBDRV_O_CACHE_WB).
Quoted 3 lines
Then patch 2/16 would change to having
bdrv_set_enable_write_cache()toggleBDRV_O_CACHE_WCE.
Yeah, that would work.
Alternatively, we can keep this patch and leave bdrv_open_common as is; but then I would also prefer if raw-{posix,win32} took care of setting BDRV_O_CACHE_WB in bs->open_flags, so that the flags are consistent. This setting of BDRV_O_CACHE_WB can be extended later to other formats.
Considering this and my comments to patch 3/16 we would have:
- after opening with cache=writethrough:
bs bs->file enable_write_cache true true BDRV_O_CACHE_WB false true bdrv_enable_write_cache() false true
- after opening with cache=writeback:
bs bs->file enable_write_cache true true BDRV_O_CACHE_WB true true bdrv_enable_write_cache() true true
Paolo
Re: [Qemu-devel] [PATCH v2 03/16] block: Framework for reopening files safely
In reply to Jeff CodyIl 13/09/2012 17:49, Jeff Cody ha scritto:
block.h typedef enum { · line 106
Quoted 7 lines
block.h +7
-0+typedef QSIMPLEQ_HEAD(BlockReopenQueue, BlockReopenQueueEntry) BlockReopenQueue; + +typedef struct BDRVReopenState { + BlockDriverState *bs; + int flags; + void *opaque; + BlockReopenQueue *queue;
Do we need the queue pointer here? Or it can be a separate argument to prepare? Commit and abort don't need it, and it may mess things up a bit if commit calls bdrv_reopen_queue (because then the newly-added element will get a commit without being prepared).
block.h typedef enum { · line 114
Quoted 8 lines
block.h +8
-0+} BDRVReopenState; + +typedef struct BlockReopenQueueEntry { +bool prepared; +BDRVReopenState state; +QSIMPLEQ_ENTRY (BlockReopenQueueEntry) entry; +}BlockReopenQueueEntry ; +
A small change: please move struct BlockReopenQueueEntry to block.c.
Paolo
Re: [Qemu-devel] [PATCH v2 06/16] block: do not parse BDRV_O_CACHE_WB in raw block drivers
In reply to Paolo BonziniIl 13/09/2012 21:44, Paolo Bonzini ha scritto:
Quoted 16 lines
Il 13/09/2012 21:04, Jeff Cody ha scritto:
Quoted 11 lines
Quoted 2 lines
Quoted 2 lines
Perhaps we _should_ preserve that in
bs->open_flags, while still using the initial value ofBDRV_O_CACHE_WBto initializebs->enable_write_cache.That would work. Part of the problem is that
BDRV_O_CACHE_WBseems overloaded; maybebdrv_parse_cache_flags()should set a new flag, calledBDRV_O_CACHE_WCE, which can be used in lieu ofenable_write_cache(similar to getting rid ofkeep_read_onlyin favor of
BDRV_O_ALLOW_RDWR). And thenbdrv_parse_cache_flags()would just not
setBDRV_O_CACHE_WB, which can then be used exclusively by the lower layers for their parsing (andbdrv_open_commonwould just setbs->open_flagsto always haveBDRV_O_CACHE_WB).Quoted 3 lines
Then patch 2/16 would change to having
bdrv_set_enable_write_cache()toggleBDRV_O_CACHE_WCE.
Quoted 6 lines
Yeah, that would work.
Alternatively, we can keep this patch and leave
bdrv_open_commonas is; but then I would also prefer if raw-{posix,win32} took care of settingBDRV_O_CACHE_WBinbs->open_flags, so that the flags are consistent. This setting ofBDRV_O_CACHE_WBcan be extended later to other formats.
Hmm, no, what was I thinking...
But there is a very simple thing we can do:
- patch 2 can be left as is
- in patch 3 bdrv_reopen_queue, always add BDRV_O_CACHE_WB to bs_entry->state.flags
- in patch 3 bdrv_reopen_commit, always leave BDRV_O_CACHE_WB aside:
reopen_state->bs->open_flags =
(bs->open_flags & BDRV_O_CACHE_WB) |
(reopen_state->flags & ~BDRV_O_CACHE_WB);- this patch can be dropped completely.
Paolo
Re: [Qemu-devel] [PATCH v2 03/16] block: Framework for reopening files safely
In reply to Paolo BonziniQuoted 14 lines · Paolo Bonzini
Il 13/09/2012 17:49, Jeff Cody ha scritto:
block.h typedef enum { · line 106
Quoted 7 lines
block.h +7
-0+typedef QSIMPLEQ_HEAD(BlockReopenQueue, BlockReopenQueueEntry) BlockReopenQueue; + +typedef struct BDRVReopenState { + BlockDriverState *bs; + int flags; + void *opaque; + BlockReopenQueue *queue;Do we need the queue pointer here? Or it can be a separate argument to prepare? Commit and abort don't need it, and it may mess things up a bit if commit calls
bdrv_reopen_queue(because then the newly-added element will get a commit without being prepared).
It could, and should, be a separate argument to prepare(), as you are correct - commit() and abort() should never use it.
Quoted 11 lines
block.h typedef enum { · line 114
Quoted 8 lines
block.h +8
-0+} BDRVReopenState; + +typedef struct BlockReopenQueueEntry { +bool prepared; +BDRVReopenState state; +QSIMPLEQ_ENTRY (BlockReopenQueueEntry) entry; +}BlockReopenQueueEntry ; +A small change: please move struct BlockReopenQueueEntry to
block.c.
OK
Re: [Qemu-devel] [PATCH v2 06/16] block: do not parse BDRV_O_CACHE_WB in raw block drivers
In reply to Paolo BonziniQuoted 33 lines · Paolo Bonzini
Il 13/09/2012 21:44, Paolo Bonzini ha scritto:
Quoted 16 lines
Il 13/09/2012 21:04, Jeff Cody ha scritto:
Quoted 11 lines
Quoted 2 lines
Quoted 2 lines
Perhaps we _should_ preserve that in
bs->open_flags, while still using the initial value ofBDRV_O_CACHE_WBto initializebs->enable_write_cache.That would work. Part of the problem is that
BDRV_O_CACHE_WBseems overloaded; maybebdrv_parse_cache_flags()should set a new flag, calledBDRV_O_CACHE_WCE, which can be used in lieu ofenable_write_cache(similar to getting rid ofkeep_read_onlyin favor of
BDRV_O_ALLOW_RDWR). And thenbdrv_parse_cache_flags()would just not
setBDRV_O_CACHE_WB, which can then be used exclusively by the lower layers for their parsing (andbdrv_open_commonwould just setbs->open_flagsto always haveBDRV_O_CACHE_WB).Quoted 3 lines
Then patch 2/16 would change to having
bdrv_set_enable_write_cache()toggleBDRV_O_CACHE_WCE.Quoted 6 lines
Yeah, that would work.
Alternatively, we can keep this patch and leave
bdrv_open_commonas is; but then I would also prefer if raw-{posix,win32} took care of settingBDRV_O_CACHE_WBinbs->open_flags, so that the flags are consistent. This setting ofBDRV_O_CACHE_WBcan be extended later to other formats.Hmm, no, what was I thinking...
But there is a very simple thing we can do:
- patch 2 can be left as is
- in patch 3
bdrv_reopen_queue, always addBDRV_O_CACHE_WBtobs_entry->state.flags
Quoted 8 lines
- in patch 3
bdrv_reopen_commit, always leaveBDRV_O_CACHE_WBaside:reopen_state->bs->open_flags = (bs->open_flags & BDRV_O_CACHE_WB) | (reopen_state->flags & ~BDRV_O_CACHE_WB);- this patch can be dropped completely.
Yes, I think that would work. The only thing I don't like is that BDRV_O_CACHE_WB still remains a bit confusing when looking through the code... bs->open_flags does not actually represent the open flags.
With what I proposed above, here are the steps needed:
- bdrv_parse_cache_flags() sets BDRV_O_CACHE_WCE instead of BDRV_O_CACHE_WB
- BDRV_O_CACHE_WCE is used everywhere enable_write_cache was used
- bs->enable_write_cache is removed
- this patch is dropped
- bdrv_open_common() sets bs->open_flags to have BDRV_O_CACHE_WB enabled by default.
- The only way BDRV_O_CACHE_WB would get cleared from bs->open_flags now would be if someone explicitly cleared it during a bdrv_reopen().
While there are more changes this way, I think it cleans up the code a bit. The advantage is that bs->open_flags actually reflects the open flags that are currently in use. One disadvantage I see is that it seems a bit odd to have BDRV_O_CACHE_WCE cleared and BDRV_O_CACHE_WB set, until you think about them being intended for different layers.
Maybe that is part of the underlying problem - there is one open_flags variable in the BDS, that has some flags intended for all layers in the block stack, and some flags specific to a layer.
Re: [Qemu-devel] [PATCH v2 06/16] block: do not parse BDRV_O_CACHE_WB in raw block drivers
Il 13/09/2012 23:45, Jeff Cody ha scritto:
Quoted 5 lines
While there are more changes this way, I think it cleans up the code a bit. The advantage is that
bs->open_flagsactually reflects the open flags that are currently in use. One disadvantage I see is that it seems a bit odd to haveBDRV_O_CACHE_WCEcleared andBDRV_O_CACHE_WBset, until you think about them being intended for different layers.
It's more weird to see BDRV_O_CACHE_WCE set and BDRV_O_CACHE_WB cleared. :)
Quoted 3 lines
Maybe that is part of the underlying problem - there is one
open_flagsvariable in the BDS, that has some flags intended for all layers in the block stack, and some flags specific to a layer.
Yes, that's true.
Paolo
Re: [Qemu-devel] [PATCH v2 06/16] block: do not parse BDRV_O_CACHE_WB in raw block drivers
In reply to Paolo BonziniAm 13.09.2012 18:12, schrieb Paolo Bonzini:
Quoted 38 lines
Il 13/09/2012 17:49, Jeff Cody ha scritto:
Quoted 37 lines
Block drivers should always open the files in writeback mode (see commit
e1e9b0ac), so raw-posix/raw-win32 should not parse theBDRV_O_CACHE_WBflag.Signed-off-by Jeff Cody <[email protected]>--- block/raw-posix.c | 3 --- block/raw-win32.c | 3 --- 2 files changed, 6 deletions(-)block/raw-posix.c +0
-3@@ -202,9 +202,6 @@ static void raw_parse_flags(int bdrv_flags, int *open_flags)if ((bdrv_flags &BDRV_O_NOCACHE )) { *open_flags |=O_DIRECT ; } -if (!(bdrv_flags &BDRV_O_CACHE_WB )) { - *open_flags |=O_DSYNC ; - } }static int raw_set_aio (void **aio_ctx,int *use_aio,int bdrv_flags)block/raw-win32.c +0
-3@@ -92,9 +92,6 @@ static void raw_parse_flags(int flags, int *access_flags, DWORD *overlapped)if (flags &BDRV_O_NOCACHE ) { *overlapped |=FILE_FLAG_NO_BUFFERING ; } -if (!(flags &BDRV_O_CACHE_WB )) { - *overlapped |=FILE_FLAG_WRITE_THROUGH ; - } }
Quoted 6 lines
block/raw-win32.c static void raw_parse_flags(int flags, int *access_flags, DWORD *overlapped) · line 97
Quoted 2 lines
block/raw-win32.c +0
-0static int raw_open (BlockDriverState *bs,const char *filename,int flags)Why does this matter? If raw-posix was opened directly (i.e. without the
bs->fileindirection) this would cause a writethrough file to be incorrectly opened as writeback.
--verbose, please.
I can't see how bs->file is needed here for writethrough semantics. bdrv_open_common() sets bs->enable_write_cache to false and bdrv_co_do_writev() checks it and flushes if necessary. Looks fine to me.
In fact, bdrv_open_common() even removes BDRV_O_CACHE_WB, so what Jeff removes here is really dead code (checked with strace: The file isn't opened with O_SYNC even when using -drive format=file).
Kevin
Re: [Qemu-devel] [PATCH v2 08/16] block: raw-posix image file reopen
In reply to Jeff CodyAm 13.09.2012 18:57, schrieb Jeff Cody:
Quoted 23 lines · Paolo Bonzini
Quoted 22 lines
Il 13/09/2012 17:49, Jeff Cody ha scritto:
block/raw-posix.c static int raw_open(BlockDriverState *bs, const char *filename, int flags) · line 358
Quoted 7 lines
block/raw-posix.c +7
-0+ +/* +* If we didn't have BDRV_O_NOCACHE set before, we may not have allocated +* aligned_buf +*/ + ret =raw_allocate_aligned_buf (&raw_s->aligned_buf, + &raw_s->aligned_buf_size, state->flags);
Aligned_bufis unused, except for checking if it exists here:if (s->aligned_buf) { if (!qiov_is_aligned(bs, qiov)) { type |= QEMU_AIO_MISALIGNED; #ifdef CONFIG_LINUX_AIO } else if (s->use_aio) { return laio_submit(bs, s->aio_ctx, s->fd, sector_num, qiov, nb_sectors, cb, opaque, type); #endif } }
Quoted 7 lines
Quoted 4 lines
You can instead check
BDRV_O_NOCACHEand killaligned_bufcompletely.Paolo
Hmm - yes, it looks like that is so. This patch will transform into checking for
BDRV_O_NOCACHEas you suggest, and killingaligned_buf.
Sounds good, but please make removing aligned_buf a separate patch.
Kevin
Re: [Qemu-devel] [PATCH v2 06/16] block: do not parse BDRV_O_CACHE_WB in raw block drivers
In reply to Kevin WolfIl 14/09/2012 09:27, Kevin Wolf ha scritto:
Quoted 3 lines
I can't see how
bs->fileis needed here for writethrough semantics.bdrv_open_common()setsbs->enable_write_cacheto false andbdrv_co_do_writev()checks it and flushes if necessary. Looks fine to me.
You're right.
Quoted 3 lines
In fact,
bdrv_open_common()even removesBDRV_O_CACHE_WB, so what Jeff removes here is really dead code (checked with strace: The file isn't opened withO_SYNCeven when using -drive format=file).
Yes, it's dead, on the other hand we still honor BDRV_O_CACHE_WB in all the other protocols. Either we go and touch all the protocols (effectively removing BDRV_O_CACHE_WB from the BlockDriver specification), or treating raw-{posix,win32} specially means we leave bugs everywhere else.
Paolo
Re: [Qemu-devel] [PATCH v2 06/16] block: do not parse BDRV_O_CACHE_WB in raw block drivers
In reply to Jeff CodyAm 13.09.2012 23:45, schrieb Jeff Cody:
Quoted 56 lines · Paolo Bonzini
Quoted 33 lines
Il 13/09/2012 21:44, Paolo Bonzini ha scritto:
Quoted 16 lines
Il 13/09/2012 21:04, Jeff Cody ha scritto:
Quoted 11 lines
Quoted 2 lines
Quoted 2 lines
Perhaps we _should_ preserve that in
bs->open_flags, while still using the initial value ofBDRV_O_CACHE_WBto initializebs->enable_write_cache.That would work. Part of the problem is that
BDRV_O_CACHE_WBseems overloaded; maybebdrv_parse_cache_flags()should set a new flag, calledBDRV_O_CACHE_WCE, which can be used in lieu ofenable_write_cache(similar to getting rid ofkeep_read_onlyin favor of
BDRV_O_ALLOW_RDWR). And thenbdrv_parse_cache_flags()would just not
setBDRV_O_CACHE_WB, which can then be used exclusively by the lower layers for their parsing (andbdrv_open_commonwould just setbs->open_flagsto always haveBDRV_O_CACHE_WB).Quoted 3 lines
Then patch 2/16 would change to having
bdrv_set_enable_write_cache()toggleBDRV_O_CACHE_WCE.Quoted 6 lines
Yeah, that would work.
Alternatively, we can keep this patch and leave
bdrv_open_commonas is; but then I would also prefer if raw-{posix,win32} took care of settingBDRV_O_CACHE_WBinbs->open_flags, so that the flags are consistent. This setting ofBDRV_O_CACHE_WBcan be extended later to other formats.Hmm, no, what was I thinking...
But there is a very simple thing we can do:
- patch 2 can be left as is
- in patch 3
bdrv_reopen_queue, always addBDRV_O_CACHE_WBtobs_entry->state.flagsQuoted 8 lines
- in patch 3
bdrv_reopen_commit, always leaveBDRV_O_CACHE_WBaside:reopen_state->bs->open_flags = (bs->open_flags & BDRV_O_CACHE_WB) | (reopen_state->flags & ~BDRV_O_CACHE_WB);- this patch can be dropped completely.
Yes, I think that would work. The only thing I don't like is that
BDRV_O_CACHE_WBstill remains a bit confusing when looking through the code...bs->open_flagsdoes not actually represent the open flags.With what I proposed above, here are the steps needed:
-
bdrv_parse_cache_flags()setsBDRV_O_CACHE_WCEinstead ofBDRV_O_CACHE_WB-
BDRV_O_CACHE_WCEis used everywhereenable_write_cachewas used-
bs->enable_write_cacheis removed
Quoted 7 lines
- this patch is dropped
-
bdrv_open_common()setsbs->open_flagsto haveBDRV_O_CACHE_WBenabled by default.- The only way
BDRV_O_CACHE_WBwould get cleared frombs->open_flagsnow would be if someone explicitly cleared it during abdrv_reopen().
This feels totally wrong. Your BDRV_O_CACHE_WCE is what BDRV_O_CACHE_WB should be; removing bs->enable_write_cache in favour of a fix BDRV_O_CACHE_WB makes sense, though (maybe we need to consider renaming bs->open_flags then, it really hasn't anything to do with open and more). All block drivers, including raw-{posix,win32}, should completely ignore the flag in their .bdrv_open implementation, this flag is purely for the generic block layer. Drivers always open everything writeback and provide a flush function. They already do today, because today's broken BDRV_O_CACHE_WB is always set.
Kevin
Re: [Qemu-devel] [PATCH v2 06/16] block: do not parse BDRV_O_CACHE_WB in raw block drivers
In reply to Paolo BonziniAm 14.09.2012 09:50, schrieb Paolo Bonzini:
Quoted 11 lines
Il 14/09/2012 09:27, Kevin Wolf ha scritto:
Quoted 3 lines
I can't see how
bs->fileis needed here for writethrough semantics.bdrv_open_common()setsbs->enable_write_cacheto false andbdrv_co_do_writev()checks it and flushes if necessary. Looks fine to me.You're right.
Quoted 3 lines
In fact,
bdrv_open_common()even removesBDRV_O_CACHE_WB, so what Jeff removes here is really dead code (checked with strace: The file isn't opened withO_SYNCeven when using -drive format=file).
Quoted 5 lines
Yes, it's dead, on the other hand we still honor
BDRV_O_CACHE_WBin all the other protocols. Either we go and touch all the protocols (effectively removingBDRV_O_CACHE_WBfrom the BlockDriver specification), or treating raw-{posix,win32} specially means we leave bugs everywhere else.
Yes, touch all protocols and fix them. BDRV_O_CACHE_WB should be for block.c only.
Kevin
Re: [Qemu-devel] [PATCH v2 06/16] block: do not parse BDRV_O_CACHE_WB in raw block drivers
Il 14/09/2012 09:55, Kevin Wolf ha scritto:
Quoted 6 lines
Quoted 4 lines
Either we go and touch all the protocols
Quoted 3 lines
(effectively removing
BDRV_O_CACHE_WBfrom the BlockDriver specification), or treating raw-{posix,win32} specially means we leave bugs everywhere else.Yes, touch all protocols and fix them.
BDRV_O_CACHE_WBshould be forblock.conly.
That's also fine, of course.
Paolo