Am 04.03.2022 um 13:44 hat Hanna Reitz geschrieben:
On 03.03.22 17:56, Kevin Wolf wrote:
Am 16.02.2022 um 11:53 hat Hanna Reitz geschrieben:
bdrv_refresh_limits() recurses down to the node's children. That does
not seem necessary: When we refresh limits on some node, and then
recurse down and were to change one of its children's BlockLimits, then
that would mean we noticed the changed limits by pure chance. The fact
that we refresh the parent's limits has nothing to do with it, so the
reason for the change probably happened before this point in time, and
we should have refreshed the limits then.
On the other hand, we do not have infrastructure for noticing that block
limits change after they have been initialized for the first time (this
would require propagating the change upwards to the respective node's
parents), and so evidently we consider this case impossible.
I like your optimistic approach, but my interpretation would have been
that this is simply a bug. ;-)
blockdev-reopen allows changing options that affect the block limits
(most importantly probably request_alignment), so this should be
propagated to the parents. I think we'll actually not see failures if we
forget to do this, but parents can either advertise excessive alignment
requirements or they may run into RMW when accessing the child, so this
would only affect performance. This is probably why nobody reported it
yet.
Ah, right, I forgot this for parents of parents... I thought the
block limits of a node might change if its children list changes, and
so we should bdrv_refresh_limits() when that children list changes,
but forgot that we really do need to propagate this up, right.
I mean the case that you mention is true as well. A few places do call
bdrv_refresh_limits() after changing the graph, but I don't know if it
covers all cases.
If this case is impossible, then we will not need to recurse down in
bdrv_refresh_limits(). Every node's limits are initialized in
bdrv_open_driver(), and are refreshed whenever its children change.
We want to use the childrens' limits to get some initial default, but
we can just take them, we do not need to refresh them.
I think even if we need to propagate to the parents, we still don't need
to propagate to the children because the children have already been
refreshed by whatever changed their options (like bdrv_reopen_commit()).
And parent limits don't influence the child limits at all.
So this patch looks good to me, just not the reasoning.
OK, so, uh, can we just drop these two paragraphs? (“On the other hand...”
and “If this case is impossible…”)
Or we could replace them with a note hinting at the potential bug that would
need to be fixed, e.g.
“
Consequently, we should actually propagate block limits changes upwards,
not downwards. That is a separate and pre-existing issue, though, and
so will not be addressed in this patch.
”
Ok, I'm replacing this in my tree.
Question is, if we at some point do propagate this upwards, won’t this cause
exactly the same problem that this patch is trying to get around, i.e. that
we might call bdrv_refresh_limits() on non-drained parent nodes?
Drain also propagates upwards, so at least those callers that drain the
node itself won't have the problem. And the other cases from the commit
messages look like they shouldn't have any parents.