| Message ID | 31ccb1061199ee11bf9879f6c60608a19b83263d.1616000692.git.berto@igalia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Allow changing bs->file on reopen | expand |
17.03.2021 20:15, Alberto Garcia wrote: > When the x-blockdev-reopen was added it allowed reconfiguring the > graph by replacing backing files, but changing the 'file' option was > forbidden. Because of this restriction some operations are not > possible, notably inserting and removing block filters. > > This patch adds support for replacing the 'file' option. This is > similar to replacing the backing file and the user is likewise > responsible for the correctness of the resulting graph, otherwise this > can lead to data corruption. > > Signed-off-by: Alberto Garcia <berto@igalia.com> In general patch looks OK for me, some comments below. > --- > include/block/block.h | 1 + > block.c | 119 ++++++++++++++++++++++++++--------------- > tests/qemu-iotests/245 | 9 ++-- > 3 files changed, 81 insertions(+), 48 deletions(-) > > diff --git a/include/block/block.h b/include/block/block.h > index 5eb1e4cab9..e2732a0187 100644 > --- a/include/block/block.h > +++ b/include/block/block.h > @@ -209,6 +209,7 @@ typedef struct BDRVReopenState { > bool backing_missing; > bool replace_backing_bs; /* new_backing_bs is ignored if this is false */ > BlockDriverState *old_backing_bs; /* keep pointer for permissions update */ > + BlockDriverState *old_file_bs; /* keep pointer for permissions update */ > QDict *options; > QDict *explicit_options; > void *opaque; > diff --git a/block.c b/block.c > index 764cdbec7d..8ff0afd77b 100644 > --- a/block.c > +++ b/block.c > @@ -98,7 +98,7 @@ static void bdrv_remove_filter_or_cow_child(BlockDriverState *bs, > > static int bdrv_reopen_prepare(BDRVReopenState *reopen_state, > BlockReopenQueue *queue, > - Transaction *set_backings_tran, Error **errp); > + Transaction *tran, Error **errp); I'd not call it just "tran" to not interfere with transaction actions. Of course, reopen should be finally refactored to work cleanly on Transaction API, but that is not done yet. And here we pass a transaction pointer only to keep children modification.. So, let's make it change_child_tran, or something like this. > static void bdrv_reopen_commit(BDRVReopenState *reopen_state); > static void bdrv_reopen_abort(BDRVReopenState *reopen_state); > > @@ -4049,6 +4049,10 @@ int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, Error **errp) > refresh_list = bdrv_topological_dfs(refresh_list, found, > state->old_backing_bs); > } > + if (state->old_file_bs) { > + refresh_list = bdrv_topological_dfs(refresh_list, found, > + state->old_file_bs); > + } > } > > /* > @@ -4161,65 +4165,77 @@ static bool bdrv_reopen_can_attach(BlockDriverState *parent, > * > * Return 0 on success, otherwise return < 0 and set @errp. > */ > -static int bdrv_reopen_parse_backing(BDRVReopenState *reopen_state, > - Transaction *set_backings_tran, > - Error **errp) > +static int bdrv_reopen_parse_file_or_backing(BDRVReopenState *reopen_state, > + bool parse_file, Transaction *tran, > + Error **errp) > { > BlockDriverState *bs = reopen_state->bs; > - BlockDriverState *overlay_bs, *below_bs, *new_backing_bs; > + BlockDriverState *overlay_bs, *below_bs, *new_child_bs; > + BdrvChild *child = parse_file ? bs->file : bs->backing; > QObject *value; > const char *str; > > - value = qdict_get(reopen_state->options, "backing"); > + value = qdict_get(reopen_state->options, parse_file ? "file" : "backing"); > if (value == NULL) { > return 0; > } > > switch (qobject_type(value)) { > case QTYPE_QNULL: > - new_backing_bs = NULL; > + assert(!parse_file); /* The 'file' option does not allow a null value */ > + new_child_bs = NULL; > break; > case QTYPE_QSTRING: > str = qstring_get_str(qobject_to(QString, value)); > - new_backing_bs = bdrv_lookup_bs(NULL, str, errp); > - if (new_backing_bs == NULL) { > + new_child_bs = bdrv_lookup_bs(NULL, str, errp); > + if (new_child_bs == NULL) { > return -EINVAL; > - } else if (bdrv_recurse_has_child(new_backing_bs, bs)) { > - error_setg(errp, "Making '%s' a backing file of '%s' " > - "would create a cycle", str, bs->node_name); > + } else if (bdrv_recurse_has_child(new_child_bs, bs)) { > + error_setg(errp, "Making '%s' a %s of '%s' would create a cycle", > + str, parse_file ? "file" : "backing file", maybe s/"file"/"file child"/ > + bs->node_name); > return -EINVAL; > } > break; > default: > - /* 'backing' does not allow any other data type */ > + /* The options QDict has been flattened, so 'backing' and 'file' > + * do not allow any other data type here. */ checkpatch should complain that you didn't fix style of the comment... > g_assert_not_reached(); > } > > - /* > - * Check AioContext compatibility so that the bdrv_set_backing_hd() call in > - * bdrv_reopen_commit() won't fail. > - */ > - if (new_backing_bs) { > - if (!bdrv_reopen_can_attach(bs, bs->backing, new_backing_bs, errp)) { > + /* If 'file' points to the current child then there's nothing to do */ > + if (child_bs(child) == new_child_bs) { > + return 0; > + } > + > + /* Check AioContext compatibility */ > + if (new_child_bs) { > + if (!bdrv_reopen_can_attach(bs, child, new_child_bs, errp)) { > return -EINVAL; > } > } > > - /* > - * Ensure that @bs can really handle backing files, because we are > - * about to give it one (or swap the existing one) > - */ > - if (bs->drv->is_filter) { > - /* Filters always have a file or a backing child */ > - if (!bs->backing) { > - error_setg(errp, "'%s' is a %s filter node that does not support a " > - "backing child", bs->node_name, bs->drv->format_name); > + if (parse_file) { > + assert(child && child->bs); I'm not sure, that we can't get children without a bs at some point.. And we have so many checks about it in the code. Probably we can drop them all? But I don't want to care to much. If this assertion fires, we'll fix a bug. > + } else { > + /* > + * Ensure that @bs can really handle backing files, because we are > + * about to give it one (or swap the existing one) > + */ > + if (bs->drv->is_filter) { > + /* Filters always have a file or a backing child */ Probably we can assert bs->backing, as otherwise backing option should be unsupported [preexisting, not about this patch] > + if (!bs->backing) { > + error_setg(errp, "'%s' is a %s filter node " > + "that does not support a backing child", > + bs->node_name, bs->drv->format_name); > + return -EINVAL; > + } > + } else if (!bs->drv->supports_backing) { Probably we can assert bs->drv->supports_backing, as otherwise backing option should be unsupported [preexisting, not about this patch] > + error_setg(errp, "Driver '%s' of node '%s' " > + "does not support backing files", > + bs->drv->format_name, bs->node_name); > return -EINVAL; > } > - } else if (!bs->drv->supports_backing) { > - error_setg(errp, "Driver '%s' of node '%s' does not support backing " > - "files", bs->drv->format_name, bs->node_name); > - return -EINVAL; > } > > /* > @@ -4238,13 +4254,13 @@ static int bdrv_reopen_parse_backing(BDRVReopenState *reopen_state, > } > > /* If we want to replace the backing file we need some extra checks */ You didn't update the comment. > - if (new_backing_bs != bdrv_filter_or_cow_bs(overlay_bs)) { > + if (new_child_bs != bdrv_filter_or_cow_bs(overlay_bs)) { > int ret; > > /* Check for implicit nodes between bs and its backing file */ > if (bs != overlay_bs) { > - error_setg(errp, "Cannot change backing link if '%s' has " > - "an implicit backing file", bs->node_name); > + error_setg(errp, "Cannot change %s link if '%s' has an implicit " > + "child", parse_file ? "file" : "backing", bs->node_name); > return -EPERM; > } > /* > @@ -4256,16 +4272,24 @@ static int bdrv_reopen_parse_backing(BDRVReopenState *reopen_state, > * with bs->drv->supports_backing == true. > */ > if (bdrv_is_backing_chain_frozen(overlay_bs, > - child_bs(overlay_bs->backing), errp)) > + bdrv_filter_or_cow_bs(overlay_bs), > + errp)) > { > return -EPERM; > } > - reopen_state->replace_backing_bs = true; > - reopen_state->old_backing_bs = bs->backing ? bs->backing->bs : NULL; > - ret = bdrv_set_backing_noperm(bs, new_backing_bs, set_backings_tran, > - errp); > - if (ret < 0) { > - return ret; > + if (parse_file) { > + /* Store the old file bs, we'll need to refresh its permissions */ > + reopen_state->old_file_bs = bs->file->bs; > + > + /* And finally replace the child */ > + bdrv_replace_child(bs->file, new_child_bs, tran); I think that actually, we need also to update inherits_from and do refresh_limits like in bdrv_set_backing_noperm(). Probably, bdrv_replace_child should do it. Probably not (there are still a lot of things to refactor in block.c :).. Hm. Also, using blockdev-reopen probably means that we are in a blockdev word, so we should not care about inherits_from here. But at least calling bdrv_refresh_limits(bs, tran, NULL) will not hurt. (or we can check an error code and honestly return it as well). Also, you don't create reopen_state->replace_file_bs, like for backing.. On bdrv_reopen_comnmit replace_backing_bs is used to remove corresponding options.. Shouldn't we do the same with file options? > + } else { > + reopen_state->replace_backing_bs = true; > + reopen_state->old_backing_bs = child_bs(bs->backing); > + ret = bdrv_set_backing_noperm(bs, new_child_bs, tran, errp); > + if (ret < 0) { > + return ret; > + } > } > } > > @@ -4291,7 +4315,7 @@ static int bdrv_reopen_parse_backing(BDRVReopenState *reopen_state, > */ > static int bdrv_reopen_prepare(BDRVReopenState *reopen_state, > BlockReopenQueue *queue, > - Transaction *set_backings_tran, Error **errp) > + Transaction *tran, Error **errp) > { > int ret = -1; > int old_flags; > @@ -4411,12 +4435,19 @@ static int bdrv_reopen_prepare(BDRVReopenState *reopen_state, > * either a reference to an existing node (using its node name) > * or NULL to simply detach the current backing file. > */ > - ret = bdrv_reopen_parse_backing(reopen_state, set_backings_tran, errp); > + ret = bdrv_reopen_parse_file_or_backing(reopen_state, false, tran, errp); > if (ret < 0) { > goto error; > } > qdict_del(reopen_state->options, "backing"); > > + /* Allow changing the 'file' option. In this case NULL is not allowed */ > + ret = bdrv_reopen_parse_file_or_backing(reopen_state, true, tran, errp); > + if (ret < 0) { > + goto error; > + } > + qdict_del(reopen_state->options, "file"); > + > /* Options that are not handled are only okay if they are unchanged > * compared to the old state. It is expected that some options are only > * used for the initial open, but not reopen (e.g. filename) */ > diff --git a/tests/qemu-iotests/245 b/tests/qemu-iotests/245 > index fc5297e268..a4d0b10e9d 100755 > --- a/tests/qemu-iotests/245 > +++ b/tests/qemu-iotests/245 > @@ -146,8 +146,8 @@ class TestBlockdevReopen(iotests.QMPTestCase): > self.reopen(opts, {'driver': 'raw'}, "Cannot change the option 'driver'") > self.reopen(opts, {'driver': ''}, "Invalid parameter ''") > self.reopen(opts, {'driver': None}, "Invalid parameter type for 'driver', expected: string") > - self.reopen(opts, {'file': 'not-found'}, "Cannot change the option 'file'") > - self.reopen(opts, {'file': ''}, "Cannot change the option 'file'") > + self.reopen(opts, {'file': 'not-found'}, "Cannot find device='' nor node-name='not-found'") Interesting that error-message say about device='', not 'not-found'... > + self.reopen(opts, {'file': ''}, "Cannot find device='' nor node-name=''") > self.reopen(opts, {'file': None}, "Invalid parameter type for 'file', expected: BlockdevRef") > self.reopen(opts, {'file.node-name': 'newname'}, "Cannot change the option 'node-name'") > self.reopen(opts, {'file.driver': 'host_device'}, "Cannot change the option 'driver'") > @@ -455,7 +455,8 @@ class TestBlockdevReopen(iotests.QMPTestCase): > # More illegal operations > self.reopen(opts[2], {'backing': 'hd1'}, > "Making 'hd1' a backing file of 'hd2' would create a cycle") > - self.reopen(opts[2], {'file': 'hd0-file'}, "Cannot change the option 'file'") > + self.reopen(opts[2], {'file': 'hd0-file'}, > + "Conflicts with use by hd2 as 'file', which does not allow 'write, resize' on hd0-file") > > result = self.vm.qmp('blockdev-del', conv_keys = True, node_name = 'hd2') > self.assert_qmp(result, 'error/class', 'GenericError') > @@ -969,7 +970,7 @@ class TestBlockdevReopen(iotests.QMPTestCase): > > # We can't remove hd1 while the commit job is ongoing > opts['backing'] = None > - self.reopen(opts, {}, "Cannot change backing link if 'hd0' has an implicit backing file") > + self.reopen(opts, {}, "Cannot change backing link if 'hd0' has an implicit child") > > # hd2 <- hd0 > self.vm.run_job('commit0', auto_finalize = False, auto_dismiss = True) >
On Thu 18 Mar 2021 03:25:07 PM CET, Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote: >> static int bdrv_reopen_prepare(BDRVReopenState *reopen_state, >> BlockReopenQueue *queue, >> - Transaction *set_backings_tran, Error **errp); >> + Transaction *tran, Error **errp); > > I'd not call it just "tran" to not interfere with transaction > actions. Of course, reopen should be finally refactored to work > cleanly on Transaction API, but that is not done yet. And here we pass > a transaction pointer only to keep children modification.. So, let's > make it change_child_tran, or something like this. The name change looks good to me. >> + } else if (bdrv_recurse_has_child(new_child_bs, bs)) { >> + error_setg(errp, "Making '%s' a %s of '%s' would create a cycle", >> + str, parse_file ? "file" : "backing file", > > maybe s/"file"/"file child"/ Ok. >> default: >> - /* 'backing' does not allow any other data type */ >> + /* The options QDict has been flattened, so 'backing' and 'file' >> + * do not allow any other data type here. */ > > checkpatch should complain that you didn't fix style of the comment... I actually don't like to use the proposed style for 2-line comments in many cases. I think it makes sense for big comment blocks but adds noise for shorter comments. >> + } else { >> + /* >> + * Ensure that @bs can really handle backing files, because we are >> + * about to give it one (or swap the existing one) >> + */ >> + if (bs->drv->is_filter) { >> + /* Filters always have a file or a backing child */ > > Probably we can assert bs->backing, as otherwise backing option should > be unsupported [preexisting, not about this patch] Yes, I see that this was added in commit 1d42f48c3a, maybe Max has good reasons to keep it this way? >> if (bdrv_is_backing_chain_frozen(overlay_bs, >> - child_bs(overlay_bs->backing), errp)) >> + bdrv_filter_or_cow_bs(overlay_bs), >> + errp)) >> { >> return -EPERM; >> } I just realized that this part is probably not ok if you want to change bs->file on a node that is not a filter, because this would check bs->backing->frozen and not bs->file->frozen. >> + if (parse_file) { >> + /* Store the old file bs, we'll need to refresh its permissions */ >> + reopen_state->old_file_bs = bs->file->bs; >> + >> + /* And finally replace the child */ >> + bdrv_replace_child(bs->file, new_child_bs, tran); > > I think that actually, we need also to update inherits_from and do > refresh_limits like in bdrv_set_backing_noperm(). Yes, I think you're right. > Probably, bdrv_replace_child should do it. Probably not (there are > still a lot of things to refactor in block.c :).. > > Hm. Also, using blockdev-reopen probably means that we are in a >blockdev word, so we should not care about inherits_from here. But with blockdev-reopen we do update inherits_from for backing files, don't we? > Also, you don't create reopen_state->replace_file_bs, like for > backing.. On bdrv_reopen_comnmit replace_backing_bs is used to remove > corresponding options.. Shouldn't we do the same with file options? I think you're right. >> - self.reopen(opts, {'file': 'not-found'}, "Cannot change the option 'file'") >> - self.reopen(opts, {'file': ''}, "Cannot change the option 'file'") >> + self.reopen(opts, {'file': 'not-found'}, "Cannot find device='' nor node-name='not-found'") > > Interesting that error-message say about device='', not 'not-found'... That's because 'file' refers to a node name. Thanks for reviewing, Berto
24.03.2021 15:25, Alberto Garcia wrote: > On Thu 18 Mar 2021 03:25:07 PM CET, Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote: >>> static int bdrv_reopen_prepare(BDRVReopenState *reopen_state, >>> BlockReopenQueue *queue, >>> - Transaction *set_backings_tran, Error **errp); >>> + Transaction *tran, Error **errp); >> >> I'd not call it just "tran" to not interfere with transaction >> actions. Of course, reopen should be finally refactored to work >> cleanly on Transaction API, but that is not done yet. And here we pass >> a transaction pointer only to keep children modification.. So, let's >> make it change_child_tran, or something like this. > > The name change looks good to me. > >>> + } else if (bdrv_recurse_has_child(new_child_bs, bs)) { >>> + error_setg(errp, "Making '%s' a %s of '%s' would create a cycle", >>> + str, parse_file ? "file" : "backing file", >> >> maybe s/"file"/"file child"/ > > Ok. > >>> default: >>> - /* 'backing' does not allow any other data type */ >>> + /* The options QDict has been flattened, so 'backing' and 'file' >>> + * do not allow any other data type here. */ >> >> checkpatch should complain that you didn't fix style of the comment... > > I actually don't like to use the proposed style for 2-line comments in > many cases. I think it makes sense for big comment blocks but adds noise > for shorter comments. > >>> + } else { >>> + /* >>> + * Ensure that @bs can really handle backing files, because we are >>> + * about to give it one (or swap the existing one) >>> + */ >>> + if (bs->drv->is_filter) { >>> + /* Filters always have a file or a backing child */ >> >> Probably we can assert bs->backing, as otherwise backing option should >> be unsupported [preexisting, not about this patch] > > Yes, I see that this was added in commit 1d42f48c3a, maybe Max has good > reasons to keep it this way? > >>> if (bdrv_is_backing_chain_frozen(overlay_bs, >>> - child_bs(overlay_bs->backing), errp)) >>> + bdrv_filter_or_cow_bs(overlay_bs), >>> + errp)) >>> { >>> return -EPERM; >>> } > > I just realized that this part is probably not ok if you want to change > bs->file on a node that is not a filter, because this would check > bs->backing->frozen and not bs->file->frozen. > >>> + if (parse_file) { >>> + /* Store the old file bs, we'll need to refresh its permissions */ >>> + reopen_state->old_file_bs = bs->file->bs; >>> + >>> + /* And finally replace the child */ >>> + bdrv_replace_child(bs->file, new_child_bs, tran); >> >> I think that actually, we need also to update inherits_from and do >> refresh_limits like in bdrv_set_backing_noperm(). > > Yes, I think you're right. > >> Probably, bdrv_replace_child should do it. Probably not (there are >> still a lot of things to refactor in block.c :).. >> >> Hm. Also, using blockdev-reopen probably means that we are in a >> blockdev word, so we should not care about inherits_from here. > > But with blockdev-reopen we do update inherits_from for backing files, > don't we? We do.. But as I understand resent Kevin's explanation on my "[PATCH RFC 0/3] block: drop inherits_from", inherits_from exists to support pre-blockdev era.. Anyway, better to support it and don't care, and drop all inherits_from logic at some bright future point. > >> Also, you don't create reopen_state->replace_file_bs, like for >> backing.. On bdrv_reopen_comnmit replace_backing_bs is used to remove >> corresponding options.. Shouldn't we do the same with file options? > > I think you're right. > >>> - self.reopen(opts, {'file': 'not-found'}, "Cannot change the option 'file'") >>> - self.reopen(opts, {'file': ''}, "Cannot change the option 'file'") >>> + self.reopen(opts, {'file': 'not-found'}, "Cannot find device='' nor node-name='not-found'") >> >> Interesting that error-message say about device='', not 'not-found'... > > That's because 'file' refers to a node name. > > Thanks for reviewing, > > Berto >
Am 17.03.2021 um 18:15 hat Alberto Garcia geschrieben: > When the x-blockdev-reopen was added it allowed reconfiguring the > graph by replacing backing files, but changing the 'file' option was > forbidden. Because of this restriction some operations are not > possible, notably inserting and removing block filters. > > This patch adds support for replacing the 'file' option. This is > similar to replacing the backing file and the user is likewise > responsible for the correctness of the resulting graph, otherwise this > can lead to data corruption. > > Signed-off-by: Alberto Garcia <berto@igalia.com> > @@ -4238,13 +4254,13 @@ static int bdrv_reopen_parse_backing(BDRVReopenState *reopen_state, > } > > /* If we want to replace the backing file we need some extra checks */ > - if (new_backing_bs != bdrv_filter_or_cow_bs(overlay_bs)) { > + if (new_child_bs != bdrv_filter_or_cow_bs(overlay_bs)) { I may be missing something, but I don't see how this whole block makes sense for changing 'file'. overlay_bs was found by going down the backing chain, so of course it will be different from new_child_bs (unless you use the same node as 'backing' and as 'file'). So we run all this code that seems to be concerned only with backing files. Probably overlay_bs should be found by starting from child and using bdrv_filter_or_cow_bs() only for the following loop iterations. > int ret; > > /* Check for implicit nodes between bs and its backing file */ > if (bs != overlay_bs) { > - error_setg(errp, "Cannot change backing link if '%s' has " > - "an implicit backing file", bs->node_name); > + error_setg(errp, "Cannot change %s link if '%s' has an implicit " > + "child", parse_file ? "file" : "backing", bs->node_name); > return -EPERM; > } With fixed overlay_bs, this check makes sense, though the comment needs an update. > /* > @@ -4256,16 +4272,24 @@ static int bdrv_reopen_parse_backing(BDRVReopenState *reopen_state, > * with bs->drv->supports_backing == true. > */ > if (bdrv_is_backing_chain_frozen(overlay_bs, > - child_bs(overlay_bs->backing), errp)) > + bdrv_filter_or_cow_bs(overlay_bs), > + errp)) > { > return -EPERM; > } This checks if bs->backing is frozen (overlay_bs == bs because of the check above). What we really want to check is if child is frozen (i.e. bs->backing for updating 'backing', bs->file for updating 'file). So maybe this should be just written as that: if (child && child->frozen) { error_setg(errp, ...); return -EPERM; } Or factor this part out from bdrv_is_backing_chain_frozen() into a bdrv_is_child_frozen() or something like that. Either way, this might make the whole (outdated) comment above unnecessary because things would become a lot clearer. > - reopen_state->replace_backing_bs = true; > - reopen_state->old_backing_bs = bs->backing ? bs->backing->bs : NULL; > - ret = bdrv_set_backing_noperm(bs, new_backing_bs, set_backings_tran, > - errp); > - if (ret < 0) { > - return ret; > + if (parse_file) { > + /* Store the old file bs, we'll need to refresh its permissions */ > + reopen_state->old_file_bs = bs->file->bs; > + > + /* And finally replace the child */ > + bdrv_replace_child(bs->file, new_child_bs, tran); > + } else { > + reopen_state->replace_backing_bs = true; > + reopen_state->old_backing_bs = child_bs(bs->backing); > + ret = bdrv_set_backing_noperm(bs, new_child_bs, tran, errp); > + if (ret < 0) { > + return ret; > + } > } > } Kevin
17.03.2021 20:15, Alberto Garcia wrote: > When the x-blockdev-reopen was added it allowed reconfiguring the > graph by replacing backing files, but changing the 'file' option was > forbidden. Because of this restriction some operations are not > possible, notably inserting and removing block filters. I now started to work on making backup-top filter public.. And I think, we'll need separate commands to insert/remove filters anyway.. As blockdev-reopen has the following problems: 1. It can't append filter above top node, connected to block-device. (but bdrv_append() can do it) 2. It can't simultaneously create new node and append it. This is important for backup-top filter, which unshares write even when has no writing parent. Now bdrv_append() works in a smart way for it: it first do both graph modification (add child to filter, and replace original node by filter) and then update graph permissions. So, we'll need a command which in one roll create filter node and inserts it where needed. 3. blockdev-reopen requires to specify all options (otherwise, they will be changed to default). So it requires passing a lot of information. But we don't need to touch any option of original bs parent to insert a filter between parent and bs. In other words, we don't want to reopen something. We want to insert filter. === Hmm, another mentioned use of blockdev-reopen was reopening some RO node to RW, to modify bitmaps.. And here again, blockdev-reopen is not very convenient: 1. Again, it requires to specify all options (at least, that was not default on node open).. And this only to change one property: read-only. Seems overcomplicated. 2. Bitmaps modifications are usually done in transactions. Adding a clean transaction support for small command that reopens only to RW, and back to RO on transaction finalization seems simpler, than for entire generic blockdev-reopen. === Hmm, interesting. x-blockdev-reopen says that not specified options are reset to default. x-blockdev-reopen works through bdrv_reopen_multiple, so I think bdrv_reopen_mutliple should reset options to default as well. Now look at bdrv_reopen_set_read_only(): it specifies only one option: "read-only". This means that all other options will be reset to default. But for sure, callers of bdrv_reopen_set_read_only() want only to change read-only status of node, not all other options. Do we have a bug here?
Am 07.05.2021 um 09:11 hat Vladimir Sementsov-Ogievskiy geschrieben: > 17.03.2021 20:15, Alberto Garcia wrote: > > When the x-blockdev-reopen was added it allowed reconfiguring the > > graph by replacing backing files, but changing the 'file' option was > > forbidden. Because of this restriction some operations are not > > possible, notably inserting and removing block filters. > > > I now started to work on making backup-top filter public.. > > And I think, we'll need separate commands to insert/remove filters > anyway.. As blockdev-reopen has the following problems: > > 1. It can't append filter above top node, connected to block-device. > (but bdrv_append() can do it) We once had some patches that made the 'drive' qdev property runtime writable. What happened to them? > 2. It can't simultaneously create new node and append it. This is > important for backup-top filter, which unshares write even when has no > writing parent. Now bdrv_append() works in a smart way for it: it > first do both graph modification (add child to filter, and replace > original node by filter) and then update graph permissions. So, we'll > need a command which in one roll create filter node and inserts it > where needed. What backup-top could do, however, is enabling restrictions only if it has a parent (no matter whether that parent requires writing or not). > 3. blockdev-reopen requires to specify all options (otherwise, they > will be changed to default). So it requires passing a lot of > information. But we don't need to touch any option of original bs > parent to insert a filter between parent and bs. In other words, we > don't want to reopen something. We want to insert filter. Yeah, but this was a decision we made consciously because otherwise we'd have a hard time telling which options should be updated and which shouldn't, and we would need separate QAPI types for open and reopen. If we now say that this is a reason for avoiding blockdev-reopen even though changing some option is the goal, that would be inconsistent. > > === > > Hmm, another mentioned use of blockdev-reopen was reopening some RO > node to RW, to modify bitmaps.. And here again, blockdev-reopen is not > very convenient: > > 1. Again, it requires to specify all options (at least, that was not > default on node open).. And this only to change one property: > read-only. Seems overcomplicated. > > 2. Bitmaps modifications are usually done in transactions. Adding a > clean transaction support for small command that reopens only to RW, > and back to RO on transaction finalization seems simpler, than for > entire generic blockdev-reopen. > > > === > > Hmm, interesting. x-blockdev-reopen says that not specified options > are reset to default. x-blockdev-reopen works through > bdrv_reopen_multiple, so I think bdrv_reopen_mutliple should reset > options to default as well. Now look at bdrv_reopen_set_read_only(): > it specifies only one option: "read-only". This means that all other > options will be reset to default. But for sure, callers of > bdrv_reopen_set_read_only() want only to change read-only status of > node, not all other options. Do we have a bug here? The difference between these cases is the keep_old_opts parameter to bdrv_reopen_queue(). It is false for x-blockdev-reopen, but true in bdrv_reopen_set_read_only(). Kevin
07.05.2021 17:09, Kevin Wolf wrote: > Am 07.05.2021 um 09:11 hat Vladimir Sementsov-Ogievskiy geschrieben: >> 17.03.2021 20:15, Alberto Garcia wrote: >>> When the x-blockdev-reopen was added it allowed reconfiguring the >>> graph by replacing backing files, but changing the 'file' option was >>> forbidden. Because of this restriction some operations are not >>> possible, notably inserting and removing block filters. >> >> >> I now started to work on making backup-top filter public.. >> >> And I think, we'll need separate commands to insert/remove filters >> anyway.. As blockdev-reopen has the following problems: >> >> 1. It can't append filter above top node, connected to block-device. >> (but bdrv_append() can do it) > > We once had some patches that made the 'drive' qdev property runtime > writable. What happened to them? > >> 2. It can't simultaneously create new node and append it. This is >> important for backup-top filter, which unshares write even when has no >> writing parent. Now bdrv_append() works in a smart way for it: it >> first do both graph modification (add child to filter, and replace >> original node by filter) and then update graph permissions. So, we'll >> need a command which in one roll create filter node and inserts it >> where needed. > > What backup-top could do, however, is enabling restrictions only if it > has a parent (no matter whether that parent requires writing or not). > >> 3. blockdev-reopen requires to specify all options (otherwise, they >> will be changed to default). So it requires passing a lot of >> information. But we don't need to touch any option of original bs >> parent to insert a filter between parent and bs. In other words, we >> don't want to reopen something. We want to insert filter. > > Yeah, but this was a decision we made consciously because otherwise we'd > have a hard time telling which options should be updated and which > shouldn't, and we would need separate QAPI types for open and reopen. > > If we now say that this is a reason for avoiding blockdev-reopen even > though changing some option is the goal, that would be inconsistent. > >> >> === >> >> Hmm, another mentioned use of blockdev-reopen was reopening some RO >> node to RW, to modify bitmaps.. And here again, blockdev-reopen is not >> very convenient: >> >> 1. Again, it requires to specify all options (at least, that was not >> default on node open).. And this only to change one property: >> read-only. Seems overcomplicated. >> >> 2. Bitmaps modifications are usually done in transactions. Adding a >> clean transaction support for small command that reopens only to RW, >> and back to RO on transaction finalization seems simpler, than for >> entire generic blockdev-reopen. >> >> >> === >> >> Hmm, interesting. x-blockdev-reopen says that not specified options >> are reset to default. x-blockdev-reopen works through >> bdrv_reopen_multiple, so I think bdrv_reopen_mutliple should reset >> options to default as well. Now look at bdrv_reopen_set_read_only(): >> it specifies only one option: "read-only". This means that all other >> options will be reset to default. But for sure, callers of >> bdrv_reopen_set_read_only() want only to change read-only status of >> node, not all other options. Do we have a bug here? > > The difference between these cases is the keep_old_opts parameter to > bdrv_reopen_queue(). It is false for x-blockdev-reopen, but true in > bdrv_reopen_set_read_only(). > Thanks for explanations, seems I panicked too early :) Will try your suggestions.
diff --git a/include/block/block.h b/include/block/block.h index 5eb1e4cab9..e2732a0187 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -209,6 +209,7 @@ typedef struct BDRVReopenState { bool backing_missing; bool replace_backing_bs; /* new_backing_bs is ignored if this is false */ BlockDriverState *old_backing_bs; /* keep pointer for permissions update */ + BlockDriverState *old_file_bs; /* keep pointer for permissions update */ QDict *options; QDict *explicit_options; void *opaque; diff --git a/block.c b/block.c index 764cdbec7d..8ff0afd77b 100644 --- a/block.c +++ b/block.c @@ -98,7 +98,7 @@ static void bdrv_remove_filter_or_cow_child(BlockDriverState *bs, static int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue, - Transaction *set_backings_tran, Error **errp); + Transaction *tran, Error **errp); static void bdrv_reopen_commit(BDRVReopenState *reopen_state); static void bdrv_reopen_abort(BDRVReopenState *reopen_state); @@ -4049,6 +4049,10 @@ int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, Error **errp) refresh_list = bdrv_topological_dfs(refresh_list, found, state->old_backing_bs); } + if (state->old_file_bs) { + refresh_list = bdrv_topological_dfs(refresh_list, found, + state->old_file_bs); + } } /* @@ -4161,65 +4165,77 @@ static bool bdrv_reopen_can_attach(BlockDriverState *parent, * * Return 0 on success, otherwise return < 0 and set @errp. */ -static int bdrv_reopen_parse_backing(BDRVReopenState *reopen_state, - Transaction *set_backings_tran, - Error **errp) +static int bdrv_reopen_parse_file_or_backing(BDRVReopenState *reopen_state, + bool parse_file, Transaction *tran, + Error **errp) { BlockDriverState *bs = reopen_state->bs; - BlockDriverState *overlay_bs, *below_bs, *new_backing_bs; + BlockDriverState *overlay_bs, *below_bs, *new_child_bs; + BdrvChild *child = parse_file ? bs->file : bs->backing; QObject *value; const char *str; - value = qdict_get(reopen_state->options, "backing"); + value = qdict_get(reopen_state->options, parse_file ? "file" : "backing"); if (value == NULL) { return 0; } switch (qobject_type(value)) { case QTYPE_QNULL: - new_backing_bs = NULL; + assert(!parse_file); /* The 'file' option does not allow a null value */ + new_child_bs = NULL; break; case QTYPE_QSTRING: str = qstring_get_str(qobject_to(QString, value)); - new_backing_bs = bdrv_lookup_bs(NULL, str, errp); - if (new_backing_bs == NULL) { + new_child_bs = bdrv_lookup_bs(NULL, str, errp); + if (new_child_bs == NULL) { return -EINVAL; - } else if (bdrv_recurse_has_child(new_backing_bs, bs)) { - error_setg(errp, "Making '%s' a backing file of '%s' " - "would create a cycle", str, bs->node_name); + } else if (bdrv_recurse_has_child(new_child_bs, bs)) { + error_setg(errp, "Making '%s' a %s of '%s' would create a cycle", + str, parse_file ? "file" : "backing file", + bs->node_name); return -EINVAL; } break; default: - /* 'backing' does not allow any other data type */ + /* The options QDict has been flattened, so 'backing' and 'file' + * do not allow any other data type here. */ g_assert_not_reached(); } - /* - * Check AioContext compatibility so that the bdrv_set_backing_hd() call in - * bdrv_reopen_commit() won't fail. - */ - if (new_backing_bs) { - if (!bdrv_reopen_can_attach(bs, bs->backing, new_backing_bs, errp)) { + /* If 'file' points to the current child then there's nothing to do */ + if (child_bs(child) == new_child_bs) { + return 0; + } + + /* Check AioContext compatibility */ + if (new_child_bs) { + if (!bdrv_reopen_can_attach(bs, child, new_child_bs, errp)) { return -EINVAL; } } - /* - * Ensure that @bs can really handle backing files, because we are - * about to give it one (or swap the existing one) - */ - if (bs->drv->is_filter) { - /* Filters always have a file or a backing child */ - if (!bs->backing) { - error_setg(errp, "'%s' is a %s filter node that does not support a " - "backing child", bs->node_name, bs->drv->format_name); + if (parse_file) { + assert(child && child->bs); + } else { + /* + * Ensure that @bs can really handle backing files, because we are + * about to give it one (or swap the existing one) + */ + if (bs->drv->is_filter) { + /* Filters always have a file or a backing child */ + if (!bs->backing) { + error_setg(errp, "'%s' is a %s filter node " + "that does not support a backing child", + bs->node_name, bs->drv->format_name); + return -EINVAL; + } + } else if (!bs->drv->supports_backing) { + error_setg(errp, "Driver '%s' of node '%s' " + "does not support backing files", + bs->drv->format_name, bs->node_name); return -EINVAL; } - } else if (!bs->drv->supports_backing) { - error_setg(errp, "Driver '%s' of node '%s' does not support backing " - "files", bs->drv->format_name, bs->node_name); - return -EINVAL; } /* @@ -4238,13 +4254,13 @@ static int bdrv_reopen_parse_backing(BDRVReopenState *reopen_state, } /* If we want to replace the backing file we need some extra checks */ - if (new_backing_bs != bdrv_filter_or_cow_bs(overlay_bs)) { + if (new_child_bs != bdrv_filter_or_cow_bs(overlay_bs)) { int ret; /* Check for implicit nodes between bs and its backing file */ if (bs != overlay_bs) { - error_setg(errp, "Cannot change backing link if '%s' has " - "an implicit backing file", bs->node_name); + error_setg(errp, "Cannot change %s link if '%s' has an implicit " + "child", parse_file ? "file" : "backing", bs->node_name); return -EPERM; } /* @@ -4256,16 +4272,24 @@ static int bdrv_reopen_parse_backing(BDRVReopenState *reopen_state, * with bs->drv->supports_backing == true. */ if (bdrv_is_backing_chain_frozen(overlay_bs, - child_bs(overlay_bs->backing), errp)) + bdrv_filter_or_cow_bs(overlay_bs), + errp)) { return -EPERM; } - reopen_state->replace_backing_bs = true; - reopen_state->old_backing_bs = bs->backing ? bs->backing->bs : NULL; - ret = bdrv_set_backing_noperm(bs, new_backing_bs, set_backings_tran, - errp); - if (ret < 0) { - return ret; + if (parse_file) { + /* Store the old file bs, we'll need to refresh its permissions */ + reopen_state->old_file_bs = bs->file->bs; + + /* And finally replace the child */ + bdrv_replace_child(bs->file, new_child_bs, tran); + } else { + reopen_state->replace_backing_bs = true; + reopen_state->old_backing_bs = child_bs(bs->backing); + ret = bdrv_set_backing_noperm(bs, new_child_bs, tran, errp); + if (ret < 0) { + return ret; + } } } @@ -4291,7 +4315,7 @@ static int bdrv_reopen_parse_backing(BDRVReopenState *reopen_state, */ static int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue, - Transaction *set_backings_tran, Error **errp) + Transaction *tran, Error **errp) { int ret = -1; int old_flags; @@ -4411,12 +4435,19 @@ static int bdrv_reopen_prepare(BDRVReopenState *reopen_state, * either a reference to an existing node (using its node name) * or NULL to simply detach the current backing file. */ - ret = bdrv_reopen_parse_backing(reopen_state, set_backings_tran, errp); + ret = bdrv_reopen_parse_file_or_backing(reopen_state, false, tran, errp); if (ret < 0) { goto error; } qdict_del(reopen_state->options, "backing"); + /* Allow changing the 'file' option. In this case NULL is not allowed */ + ret = bdrv_reopen_parse_file_or_backing(reopen_state, true, tran, errp); + if (ret < 0) { + goto error; + } + qdict_del(reopen_state->options, "file"); + /* Options that are not handled are only okay if they are unchanged * compared to the old state. It is expected that some options are only * used for the initial open, but not reopen (e.g. filename) */ diff --git a/tests/qemu-iotests/245 b/tests/qemu-iotests/245 index fc5297e268..a4d0b10e9d 100755 --- a/tests/qemu-iotests/245 +++ b/tests/qemu-iotests/245 @@ -146,8 +146,8 @@ class TestBlockdevReopen(iotests.QMPTestCase): self.reopen(opts, {'driver': 'raw'}, "Cannot change the option 'driver'") self.reopen(opts, {'driver': ''}, "Invalid parameter ''") self.reopen(opts, {'driver': None}, "Invalid parameter type for 'driver', expected: string") - self.reopen(opts, {'file': 'not-found'}, "Cannot change the option 'file'") - self.reopen(opts, {'file': ''}, "Cannot change the option 'file'") + self.reopen(opts, {'file': 'not-found'}, "Cannot find device='' nor node-name='not-found'") + self.reopen(opts, {'file': ''}, "Cannot find device='' nor node-name=''") self.reopen(opts, {'file': None}, "Invalid parameter type for 'file', expected: BlockdevRef") self.reopen(opts, {'file.node-name': 'newname'}, "Cannot change the option 'node-name'") self.reopen(opts, {'file.driver': 'host_device'}, "Cannot change the option 'driver'") @@ -455,7 +455,8 @@ class TestBlockdevReopen(iotests.QMPTestCase): # More illegal operations self.reopen(opts[2], {'backing': 'hd1'}, "Making 'hd1' a backing file of 'hd2' would create a cycle") - self.reopen(opts[2], {'file': 'hd0-file'}, "Cannot change the option 'file'") + self.reopen(opts[2], {'file': 'hd0-file'}, + "Conflicts with use by hd2 as 'file', which does not allow 'write, resize' on hd0-file") result = self.vm.qmp('blockdev-del', conv_keys = True, node_name = 'hd2') self.assert_qmp(result, 'error/class', 'GenericError') @@ -969,7 +970,7 @@ class TestBlockdevReopen(iotests.QMPTestCase): # We can't remove hd1 while the commit job is ongoing opts['backing'] = None - self.reopen(opts, {}, "Cannot change backing link if 'hd0' has an implicit backing file") + self.reopen(opts, {}, "Cannot change backing link if 'hd0' has an implicit child") # hd2 <- hd0 self.vm.run_job('commit0', auto_finalize = False, auto_dismiss = True)
When the x-blockdev-reopen was added it allowed reconfiguring the graph by replacing backing files, but changing the 'file' option was forbidden. Because of this restriction some operations are not possible, notably inserting and removing block filters. This patch adds support for replacing the 'file' option. This is similar to replacing the backing file and the user is likewise responsible for the correctness of the resulting graph, otherwise this can lead to data corruption. Signed-off-by: Alberto Garcia <berto@igalia.com> --- include/block/block.h | 1 + block.c | 119 ++++++++++++++++++++++++++--------------- tests/qemu-iotests/245 | 9 ++-- 3 files changed, 81 insertions(+), 48 deletions(-)