On 29.03.22 11:55, Vladimir Sementsov-Ogievskiy wrote:
29.03.2022 11:54, Hanna Reitz wrote:
On 28.03.22 12:24, Vladimir Sementsov-Ogievskiy wrote:
28.03.2022 11:09, Hanna Reitz wrote:
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?
It should solve the same issue as your patch. You don't add
subtree_drain around every graph modification.. Or we already have it?
Well, I’m not saying it will solve every single bug, but draining in
stream_prepare() will at least mean that that is safe from basically
anything else, because it will prevent concurrent automatic graph
changes (e.g. because of jobs finishing), and QMP commands shouldn’t
be executed in drained sections either (when they do, it’s wrong, but
that seems to occur only extremely rarely). Draining alone should
make a place safe, it isn’t a lock that all sides need to take.
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.
Same way, with draining solution you should make a subtree-drain for
every graph change operation.
Since we don’t have any lock yet, draining is the de-facto way we use
to forbid concurrent graph modifications. I’m not saying we use it
correctly and thoroughly, but it is what we do right now.
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.
Yes that's seems an alternative to global lock, that doesn't block
the whole graph. Still, I don't think that is bad to lock the whole
graph for graph modificaiton, as modification should be rare and fast.
Fair enough.
Another thought: does subtree-drain really drain the whole
connectivity component of the graph?
imagine something like this:
[A] [ C ]
| | |
v v v
[ B ] [ D ]
If we do subtree drain at node A, this will drain B and C, but not D..
Imagine, some another job is attached to node D, and it will start
drained section too. So, for example both jobs will share drained
section on node C. That doesn't seem save, and draining is not a lock.
So, if we are going to rely on drained section as on lock, that
isolates graph modifications from each other, we should drain the
whole connectivity component of the graph.
The drained section is not a lock, but if the other job is only
attached to node D, it won’t change node C. It might change the link
from C to D, but that doesn’t concern the job that is concerned about
A and B. Overlapping drains are fine.
OK. Maybe it works. It's just not obvious to me that subtree_drain
works good in all cases. And global graph-modification-lock should
obviously work.
Next, I'm not relly sure that two jobs can simultaneusly enter
drained section and do graph modifications. What prevents this?
Assume two block-stream jobs reaches their finish simultaneously and
go to subtree-drain. That just means that job_pause will be called
for both jobs.. But what that means for the block-stream jobs that
is in bdrv_subtree_drained_beeing() call in stream_prepare()? Seems
nothing? Then both jobs will start graph modification process
simultaneously and can interleave on any yield point (for exmaple
rewriting backing_file in qcow2 metadata).
So I don’t think that scenario can really happen, because the stream
job freezes the chain between above_base and top, so you can’t really
have two simultaneous stream jobs that cause problems between each
other.
They cause problem on the boundary, as base of one stream job may be
top of another, and we have also a filter, that should be
inserted/removed at some moment. As I remember, that's the problematic
case in 030..
Furthermore, the prepare() functions are run in the main thread, so
the only real danger is actually that draining around the actual
graph modification (bdrv_set_backing_hd()) causes another block job
to finish, modifying the block graph. But then that job will also
actually finish, because it’s all in the main thread.
It is true that child_job_drained_poll() says that job that are about
to prepare() are quiesced, but I don’t think that’s a problem, given
that all jobs in that state run in the main thread.
Another reason, why I think that subtree drain is a wrong tool, as I
said, is extra IO-stop.
I know and agree, but that’s an optimization question.
Imaging the following graph:
[A]
|
v
[B] [C]
| |
v v
[base]
If we want to rebase A onto base, we actually need only stop IO
requests in A and B. Why C should suffer from this graph
modification? IO requests produced by C, and that are living in C
and in base don't intersect with rebasing A on base process in any way.
====
Actually, I'm not strictly against your patch, and believe that it
fixes the problem in most cases. And it's probably OK in short term.
The only real doubt on including it now is that drained sections
sometimes lead to dead locks, and is it possible that we now fix the
bug that happens only in iotest 30 (or is it reported somewhere?)
and risking to introduce some dead-lock?
Saying that the example in 030 is contrived would mean we
could/should re-include the base into the list of nodes that belong
to the stream job, which would simply disallow the case in 030 that’s
being tested and fails.
Then we don’t need a subtree drain, and the plain drain in
bdrv_set_backing_hd() would be fine.
Seems that if in some code it's safe to call drained_begin(), it
should be safe to call subtree_drained_begin(). And if it trigger
some deadlock, it just shows some another bug.. Is it worth fixing
now, near to 7.0 release? We live with this bug for years.. Or
something changed that I missed?
I mean... I can understand your concern that adding a subtree drain
has performance implications (when a stream job ends, which shouldn’t
be often). But I’m not sure whether I should share the deadlock
concern. Sounds like a sad state of affairs if I can’t just drain
something when I need it to be drained.
I wasn’t aware of this bug, actually. Now I am, and I feel rather
uncomfortable living with a use-after-free bug, because that’s on the
worse end of the bug spectrum.
OK, I don't know:) And others are silent. Agree that global-lock
solution is not for 7.0 anyway. And this one is simple enough. So,
take my
Acked-by: Vladimir Sementsov-Ogievskiy <v.sementsov...@mail.ru>
Thanks!
A global lock solution sounds good to me for 7.1+, and it even provides
a solution to solving the problem with QMP commands being executed while
other main-thread code is running. (I mean, the QMP command would still
be executed, but at least concurrent graph changes would be impossible.)
