* [Qemu-devel] [PATCH] throttle: fix a qemu crash problem when calling blk_delete
@ 2017-10-19 13:16 sochin.jiang
2017-10-20 11:43 ` [Qemu-devel] [Qemu-block] " Alberto Garcia
0 siblings, 1 reply; 3+ messages in thread
From: sochin.jiang @ 2017-10-19 13:16 UTC (permalink / raw)
To: kwolf, jcody, pbonzini, mreitz
Cc: qemu-block, qemu-devel, sochin.jiang, eric.fangyi, subo7,
xieyingtai, lina.lulina, zhangshuai13, lizhengui
From: "sochin.jiang" <sochin.jiang@huawei.com>
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.
Signed-off-by: sochin.jiang <sochin.jiang@huawei.com>
---
block/io.c | 8 ++++++++
util/throttle.c | 6 +++---
2 files changed, 11 insertions(+), 3 deletions(-)
diff --git a/block/io.c b/block/io.c
index 0854e0f..c411a87 100644
--- a/block/io.c
+++ b/block/io.c
@@ -269,6 +269,10 @@ static void coroutine_fn bdrv_co_yield_to_drain(BlockDriverState *bs,
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;
+ }
+
if (qemu_in_coroutine()) {
bdrv_co_yield_to_drain(bs, false);
return;
diff --git a/util/throttle.c b/util/throttle.c
index b38e742..35a21fc 100644
--- a/util/throttle.c
+++ b/util/throttle.c
@@ -245,8 +245,6 @@ void throttle_timers_init(ThrottleTimers *tt,
/* destroy a timer */
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]);
+ }
}
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH] throttle: fix a qemu crash problem when calling blk_delete
2017-10-19 13:16 [Qemu-devel] [PATCH] throttle: fix a qemu crash problem when calling blk_delete sochin.jiang
@ 2017-10-20 11:43 ` Alberto Garcia
2017-10-21 8:02 ` sochin.jiang
0 siblings, 1 reply; 3+ messages in thread
From: Alberto Garcia @ 2017-10-20 11:43 UTC (permalink / raw)
To: sochin.jiang, kwolf, jcody, pbonzini, mreitz
Cc: lina.lulina, lizhengui, qemu-block, subo7, eric.fangyi,
zhangshuai13, qemu-devel
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH] throttle: fix a qemu crash problem when calling blk_delete
2017-10-20 11:43 ` [Qemu-devel] [Qemu-block] " Alberto Garcia
@ 2017-10-21 8:02 ` sochin.jiang
0 siblings, 0 replies; 3+ messages in thread
From: sochin.jiang @ 2017-10-21 8:02 UTC (permalink / raw)
To: Alberto Garcia, kwolf, jcody, pbonzini, mreitz
Cc: lina.lulina, lizhengui, qemu-block, subo7, eric.fangyi,
zhangshuai13, qemu-devel
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
>
> .
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2017-10-21 8:03 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-10-19 13:16 [Qemu-devel] [PATCH] throttle: fix a qemu crash problem when calling blk_delete sochin.jiang
2017-10-20 11:43 ` [Qemu-devel] [Qemu-block] " Alberto Garcia
2017-10-21 8:02 ` sochin.jiang
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.