Re: [Qemu-block] [PATCH for-3.1 1/2] block: Don't inactivate children before parents

[Max Reitz]

On 26.11.18 13:33, Kevin Wolf wrote:
> Am 26.11.2018 um 13:05 hat Max Reitz geschrieben:
>> On 26.11.18 12:28, Kevin Wolf wrote:
>>> bdrv_child_cb_inactivate() asserts that parents are already inactive
>>> when children get inactivated. This precondition is necessary because
>>> parents could still issue requests in their inactivation code.
>>>
>>> When block nodes are created individually with -blockdev, all of them
>>> are monitor owned and will be returned by bdrv_next() in an undefined
>>> order (in practice, in the order of their creation, which is usually
>>> children before parents), which obviously fails the assertion.
>>>
>>> This patch fixes the ordering by skipping nodes with still active
>>> parents in bdrv_inactivate_recurse() because we know that they will be
>>> covered by recursion when the last active parent becomes inactive.
>>>
>>> Signed-off-by: Kevin Wolf <address@hidden>
>>> ---
>>>  block.c | 22 ++++++++++++++++++++++
>>>  1 file changed, 22 insertions(+)
>>>
>>> diff --git a/block.c b/block.c
>>> index 5ba3435f8f..0569275e31 100644
>>> --- a/block.c
>>> +++ b/block.c
>>> @@ -4612,6 +4612,22 @@ void bdrv_invalidate_cache_all(Error **errp)
>>>      }
>>>  }
>>>  
>>> +static bool bdrv_has_active_bds_parent(BlockDriverState *bs)
>>> +{
>>> +    BdrvChild *parent;
>>> +
>>> +    QLIST_FOREACH(parent, &bs->parents, next_parent) {
>>> +        if (parent->role->parent_is_bds) {
>>> +            BlockDriverState *parent_bs = parent->opaque;
>>> +            if (!(parent_bs->open_flags & BDRV_O_INACTIVE)) {
>>> +                return true;
>>> +            }
>>> +        }
>>> +    }
>>
>> Now I see why you say this might make sense as a permission.
> 
> You do? To be honest, now that I found this solution, I don't think a
> permission makes much sense any more, because you would have the same
> loop, and you would only be checking a different flag in the end.
> 
>>> +
>>> +    return false;
>>> +}
>>> +
>>>  static int bdrv_inactivate_recurse(BlockDriverState *bs,
>>>                                     bool setting_flag)
>>>  {
>>> @@ -4622,6 +4638,12 @@ static int bdrv_inactivate_recurse(BlockDriverState 
>>> *bs,
>>>          return -ENOMEDIUM;
>>>      }
>>>  
>>> +    /* Make sure that we don't inactivate a child before its parent.
>>> +     * It will be covered by recursion from the yet active parent. */
>>> +    if (bdrv_has_active_bds_parent(bs)) {
>>> +        return 0;
>>> +    }
>>> +
>>
>> Hm.  Wouldn't it make more sense to always return early when there are
>> any BDS parents?  Because if there are any BDS parents and none of them
>> are active (anymore), then this child will have been inactivated
>> already, and we can save ourselves the trouble of going through the rest
>> of the function again.
> 
> I don't quite follow... If we always return early no matter whether
> there is an active parent, who will have inactivated the child?
> 
> After trying to write up why you're wrong, I think there are two cases
> and both of us have a point:
> 
> 1. bdrv_next() enumerates the child node first and then the last BDS
>    parent. This is what this patch fixes.
> 
>    It will inactivate the child exactly once, at the time that the last
>    parent has become inactive (and recursively calls this function for
>    each of its children). If you remove that one inactivation, too,
>    children won't be inactivated at all.
> 
> 2. bdrv_next() enumerates the last BDS parent first and then the child.
>    This is unlikely and might even be impossible today, but once we
>    expose bdrv_reopen() in QMP and let the user reconfigure the edges,
>    it probably becomes possible.

blockdev-snapshot exists today.

>    In this case, even after my patch we inactivate drivers twice. Maybe
>    we should just return early if BDRV_O_INACTIVE is already set. What
>    makes me kind of unsure is that we already test for this flag, but
>    only for part of the function.
> 
>    Any ideas how to test this? Can we create such a situation today?

As I wrote in my second mail just now, I think bdrv_inactivate_all()
needs to check this.

See attached diff to 234, but I don't know whether we can really test
this, as there is no failure when .bdrv_inactivate() is called multiple
times.  (What I've done is add debugging code to see that it is called
multiple times).

Max

>> Do drivers support multiple calls to .bdrv_in_activate() at all?
> 
> They were probably not designed for that... Not sure if there was ever a
> commit where we used to call them multiple times without failing the
> assertion first.
> 
> Kevin
>

diff
Description: Text document

signature.asc
Description: OpenPGP digital signature