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
