Re: [PATCH v2] block/stream: Drain subtree around graph change

[Hanna Reitz <hreitz@redhat.com>]

On 28.03.22 09:44, Hanna Reitz wrote:
> On 25.03.22 17:37, Vladimir Sementsov-Ogievskiy wrote:
>> 24.03.2022 17:09, Hanna Reitz wrote:
>>> When the stream block job cuts out the nodes between top and base in
>>> stream_prepare(), it does not drain the subtree manually; it fetches 
>>> the
>>> base node, and tries to insert it as the top node's backing node with
>>> bdrv_set_backing_hd().  bdrv_set_backing_hd() however will drain, 
>>> and so
>>> the actual base node might change (because the base node is actually 
>>> not
>>> part of the stream job) before the old base node passed to
>>> bdrv_set_backing_hd() is installed.
>>>
>>> This has two implications:
>>>
>>> First, the stream job does not keep a strong reference to the base 
>>> node.
>>> Therefore, if it is deleted in bdrv_set_backing_hd()'s drain (e.g.
>>> because some other block job is drained to finish), we will get a
>>> use-after-free.  We should keep a strong reference to that node.
>>>
>>> Second, even with such a strong reference, the problem remains that the
>>> base node might change before bdrv_set_backing_hd() actually runs 
>>> and as
>>> a result the wrong base node is installed.
>>
>> Hmm.
>>
>> So, we don't really need a strong reference, as if it helps to avoid 
>> some use-after-free, it means that we'll finish up with wrong block 
>> graph..
>
> Sure.  But I found it better style to strongly reference a node while 
> it’s used.  I’d rather have an outdated block graph (as in: A node 
> that was supposed to disappear would still be in use) than a 
> use-after-free.
>
>> Graph modifying operations must be somehow isolated from each other.
>>
>>>
>>> Both effects can be seen in 030's TestParallelOps.test_overlapping_5()
>>> case, which has five nodes, and simultaneously streams from the middle
>>> node to the top node, and commits the middle node down to the base 
>>> node.
>>> As it is, this will sometimes crash, namely when we encounter the
>>> above-described use-after-free.
>>>
>>> Taking a strong reference to the base node, we no longer get a crash,
>>> but the resuling block graph is less than ideal: The expected result is
>>> obviously that all middle nodes are cut out and the base node is the
>>> immediate backing child of the top node.  However, if stream_prepare()
>>> takes a strong reference to its base node (the middle node), and then
>>> the commit job finishes in bdrv_set_backing_hd(), supposedly dropping
>>> that middle node, the stream job will just reinstall it again.
>>>
>>> Therefore, we need to keep the whole subtree drained in
>>> stream_prepare(), so that the graph modification it performs is
>>> effectively atomic, i.e. that the base node it fetches is still the 
>>> base
>>> node when bdrv_set_backing_hd() sets it as the top node's backing node.
>>
>> Emanuele has similar idea of isolating graph changes from each other 
>> by subtree-drain.
>>
>> If I understand correctly the idea is that we'll drain all other 
>> block jobs, so the wouldn't do their block-graph modification during 
>> drained section. So, we can safely modify the graph.
>>
>> I don't like this idea:
>>
>> 1. drained section = stop IO. But we don't need to stop IO in the 
>> whole subtree to do a needed block-graph modification.
>
> If you mean to say that draining just the single node should be 
> sufficient, I’ll be happy to change it.
>
> Not sure which node, though, because I’d think it would be `base`, but 
> to safely fetch it I’d need to drain it, which seems to bite itself in 
> the tail.  That’s why I went for a subtree drain from `above_base`.
>
>> 2. Drained section is not a lock, several clients may drain same set 
>> of nodes.. So we exploit the fact that concurrent clients will be 
>> paused by drained section and don't proceed to graph-modification 
>> code.. But are we sure that block-jobs are (and will be?) the only 
>> concurrent block-graph modifying clients? Can qmp commands interleave 
>> somehow?
>
> They can under very specific circumstances and that’s a bug.  See 
> https://lists.nongnu.org/archive/html/qemu-block/2022-03/msg00582.html .
>
>> Can some jobs from other subtree start a block-graph modification 
>> that touches our subtree?
>
> That would be wrong.  A block job shouldn’t change nodes it doesn’t 
> own; stream doesn’t own the base, but it also doesn’t change it, it 
> only needs to have the top node point to it.
>
>> If go this way, that would be more safe to drain the whole 
>> block-graph on any block-graph modification..
>>
>> I think we'd better have a separate global mechanism for isolating 
>> graph modifications. Something like a global co-mutex or queue, where 
>> clients waits for their turn in block graph modifications.
>>
>> Here is my old proposal on that topic: 
>> https://patchew.org/QEMU/20201120161622.1537-1-vsementsov@virtuozzo.com/
>
> That would only solve the very specific issue in 030, right?  The 
> stream job isn’t protected from any graph modifications but those 
> coming from mirror.  Might be a solution going forward (I didn’t look 
> closer at it at the time, given I saw you had a discussion with 
> Kevin), if we lock every graph change operation (though a global lock 
> honestly doesn’t sound strictly better than draining subsections of 
> the graph, both have their drawbacks), but that doesn’t look like it’d 
> be something for 7.1.

I wonder whether we could have a short-term version of 
`BdrvChild.frozen` that’s a coroutine mutex.  If `.frozen` is set, you 
just can’t change the graph, and you also can’t wait, so that’s just an 
error.  But if `.frozen_lock` is set, you can wait on it. Here, we’d 
keep `.frozen` set for all links between top and above_base, and then in 
prepare() take `.frozen_lock` on the link between above_base and base.