Add "read-only" to the options QDict
v2 cover Add "read-only" to the options QDict
This series adds "read-only" to the options QDict, fixing a long-standing problem with the reopening code.
[E] <- [D] <- [C] <- [B] <- [A]
In a normal scenario, the active layer [A] is in read-write mode and everything else is read-only. If we reopen [D] in read-write mode and later reopen [B], then [D] will become read-only when inheriting the flags from its parent (see bdrv_backing_options()). With this series, inheriting options doesn't need to override values that have been explicitly set before.
The BDRV_O_RDWR flag is not removed yet, but its keep in sync with the value of the "read-only" option. Removing the BDRV_O_RDWR flag is not straightforward in all cases and can result in code that is significantly slower and less readable. Therefore it will be dealt with in the future.
Regards,
Berto
v2:
- Patch 2:
- Update description [Kevin]
- Patch 4:
- Inherit the value of "read-only" in bdrv_temp_snapshot_options().
[Kevin]
- Remove "read-only" from qemu_root_bds_opts and simply set a default value in bds_tree_init(). [Kevin]
- Patch 7:
- Rebase after the changes made to qemu_root_bds_opts in patch 4.
v1: https://lists.gnu.org/archive/html/qemu-block/2016-09/msg00191.html
- Initial release
git backport-diff against v1:
Key:
[----] : patches are identical
[####] : number of functional differences between upstream/downstream patch [down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively
001/7:[----] [--] 'block: Remove bdrv_is_snapshot'
002/7:[----] [--] 'block: Set BDRV_O_ALLOW_RDWR and snapshot_options before storing the flags' 003/7:[----] [--] 'block: Update bs->open_flags earlier in bdrv_open_common()' 004/7:[0013] [FC] 'block: Add "read-only" to the options QDict'
005/7:[----] [--] 'block: Don't queue the same BDS twice in bdrv_reopen_queue_child()' 006/7:[----] [--] 'commit: Add 'base' to the reopen queue before 'overlay_bs'' 007/7:[0002] [FC] 'block: rename "read-only" to BDRV_OPT_READ_ONLY'
Alberto Garcia (7):
block: Remove bdrv_is_snapshot
block: Set BDRV_O_ALLOW_RDWR and snapshot_options before storing the
flags
block: Update bs->open_flags earlier in bdrv_open_common()
block: Add "read-only" to the options QDict
block: Don't queue the same BDS twice in bdrv_reopen_queue_child()
commit: Add 'base' to the reopen queue before 'overlay_bs'
block: rename "read-only" to BDRV_OPT_READ_ONLY
block.c | 89 +++++++++++++++++++++++++++++++++++++-------------- block/commit.c | 8 ++--- block/vvfat.c | 3 +- blockdev.c | 26 +++++++-------- include/block/block.h | 2 +- 5 files changed, 84 insertions(+), 44 deletions(-)
Signature · 1 line
2.9.3
PATCH v2 1/7 block: Remove bdrv_is_snapshot
This is unnecessary and has been unused since 5433c24f0f9306c82ad9bcc.
--- block.c | 5 ----- include/block/block.h | 1 - 2 files changed, 6 deletions(-)
block.c +0 -5
@@ -2965,11 +2965,6 @@ bool bdrv_debug_is_suspended(BlockDriverState *bs, const char *tag)return false; } -int bdrv_is_snapshot (BlockDriverState *bs) -{ -return !!(bs->open_flags &BDRV_O_SNAPSHOT ); -} -/* backing_file can either be relative, or absolute, or a protocol. If it is * relative, it must be relative to the chain. So, passing in bs->filename * from a BDS as backing_file should not be done, as that may be relative to
include/block/block.h +0 -1
@@ -420,7 +420,6 @@ void bdrv_get_full_backing_filename_from_filename(const char *backed,const char *backing, char *dest, size_t sz, Error **errp); -int bdrv_is_snapshot (BlockDriverState *bs);int path_has_protocol (const char *path);int path_is_absolute (const char *path);
Signature · 1 line
2.9.3
PATCH v2 2/7 block: Set BDRV_O_ALLOW_RDWR and snapshot_options before storing the flags
In reply to Alberto GarciaIf an image is opened with snapshot=on, its flags are modified by bdrv_backing_options() and then bs->open_flags is updated accordingly. This last step is unnecessary if we calculate the new flags before setting bs->open_flags.
Soon we'll introduce the "read-only" option, and then we'll need to be able to modify its value in the QDict when snapshot=on. This is more cumbersome if bs->options is already set. This patch simplifies that. Other than that, there are no semantic changes. Although it might seem that bs->options can have a different value now because it is stored after calling bdrv_backing_options(), this call doesn't actually modify them in this scenario.
The code that sets BDRV_O_ALLOW_RDWR is also moved for the same reason.
--- block.c | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-)
block.c +11 -12
@@ -1627,6 +1627,17 @@ static BlockDriverState *bdrv_open_inherit(const char *filename, goto fail; } +if (flags &BDRV_O_RDWR ) { + flags |=BDRV_O_ALLOW_RDWR ; + } + +if (flags &BDRV_O_SNAPSHOT ) { + snapshot_options =qdict_new (); +bdrv_temp_snapshot_options (&snapshot_flags, snapshot_options, + flags, options); +bdrv_backing_options (&flags, options, flags, options); + } + bs->open_flags = flags; bs->options = options; options =qdict_clone_shallow (options);
@@ -1651,18 +1662,6 @@ static BlockDriverState *bdrv_open_inherit(const char *filename,/* Open image file without format layer */ if ((flags &BDRV_O_PROTOCOL ) ==0 ) { -if (flags &BDRV_O_RDWR ) { - flags |=BDRV_O_ALLOW_RDWR ; - } -if (flags &BDRV_O_SNAPSHOT ) { - snapshot_options =qdict_new (); -bdrv_temp_snapshot_options (&snapshot_flags, snapshot_options, - flags, options); -bdrv_backing_options (&flags, options, flags, options); - } - - bs->open_flags = flags; - file =bdrv_open_child (filename, options,"file" , bs, &child_file, true, &local_err);if (local_err) {
Signature · 1 line
2.9.3
PATCH v2 3/7 block: Update bs->open_flags earlier in bdrv_open_common()
In reply to Alberto GarciaWe're only doing this immediately before opening the image, but bs->open_flags is used earlier in the function. At the moment this is not causing problems because none of the checked flags are modified by update_flags_from_options(), but this will change when we introduce the "read-only" option.
This patch calls update_flags_from_options() at the beginning of the function, immediately after creating the QemuOpts.
--- block.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
block.c +2 -3
@@ -913,6 +913,8 @@ static int bdrv_open_common(BlockDriverState *bs, BdrvChild *file, goto fail_opts; } +update_flags_from_options (&bs->open_flags, opts); + driver_name =qemu_opt_get (opts,"driver" ); drv =bdrv_find_format (driver_name);assert (drv !=NULL );
@@ -974,9 +976,6 @@ static int bdrv_open_common(BlockDriverState *bs, BdrvChild *file, bs->drv = drv; bs->opaque =g_malloc0 (drv->instance_size); -/* Apply cache mode options */ -update_flags_from_options (&bs->open_flags, opts); -/* Open the image, either directly or using a protocol */ open_flags =bdrv_open_flags (bs, bs->open_flags);if (drv->bdrv_file_open) {
Signature · 1 line
2.9.3
PATCH v2 5/7 block: Don't queue the same BDS twice in bdrv_reopen_queue_child()
In reply to Alberto Garciabdrv_reopen_queue_child() assumes that a BlockDriverState is never added twice to BlockReopenQueue.
That's however not the case: commit_start() adds 'base' (and its children) to a new reopen queue, and then 'overlay_bs' (and its children, which include 'base') to the same queue. The effect of this is that the first set of options is ignored and overriden by the second.
We fixed this by swapping the order in which both BDSs were added to the queue in 3db2bd5508c86a1605258bc77c9672d93b5c350e. This patch checks if a BDS is already in the reopen queue and keeps its options.
--- block.c | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-)
block.c +19 -3
@@ -1877,6 +1877,13 @@ static BlockReopenQueue *bdrv_reopen_queue_child(BlockReopenQueue *bs_queue, options =qdict_new (); } +/* Check if this BlockDriverState is already in the queue */ + QSIMPLEQ_FOREACH(bs_entry, bs_queue, entry) { +if (bs == bs_entry->state.bs) { +break ; + } + } +/* * Precedence of options: * 1. Explicitly passed in options (highest)
@@ -1897,7 +1904,11 @@ static BlockReopenQueue *bdrv_reopen_queue_child(BlockReopenQueue *bs_queue, }/* Old explicitly set values (don't overwrite by inherited value) */ - old_options =qdict_clone_shallow (bs->explicit_options); +if (bs_entry) { + old_options =qdict_clone_shallow (bs_entry->state.explicit_options); + }else { + old_options =qdict_clone_shallow (bs->explicit_options); + }bdrv_join_options (bs, options, old_options);QDECREF (old_options);
@@ -1936,8 +1947,13 @@ static BlockReopenQueue *bdrv_reopen_queue_child(BlockReopenQueue *bs_queue, child->role, options, flags); } - bs_entry =g_new0 (BlockReopenQueueEntry,1 ); -QSIMPLEQ_INSERT_TAIL (bs_queue, bs_entry, entry); +if (!bs_entry) { + bs_entry =g_new0 (BlockReopenQueueEntry,1 ); +QSIMPLEQ_INSERT_TAIL (bs_queue, bs_entry, entry); + }else { +QDECREF (bs_entry->state.options); +QDECREF (bs_entry->state.explicit_options); + } bs_entry->state.bs = bs; bs_entry->state.options = options;
Signature · 1 line
2.9.3
PATCH v2 6/7 commit: Add 'base' to the reopen queue before 'overlay_bs'
In reply to Alberto GarciaNow that we're checking for duplicates in the reopen queue, there's no need to force a specific order in which the queue is constructed so we can revert 3db2bd5508c86a1605258bc77c9672d93b5c350e.
Since both ways of constructing the queue are now valid, this patch doesn't have any effect on the behavior of QEMU and is not strictly necessary. However it can help us check that the fix for the reopen queue is robust: if it stops working properly at some point, iotest 040 will break.
--- block/commit.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
block/commit.c +4 -4
@@ -243,14 +243,14 @@ void commit_start(const char *job_id, BlockDriverState *bs, orig_overlay_flags =bdrv_get_flags (overlay_bs);/* convert base & overlay_bs to r/w, if necessary */ -if (!(orig_overlay_flags &BDRV_O_RDWR )) { - reopen_queue =bdrv_reopen_queue (reopen_queue, overlay_bs,NULL , - orig_overlay_flags |BDRV_O_RDWR ); - }if (!(orig_base_flags &BDRV_O_RDWR )) { reopen_queue =bdrv_reopen_queue (reopen_queue, base,NULL , orig_base_flags |BDRV_O_RDWR ); } +if (!(orig_overlay_flags &BDRV_O_RDWR )) { + reopen_queue =bdrv_reopen_queue (reopen_queue, overlay_bs,NULL , + orig_overlay_flags |BDRV_O_RDWR ); + }if (reopen_queue) {bdrv_reopen_multiple (reopen_queue, &local_err);if (local_err !=NULL ) {
Signature · 1 line
2.9.3
PATCH v2 7/7 block: rename "read-only" to BDRV_OPT_READ_ONLY
In reply to Alberto GarciaThere were a few instances left. After this patch we're using the macro in all places.
--- blockdev.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
blockdev.c +5 -5
@@ -807,7 +807,7 @@ QemuOptsList qemu_legacy_drive_opts = {/* Options that are passed on, but have special semantics with -drive */ { - .name = "read-only", + .name = BDRV_OPT_READ_ONLY, .type = QEMU_OPT_BOOL, .help = "open drive file as read-only", },{
@@ -873,7 +873,7 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type) { "group", "throttling.group" }, - { "readonly", "read-only" }, + { "readonly", BDRV_OPT_READ_ONLY }, };for (i =0 ; i <ARRAY_SIZE (opt_renames); i++) {
@@ -945,7 +945,7 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type) }/* copy-on-read is disabled with a warning for read-only devices */ - read_only |=qemu_opt_get_bool (legacy_opts,"read-only" , false); + read_only |=qemu_opt_get_bool (legacy_opts,BDRV_OPT_READ_ONLY , false); copy_on_read =qemu_opt_get_bool (legacy_opts,"copy-on-read" , false);if (read_only && copy_on_read) {
@@ -953,7 +953,7 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type) copy_on_read = false; } -qdict_put (bs_opts,"read-only" , +qdict_put (bs_opts,BDRV_OPT_READ_ONLY ,qstring_from_str (read_only ?"on" :"off" ));qdict_put (bs_opts,"copy-on-read" ,qstring_from_str (copy_on_read ?"on" :"off" ));
@@ -4042,7 +4042,7 @@ QemuOptsList qemu_common_drive_opts = { .type = QEMU_OPT_STRING, .help = "write error action", },{ - .name = "read-only", + .name = BDRV_OPT_READ_ONLY, .type = QEMU_OPT_BOOL, .help = "open drive file as read-only", },{
Signature · 1 line
2.9.3
PATCH v2 4/7 block: Add "read-only" to the options QDict
In reply to Alberto GarciaThis adds the "read-only" option to the QDict. One important effect of this change is that when a child inherits options from its parent, the existing "read-only" mode can be preserved if it was explicitly set previously.
This addresses scenarios like this:
[E] <- [D] <- [C] <- [B] <- [A]
In this case, if we reopen [D] with read-only=off, and later reopen [B], then [D] will not inherit read-only=on from its parent during the bdrv_reopen_queue_child() stage.
The BDRV_O_RDWR flag is not removed yet, but its keep in sync with the value of the "read-only" option.
--- block.c | 38 +++++++++++++++++++++++++++++++++++--- block/vvfat.c | 3 ++- blockdev.c | 16 +++++++--------- include/block/block.h | 1 + 4 files changed, 45 insertions(+), 13 deletions(-)
block.c +35 -3
@@ -685,6 +685,9 @@ static void bdrv_temp_snapshot_options(int *child_flags, QDict *child_options,qdict_set_default_str (child_options,BDRV_OPT_CACHE_DIRECT ,"off" );qdict_set_default_str (child_options,BDRV_OPT_CACHE_NO_FLUSH ,"on" ); +/* Copy the read-only option from the parent */ +qdict_copy_default (child_options, parent_options,BDRV_OPT_READ_ONLY ); +/* aio=native doesn't work for cache.direct=off, so disable it for the * temporary snapshot */ *child_flags &= ~BDRV_O_NATIVE_AIO ;
@@ -707,6 +710,9 @@ static void bdrv_inherited_options(int *child_flags, QDict *child_options,qdict_copy_default (child_options, parent_options,BDRV_OPT_CACHE_DIRECT );qdict_copy_default (child_options, parent_options,BDRV_OPT_CACHE_NO_FLUSH ); +/* Inherit the read-only option from the parent if it's not set */ +qdict_copy_default (child_options, parent_options,BDRV_OPT_READ_ONLY ); +/* Our block drivers take care to send flushes and respect unmap policy, * so we can default to enable both on lower layers regardless of the * corresponding parent options. */
@@ -760,7 +766,8 @@ static void bdrv_backing_options(int *child_flags, QDict *child_options,qdict_copy_default (child_options, parent_options,BDRV_OPT_CACHE_NO_FLUSH );/* backing files always opened read-only */ - flags &= ~(BDRV_O_RDWR |BDRV_O_COPY_ON_READ ); +qdict_set_default_str (child_options,BDRV_OPT_READ_ONLY ,"on" ); + flags &= ~BDRV_O_COPY_ON_READ ;/* snapshot=on is handled on the top layer */ flags &= ~(BDRV_O_SNAPSHOT |BDRV_O_TEMPORARY );
@@ -807,6 +814,14 @@ static void update_flags_from_options(int *flags, QemuOpts *opts)if (qemu_opt_get_bool (opts,BDRV_OPT_CACHE_DIRECT , false)) { *flags |=BDRV_O_NOCACHE ; } + + *flags &= ~BDRV_O_RDWR ; + +assert (qemu_opt_find (opts,BDRV_OPT_READ_ONLY )); +if (!qemu_opt_get_bool (opts,BDRV_OPT_READ_ONLY , false)) { + *flags |=BDRV_O_RDWR ; + } + }static void update_options_from_flags (QDict *options,int flags)
@@ -819,6 +834,10 @@ static void update_options_from_flags(QDict *options, int flags)qdict_put (options,BDRV_OPT_CACHE_NO_FLUSH ,qbool_from_bool (flags &BDRV_O_NO_FLUSH )); } +if (!qdict_haskey (options,BDRV_OPT_READ_ONLY )) { +qdict_put (options,BDRV_OPT_READ_ONLY , +qbool_from_bool (!(flags &BDRV_O_RDWR ))); + } } static void bdrv_assign_node_name(BlockDriverState *bs,
@@ -882,6 +901,11 @@ static QemuOptsList bdrv_runtime_opts = { .type = QEMU_OPT_BOOL, .help = "Ignore flush requests", }, + { + .name = BDRV_OPT_READ_ONLY, + .type = QEMU_OPT_BOOL, + .help = "Node is opened in read-only mode", + }, {/* end of list */ } }, };
@@ -1626,14 +1650,22 @@ static BlockDriverState *bdrv_open_inherit(const char *filename, goto fail; } -if (flags &BDRV_O_RDWR ) { - flags |=BDRV_O_ALLOW_RDWR ; +/* Set the BDRV_O_RDWR and BDRV_O_ALLOW_RDWR flags. +* FIXME: we're parsing the QDict to avoid having to create a +* QemuOpts just for this, but neither option is optimal. */ +if (g_strcmp0 (qdict_get_try_str (options,BDRV_OPT_READ_ONLY ),"on" ) && + !qdict_get_try_bool (options,BDRV_OPT_READ_ONLY , false)) { + flags |= (BDRV_O_RDWR |BDRV_O_ALLOW_RDWR ); + }else { + flags &= ~BDRV_O_RDWR ; }if (flags &BDRV_O_SNAPSHOT ) { snapshot_options =qdict_new ();bdrv_temp_snapshot_options (&snapshot_flags, snapshot_options, flags, options); +/* Let bdrv_backing_options() override "read-only" */ +qdict_del (options,BDRV_OPT_READ_ONLY );bdrv_backing_options (&flags, options, flags, options); }
block/vvfat.c +2 -1
@@ -2971,7 +2971,8 @@ static BlockDriver vvfat_write_target = {static void vvfat_qcow_options (int *child_flags,QDict *child_options,int parent_flags,QDict *parent_options) { - *child_flags =BDRV_O_RDWR |BDRV_O_NO_FLUSH ; +qdict_set_default_str (child_options,BDRV_OPT_READ_ONLY ,"off" ); + *child_flags =BDRV_O_NO_FLUSH ; }static const BdrvChildRole child_vvfat_qcow = {
blockdev.c +7 -9
@@ -360,9 +360,6 @@ static void extract_common_blockdev_options(QemuOpts *opts, int *bdrv_flags,const char *aio;if (bdrv_flags) { -if (!qemu_opt_get_bool (opts,"read-only" , false)) { - *bdrv_flags |=BDRV_O_RDWR ; - }if (qemu_opt_get_bool (opts,"copy-on-read" , false)) { *bdrv_flags |=BDRV_O_COPY_ON_READ ; }
@@ -471,7 +468,7 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts,int bdrv_flags =0 ;int on_read_error, on_write_error;bool account_invalid, account_failed; -bool writethrough; +bool writethrough, read_only;BlockBackend *blk;BlockDriverState *bs;ThrottleConfig cfg;
@@ -567,6 +564,8 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts, bdrv_flags |=BDRV_O_SNAPSHOT ; } + read_only =qemu_opt_get_bool (opts,BDRV_OPT_READ_ONLY , false); +/* init */ if ((!file || !*file) && !qdict_size (bs_opts)) {BlockBackendRootState *blk_rs;
@@ -574,7 +573,7 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts, blk =blk_new (); blk_rs =blk_get_root_state (blk); blk_rs->open_flags = bdrv_flags; - blk_rs->read_only = !(bdrv_flags &BDRV_O_RDWR ); + blk_rs->read_only = read_only; blk_rs->detect_zeroes = detect_zeroes;QDECREF (bs_opts);
@@ -588,6 +587,8 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts,* Apply the defaults here instead. */ qdict_set_default_str (bs_opts,BDRV_OPT_CACHE_DIRECT ,"off" );qdict_set_default_str (bs_opts,BDRV_OPT_CACHE_NO_FLUSH ,"off" ); +qdict_set_default_str (bs_opts,BDRV_OPT_READ_ONLY , + read_only ?"on" :"off" );assert ((bdrv_flags &BDRV_O_CACHE_MASK ) ==0 );if (runstate_check (RUN_STATE_INMIGRATE )) {
@@ -682,6 +683,7 @@ static BlockDriverState *bds_tree_init(QDict *bs_opts, Error **errp)* Apply the defaults here instead. */ qdict_set_default_str (bs_opts,BDRV_OPT_CACHE_DIRECT ,"off" );qdict_set_default_str (bs_opts,BDRV_OPT_CACHE_NO_FLUSH ,"off" ); +qdict_set_default_str (bs_opts,BDRV_OPT_READ_ONLY ,"off" );if (runstate_check (RUN_STATE_INMIGRATE )) { bdrv_flags |=BDRV_O_INACTIVE ;
@@ -4159,10 +4161,6 @@ static QemuOptsList qemu_root_bds_opts = { .type = QEMU_OPT_STRING, .help = "host AIO implementation (threads, native)", },{ - .name = "read-only", - .type = QEMU_OPT_BOOL, - .help = "open drive file as read-only", - },{ .name = "copy-on-read", .type = QEMU_OPT_BOOL, .help = "copy read data from backing file into image file",
include/block/block.h +1 -0
@@ -107,6 +107,7 @@ typedef struct HDGeometry {#define BDRV_OPT_CACHE_WB "cache.writeback"#define BDRV_OPT_CACHE_DIRECT "cache.direct"#define BDRV_OPT_CACHE_NO_FLUSH "cache.no-flush" +#define BDRV_OPT_READ_ONLY "read-only"#define BDRV_SECTOR_BITS 9
Signature · 1 line
2.9.3
Kevin Wolf
AppliedAm 15.09.2016 um 16:52 hat Alberto Garcia geschrieben:
Quoted 12 lines
This series adds "read-only" to the options QDict, fixing a long-standing problem with the reopening code.
[E] <- [D] <- [C] <- [B] <- [A]In a normal scenario, the active layer [A] is in read-write mode and everything else is read-only. If we reopen [D] in read-write mode and later reopen [B], then [D] will become read-only when inheriting the flags from its parent (see
bdrv_backing_options()). With this series, inheriting options doesn't need to override values that have been explicitly set before.
Quoted 5 lines
The
BDRV_O_RDWRflag is not removed yet, but its keep in sync with the value of the "read-only" option. Removing theBDRV_O_RDWRflag is not straightforward in all cases and can result in code that is significantly slower and less readable. Therefore it will be dealt with in the future.
Thanks, applied to the block branch.
Kevin