help debugging throttle crash

Commit Message

Alberto Garcia April 3, 2017, 3:24 p.m. UTC

On Mon, Apr 03, 2017 at 09:07:02AM -0500, Eric Blake wrote:

> At this point, it looks like no one is calling
> throttle_group_unregister_blk() as a result of the 'device_del',
> which leaves stale memory around

I see, I can also reproduce this very easily.

I wonder if it's not enough to simply disable I/O limits when a
BlockBackend is deleted?


Berto

Comments

Eric Blake April 6, 2017, 6:57 p.m. UTC | #1

On 04/03/2017 10:24 AM, Alberto Garcia wrote:
> On Mon, Apr 03, 2017 at 09:07:02AM -0500, Eric Blake wrote:
> 
>> At this point, it looks like no one is calling
>> throttle_group_unregister_blk() as a result of the 'device_del',
>> which leaves stale memory around
> 
> I see, I can also reproduce this very easily.
> 
> I wonder if it's not enough to simply disable I/O limits when a
> BlockBackend is deleted?

Seems to pass my testing. Do you want to submit it as a formal patch, or
shall I?

> 
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -230,6 +230,9 @@ static void blk_delete(BlockBackend *blk)
>      assert(!blk->refcnt);
>      assert(!blk->name);
>      assert(!blk->dev);
> +    if (blk->public.throttle_state) {
> +        blk_io_limits_disable(blk);
> +    }
>      if (blk->root) {
>          blk_remove_bs(blk);
>      }
> 
> Berto
>

Patch

--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -230,6 +230,9 @@  static void blk_delete(BlockBackend *blk)
     assert(!blk->refcnt);
     assert(!blk->name);
     assert(!blk->dev);
+    if (blk->public.throttle_state) {
+        blk_io_limits_disable(blk);
+    }
     if (blk->root) {
         blk_remove_bs(blk);
     }