Re: [Qemu-devel] [PATCH v2 01/13] block: Allow freezing BdrvChild links

[Alberto Garcia]

On Tue 12 Mar 2019 05:12:53 PM CET, Vladimir Sementsov-Ogievskiy wrote:
>> +    /* This function changes all links that point to top and makes
>> +     * them point to base. Check that none of them is frozen. */
>> +    QLIST_FOREACH(c, &top->parents, next_parent) {
>> +        if (c->frozen) {
>> +            goto exit;
>> +        }
>> +    }
>> +

> Hmm, looking at this I have a bit unrelated questions about
> bdrv_drop_intermediate:
>
> 1. if we fail on some iteration of QLIST_FOREACH_SAFE loop, we finish
> up with partly updated graph??

>    I don't see any kind of roll-back in this case.. Why it don't
>    operate like bdrv_replace_node, which firstly check all
>    permissions, and then update all parents in fail-free loop?

I think you're right. If you don't have something written already I
could try to do it.

> 2. And therefore, why we just don't call bdrv_replace_node(top, base,
> errp) from bdrv_drop_intermediate?

I think that would not call role->update_filename().

> 3. About updating inherits_from: is it possible for some reason that
> base->inherits_from points to some node inside removed backing chain,
> but bdrv_inherits_from_recursive(base, explicit_top) is false? In this
> case we'll finish with dead-pointer in base->inherits_from.

I'm not sure about that particular case, but I realize that if
base->inherits_from is a dead pointer then this can crash
bdrv_inherits_from_recursive(). This would not happen before because
inherits_from was only used in comparisons and never dereferenced.

> 4. Should bdrv_replace_node do something around inherits_from?

Probably, yes.

> Hmm, also, I have a related reproducer, which lead to crash, when use
> stream base as commit top:
  [...]
> But with your patches 01-04 applied, this reproducer gives
>
> {
>      "error": {
>          "class": "GenericError",
>          "desc": "Cannot change 'backing' link from '#block507' to 'img1'"
>      }
> }
>
> somewhere, and don't crash!!
>
> Could we consider it as a reason to apply 01-04 (may be with some
> updates, I didn't review them carefully yet) to v4.0 as a bug fix?

It seems that there's a merge request for the whole series already :)

> Also, related. We've faced a lot of related problems, trying to
> implement top-filter for stream job. It lead to failures in 30 iotest
> which want use parallel stream jobs, sharing a node to be base for one
> stream and top for another.

Yeah I imagine that that iotest needs a few changes if we block the base
node.

> It also touches the fact that stream stores pointer to base, but don't
> increase its refcount, so it's illegal pointer.

You mean in the StreamBlockJob structure? What's the problem exactly?

Berto