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
© 2016 - 2024 Red Hat, Inc.