Re: [Qemu-block] [PATCH v2 08/13] block: Allow changing the backing file on reopen

[Kevin Wolf]

Am 06.03.2019 um 19:11 hat Alberto Garcia geschrieben:
> This patch allows the user to change the backing file of an image that
> is being reopened. Here's what it does:
> 
>  - In bdrv_reopen_prepare(): check that the value of 'backing' points
>    to an existing node or is null. If it points to an existing node it
>    also needs to make sure that replacing the backing file will not
>    create a cycle in the node graph (i.e. you cannot reach the parent
>    from the new backing file).
> 
>  - In bdrv_reopen_commit(): perform the actual node replacement by
>    calling bdrv_set_backing_hd().
> 
> There may be temporary implicit nodes between a BDS and its backing
> file (e.g. a commit filter node). In these cases bdrv_reopen_prepare()
> looks for the real (non-implicit) backing file and requires that the
> 'backing' option points to it. Replacing or detaching a backing file
> is forbidden if there are implicit nodes in the middle.
> 
> Although x-blockdev-reopen is meant to be used like blockdev-add,
> there's an important thing that must be taken into account: the only
> way to set a new backing file is by using a reference to an existing
> node (previously added with e.g. blockdev-add).  If 'backing' contains
> a dictionary with a new set of options ({"driver": "qcow2", "file": {
> ... }}) then it is interpreted that the _existing_ backing file must
> be reopened with those options.
> 
> Signed-off-by: Alberto Garcia <address@hidden>

> @@ -3294,6 +3315,98 @@ static void bdrv_reopen_perm(BlockReopenQueue *q, 
> BlockDriverState *bs,
>  }
>  
>  /*
> + * Take a BDRVReopenState and check if the value of 'backing' in the
> + * reopen_state->options QDict is valid or not.
> + *
> + * If 'backing' is missing from the QDict then return 0.
> + *
> + * If 'backing' contains the node name of the backing file of
> + * reopen_state->bs then return 0.
> + *
> + * If 'backing' contains a different node name (or is null) then check
> + * whether the current backing file can be replaced with the new one.
> + * If that's the case then reopen_state->replace_backing_bs is set to
> + * true and reopen_state->new_backing_bs contains a pointer to the new
> + * backing BlockDriverState (or NULL).
> + *
> + * Return 0 on success, otherwise return < 0 and set @errp.
> + */
> +static int bdrv_reopen_parse_backing(BDRVReopenState *reopen_state,
> +                                     Error **errp)
> +{
> +    BlockDriverState *bs = reopen_state->bs;
> +    BlockDriverState *overlay_bs, *new_backing_bs;
> +    QObject *value;
> +    const char *str;
> +
> +    value = qdict_get(reopen_state->options, "backing");
> +    if (value == NULL) {
> +        return 0;
> +    }
> +
> +    switch (qobject_type(value)) {
> +    case QTYPE_QNULL:
> +        new_backing_bs = NULL;
> +        break;
> +    case QTYPE_QSTRING:
> +        str = qobject_get_try_str(value);
> +        new_backing_bs = bdrv_lookup_bs(NULL, str, errp);
> +        if (new_backing_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);
> +            return -EINVAL;
> +        }
> +        break;
> +    default:
> +        /* 'backing' does not allow any other data type */
> +        g_assert_not_reached();
> +    }
> +
> +    /*
> +     * TODO: before removing the x- prefix from x-blockdev-reopen we
> +     * should move the new backing file into the right AioContext
> +     * instead of returning an error.
> +     */
> +    if (new_backing_bs) {
> +        if (bdrv_get_aio_context(new_backing_bs) != 
> bdrv_get_aio_context(bs)) {
> +            error_setg(errp, "Cannot use a new backing file "
> +                       "with a different AioContext");
> +            return -EINVAL;
> +        }
> +    }
> +
> +    /*
> +     * Find the "actual" backing file by skipping all links that point
> +     * to an implicit node, if any (e.g. a commit filter node).
> +     */
> +    overlay_bs = bs;
> +    while (backing_bs(overlay_bs) && backing_bs(overlay_bs)->implicit) {
> +        overlay_bs = backing_bs(overlay_bs);
> +    }
> +
> +    /* If we want to replace the backing file we need some extra checks */
> +    if (new_backing_bs != backing_bs(overlay_bs)) {
> +        /* 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);
> +            return -EPERM;
> +        }
> +        /* Check if the backing link that we want to replace is frozen */
> +        if (bdrv_is_backing_chain_frozen(overlay_bs, backing_bs(overlay_bs),
> +                                         errp)) {
> +            return -EPERM;
> +        }
> +        reopen_state->replace_backing_bs = true;
> +        reopen_state->new_backing_bs = new_backing_bs;

Do we need to bdrv_ref(new_backing_bs) here in case its only reference
is dropped in the same reopen transaction?

> +    }
> +
> +    return 0;
> +}
> +
> +/*
>   * Prepares a BlockDriverState for reopen. All changes are staged in the
>   * 'opaque' field of the BDRVReopenState, which is used and allocated by
>   * the block driver layer .bdrv_reopen_prepare()
> @@ -3427,6 +3540,17 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, 
> BlockReopenQueue *queue,
>          goto error;
>      }
>  
> +    /*
> +     * Allow changing the 'backing' option. The new value can be
> +     * 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, errp);
> +    if (ret < 0) {
> +        goto error;
> +    }
> +    qdict_del(reopen_state->options, "backing");
> +
>      /* 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) */
> @@ -3485,6 +3609,20 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, 
> BlockReopenQueue *queue,
>          goto error;
>      }
>  
> +    /* Check if new_backing_bs would accept the new permissions */
> +    if (reopen_state->replace_backing_bs && reopen_state->new_backing_bs) {
> +        uint64_t cur_perm, cur_shared;
> +        bdrv_child_perm(reopen_state->bs, reopen_state->new_backing_bs,
> +                        NULL, &child_backing, NULL,

Why do we pass NULL instead of queue here? Shouldn't the required
permissions be calculated based on the post-reopen state?

> +                        reopen_state->perm, reopen_state->shared_perm,
> +                        &cur_perm, &cur_shared);
> +        ret = bdrv_check_update_perm(reopen_state->new_backing_bs, NULL,
> +                                     cur_perm, cur_shared, NULL, errp);
> +        if (ret < 0) {
> +            goto error;
> +        }
> +    }
> +
>      ret = 0;
>  
>      /* Restore the original reopen_state->options QDict */

Kevin