[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH for-3.1 1/2] block: Don't inactivate children be
From: |
Max Reitz |
Subject: |
Re: [Qemu-devel] [PATCH for-3.1 1/2] block: Don't inactivate children before parents |
Date: |
Mon, 26 Nov 2018 13:44:32 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.3.0 |
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