| Message ID | 054195c8f83f524ee1f4d117407ac507dd18b0d8.1535291602.git.berto@igalia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Misc reopen-related patches | expand |
On 2018-08-26 16:09, Alberto Garcia wrote: > Block drivers allow opening their children using a reference to an > existing BlockDriverState. These references remain stored in the > 'options' and 'explicit_options' QDicts, but we don't need to keep > them once everything is open. > > What is more important, these values can become wrong if the children > change: > > $ qemu-img create -f qcow2 hd0.qcow2 10M > $ qemu-img create -f qcow2 hd1.qcow2 10M > $ qemu-img create -f qcow2 hd2.qcow2 10M > $ $QEMU -drive if=none,file=hd0.qcow2,node-name=hd0 \ > -drive if=none,file=hd1.qcow2,node-name=hd1,backing=hd0 \ > -drive file=hd2.qcow2,node-name=hd2,backing=hd1 > > After this hd2 has hd1 as its backing file. Now let's remove it using > block_stream: > > (qemu) block_stream hd2 0 hd0.qcow2 > > Now hd0 is the backing file of hd2, but hd2's options QDicts still > contain backing=hd1. > > Signed-off-by: Alberto Garcia <berto@igalia.com> > --- > block.c | 12 +++++++++++- > 1 file changed, 11 insertions(+), 1 deletion(-) Also, the question is, why would we discard the child options, but keep the references (which we do not add, so we only have the ones specified by the user). > diff --git a/block.c b/block.c > index 0dbb1fcc7b..2f2484965d 100644 > --- a/block.c > +++ b/block.c > @@ -2763,12 +2763,15 @@ static BlockDriverState *bdrv_open_inherit(const char *filename, > } > } > > - /* Remove all children options from bs->options and bs->explicit_options */ > + /* Remove all children options and references > + * from bs->options and bs->explicit_options */ > QLIST_FOREACH(child, &bs->children, next) { > char *child_key_dot; > child_key_dot = g_strdup_printf("%s.", child->name); > qdict_extract_subqdict(bs->explicit_options, NULL, child_key_dot); > qdict_extract_subqdict(bs->options, NULL, child_key_dot); > + qdict_del(bs->explicit_options, child->name); > + qdict_del(bs->options, child->name); > g_free(child_key_dot); > } > > @@ -3289,6 +3292,7 @@ void bdrv_reopen_commit(BDRVReopenState *reopen_state) > { > BlockDriver *drv; > BlockDriverState *bs; > + BdrvChild *child; > bool old_can_write, new_can_write; > > assert(reopen_state != NULL); > @@ -3313,6 +3317,12 @@ void bdrv_reopen_commit(BDRVReopenState *reopen_state) > bs->open_flags = reopen_state->flags; > bs->read_only = !(reopen_state->flags & BDRV_O_RDWR); > > + /* Remove child references from bs->options and bs->explicit_options */ > + QLIST_FOREACH(child, &bs->children, next) { > + qdict_del(bs->explicit_options, child->name); > + qdict_del(bs->options, child->name); > + } > + Why don't you remove the child options as well? Max > bdrv_refresh_limits(bs, NULL); > > bdrv_set_perm(reopen_state->bs, reopen_state->perm, >
On Wed 29 Aug 2018 12:43:06 PM CEST, Max Reitz wrote: > On 2018-08-26 16:09, Alberto Garcia wrote: >> Block drivers allow opening their children using a reference to an >> existing BlockDriverState. These references remain stored in the >> 'options' and 'explicit_options' QDicts, but we don't need to keep >> them once everything is open. >> >> What is more important, these values can become wrong if the children >> change: >> >> $ qemu-img create -f qcow2 hd0.qcow2 10M >> $ qemu-img create -f qcow2 hd1.qcow2 10M >> $ qemu-img create -f qcow2 hd2.qcow2 10M >> $ $QEMU -drive if=none,file=hd0.qcow2,node-name=hd0 \ >> -drive if=none,file=hd1.qcow2,node-name=hd1,backing=hd0 \ >> -drive file=hd2.qcow2,node-name=hd2,backing=hd1 >> >> After this hd2 has hd1 as its backing file. Now let's remove it using >> block_stream: >> >> (qemu) block_stream hd2 0 hd0.qcow2 >> >> Now hd0 is the backing file of hd2, but hd2's options QDicts still >> contain backing=hd1. >> >> Signed-off-by: Alberto Garcia <berto@igalia.com> >> --- >> block.c | 12 +++++++++++- >> 1 file changed, 11 insertions(+), 1 deletion(-) > > Also, the question is, why would we discard the child options, but > keep the references (which we do not add, so we only have the ones > specified by the user). We discussed this a couple of weeks ago: https://lists.gnu.org/archive/html/qemu-block/2018-08/msg00478.html In the end I got convinced that we didn't need to keep the child references. >> @@ -3313,6 +3317,12 @@ void bdrv_reopen_commit(BDRVReopenState *reopen_state) >> bs->open_flags = reopen_state->flags; >> bs->read_only = !(reopen_state->flags & BDRV_O_RDWR); >> >> + /* Remove child references from bs->options and bs->explicit_options */ >> + QLIST_FOREACH(child, &bs->children, next) { >> + qdict_del(bs->explicit_options, child->name); >> + qdict_del(bs->options, child->name); >> + } >> + > > Why don't you remove the child options as well? Because there's no child options here at this point. They are removed from the parent QDict in bdrv_reopen_queue_child(): QLIST_FOREACH(child, &bs->children, next) { /* ... */ child_key_dot = g_strdup_printf("%s.", child->name); qdict_extract_subqdict(explicit_options, NULL, child_key_dot); qdict_extract_subqdict(options, &new_child_options, child_key_dot); g_free(child_key_dot); bdrv_reopen_queue_child(bs_queue, child->bs, new_child_options, 0, child->role, options, flags); } Berto
On 2018-08-29 13:53, Alberto Garcia wrote: > On Wed 29 Aug 2018 12:43:06 PM CEST, Max Reitz wrote: >> On 2018-08-26 16:09, Alberto Garcia wrote: >>> Block drivers allow opening their children using a reference to an >>> existing BlockDriverState. These references remain stored in the >>> 'options' and 'explicit_options' QDicts, but we don't need to keep >>> them once everything is open. >>> >>> What is more important, these values can become wrong if the children >>> change: >>> >>> $ qemu-img create -f qcow2 hd0.qcow2 10M >>> $ qemu-img create -f qcow2 hd1.qcow2 10M >>> $ qemu-img create -f qcow2 hd2.qcow2 10M >>> $ $QEMU -drive if=none,file=hd0.qcow2,node-name=hd0 \ >>> -drive if=none,file=hd1.qcow2,node-name=hd1,backing=hd0 \ >>> -drive file=hd2.qcow2,node-name=hd2,backing=hd1 >>> >>> After this hd2 has hd1 as its backing file. Now let's remove it using >>> block_stream: >>> >>> (qemu) block_stream hd2 0 hd0.qcow2 >>> >>> Now hd0 is the backing file of hd2, but hd2's options QDicts still >>> contain backing=hd1. >>> >>> Signed-off-by: Alberto Garcia <berto@igalia.com> >>> --- >>> block.c | 12 +++++++++++- >>> 1 file changed, 11 insertions(+), 1 deletion(-) >> >> Also, the question is, why would we discard the child options, but >> keep the references (which we do not add, so we only have the ones >> specified by the user). > > We discussed this a couple of weeks ago: > > https://lists.gnu.org/archive/html/qemu-block/2018-08/msg00478.html > > In the end I got convinced that we didn't need to keep the child > references. OK, so your argument was "because references are indeed options for the parent". But as I said, that's only valid if all references are there, which they aren't necessarily. (If the user specifies the @file options inline, then you won't get a reference for @file in bs->options.) But since you discussed all of that already anyway, it doesn't really matters. What matters is that I don't disagree. :-) >>> @@ -3313,6 +3317,12 @@ void bdrv_reopen_commit(BDRVReopenState *reopen_state) >>> bs->open_flags = reopen_state->flags; >>> bs->read_only = !(reopen_state->flags & BDRV_O_RDWR); >>> >>> + /* Remove child references from bs->options and bs->explicit_options */ >>> + QLIST_FOREACH(child, &bs->children, next) { >>> + qdict_del(bs->explicit_options, child->name); >>> + qdict_del(bs->options, child->name); >>> + } >>> + >> >> Why don't you remove the child options as well? > > Because there's no child options here at this point. They are removed > from the parent QDict in bdrv_reopen_queue_child(): > > QLIST_FOREACH(child, &bs->children, next) { > /* ... */ > child_key_dot = g_strdup_printf("%s.", child->name); > qdict_extract_subqdict(explicit_options, NULL, child_key_dot); > qdict_extract_subqdict(options, &new_child_options, child_key_dot); > g_free(child_key_dot); > > bdrv_reopen_queue_child(bs_queue, child->bs, new_child_options, 0, > child->role, options, flags); > } Hm. It's a bit weird to split child options and child references into two parts, but I suppose it makes sense. We need to handle inline child options before the reopen, because those are reopen options for the current child (and not intended to replace anything). We do not want to remove child references there, because specifying a reference means attaching a different child.[1] In commit, we want to remove references, because we don't want them in bs->*options. We'd also want to remove inline child options, but as you say, there is no need to, because they have been removed already. What do you think of adding a note to make it more clear? (e.g. "(The inline child options have been handled in bdrv_reopen_queue_child() already)") Max [1] On second thought, we do probably want to check the references there, at least. If you attach a new child, there is no need to reopen the current child, so we should skip the bdrv_reopen_queue_child() -- or rather, we should call it on the new child the user wants to attach. But that's not something for this series.
On Wed 29 Aug 2018 02:18:45 PM CEST, Max Reitz wrote: >>>> @@ -3313,6 +3317,12 @@ void bdrv_reopen_commit(BDRVReopenState *reopen_state) >>>> bs->open_flags = reopen_state->flags; >>>> bs->read_only = !(reopen_state->flags & BDRV_O_RDWR); >>>> >>>> + /* Remove child references from bs->options and bs->explicit_options */ >>>> + QLIST_FOREACH(child, &bs->children, next) { >>>> + qdict_del(bs->explicit_options, child->name); >>>> + qdict_del(bs->options, child->name); >>>> + } >>>> + >>> >>> Why don't you remove the child options as well? >> >> Because there's no child options here at this point. They are removed >> from the parent QDict in bdrv_reopen_queue_child(): >> >> QLIST_FOREACH(child, &bs->children, next) { >> /* ... */ >> child_key_dot = g_strdup_printf("%s.", child->name); >> qdict_extract_subqdict(explicit_options, NULL, child_key_dot); >> qdict_extract_subqdict(options, &new_child_options, child_key_dot); >> g_free(child_key_dot); >> >> bdrv_reopen_queue_child(bs_queue, child->bs, new_child_options, 0, >> child->role, options, flags); >> } > > Hm. It's a bit weird to split child options and child references into > two parts, but I suppose it makes sense. The child references have to remain in the parent until at least bdrv_reopen_prepare() because we need them if we want to allow replacing a child. The child options are removed in bdrv_reopen_queue_child() because we need to pass them to the children. We could also make a copy and remove them later in bdrv_reopen_commit() if that makes things more clear, but I don't know... > What do you think of adding a note to make it more clear? > (e.g. "(The inline child options have been handled in > bdrv_reopen_queue_child() already)") Sure, why not. > [1] On second thought, we do probably want to check the references > there, at least. If you attach a new child, there is no need to reopen > the current child, so we should skip the bdrv_reopen_queue_child() Yes, that's done in a subsequent patch (not part of this series yet). Berto
diff --git a/block.c b/block.c index 0dbb1fcc7b..2f2484965d 100644 --- a/block.c +++ b/block.c @@ -2763,12 +2763,15 @@ static BlockDriverState *bdrv_open_inherit(const char *filename, } } - /* Remove all children options from bs->options and bs->explicit_options */ + /* Remove all children options and references + * from bs->options and bs->explicit_options */ QLIST_FOREACH(child, &bs->children, next) { char *child_key_dot; child_key_dot = g_strdup_printf("%s.", child->name); qdict_extract_subqdict(bs->explicit_options, NULL, child_key_dot); qdict_extract_subqdict(bs->options, NULL, child_key_dot); + qdict_del(bs->explicit_options, child->name); + qdict_del(bs->options, child->name); g_free(child_key_dot); } @@ -3289,6 +3292,7 @@ void bdrv_reopen_commit(BDRVReopenState *reopen_state) { BlockDriver *drv; BlockDriverState *bs; + BdrvChild *child; bool old_can_write, new_can_write; assert(reopen_state != NULL); @@ -3313,6 +3317,12 @@ void bdrv_reopen_commit(BDRVReopenState *reopen_state) bs->open_flags = reopen_state->flags; bs->read_only = !(reopen_state->flags & BDRV_O_RDWR); + /* Remove child references from bs->options and bs->explicit_options */ + QLIST_FOREACH(child, &bs->children, next) { + qdict_del(bs->explicit_options, child->name); + qdict_del(bs->options, child->name); + } + bdrv_refresh_limits(bs, NULL); bdrv_set_perm(reopen_state->bs, reopen_state->perm,
Block drivers allow opening their children using a reference to an existing BlockDriverState. These references remain stored in the 'options' and 'explicit_options' QDicts, but we don't need to keep them once everything is open. What is more important, these values can become wrong if the children change: $ qemu-img create -f qcow2 hd0.qcow2 10M $ qemu-img create -f qcow2 hd1.qcow2 10M $ qemu-img create -f qcow2 hd2.qcow2 10M $ $QEMU -drive if=none,file=hd0.qcow2,node-name=hd0 \ -drive if=none,file=hd1.qcow2,node-name=hd1,backing=hd0 \ -drive file=hd2.qcow2,node-name=hd2,backing=hd1 After this hd2 has hd1 as its backing file. Now let's remove it using block_stream: (qemu) block_stream hd2 0 hd0.qcow2 Now hd0 is the backing file of hd2, but hd2's options QDicts still contain backing=hd1. Signed-off-by: Alberto Garcia <berto@igalia.com> --- block.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)