Re: [PATCH v4 2/6] block: Allow changing bs->file on reopen

[Kevin Wolf]

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