Ah seems I understand what you mean.
One of my arguments is that "drain" - is not a lock against other
clients who want to modify the graph. Because, drained section allows
nested drained sections.
And you try to solve it, by draining more things, this way, we'll drain
also the job, which is a possible client, who may want to modify the
graph in parallel.
So, in other words, when we want to modify the graph, we drain the whole
connectivity component of the graph. And we think that we are safe from
other graph modifications because all related jobs are drained.
Interesting, is that possible that some not drained job from another
connectivity component will want to connect some node from our drained
component?
You mean another job or whathever calling bdrv_find_node() on a random
graph? Yes that is not protected. But can this happen?
That's the question. What are the invariants here? Can anything happen?
I just still feel that draining is a wrong mechanism to avoid
interaction with other clients who want to modify the graph, because:
1. we stop the whole IO on all subgraph which is not necessary
2. draining is not a mutex, it allows nesting and it's ok when two
different clients drain same nodes. Draining is just a requirement to do
no IO at these nodes.
And in your way, it seems that to be absolutely safe we'll need to drain
everything..
In my feeling it's better to keep draining what it is now: requirement
to have no IO requests. And to isolate graph modifications from each
other make a new synchronization mechanism, something like a global
queue, where clients who want to get an access to graph modifications
wait for their turn.
This is a matter of definitions. Subtree drains can theoretically work,
I managed to answer to my own doubts in the last email I sent.
Yes, there is still some completely random case like the one I wrote
above, but I think it is more a matter of what we want to use and what
meaning we want to give to drains.
Global queue is what Kevin proposes, I will try to implement it.
As I understand:
You want to make drained section to be a kind of lock, so that if we
take this lock, we can modify the graph and we are sure that no other
client will modify it in parallel.
Yes
But drained sections can be nested. So to solve the problem you try to
drain more nodes: include subtree for example, or may be we need to
drain the whole graph connectivity component, or (to be more safe) the
whole block layer (to be sure that during drained section in one
connectivity component some not-drained block-job from another
connectivity component will not try to attach some node from our drained
connectivity component)..
I still feel that draining is wrong tool for isolating graph modifying
operations from each other:
1. Drained sections can be nested, and natively that's not a kind of
lock. That's just a requirement to have no IO requests. There may be
several clients that want this condition on same set of nodes.
2. Blocking IO on the whole connected subgraph or even on the whole
block layer graph is not necessary, so that's an extra blocking.
Could we instead do the following:
1. Keep draining as is - a mechanism to stop IO on some nodes
2. To isolate graph-modifying operations implement another mechanism:
something like a global queue, where clients wait until they gen an
access to modify block layer.
This way, any graph modifying process would look like this:
1. drained_begin(only where necessary, not the whole subgraph in general)
2. wait in the global queue
3. Ok, now we can do all the modifications
4. Kick the global queue, so that next client will get an access
5. drained_end()
Please give a look at what Kevin (described by me) proposed. I think
it's the same as you are suggesting. I am pasting it below.
I will try to implement this and see if it is doable or not.
I think the advantage of drains is that it isn't so complicated and
doesn't add any complication to the existing code.
But we'll see how it goes with this global queue.
His idea is to replicate what blk_wait_while_drained() currently does
but on a larger scale. It is something in between this subtree_drains
logic and a rwlock.
Basically if I understood correctly, we could implement
bdrv_wait_while_drained(), and put in all places where we would put a
read lock: all the reads to ->parents and ->children.
This function detects if the bdrv is under drain, and if so it will stop
and wait that the drain finishes (ie the graph modification).
On the other side, each write would just need to drain probably both
nodes (simple drain), to signal that we are modifying the graph. Once
bdrv_drained_begin() finishes, we are sure all coroutines are stopped.
Once bdrv_drained_end() finishes, we automatically let all coroutine
restart, and continue where they left off.
Seems a good compromise between drains and rwlock. What do you think?
I am not sure how painful it will be to implement though.