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-vsement...@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.
