Re: [RFC PATCH v2 1/4] block: Allow changing bs->file on reopen

[Kevin Wolf <kwolf@redhat.com>]

Am 08.02.2021 um 19:44 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>
> ---
>  include/block/block.h  |  1 +
>  block.c                | 65 ++++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/245 |  7 +++--
>  3 files changed, 70 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..19b62da4af 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,61 @@ 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);

This is true, but not entirely obvious: The QAPI schema has BlockdevRef,
which can be either a string or a dict. However, we're dealing with a
flattened options dict here, so no more nested dicts.

qemu-io doesn't go through the schema, but its parser represents all
scalars as strings, so it's correct even in this case.

> +
> +    str = qobject_get_try_str(value);

This function doesn't exist in master any more, but we already know that
we have a string here, so it's easy enough to replace:

str = qstring_get_str(qobject_to(QString, 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;
> +    }
> +
> +    if (bs->file->frozen) {
> +        error_setg(errp, "Cannot change the 'file' link of '%s' "
> +                   "from '%s' to '%s'", bs->node_name,
> +                   bs->file->bs->node_name, new_file_bs->node_name);
> +        return -EPERM;
> +    }
> +
> +    /* Check AioContext compatibility */
> +    if (!bdrv_reopen_can_attach(bs, bs->file, new_file_bs, errp)) {
> +        return -EINVAL;
> +    }
> +
> +    /* 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;
> +}

As Vladimir said, it would be nice to avoid some duplication with the
backing file switching code (especially when you consider that we might
get more of these cases, think of qcow2 data files or VMDK extents), but
generally this patch makes sense to me.

Kevin