| Message ID | 1a9b457d93c0732e8e4785a0cc4b5f3b935f2cf6.1610715661.git.berto@igalia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Allow changing bs->file on reopen | expand |
15.01.2021 16:02, 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> > --- > include/block/block.h | 1 + > block.c | 61 ++++++++++++++++++++++++++++++++++++++++++ > tests/qemu-iotests/245 | 7 ++--- > 3 files changed, 66 insertions(+), 3 deletions(-) > > diff --git a/include/block/block.h b/include/block/block.h > index 82271d9ccd..6dd687a69e 100644 > --- a/include/block/block.h > +++ b/include/block/block.h > @@ -196,6 +196,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 */ > uint64_t perm, shared_perm; > QDict *options; > QDict *explicit_options; > diff --git a/block.c b/block.c > index 576b145cbf..114788e58e 100644 > --- a/block.c > +++ b/block.c > @@ -3978,6 +3978,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); > + } > } > > ret = bdrv_list_refresh_perms(refresh_list, bs_queue, &tran, errp); > @@ -4196,6 +4200,57 @@ static int bdrv_reopen_parse_backing(BDRVReopenState *reopen_state, > return 0; > } > > +static int bdrv_reopen_parse_file(BDRVReopenState *reopen_state, > + GSList **tran, > + Error **errp) > +{ > + BlockDriverState *bs = reopen_state->bs; > + BlockDriverState *new_file_bs; > + QObject *value; > + const char *str; > + > + value = qdict_get(reopen_state->options, "file"); > + if (value == NULL) { > + return 0; > + } > + > + /* The 'file' option only allows strings */ > + assert(qobject_type(value) == QTYPE_QSTRING); > + > + str = qobject_get_try_str(value); > + new_file_bs = bdrv_lookup_bs(NULL, str, errp); > + if (new_file_bs == NULL) { > + return -EINVAL; > + } else if (bdrv_recurse_has_child(new_file_bs, bs)) { > + error_setg(errp, "Making '%s' a file of '%s' " > + "would create a cycle", str, bs->node_name); > + return -EINVAL; > + } > + > + assert(bs->file && bs->file->bs); why are we sure at this point? Probably, we should just return an error.. > + > + /* If 'file' points to the current child then there's nothing to do */ > + if (bs->file->bs == new_file_bs) { > + return 0; > + } > + > + /* Check AioContext compatibility */ > + if (!bdrv_reopen_can_attach(bs, bs->file, new_file_bs, errp)) { > + return -EINVAL; > + } > + > + /* At the moment only backing links are frozen */ > + assert(!bs->file->frozen); I think it can: file-child based filters can be a part of frozen backing chain currently. > + > + /* Store the old file bs because 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_file_bs, tran); > + > + return 0; > +} > + > /* > * Prepares a BlockDriverState for reopen. All changes are staged in the > * 'opaque' field of the BDRVReopenState, which is used and allocated by > @@ -4347,6 +4402,12 @@ static int bdrv_reopen_prepare(BDRVReopenState *reopen_state, > } > qdict_del(reopen_state->options, "backing"); > > + ret = bdrv_reopen_parse_file(reopen_state, set_backings_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 e60c8326d3..f9d68b3958 100755 > --- a/tests/qemu-iotests/245 > +++ b/tests/qemu-iotests/245 > @@ -145,8 +145,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'") > @@ -454,7 +454,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') >
On Mon 18 Jan 2021 11:15:17 AM CET, Vladimir Sementsov-Ogievskiy wrote: >> +static int bdrv_reopen_parse_file(BDRVReopenState *reopen_state, >> + GSList **tran, >> + Error **errp) >> +{ >> + BlockDriverState *bs = reopen_state->bs; >> + BlockDriverState *new_file_bs; >> + QObject *value; >> + const char *str; >> + >> + value = qdict_get(reopen_state->options, "file"); >> + if (value == NULL) { >> + return 0; >> + } >> + >> + /* The 'file' option only allows strings */ >> + assert(qobject_type(value) == QTYPE_QSTRING); >> + >> + str = qobject_get_try_str(value); >> + new_file_bs = bdrv_lookup_bs(NULL, str, errp); >> + if (new_file_bs == NULL) { >> + return -EINVAL; >> + } else if (bdrv_recurse_has_child(new_file_bs, bs)) { >> + error_setg(errp, "Making '%s' a file of '%s' " >> + "would create a cycle", str, bs->node_name); >> + return -EINVAL; >> + } >> + >> + assert(bs->file && bs->file->bs); > > why are we sure at this point? Probably, we should just return an > error.. Unlike 'backing', 'file' is a BlockdevRef and it is not optional, so block devices that accept that parameter must have it set. >> + /* At the moment only backing links are frozen */ >> + assert(!bs->file->frozen); > > I think it can: file-child based filters can be a part of frozen > backing chain currently. You're right, since 7b99a26600e bdrv_freeze_backing_chain() uses bdrv_filter_or_cow_child(). Berto
diff --git a/include/block/block.h b/include/block/block.h index 82271d9ccd..6dd687a69e 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -196,6 +196,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 */ uint64_t perm, shared_perm; QDict *options; QDict *explicit_options; diff --git a/block.c b/block.c index 576b145cbf..114788e58e 100644 --- a/block.c +++ b/block.c @@ -3978,6 +3978,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); + } } ret = bdrv_list_refresh_perms(refresh_list, bs_queue, &tran, errp); @@ -4196,6 +4200,57 @@ static int bdrv_reopen_parse_backing(BDRVReopenState *reopen_state, return 0; } +static int bdrv_reopen_parse_file(BDRVReopenState *reopen_state, + GSList **tran, + Error **errp) +{ + BlockDriverState *bs = reopen_state->bs; + BlockDriverState *new_file_bs; + QObject *value; + const char *str; + + value = qdict_get(reopen_state->options, "file"); + if (value == NULL) { + return 0; + } + + /* The 'file' option only allows strings */ + assert(qobject_type(value) == QTYPE_QSTRING); + + str = qobject_get_try_str(value); + new_file_bs = bdrv_lookup_bs(NULL, str, errp); + if (new_file_bs == NULL) { + return -EINVAL; + } else if (bdrv_recurse_has_child(new_file_bs, bs)) { + error_setg(errp, "Making '%s' a file of '%s' " + "would create a cycle", str, bs->node_name); + return -EINVAL; + } + + assert(bs->file && bs->file->bs); + + /* If 'file' points to the current child then there's nothing to do */ + if (bs->file->bs == new_file_bs) { + return 0; + } + + /* Check AioContext compatibility */ + if (!bdrv_reopen_can_attach(bs, bs->file, new_file_bs, errp)) { + return -EINVAL; + } + + /* At the moment only backing links are frozen */ + assert(!bs->file->frozen); + + /* Store the old file bs because 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_file_bs, tran); + + return 0; +} + /* * Prepares a BlockDriverState for reopen. All changes are staged in the * 'opaque' field of the BDRVReopenState, which is used and allocated by @@ -4347,6 +4402,12 @@ static int bdrv_reopen_prepare(BDRVReopenState *reopen_state, } qdict_del(reopen_state->options, "backing"); + ret = bdrv_reopen_parse_file(reopen_state, set_backings_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 e60c8326d3..f9d68b3958 100755 --- a/tests/qemu-iotests/245 +++ b/tests/qemu-iotests/245 @@ -145,8 +145,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'") @@ -454,7 +454,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')
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 | 61 ++++++++++++++++++++++++++++++++++++++++++ tests/qemu-iotests/245 | 7 ++--- 3 files changed, 66 insertions(+), 3 deletions(-)