Add "read-only" to the options QDict
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
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 | 86 +++++++++++++++++++++++++++++++++++++-------------- block/commit.c | 8 ++--- block/vvfat.c | 3 +- blockdev.c | 28 ++++++++++------- include/block/block.h | 2 +- 5 files changed, 86 insertions(+), 41 deletions(-)
Signature · 1 line
2.9.3
PATCH 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
Re: [Qemu-devel] [PATCH 1/7] block: Remove bdrv_is_snapshot
Am 14.09.2016 um 17:52 hat Alberto Garcia geschrieben:
Quoted 3 lines
This is unnecessary and has been unused since
5433c24f0f9306c82ad9bcc.Signed-off-by Alberto Garcia <[email protected]>
Re: [Qemu-devel] [PATCH 1/7] block: Remove bdrv_is_snapshot
In reply to Alberto GarciaQuoted 7 lines · Alberto Garcia
This is unnecessary and has been unused since
5433c24f0f9306c82ad9bcc.Signed-off-by Alberto Garcia <[email protected]>--- block.c | 5 ----- include/block/block.h | 1 - 2 files changed, 6 deletions(-)
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
PATCH 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.
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
Re: [Qemu-devel] [PATCH 2/7] block: Set BDRV_O_ALLOW_RDWR and snapshot_options before storing the flags
Am 14.09.2016 um 17:52 hat Alberto Garcia geschrieben:
Quoted 29 lines
If an image is opened with snapshot=on, its flags are modified by
bdrv_backing_options()and thenbs->open_flagsis updated accordingly. This last step is unnecessary if we calculate the new flags before settingbs->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->optionsis already set. This patch simplifies that.The code that sets
BDRV_O_ALLOW_RDWRis also moved for the same reason.Signed-off-by Alberto Garcia <[email protected]>--- block.c | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-)block.c +4
-0@@ -1627,6 +1627,17 @@ static BlockDriverState *bdrv_open_inherit(const char *filename, goto fail; } +if (flags &BDRV_O_RDWR ) { + flags |=BDRV_O_ALLOW_RDWR ; + } +
block.c static BlockDriverState *bdrv_open_inherit(const char *filename, · line 1643
Quoted 10 lines
block.c +7
-0+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);
Here I think we get different semantics now: bdrv_backing_options() only affected the cloned QDict before, and now it affects bs->options, too.
Given that the flags are already updated and don't contain BDRV_O_SNAPSHOT any more, I suspect that this is a silent bug fix. If so, it might be better to split it out into a separate patch before this one, or at the very least should be mentioned in the commit message.
If it's not a fix, however, it's probably a bug. :-)
Quoted 9 lines
block.c +0
-5@@ -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 ();
block.c static BlockDriverState *bdrv_open_inherit(const char *filename, · line 1667
Quoted 10 lines
block.c +0
-7-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) {
Kevin
Re: [Qemu-devel] [PATCH 2/7] block: Set BDRV_O_ALLOW_RDWR and snapshot_options before storing the flags
Quoted 10 lines · Kevin Wolf
Quoted 10 lines
block.c +10
-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); + }
Quoted 8 lines
Quoted 4 lines
+
bs->open_flags = flags; bs->options = options; options = qdict_clone_shallow(options);Here I think we get different semantics now:
bdrv_backing_options()only affected the cloned QDict before, and now it affectsbs->options, too.
The thing is that bdrv_backing_options() doesn't change the options in general, and in the case where it does (BDRV_OPT_READ_ONLY after the next patch) I think it makes sense that the options are changed. "snapshot=on" is a bit of a special case after all.
Berto
Re: [Qemu-devel] [PATCH 2/7] block: Set BDRV_O_ALLOW_RDWR and snapshot_options before storing the flags
Am 15.09.2016 um 13:24 hat Alberto Garcia geschrieben:
Quoted 20 lines · Kevin Wolf
Quoted 10 lines
Quoted 10 lines
block.c +10
-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); + }Quoted 8 lines
Quoted 4 lines
+
bs->open_flags = flags; bs->options = options; options = qdict_clone_shallow(options);Here I think we get different semantics now:
bdrv_backing_options()only affected the cloned QDict before, and now it affectsbs->options, too.
Quoted 4 lines
The thing is that
bdrv_backing_options()doesn't change the options in general, and in the case where it does (BDRV_OPT_READ_ONLYafter the next patch) I think it makes sense that the options are changed. "snapshot=on" is a bit of a special case after all.
Fair enough, it is correct (as I suspected), but it's not a bug fix because so far the options weren't changed, so there's no observable difference.
I think mentioning this in the commit message wouldn't hurt, though.
Kevin
Re: [Qemu-devel] [PATCH 2/7] block: Set BDRV_O_ALLOW_RDWR and snapshot_options before storing the flags
Quoted 19 lines · Kevin Wolf
Quoted 19 lines
Quoted 18 lines
Quoted 4 lines
block.c +4
-0+if (flags &BDRV_O_RDWR ) { + flags |=BDRV_O_ALLOW_RDWR ; + } +block.c static BlockDriverState *bdrv_open_inherit(const char *filename, · line 1643
Quoted 10 lines
block.c +7
-0+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);Here I think we get different semantics now:
bdrv_backing_options()only affected the cloned QDict before, and now it affectsbs->options, too.
Quoted 8 lines
Quoted 4 lines
The thing is that
bdrv_backing_options()doesn't change the options in general, and in the case where it does (BDRV_OPT_READ_ONLYafter the next patch) I think it makes sense that the options are changed. "snapshot=on" is a bit of a special case after all.Fair enough, it is correct (as I suspected), but it's not a bug fix because so far the options weren't changed, so there's no observable difference.
The idea of this change is to remove the ' bs->open_flags = flags ' line in the original code, as explained in the commit message.
That's one thing. The other (and most important one) is to prepare for the introduction of BDRV_OPT_READ_ONLY.
Without this patch we'd need to need to update not just bs->open_flags but bs->options as well, and it would be something like this:
bs->open_flags = flags; bs->options = options;
/* ... */
if ((flags & BDRV_O_PROTOCOL) == 0) { /* ... */
if (flags & BDRV_O_SNAPSHOT) {
snapshot_options = qdict_new();
bdrv_temp_snapshot_options(&snapshot_flags, snapshot_options,
flags, options);
+ qdict_del(bs->options, BDRV_OPT_READ_ONLY);
+ qdict_del(options, BDRV_OPT_READ_ONLY);
bdrv_backing_options(&flags, options, flags, options);
}
bs->open_flags = flags;+ qdict_copy_default(bs->options, options, BDRV_OPT_READ_ONLY);
/* ... */
}Moving this chunk of code saves us from having to update bs->open_flags and bs->options and makes things more readable.
Berto
Re: [Qemu-devel] [PATCH 2/7] block: Set BDRV_O_ALLOW_RDWR and snapshot_options before storing the flags
Am 15.09.2016 um 14:31 hat Alberto Garcia geschrieben:
Quoted 63 lines · Kevin Wolf
Quoted 20 lines
Quoted 20 lines
Quoted 19 lines
Quoted 5 lines
block.c +4
-0+if (flags &BDRV_O_RDWR ) { + flags |=BDRV_O_ALLOW_RDWR ; + } +block.c static BlockDriverState *bdrv_open_inherit(const char *filename, · line 1643
Quoted 10 lines
block.c +7
-0+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);Here I think we get different semantics now:
bdrv_backing_options()only affected the cloned QDict before, and now it affectsbs->options, too.Quoted 8 lines
Quoted 4 lines
The thing is that
bdrv_backing_options()doesn't change the options in general, and in the case where it does (BDRV_OPT_READ_ONLYafter the next patch) I think it makes sense that the options are changed. "snapshot=on" is a bit of a special case after all.Fair enough, it is correct (as I suspected), but it's not a bug fix because so far the options weren't changed, so there's no observable difference.
The idea of this change is to remove the '
bs->open_flags= flags ' line in the original code, as explained in the commit message.That's one thing. The other (and most important one) is to prepare for the introduction of
BDRV_OPT_READ_ONLY.Without this patch we'd need to need to update not just
bs->open_flagsbutbs->optionsas well, and it would be something like this:bs->open_flags = flags; bs->options = options;/* ... */if ((flags & BDRV_O_PROTOCOL) == 0) {/* ... */ if (flags & BDRV_O_SNAPSHOT) { snapshot_options = qdict_new(); bdrv_temp_snapshot_options(&snapshot_flags, snapshot_options, flags, options);(quoted diff) +3
-0+ qdict_del(bs->options, BDRV_OPT_READ_ONLY); + qdict_del(options, BDRV_OPT_READ_ONLY); bdrv_backing_options(&flags, options, flags, options); } bs->open_flags = flags; + qdict_copy_default(bs->options, options, BDRV_OPT_READ_ONLY); /* ... */ }
Quoted 2 lines
Moving this chunk of code saves us from having to update
bs->open_flagsandbs->optionsand makes things more readable.
I fully understand understand what the patch is for, and I don't want you to change any code. I was asking whether there was a hidden bug fix in here (which should have been moved into a separate patch then), but you explained to me why there isn't, so we're good. The only thing I'm looking for now is putting this explanation into the commit message.
So just include in the commit message that moving across the clone of bs->options doesn't make an observable difference (at the point of this patch) because the bdrv_backing_options() call doesn't actually change options yet, and that applying any changes made by it in the future to bs->options makes more sense anyway.
Kevin
Re: [Qemu-devel] [PATCH 2/7] block: Set BDRV_O_ALLOW_RDWR and snapshot_options before storing the flags
Quoted 4 lines · Kevin Wolf
Quoted 2 lines
Moving this chunk of code saves us from having to update
bs->open_flagsandbs->optionsand makes things more readable.I fully understand understand what the patch is for
I see, I thought it wasn't clear, I'll update the message then :)
Berto
Re: [Qemu-devel] [Qemu-block] [PATCH 2/7] block: Set BDRV_O_ALLOW_RDWR and snapshot_options before storing the flags
In reply to Alberto GarciaQuoted 5 lines · Alberto Garcia
If an image is opened with snapshot=on, its flags are modified by
bdrv_backing_options()and thenbs->open_flagsis updated accordingly. This last step is unnecessary if we calculate the new flags before settingbs->open_flags.
Quoted 7 lines
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->optionsis already set. This patch simplifies that.The code that sets
BDRV_O_ALLOW_RDWRis also moved for the same reason.
Before, we would not set BDRV_O_ALLOW_RDWR for protocols, but this will change that. Is that side-affect intentional?
Quoted 46 lines
Signed-off-by Alberto Garcia <[email protected]>--- 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) {
Re: [Qemu-devel] [Qemu-block] [PATCH 2/7] block: Set BDRV_O_ALLOW_RDWR and snapshot_options before storing the flags
Quoted 9 lines · Jeff Cody
Quoted 9 lines
If an image is opened with snapshot=on, its flags are modified by
bdrv_backing_options()and thenbs->open_flagsis updated accordingly. This last step is unnecessary if we calculate the new flags before settingbs->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->optionsis already set. This patch simplifies that.
Quoted 5 lines
Quoted 2 lines
The code that sets
BDRV_O_ALLOW_RDWRis also moved for the same reason.Before, we would not set
BDRV_O_ALLOW_RDWRfor protocols, but this will change that. Is that side-affect intentional?
BDRV_O_ALLOW_RDWR is set in the root BDS, and then all children inherit that flag (both bdrv_inherited_options() and bdrv_backing_options() copy it), so I don't think the ((flags & BDRV_O_PROTOCOL) == 0) check makes any difference in practice.
Berto
PATCH 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
Re: [Qemu-devel] [PATCH 3/7] block: Update bs->open_flags earlier in bdrv_open_common()
Am 14.09.2016 um 17:52 hat Alberto Garcia geschrieben:
Quoted 10 lines
We're only doing this immediately before opening the image, but
bs->open_flagsis used earlier in the function. At the moment this is not causing problems because none of the checked flags are modified byupdate_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.Signed-off-by Alberto Garcia <[email protected]>
PATCH 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 | 35 ++++++++++++++++++++++++++++++++--- block/vvfat.c | 3 ++- blockdev.c | 16 +++++++++++----- include/block/block.h | 1 + 4 files changed, 46 insertions(+), 9 deletions(-)
block.c +32 -3
@@ -707,6 +707,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 +763,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 +811,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 +831,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 +898,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 +1647,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 +11 -5
@@ -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 )) {
@@ -657,6 +658,7 @@ static BlockDriverState *bds_tree_init(QDict *bs_opts, Error **errp)QemuOpts *opts;Error *local_error =NULL ;BlockdevDetectZeroesOptions detect_zeroes; +bool read_only;int bdrv_flags =0 ; opts =qemu_opts_create (&qemu_root_bds_opts,NULL ,1 , errp);
@@ -677,11 +679,15 @@ static BlockDriverState *bds_tree_init(QDict *bs_opts, Error **errp) goto fail; } + read_only =qemu_opt_get_bool (opts,BDRV_OPT_READ_ONLY , false); +/* bdrv_open() defaults to the values in bdrv_flags (for compatibility * with other callers) rather than what we want as the real defaults. * 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" );if (runstate_check (RUN_STATE_INMIGRATE )) { bdrv_flags |=BDRV_O_INACTIVE ;
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
Re: [Qemu-devel] [PATCH 4/7] block: Add "read-only" to the options QDict
Am 14.09.2016 um 17:52 hat Alberto Garcia geschrieben:
Quoted 29 lines
This 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_RDWRflag is not removed yet, but its keep in sync with the value of the "read-only" option.Signed-off-by Alberto Garcia <[email protected]>--- block.c | 35 ++++++++++++++++++++++++++++++++--- block/vvfat.c | 3 ++- blockdev.c | 16 +++++++++++----- include/block/block.h | 1 + 4 files changed, 46 insertions(+), 9 deletions(-)block.c +0
-0
block.c static void bdrv_inherited_options(int *child_flags, QDict *child_options, · line 715
Quoted 9 lines
block.c +3
-0@@ -707,6 +707,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. */
We need another qdict_copy_default() in bdrv_temp_snapshot_options(), I think, so that flags and options stay consistent there.
Currently it seems to be working because we still have the flag and the flag is what is actually used in bdrv_open_common(). This means that we parse the "read-only" option into the QemuOpts there, but we never read it from there. Shouldn't we use the option rather than the flags?
block.c static BlockDriverState *bds_tree_init(QDict *bs_opts, Error **errp) · line 690
Quoted 13 lines
block.c +4
-0@@ -677,11 +679,15 @@ static BlockDriverState *bds_tree_init(QDict *bs_opts, Error **errp) goto fail; } + read_only =qemu_opt_get_bool (opts,BDRV_OPT_READ_ONLY , false); +/* bdrv_open() defaults to the values in bdrv_flags (for compatibility * with other callers) rather than what we want as the real defaults. * 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" );
Why do you parse read_only into the QemuOpts just to add it right back to bs_opts? Wouldn't it be easier to remove it from qemu_root_bds_opts and do a simple set_default_str("on") here? (Which is how the cache options work.)
Kevin
Re: [Qemu-devel] [PATCH 4/7] block: Add "read-only" to the options QDict
Quoted 4 lines · Kevin Wolf
Quoted 4 lines
block.c +0
-0@@ -707,6 +707,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 );
Quoted 9 lines
block.c static void bdrv_inherited_options(int *child_flags, QDict *child_options, · line 715
Quoted 6 lines
block.c +3
-0+/* 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. */ We need another
qdict_copy_default()inbdrv_temp_snapshot_options(), I think, so that flags and options stay consistent there.
You're right, I assumed that with read-only=on,snapshot=on the temporary file would still be r/w.
I'll fix it.
Quoted 10 lines
blockdev.c static BlockDriverState *bds_tree_init(QDict *bs_opts, Error **errp) · line 690
Quoted 9 lines
blockdev.c +4
-0+ read_only =qemu_opt_get_bool (opts,BDRV_OPT_READ_ONLY , false); +/* bdrv_open() defaults to the values in bdrv_flags (for compatibility * with other callers) rather than what we want as the real defaults. * 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" );
Quoted 4 lines
Why do you parse
read_onlyinto the QemuOpts just to add it right back tobs_opts? Wouldn't it be easier to remove it fromqemu_root_bds_optsand do a simpleset_default_str("on")here? (Which is how the cache options work.)
Yeah, I was mirroring blockdev_init() (there it's easier to keep it in qemu_common_drive_opts), but in this case there's no need to do it.
I'll fix it too.
Thanks!
Berto
PATCH 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
@@ -1874,6 +1874,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)
@@ -1894,7 +1901,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);
@@ -1933,8 +1944,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
Re: [Qemu-devel] [PATCH 5/7] block: Don't queue the same BDS twice in bdrv_reopen_queue_child()
Am 14.09.2016 um 17:52 hat Alberto Garcia geschrieben:
Quoted 9 lines
bdrv_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.
Quoted 5 lines
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.Signed-off-by Alberto Garcia <[email protected]>
PATCH 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
Re: [Qemu-devel] [PATCH 6/7] commit: Add 'base' to the reopen queue before 'overlay_bs'
Am 14.09.2016 um 17:52 hat Alberto Garcia geschrieben:
Quoted 11 lines
Now 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.
Signed-off-by Alberto Garcia <[email protected]>
PATCH 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 | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
blockdev.c +6 -6
@@ -811,7 +811,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", },{
@@ -877,7 +877,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++) {
@@ -949,7 +949,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) {
@@ -957,7 +957,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" ));
@@ -4046,7 +4046,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", },{
@@ -4165,7 +4165,7 @@ static QemuOptsList qemu_root_bds_opts = { .type = QEMU_OPT_STRING, .help = "host AIO implementation (threads, native)", },{ - .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
Re: [Qemu-devel] [PATCH 7/7] block: rename "read-only" to BDRV_OPT_READ_ONLY
Am 14.09.2016 um 17:52 hat Alberto Garcia geschrieben:
Quoted 4 lines
There were a few instances left. After this patch we're using the macro in all places.
Signed-off-by Alberto Garcia <[email protected]>
blockdev.c static QemuOptsList qemu_root_bds_opts = { · line 4171
Quoted 9 lines
blockdev.c +1
-1@@ -4165,7 +4165,7 @@ static QemuOptsList qemu_root_bds_opts = { .type = QEMU_OPT_STRING, .help = "host AIO implementation (threads, native)", },{ - .name = "read-only", + .name = BDRV_OPT_READ_ONLY, .type = QEMU_OPT_BOOL, .help = "open drive file as read-only", },{
This hunk will go away with the change you're going to make to the earlier patch, but you can keep my R-b.
Kevin