Re: [RFC PATCH 7/8] block: use the new _change_ API instead of _can_set_ and _set_

[Emanuele Giuseppe Esposito]

Am 18/07/2022 um 18:39 schrieb Paolo Bonzini:
> On 7/12/22 23:19, Emanuele Giuseppe Esposito wrote:
>> diff --git a/block/block-backend.c b/block/block-backend.c
>> index 674eaaa2bf..6e90ac3a6a 100644
>> --- a/block/block-backend.c
>> +++ b/block/block-backend.c
>> @@ -2184,8 +2184,12 @@ static int blk_do_set_aio_context(BlockBackend
>> *blk, AioContext *new_context,
>>           bdrv_ref(bs);
>>             if (update_root_node) {
>> -            ret = bdrv_child_try_set_aio_context(bs, new_context,
>> blk->root,
>> -                                                 errp);
>> +            /*
>> +             * update_root_node MUST be false for
>> blk_root_set_aio_ctx_commit(),
>> +             * as we are already in the commit function of a
>> transaction.
>> +             */
>> +            ret = bdrv_child_try_change_aio_context(bs, new_context,
>> blk->root,
>> +                                                    errp);
>>               if (ret < 0) {
>>                   bdrv_unref(bs);
>>                   return ret;
> 
> 
> Looking further at blk_do_set_aio_context:
> 
>         if (tgm->throttle_state) {
>             bdrv_drained_begin(bs);
>             throttle_group_detach_aio_context(tgm);
>             throttle_group_attach_aio_context(tgm, new_context);
>             bdrv_drained_end(bs);
>         }
> 
> Perhaps the drained_begin/drained_end pair can be moved to
> blk_set_aio_context?  It shouldn't be needed from the change_aio_ctx
> callback, because bs is already drained.  If so, blk_do_set_aio_context
> would become just:
> 
>      if (tgm->throttle_state) {
>          throttle_group_detach_aio_context(tgm);
>          throttle_group_attach_aio_context(tgm, new_context);
>      }
>      blk->ctx = new_context;
> 
> and blk_set_aio_context would be something like:
> 
>     if (bs) {
>         bdrv_ref(bs);
>         ret = bdrv_child_try_set_aio_context(bs, new_context, blk->root,
>                                              errp);
>         if (ret < 0) {
>             goto out_no_drain;
>         }
>         bdrv_drained_begin(bs);
>     }
>     ret = blk_do_set_aio_context(blk, new_context, errp);
>     if (bs) {
>         bdrv_drained_end(bs);
> out_no_drain;
>         bdrv_unref(bs);
>     }
>     return ret;
> 
> Paolo
> 

This is another example on how things here do not take into account many
other use cases outside the common ones: if I use the above suggestion,
test-block-iothread fails.

The reason why it fails is simple: now we drain regardless of
tgm->throttle_state being true or false. And this requires yet another
aiocontext lock.

Which means that if we are calling blk_set_aio_context where the bs
exists and tgm->throttle_state is true, we have a bug.

Test is test_attach_blockjob and function is line 435
blk_set_aio_context(blk, ctx, &error_abort);

The reason is that bs is first switched to the new aiocontext and then
we try to drain it without holding the lock.

Wrapping the new drains in aio_context_acquire/release(new_context) is
not so much helpful either, since apparently the following
blk_set_aio_context makes aio_poll() hang.
I am not sure why, any ideas?


Code:

static int blk_do_set_aio_context(BlockBackend *blk, AioContext
*new_context,
                                  Error **errp)
{
    BlockDriverState *bs = blk_bs(blk);
    ThrottleGroupMember *tgm = &blk->public.throttle_group_member;

    if (bs) {
        bdrv_ref(bs);

        if (tgm->throttle_state) {
            throttle_group_detach_aio_context(tgm);
            throttle_group_attach_aio_context(tgm, new_context);
        }

        bdrv_unref(bs);
    }

    blk->ctx = new_context;
    return 0;
}


int blk_set_aio_context(BlockBackend *blk, AioContext *new_context,
                        Error **errp)
{
    BlockDriverState *bs = blk_bs(blk);
    int ret;
    GLOBAL_STATE_CODE();

    if (bs) {
        bdrv_ref(bs);
        ret = bdrv_child_try_change_aio_context(bs, new_context, blk->root,
                                                errp);
        if (ret < 0) {
            goto out_no_drain;
        }
        if (new_context != qemu_get_aio_context()) {
            aio_context_acquire(new_context);
        }
        bdrv_drained_begin(bs); // <-------------------- hangs here!
    }

    ret = blk_do_set_aio_context(blk, new_context, errp);

    if (bs) {
        bdrv_drained_end(bs);
        if (new_context != qemu_get_aio_context()) {
            aio_context_release(new_context);
        }
out_no_drain:
        bdrv_unref(bs);
    }

    return ret;
}

Emanuele