Re: [PATCH v2 1/3] block: Make bdrv_refresh_limits() non-recursive

[Hanna Reitz <hreitz@redhat.com>]

On 04.03.22 15:14, Kevin Wolf wrote:
> 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.

Finally some good news today :)