Re: [Qemu-block] [PATCH v2] throttle: fix a qemu crash problem when calling blk_delete

[Alberto Garcia]

On Thu 09 Nov 2017 06:12:10 PM CET, Stefan Hajnoczi wrote:
>> diff --git a/block/block-backend.c b/block/block-backend.c
>> index 45d9101..39c7cca 100644
>> --- a/block/block-backend.c
>> +++ b/block/block-backend.c
>> @@ -341,7 +341,7 @@ static void blk_delete(BlockBackend *blk)
>>      assert(!blk->name);
>>      assert(!blk->dev);
>>      if (blk->public.throttle_group_member.throttle_state) {
>> -        blk_io_limits_disable(blk);
>> +        throttle_group_unregister_tgm(&blk->public.throttle_group_member);
>
> The following assertions fail without the drain when there are pending
> requests:
>
>   void throttle_group_unregister_tgm(ThrottleGroupMember *tgm)
>   {
>       ThrottleState *ts = tgm->throttle_state;
>       ThrottleGroup *tg = container_of(ts, ThrottleGroup, ts);
>       ThrottleGroupMember *token;
>       int i;
>
>       if (!ts) {
>           /* Discard already unregistered tgm */
>           return;
>       }
>
>       assert(tgm->pending_reqs[0] == 0 && tgm->pending_reqs[1] == 0);
>       assert(qemu_co_queue_empty(&tgm->throttled_reqs[0]));
>       assert(qemu_co_queue_empty(&tgm->throttled_reqs[1]));
>
> A safer approach is making blk_io_limits_disable(blk) skip the
> draining when blk_bs(blk) == NULL.  That is the only case where we are
> 100% sure that there are no pending requests.

I think so too.

And as I said in a previous e-mail I don't know if we should have a
valid blk->public.throttle_group_member.throttle_state with no timers at
all. I'd rather attach the default AioContext while the BDS is removed.

I'll try to prepare a patch with all of this together.

Berto