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

[sochin.jiang]

Thanks for replying.

Indeed, the problem comes from the caller. I guest

some code should be reconsidered in blk_remove_bs

and blk_delete, especially with throttling.

Secondly, when handling drive_del in hmp_drive_del,

throttle_timers_detach_aio_context() is called

first time in blk_remove_bs and again throughing blk_unref,

see below:

hmp_drive_del->
    blk_remove_bs->
        *throttle_timers_detach_aio_context*->
    ...
    blk_unref->
        blk_delete->
            blk_io_limits_disable->
                throttle_group_unregister_tgm->
                    throttle_timers_destroy->
                        *throttle_timers_detach_aio_context*->**...**


sochin

.      


On 2017/10/20 19:43, Alberto Garcia wrote:
> On Sun 24 Nov 2013 04:55:52 AM CET, sochin.jiang wrote:
>        ^^^^^^^^^^^
> I guess the date in your computer is wrong :-)
>
>> commit 7ca7f0 moves the throttling related part of the BDS life cycle
>> management to BlockBackend, adds call to
>> throttle_timers_detach_aio_context in blk_remove_bs.  commit 1606e
>> remove a block device from its throttle group in blk_delete by calling
>> blk_io_limits_disable, this fix an easily reproducible qemu crash. But
>> delete a BB without a BDS inserted could easily cause a qemu crash too
>> by calling bdrv_drained_begin in blk_io_limits_disable. Say, a simply
>> drive_add and then a drive_del command.
> Thanks, I can reproduce this easily by running QEMU and doing
>
>    drive_add 0 if=none,throttling.iops-total=5000
>
> followed by
>
>    drive_del none0
>
>>  void bdrv_drained_begin(BlockDriverState *bs)
>>  {
>> +    if (!bs) {
>> +        return;
>> +    }
>> +
>>      if (qemu_in_coroutine()) {
>>          bdrv_co_yield_to_drain(bs, true);
>>          return;
>> @@ -284,6 +288,10 @@ void bdrv_drained_begin(BlockDriverState *bs)
>>  
>>  void bdrv_drained_end(BlockDriverState *bs)
>>  {
>> +    if (!bs) {
>> +        return;
>> +    }
>> +
> I'd say that if someone calls bdrv_drained_begin() with a NULL pointer
> then the problem is in the caller...
>
>>  static void throttle_timer_destroy(QEMUTimer **timer)
>>  {
>> -    assert(*timer != NULL);
>> -
>>      timer_del(*timer);
>>      timer_free(*timer);
>>      *timer = NULL;
>> @@ -258,7 +256,9 @@ void throttle_timers_detach_aio_context(ThrottleTimers 
>> *tt)
>>      int i;
>>  
>>      for (i = 0; i < 2; i++) {
>> -        throttle_timer_destroy(&tt->timers[i]);
>> +        if (tt->timers[i]) {
>> +            throttle_timer_destroy(&tt->timers[i]);
>> +        }
>>      }
>>  }
> Why is this part necessary? In what situation you end up calling
> throttle_timers_detach_aio_context() twice?
>
> Berto
>
> .
>