The following changes since commit 52ed34cbddde1cb89b2ac263e758e349a77f21e1:

Merge tag 'pull-request-2023-06-26' of https://gitlab.com/thuth/qemu into staging (2023-06-26 10:38:41 +0200)

https://repo.or.cz/qemu/kevin.git tags/for-upstream

for you to fetch changes up to 17362398ee1a7f04e8006a46333145d8b707fd35:

block: use bdrv_co_debug_event in coroutine context (2023-06-28 09:46:34 +0200)

Block layer patches

- Re-enable the graph lock

- More fixes to coroutine_fn marking

Kevin Wolf (11):

iotests: Test active commit with iothread and background I/O

qdev-properties-system: Lock AioContext for blk_insert_bs()

test-block-iothread: Lock AioContext for blk_insert_bs()

block: Fix AioContext locking in bdrv_open_child()

block: Fix AioContext locking in bdrv_attach_child_common()

block: Fix AioContext locking in bdrv_reopen_parse_file_or_backing()

block: Fix AioContext locking in bdrv_open_inherit()

block: Fix AioContext locking in bdrv_open_backing_file()

blockjob: Fix AioContext locking in block_job_add_bdrv()

graph-lock: Unlock the AioContext while polling

Revert "graph-lock: Disable locking for now"

Paolo Bonzini (12):

file-posix: remove incorrect coroutine_fn calls

qed: mark more functions as coroutine_fns and GRAPH_RDLOCK

vpc: mark more functions as coroutine_fns and GRAPH_RDLOCK

bochs: mark more functions as coroutine_fns and GRAPH_RDLOCK

block: mark another function as coroutine_fns and GRAPH_UNLOCKED

cloop: mark more functions as coroutine_fns and GRAPH_RDLOCK

dmg: mark more functions as coroutine_fns and GRAPH_RDLOCK

vmdk: mark more functions as coroutine_fns and GRAPH_RDLOCK

vhdx: mark more functions as coroutine_fns and GRAPH_RDLOCK

qcow2: mark more functions as coroutine_fns and GRAPH_RDLOCK

block: use bdrv_co_getlength in coroutine context

block: use bdrv_co_debug_event in coroutine context

block/qcow2.h | 33 +++--

block/vhdx.h | 5 +-

include/block/block-io.h | 7 ++

include/block/graph-lock.h | 6 +-

block.c | 114 ++++++++++++++++--

block/bochs.c | 7 +-

block/cloop.c | 9 +-

block/dmg.c | 21 ++--

block/file-posix.c | 29 +++--

block/graph-lock.c | 43 +++----

block/io.c | 14 +--

block/parallels.c | 4 +-

block/qcow.c | 30 ++---

block/qcow2-bitmap.c | 26 ++--

block/qcow2-cluster.c | 24 ++--

block/qcow2-refcount.c | 134 +++++++++++----------

block/qcow2.c | 20 +--

block/qed-check.c | 5 +-

block/qed-table.c | 6 +-

block/qed.c | 15 +--

block/raw-format.c | 4 +-

block/vhdx-log.c | 36 +++---

block/vhdx.c | 73 ++++++-----

block/vmdk.c | 55 ++++-----

block/vpc.c | 52 ++++----

blockjob.c | 17 ++-

hw/core/qdev-properties-system.c | 8 +-

tests/unit/test-block-iothread.c | 7 +-

tests/qemu-iotests/tests/iothreads-commit-active | 85 +++++++++++++

.../qemu-iotests/tests/iothreads-commit-active.out | 23 ++++

30 files changed, 573 insertions(+), 339 deletions(-)

create mode 100755 tests/qemu-iotests/tests/iothreads-commit-active

create mode 100644 tests/qemu-iotests/tests/iothreads-commit-active.out

This is a better regression test for the bugs hidden by commit 80fc5d26

('graph-lock: Disable locking for now'). With that commit reverted, it

hangs instantaneously and reliably for me.

It is important to have a reliable test like this, because the following

commits will set out to fix the actual root cause of the deadlocks and

then finally revert commit 80fc5d26, which was only a stopgap solution.

Message-ID: <20230605085711.21261-2-kwolf@redhat.com>

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

.../tests/iothreads-commit-active | 85 +++++++++++++++++++

.../tests/iothreads-commit-active.out | 23 +++++

2 files changed, 108 insertions(+)

create mode 100755 tests/qemu-iotests/tests/iothreads-commit-active

create mode 100644 tests/qemu-iotests/tests/iothreads-commit-active.out

diff --git a/tests/qemu-iotests/tests/iothreads-commit-active b/tests/qemu-iotests/tests/iothreads-commit-active

+++ b/tests/qemu-iotests/tests/iothreads-commit-active

+#!/usr/bin/env python3

+# group: rw quick auto

+# Copyright (C) 2023 Red Hat, Inc.

