This fixes the following crash:

{ "execute": "blockdev-add",
  "arguments": {"driver": "null-co", "node-name": "hd0"}}
{ "execute": "object-add",
  "arguments": {"qom-type": "iothread", "id": "iothread0"}}
{ "execute": "x-blockdev-set-iothread",
  "arguments": {"node-name": "hd0", "iothread": "iothread0"}}
{ "execute": "device_add",
  "arguments": {"id": "virtio0", "driver": "virtio-blk-pci",
                "drive": "hd0"}}
qemu: qemu_mutex_unlock_impl: Operation not permitted
Aborted

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 hw/block/hd-geometry.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/hw/block/hd-geometry.c b/hw/block/hd-geometry.c
index 79384a2b0a..0d9a7b7e7d 100644
--- a/hw/block/hd-geometry.c
+++ b/hw/block/hd-geometry.c
@@ -55,6 +55,7 @@ struct partition {
 static int guess_disk_lchs(BlockBackend *blk,
                            int *pcylinders, int *pheads, int *psectors)
 {
+    AioContext *ctx = blk_get_aio_context(blk);
     uint8_t buf[BDRV_SECTOR_SIZE];
     int i, heads, sectors, cylinders;
     struct partition *p;
@@ -68,7 +69,10 @@ static int guess_disk_lchs(BlockBackend *blk,
      * but also in async I/O mode. So the I/O throttling function has to
      * be disabled temporarily here, not permanently.
      */
-    if (blk_pread_unthrottled(blk, 0, buf, BDRV_SECTOR_SIZE) < 0) {
+    aio_context_acquire(ctx);
+    i = blk_pread_unthrottled(blk, 0, buf, BDRV_SECTOR_SIZE);
+    aio_context_release(ctx);
+    if (i < 0) {
         return -1;
     }
     /* test msdos magic */
-- 
2.11.0

On Wed, Jan 09, 2019 at 04:28:50PM +0200, Alberto Garcia wrote:
> This fixes the following crash:
> 
> { "execute": "blockdev-add",
>   "arguments": {"driver": "null-co", "node-name": "hd0"}}
> { "execute": "object-add",
>   "arguments": {"qom-type": "iothread", "id": "iothread0"}}
> { "execute": "x-blockdev-set-iothread",
>   "arguments": {"node-name": "hd0", "iothread": "iothread0"}}
> { "execute": "device_add",
>   "arguments": {"id": "virtio0", "driver": "virtio-blk-pci",
>                 "drive": "hd0"}}
> qemu: qemu_mutex_unlock_impl: Operation not permitted
> Aborted
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  hw/block/hd-geometry.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

Am 09.01.2019 um 15:28 hat Alberto Garcia geschrieben:
> This fixes the following crash:
> 
> { "execute": "blockdev-add",
>   "arguments": {"driver": "null-co", "node-name": "hd0"}}
> { "execute": "object-add",
>   "arguments": {"qom-type": "iothread", "id": "iothread0"}}
> { "execute": "x-blockdev-set-iothread",
>   "arguments": {"node-name": "hd0", "iothread": "iothread0"}}
> { "execute": "device_add",
>   "arguments": {"id": "virtio0", "driver": "virtio-blk-pci",
>                 "drive": "hd0"}}
> qemu: qemu_mutex_unlock_impl: Operation not permitted
> Aborted
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>

This looks like the same thing that I talked about with Markus
yesterday. He asked me where to put the acquire/release pair. My answer
was that there is more than one way to do it, but I suspect that the
realize functions of the devices may call the block layer more than once
and putting the acquire/release there might be nicer. We would then
document blkconf_geometry() to assume that the AioContext is already
locked.

There are many calls for node management stuff where we never defined
the locking rules, but where we might reasonably assume that they only
affect things that are only ever accessed from the main thread. But
there are also things like blk_ioctl() in scsi_block_realize(), which
should most certainly run with the lock held.

What do you think?

(I also wondered whether virtio-blk is affected as well or whether it
moves the BlockBackend to the iothread AioContext only later. Your
reproducer answers this question. :-))

Kevin

On Wed 09 Jan 2019 04:07:33 PM CET, Kevin Wolf wrote:
> This looks like the same thing that I talked about with Markus
> yesterday. He asked me where to put the acquire/release pair. My
> answer was that there is more than one way to do it, but I suspect
> that the realize functions of the devices may call the block layer
> more than once and putting the acquire/release there might be nicer.

Makes sense I guess, and it should be safe. I'll give it a try tomorrow
and send a new patch.

Berto