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

[Alberto Garcia]

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