Re: [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

.