+import asyncio

+import iotests

+iotests.script_initialize(supported_fmts=['qcow2'],

+ supported_platforms=['linux'])

+iotests.verify_virtio_scsi_pci_or_ccw()

+with iotests.FilePath('disk0.img') as img_path, \

+ iotests.FilePath('disk0-snap.img') as snap_path, \

+ iotests.FilePath('mirror-src.img') as src_path, \

+ iotests.FilePath('mirror-dst.img') as dst_path, \

+ iotests.VM() as vm:

+ img_size = '10M'

+ iotests.qemu_img_create('-f', iotests.imgfmt, img_path, img_size)

+ iotests.qemu_img_create('-f', iotests.imgfmt, '-b', img_path,

+ '-F', iotests.imgfmt, snap_path)

+ iotests.qemu_img_create('-f', iotests.imgfmt, src_path, img_size)

+ iotests.qemu_img_create('-f', iotests.imgfmt, dst_path, img_size)

+ iotests.qemu_io_log('-c', 'write 0 64k', img_path)

+ iotests.qemu_io_log('-c', 'write 1M 64k', snap_path)

+ iotests.qemu_io_log('-c', 'write 3M 64k', snap_path)

+ iotests.qemu_io_log('-c', f'write 0 {img_size}', src_path)

+ iotests.log('Launching VM...')

+ vm.add_object('iothread,id=iothread0')

+ vm.add_object('throttle-group,x-bps-write=1048576,id=tg0')

+ vm.add_blockdev(f'file,node-name=disk0-file,filename={img_path}')

+ vm.add_blockdev('qcow2,node-name=disk0-fmt,file=disk0-file')

+ vm.add_drive(snap_path, 'backing=disk0-fmt,node-name=disk0',

+ interface='none')

+ vm.add_device('virtio-scsi,iothread=iothread0')

+ vm.add_device('scsi-hd,drive=drive0')

+

+ vm.add_blockdev(f'file,filename={src_path},node-name=mirror-src-file')

+ vm.add_blockdev('qcow2,file=mirror-src-file,node-name=mirror-src')

+ vm.add_blockdev(f'file,filename={dst_path},node-name=mirror-dst-file')

+ vm.add_blockdev('qcow2,file=mirror-dst-file,node-name=mirror-dst-fmt')

+ vm.add_blockdev('throttle,throttle-group=tg0,file=mirror-dst-fmt,'

+ 'node-name=mirror-dst')

+ vm.add_device('scsi-hd,drive=mirror-src')

+

+ vm.launch()

+

+ # The background I/O is created on unrelated nodes (so that they won't be

+ # drained together with the other ones), but on the same iothread

+ iotests.log('Creating some background I/O...')

+ iotests.log(vm.qmp('blockdev-mirror', job_id='job0', sync='full',

+ device='mirror-src', target='mirror-dst',

+ auto_dismiss=False))

+

+ iotests.log('Starting active commit...')

+ iotests.log(vm.qmp('block-commit', device='disk0', job_id='job1',

+ auto_dismiss=False))

+

+ # Should succeed and not time out

+ try:

+ vm.run_job('job1', wait=5.0)

+ vm.shutdown()

+ except asyncio.TimeoutError:

+ # VM may be stuck, kill it

+ vm.kill()

+ raise

diff --git a/tests/qemu-iotests/tests/iothreads-commit-active.out b/tests/qemu-iotests/tests/iothreads-commit-active.out

+++ b/tests/qemu-iotests/tests/iothreads-commit-active.out

+wrote 65536/65536 bytes at offset 0

+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)

+

+wrote 65536/65536 bytes at offset 1048576

+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)

+

+wrote 65536/65536 bytes at offset 3145728

+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)

+

+wrote 10485760/10485760 bytes at offset 0

+10 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)

+

+Launching VM...

+Creating some background I/O...

+{"return": {}}

+Starting active commit...

+{"return": {}}

+{"execute": "job-complete", "arguments": {"id": "job1"}}

+{"return": {}}

+{"data": {"device": "job1", "len": 131072, "offset": 131072, "speed": 0, "type": "commit"}, "event": "BLOCK_JOB_READY", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}

