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

[Vladimir Sementsov-Ogievskiy]

13.03.2019 17:14, Alberto Garcia wrote:
> 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.

No, I don't.

> 
>> 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().

Honestly, I'm lost about filenames in the graph..

> 
>> 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 :)

Yes and that's cool)

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

I hope it anyway should be handled be freeze


-- 
Best regards,
Vladimir