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

[Alberto Garcia]

On Tue 12 Feb 2019 06:27:56 PM CET, Kevin Wolf wrote:
>>  - In bdrv_reopen_commit(): perform the actual node replacement by
>>    calling bdrv_set_backing_hd(). It may happen that there are
>>    temporary implicit nodes between the BDS that we are reopening and
>>    the backing file that we want to replace (e.g. a commit fiter node),
>>    so we must skip them.
>
> Hmm... I really dislike getting into the business of dealing with
> implicit nodes here. If you want to manage the block graph at the node
> level, you should manage all of it and just avoid getting implicit
> nodes in the first place. Otherwise, we'd have to guess where in the
> implicit chain you really want to add or remove nodes, and we're bound
> to guess wrong for some users.
>
> There is one problem with not skipping implicit nodes, though: Even if
> you don't want to manage things at the node level, we already force
> you to specify 'backing'. If there are implicit nodes, you don't knwo
> the real node name of the first backing child.
>
> So I suggest that we allow skipping implicit nodes for the purpose of
> leaving the backing link unchanged; but we return an error if you want
> to change the backing link and there are implicit nodes in the way.

, that sounds good to me. It doesn't really affect any of the test
cases that I had


>
>> 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>
>> ---
>>  block.c | 124 
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
>>  1 file changed, 122 insertions(+), 2 deletions(-)
>> 
>> diff --git a/block.c b/block.c
>> index 897c8b85cd..10847416b2 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -2909,6 +2909,27 @@ BlockDriverState *bdrv_open(const char *filename, 
>> const char *reference,
>>  }
>>  
>>  /*
>> + * Returns true if @child can be reached recursively from @bs
>> + */
>> +static bool bdrv_recurse_has_child(BlockDriverState *bs,
>> +                                   BlockDriverState *child)
>> +{
>> +    BdrvChild *c;
>> +
>> +    if (bs == child) {
>> +        return true;
>> +    }
>> +
>> +    QLIST_FOREACH(c, &bs->children, next) {
>> +        if (bdrv_recurse_has_child(c->bs, child)) {
>> +            return true;
>> +        }
>> +    }
>> +
>> +    return false;
>> +}
>> +
>> +/*
>>   * Adds a BlockDriverState to a simple queue for an atomic, transactional
>>   * reopen of multiple devices.
>>   *
>> @@ -3217,6 +3238,63 @@ static void bdrv_reopen_perm(BlockReopenQueue *q, 
>> BlockDriverState *bs,
>>  }
>>  
>>  /*
>> + * Return 0 if the 'backing' option of @bs can be replaced with
>> + * @value, otherwise return < 0 and set @errp.
>> + */
>> +static int bdrv_reopen_parse_backing(BlockDriverState *bs, QObject *value,
>> +                                     Error **errp)
>> +{
>> +    BlockDriverState *overlay_bs, *new_backing_bs;
>> +    const char *str;
>> +
>> +    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();
>> +    }
>> +
>> +    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;
>> +        }
>> +    }
>
> This is okay for a first version, but in reality, you'd usually want to
> just move the backing file into the right AioContext. Of course, this is
> only correct if the backing file doesn't have other users in different
> AioContexts. To get a good implementation for this, we'd probably need
> to store the AioContext in BdrvChild, like we already concluded for
> other use cases.
>
> Maybe document this as one of the problems to be solved before we can
> remove the x- prefix.
>
>> +
>> +    /*
>> +     * Skip all links that point to an implicit node, if any
>> +     * (e.g. a commit filter node). We don't want to change those.
>> +     */
>> +    overlay_bs = bs;
>> +    while (backing_bs(overlay_bs) && backing_bs(overlay_bs)->implicit) {
>> +        overlay_bs = backing_bs(overlay_bs);
>> +    }
>> +
>> +    if (new_backing_bs != backing_bs(overlay_bs)) {
>> +        if (bdrv_is_backing_chain_frozen(overlay_bs, backing_bs(overlay_bs),
>> +                                         errp)) {
>> +            return -EPERM;
>> +        }
>> +    }
>
> Should this function check new_backing_bs->drv->supports_backing, too?
>
>> +    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()
>> @@ -3359,6 +3437,19 @@ int bdrv_reopen_prepare(BDRVReopenState 
>> *reopen_state, BlockReopenQueue *queue,
>>              QObject *new = entry->value;
>>              QObject *old = qdict_get(reopen_state->bs->options, entry->key);
>>  
>> +            /*
>> +             * 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.
>> +             */
>> +            if (!strcmp(entry->key, "backing")) {
>> +                ret = bdrv_reopen_parse_backing(reopen_state->bs, new, 
>> errp);
>> +                if (ret < 0) {
>> +                    goto error;
>> +                }
>> +                continue; /* 'backing' option processed, go to the next one 
>> */
>> +            }
>> +
>>              /* Allow child references (child_name=node_name) as long as they
>>               * point to the current child (i.e. everything stays the same). 
>> */
>>              if (qobject_type(new) == QTYPE_QSTRING) {
>> @@ -3437,9 +3528,10 @@ error:
>>  void bdrv_reopen_commit(BDRVReopenState *reopen_state)
>>  {
>>      BlockDriver *drv;
>> -    BlockDriverState *bs;
>> +    BlockDriverState *bs, *new_backing_bs;
>>      BdrvChild *child;
>> -    bool old_can_write, new_can_write;
>> +    bool old_can_write, new_can_write, change_backing_bs;
>> +    QObject *qobj;
>>  
>>      assert(reopen_state != NULL);
>>      bs = reopen_state->bs;
>> @@ -3464,6 +3556,15 @@ void bdrv_reopen_commit(BDRVReopenState *reopen_state)
>>      bs->read_only = !(reopen_state->flags & BDRV_O_RDWR);
>>      bs->detect_zeroes      = reopen_state->detect_zeroes;
>>  
>> +    qobj = qdict_get(bs->options, "backing");
>> +    change_backing_bs = (qobj != NULL);
>> +    if (change_backing_bs) {
>> +        const char *str = qobject_get_try_str(qobj);
>> +        new_backing_bs = str ? bdrv_find_node(str) : NULL;
>> +        qdict_del(bs->explicit_options, "backing");
>> +        qdict_del(bs->options, "backing");
>> +    }
>> +
>>      /* Remove child references from bs->options and bs->explicit_options.
>>       * Child options were already removed in bdrv_reopen_queue_child() */
>>      QLIST_FOREACH(child, &bs->children, next) {
>> @@ -3471,6 +3572,25 @@ void bdrv_reopen_commit(BDRVReopenState *reopen_state)
>>          qdict_del(bs->options, child->name);
>>      }
>>  
>> +    /*
>> +     * Change the backing file if a new one was specified. We do this
>> +     * after updating bs->options, so bdrv_refresh_filename() (called
>> +     * from bdrv_set_backing_hd()) has the new values.
>> +     */
>> +    if (change_backing_bs) {
>> +        BlockDriverState *overlay = bs;
>> +        /*
>> +         * Skip all links that point to an implicit node, if any
>> +         * (e.g. a commit filter node). We don't want to change those.
>> +         */
>> +        while (backing_bs(overlay) && backing_bs(overlay)->implicit) {
>> +            overlay = backing_bs(overlay);
>> +        }
>> +        if (new_backing_bs != backing_bs(overlay)) {
>> +            bdrv_set_backing_hd(overlay, new_backing_bs, &error_abort);
>
> I'm afraid we can't use &error_abort here because bdrv_attach_child()
> could still fail due to permission conflicts.
>
>> +        }
>> +    }
>> +
>>      bdrv_refresh_limits(bs, NULL);
>>  
>>      bdrv_set_perm(reopen_state->bs, reopen_state->perm,
>
> Hm... Does bdrv_set_perm() work correctly when between bdrv_check_perm()
> and here the graph was changed?
>
> Kevin