[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v4 2/6] block: Allow changing bs->file on reopen
From: |
Kevin Wolf |
Subject: |
Re: [PATCH v4 2/6] block: Allow changing bs->file on reopen |
Date: |
Wed, 5 May 2021 15:58:57 +0200 |
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
- Re: [PATCH v4 2/6] block: Allow changing bs->file on reopen,
Kevin Wolf <=