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

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

[sochin jiang]

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.

This patch removes draining BDS by calling throttle_group_unregister_tgm
directly instead of blk_io_limits_disable, leaves draining operation to
blk_remove_bs in case that there is no BDS inserted. Futhermore, make sure
throttle timers are initialized or attached before throttle_timers_destroy
is called in throttle_group_unregister_tgm.

Signed-off-by: sochin jiang <sochin.jiang@huawei.com>
---
 block/block-backend.c   | 2 +-
 block/throttle-groups.c | 4 +++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index 45d9101..39c7cca 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -341,7 +341,7 @@ static void blk_delete(BlockBackend *blk)
     assert(!blk->name);
     assert(!blk->dev);
     if (blk->public.throttle_group_member.throttle_state) {
-        blk_io_limits_disable(blk);
+        throttle_group_unregister_tgm(&blk->public.throttle_group_member);
     }
     if (blk->root) {
         blk_remove_bs(blk);
diff --git a/block/throttle-groups.c b/block/throttle-groups.c
index b291a88..c5f9af3 100644
--- a/block/throttle-groups.c
+++ b/block/throttle-groups.c
@@ -576,7 +576,9 @@ void throttle_group_unregister_tgm(ThrottleGroupMember *tgm)
 
     /* remove the current tgm from the list */
     QLIST_REMOVE(tgm, round_robin);
-    throttle_timers_destroy(&tgm->throttle_timers);
+    if (throttle_timers_are_initialized(&tgm->throttle_timers)) {
+        throttle_timers_destroy(&tgm->throttle_timers);
+    }
     qemu_mutex_unlock(&tg->lock);
 
     throttle_group_unref(&tg->ts);
-- 
1.8.3.1

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

[Alberto Garcia]

On Tue 24 Oct 2017 05:33:51 AM CEST, sochin jiang wrote:
> --- a/block/throttle-groups.c
> +++ b/block/throttle-groups.c
> @@ -576,7 +576,9 @@ void throttle_group_unregister_tgm(ThrottleGroupMember *tgm)
>  
>      /* remove the current tgm from the list */
>      QLIST_REMOVE(tgm, round_robin);
> -    throttle_timers_destroy(&tgm->throttle_timers);
> +    if (throttle_timers_are_initialized(&tgm->throttle_timers)) {
> +        throttle_timers_destroy(&tgm->throttle_timers);
> +    }
>      qemu_mutex_unlock(&tg->lock);
>  
>      throttle_group_unref(&tg->ts);

I don't know what the rest of the people think, but I'm not completely
convinced that it's a good idea to have an active ThrottleState inside a
ThrottleGroupMember without timers.

Perhaps in blk_remove_bs() after detaching the AioContext from the BDS
we can attach the default one (what you would get with
blk_get_aio_context()).

On the other hand I think that Manos's series to remove the legacy
throttling code gets rid of BlockBackend.ThrottleGroupMember completely.

Berto

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

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1320 bytes --]

On Wed, Oct 25, 2017 at 10:40:47AM +0200, Alberto Garcia wrote:
> On Tue 24 Oct 2017 05:33:51 AM CEST, sochin jiang wrote:
> > --- a/block/throttle-groups.c
> > +++ b/block/throttle-groups.c
> > @@ -576,7 +576,9 @@ void throttle_group_unregister_tgm(ThrottleGroupMember *tgm)
> >  
> >      /* remove the current tgm from the list */
> >      QLIST_REMOVE(tgm, round_robin);
> > -    throttle_timers_destroy(&tgm->throttle_timers);
> > +    if (throttle_timers_are_initialized(&tgm->throttle_timers)) {
> > +        throttle_timers_destroy(&tgm->throttle_timers);
> > +    }
> >      qemu_mutex_unlock(&tg->lock);
> >  
> >      throttle_group_unref(&tg->ts);
> 
> I don't know what the rest of the people think, but I'm not completely
> convinced that it's a good idea to have an active ThrottleState inside a
> ThrottleGroupMember without timers.
> 
> Perhaps in blk_remove_bs() after detaching the AioContext from the BDS
> we can attach the default one (what you would get with
> blk_get_aio_context()).
> 
> On the other hand I think that Manos's series to remove the legacy
> throttling code gets rid of BlockBackend.ThrottleGroupMember completely.

What is the status of Manos' series?

It would be good to merge it and then consider currently open throttling
bugs again.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

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

[Manos Pitsidianakis]

