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

[Alberto Garcia <berto@igalia.com>]

(sorry I accidentally sent an incomplete reply a few minutes ago)

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, and it doesn't really affect any of the test
cases that I had written.

>> +    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.

I agree, but I didn't want to mess with that yet. I added a comment
explaining that.

>> +    /*
>> +     * 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?

I don't think the new backing file needs to support backing files
itself, one could e.g. replace a backing file (or even a longer chain)
with a raw file containing the same data.

>> +    /*
>> +     * 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.

But those permissions should have been checked already in
bdrv_reopen_prepare(). This function cannot return an error.

>>      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?

I think you're right, bdrv_check_perm() might have been called on the
old backing file and it's not followed by set or abort if we change it.

I'll have a look at this.

Berto