On 26.11.18 13:05, Max Reitz wrote:
> 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 <kw...@redhat.com>
>> ---
>> 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.
>
>> +
>> + 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)) {
Another thing I found while testing: I think this should only return
early if setting_flag is true. BDRV_O_INACTIVE will only be set on the
second pass. If you skip nodes with active parents unconditionally,
bs->drv->bdrv_inactivate() will not be called for any non-root BDS
(because bdrv_has_active_bds_parents() returns true for all non-root
BDSs during the first pass).
>> + 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.
Hm, well, no, at least not directly here. (Otherwise
bdrv_inactive_recurse() will not really recurse when it calls itself...)
But bdrv_inactive_all() could check that before invoking this function.
Ah, but then it is possible to still run into the exact bug you're
fixing here, because a node might inactivate a child that has another
parent still (which is inactivated later).
But still, I think bdrv_inactive_all() should skip non-root BDSs,
because calling bs->drv->bdrv_inactive() and parent->role->inactivate()
multiple times cannot be right.
Max
> Do drivers support multiple calls to .bdrv_in_activate() at all?
>
> Max
>
>> if (!setting_flag && bs->drv->bdrv_inactivate) {
>> ret = bs->drv->bdrv_inactivate(bs);
>> if (ret < 0) {
>>
>
>
signature.asc
Description: OpenPGP digital signature