[-- Attachment #1: Type: text/plain, Size: 1566 bytes --]

On Fri, Nov 03, 2017 at 02:26:33PM +0000, Stefan Hajnoczi wrote:
>On Wed, Oct 25, 2017 at 10:40:47AM +0200, Alberto Garcia wrote:
>> On Tue 24 Oct 2017 05:33:51 AM CEST, sochin jiang wrote:
>> > --- a/block/throttle-groups.c
>> > +++ b/block/throttle-groups.c
>> > @@ -576,7 +576,9 @@ void throttle_group_unregister_tgm(ThrottleGroupMember *tgm)
>> >
>> >      /* remove the current tgm from the list */
>> >      QLIST_REMOVE(tgm, round_robin);
>> > -    throttle_timers_destroy(&tgm->throttle_timers);
>> > +    if (throttle_timers_are_initialized(&tgm->throttle_timers)) {
>> > +        throttle_timers_destroy(&tgm->throttle_timers);
>> > +    }
>> >      qemu_mutex_unlock(&tg->lock);
>> >
>> >      throttle_group_unref(&tg->ts);
>>
>> I don't know what the rest of the people think, but I'm not completely
>> convinced that it's a good idea to have an active ThrottleState inside a
>> ThrottleGroupMember without timers.
>>
>> Perhaps in blk_remove_bs() after detaching the AioContext from the BDS
>> we can attach the default one (what you would get with
>> blk_get_aio_context()).
>>
>> On the other hand I think that Manos's series to remove the legacy
>> throttling code gets rid of BlockBackend.ThrottleGroupMember completely.
>
>What is the status of Manos' series?
>
>It would be good to merge it and then consider currently open throttling
>bugs again.
>

I have been busy with schoolwork the past weeks, but I'm close to 
finishing some changes Kevin recommended for this series. I hope they 
will be ready soon.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2398 bytes --]

On Tue, Oct 24, 2017 at 11:33:51AM +0800, sochin jiang wrote:
> 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.
> 
> This patch removes draining BDS by calling throttle_group_unregister_tgm
> directly instead of blk_io_limits_disable, leaves draining operation to
> blk_remove_bs in case that there is no BDS inserted. Futhermore, make sure
> throttle timers are initialized or attached before throttle_timers_destroy
> is called in throttle_group_unregister_tgm.
> 
> Signed-off-by: sochin jiang <sochin.jiang@huawei.com>
> ---
>  block/block-backend.c   | 2 +-
>  block/throttle-groups.c | 4 +++-
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/block/block-backend.c b/block/block-backend.c
> index 45d9101..39c7cca 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -341,7 +341,7 @@ static void blk_delete(BlockBackend *blk)
>      assert(!blk->name);
>      assert(!blk->dev);
>      if (blk->public.throttle_group_member.throttle_state) {
> -        blk_io_limits_disable(blk);
> +        throttle_group_unregister_tgm(&blk->public.throttle_group_member);

The following assertions fail without the drain when there are pending
requests:

  void throttle_group_unregister_tgm(ThrottleGroupMember *tgm)
  {
      ThrottleState *ts = tgm->throttle_state;
      ThrottleGroup *tg = container_of(ts, ThrottleGroup, ts);
      ThrottleGroupMember *token;
      int i;

      if (!ts) {
          /* Discard already unregistered tgm */
          return;
      }

      assert(tgm->pending_reqs[0] == 0 && tgm->pending_reqs[1] == 0);
      assert(qemu_co_queue_empty(&tgm->throttled_reqs[0]));
      assert(qemu_co_queue_empty(&tgm->throttled_reqs[1]));

A safer approach is making blk_io_limits_disable(blk) skip the draining
when blk_bs(blk) == NULL.  That is the only case where we are 100% sure
that there are no pending requests.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

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

[Alberto Garcia]

On Thu 09 Nov 2017 06:12:10 PM CET, Stefan Hajnoczi wrote:
>> diff --git a/block/block-backend.c b/block/block-backend.c
>> index 45d9101..39c7cca 100644
>> --- a/block/block-backend.c
>> +++ b/block/block-backend.c
>> @@ -341,7 +341,7 @@ static void blk_delete(BlockBackend *blk)
>>      assert(!blk->name);
>>      assert(!blk->dev);
>>      if (blk->public.throttle_group_member.throttle_state) {
>> -        blk_io_limits_disable(blk);
>> +        throttle_group_unregister_tgm(&blk->public.throttle_group_member);
>
> The following assertions fail without the drain when there are pending
> requests:
>
>   void throttle_group_unregister_tgm(ThrottleGroupMember *tgm)
>   {
>       ThrottleState *ts = tgm->throttle_state;
>       ThrottleGroup *tg = container_of(ts, ThrottleGroup, ts);
>       ThrottleGroupMember *token;
>       int i;
>
>       if (!ts) {
>           /* Discard already unregistered tgm */
>           return;
>       }
>
>       assert(tgm->pending_reqs[0] == 0 && tgm->pending_reqs[1] == 0);
>       assert(qemu_co_queue_empty(&tgm->throttled_reqs[0]));
>       assert(qemu_co_queue_empty(&tgm->throttled_reqs[1]));
>
> A safer approach is making blk_io_limits_disable(blk) skip the
> draining when blk_bs(blk) == NULL.  That is the only case where we are
> 100% sure that there are no pending requests.

I think so too.

And as I said in a previous e-mail I don't know if we should have a
valid blk->public.throttle_group_member.throttle_state with no timers at
all. I'd rather attach the default AioContext while the BDS is removed.

I'll try to prepare a patch with all of this together.

Berto