[Qemu-devel] help debugging throttle crash

[Qemu-devel] help debugging throttle crash

[Eric Blake]

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

I'm trying to investigate
https://bugzilla.redhat.com/show_bug.cgi?id=1428810, which is a crash
that can be easily reproduced with the following steps:

$ ./x86_64-softmmu/qemu-system-x86_64 -nodefaults -nographic -qmp stdio
-device virtio-scsi-pci,bus=pci.0  -drive
id=drive_image2,if=none,format=raw,file=file2,bps=512000,iops=100,group=foo
-device scsi-hd,id=image2,drive=drive_image2 -drive
id=drive_image3,if=none,format=raw,file=file3,bps=512000,iops=100,group=foo
-device scsi-hd,id=image3,drive=drive_image3
{'execute':'qmp_capabilities'}
{'execute':'device_del','arguments':{'id':'image3'}}
{'execute':'system_reset'}

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 was able to confirm this under gcc - a
breakpoint on that function never fires); then the 'system_reset' causes
next_throttle_token() to dereference the stale memory and crash.
However, I have no idea where the unplug action should be removing the
BB from the throttle group.  Is it as simple as adding it to
blk_io_unplug(), or will that be violating other constraints on making
sure the throttle group is first drained before removing the BB from the
group as one of the final steps during its hot unplug?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org




[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] help debugging throttle crash

[Alberto Garcia]

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?

--- 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

Re: [Qemu-devel] help debugging throttle crash

[Eric Blake]

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

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
> 

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]