+{"data": {"device": "job1", "len": 131072, "offset": 131072, "speed": 0, "type": "commit"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}

+{"execute": "job-dismiss", "arguments": {"id": "job1"}}

+{"return": {}}

2.41.0

blk_insert_bs() requires that callers hold the AioContext lock for the

node that should be inserted. Take it.

Message-ID: <20230605085711.21261-3-kwolf@redhat.com>

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

hw/core/qdev-properties-system.c | 8 ++++++--

1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c

index XXXXXXX..XXXXXXX 100644

--- a/hw/core/qdev-properties-system.c

+++ b/hw/core/qdev-properties-system.c

@@ -XXX,XX +XXX,XX @@ static void set_drive_helper(Object *obj, Visitor *v, const char *name,

* aware of iothreads require their BlockBackends to be in the main

* AioContext.

*/

- ctx = iothread ? bdrv_get_aio_context(bs) : qemu_get_aio_context();

- blk = blk_new(ctx, 0, BLK_PERM_ALL);

+ ctx = bdrv_get_aio_context(bs);

+ blk = blk_new(iothread ? ctx : qemu_get_aio_context(),

+ 0, BLK_PERM_ALL);

blk_created = true;

+ aio_context_acquire(ctx);

ret = blk_insert_bs(blk, bs, errp);

+ aio_context_release(ctx);

if (ret < 0) {

goto fail;

}

2.41.0

blk_insert_bs() requires that callers hold the AioContext lock for the

node that should be inserted. Take it.

Message-ID: <20230605085711.21261-4-kwolf@redhat.com>

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

tests/unit/test-block-iothread.c | 7 ++++++-

1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/tests/unit/test-block-iothread.c b/tests/unit/test-block-iothread.c

--- a/tests/unit/test-block-iothread.c

+++ b/tests/unit/test-block-iothread.c

@@ -XXX,XX +XXX,XX @@ static void test_attach_second_node(void)

BlockDriverState *bs, *filter;

QDict *options;

+ aio_context_acquire(main_ctx);

blk = blk_new(ctx, BLK_PERM_ALL, BLK_PERM_ALL);

bs = bdrv_new_open_driver(&bdrv_test, "base", BDRV_O_RDWR, &error_abort);

blk_insert_bs(blk, bs, &error_abort);

@@ -XXX,XX +XXX,XX @@ static void test_attach_second_node(void)

qdict_put_str(options, "driver", "raw");

qdict_put_str(options, "file", "base");

- aio_context_acquire(main_ctx);

filter = bdrv_open(NULL, NULL, options, BDRV_O_RDWR, &error_abort);

aio_context_release(main_ctx);

@@ -XXX,XX +XXX,XX @@ static void test_attach_preserve_blk_ctx(void)

{

IOThread *iothread = iothread_new();

AioContext *ctx = iothread_get_aio_context(iothread);

+ AioContext *main_ctx = qemu_get_aio_context();

BlockBackend *blk;

BlockDriverState *bs;

+ aio_context_acquire(main_ctx);

blk = blk_new(ctx, BLK_PERM_ALL, BLK_PERM_ALL);

bs = bdrv_new_open_driver(&bdrv_test, "base", BDRV_O_RDWR, &error_abort);

bs->total_sectors = 65536 / BDRV_SECTOR_SIZE;

@@ -XXX,XX +XXX,XX @@ static void test_attach_preserve_blk_ctx(void)

blk_insert_bs(blk, bs, &error_abort);

g_assert(blk_get_aio_context(blk) == ctx);

g_assert(bdrv_get_aio_context(bs) == ctx);

+ aio_context_release(main_ctx);

/* Remove the node again */

aio_context_acquire(ctx);

@@ -XXX,XX +XXX,XX @@ static void test_attach_preserve_blk_ctx(void)

g_assert(bdrv_get_aio_context(bs) == qemu_get_aio_context());

/* Re-attach the node */

+ aio_context_acquire(main_ctx);

blk_insert_bs(blk, bs, &error_abort);

+ aio_context_release(main_ctx);

g_assert(blk_get_aio_context(blk) == ctx);

g_assert(bdrv_get_aio_context(bs) == ctx);

2.41.0

bdrv_attach_child() requires that the caller holds the AioContext lock

for the new child node. Take it in bdrv_open_child() and document that

the caller must not hold any AioContext apart from the main AioContext.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>

Message-ID: <20230605085711.21261-5-kwolf@redhat.com>

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

block.c | 13 +++++++++++--

1 file changed, 11 insertions(+), 2 deletions(-)

@@ -XXX,XX +XXX,XX @@ done:

*

* The BlockdevRef will be removed from the options QDict.

*

+ * The caller must hold the lock of the main AioContext and no other AioContext.

* @parent can move to a different AioContext in this function. Callers must

* make sure that their AioContext locking is still correct after this.

@@ -XXX,XX +XXX,XX @@ BdrvChild *bdrv_open_child(const char *filename,

+ BdrvChild *child;

+ AioContext *ctx;

GLOBAL_STATE_CODE();

@@ -XXX,XX +XXX,XX @@ BdrvChild *bdrv_open_child(const char *filename,

- return bdrv_attach_child(parent, bs, bdref_key, child_class, child_role,

- errp);

+ ctx = bdrv_get_aio_context(bs);

+ aio_context_acquire(ctx);

+ child = bdrv_attach_child(parent, bs, bdref_key, child_class, child_role,

+ errp);

+ aio_context_release(ctx);

+

+ return child;