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)

are available in the Git repository at:

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.

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

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>

Signed-off-by: Kevin Wolf <kwolf@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

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

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

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.

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

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

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

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

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

diff --git a/block.c b/block.c

--- a/block.c

+++ b/block.c

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

bool allow_none, Error **errp)

{

BlockDriverState *bs;

+ BdrvChild *child;

+ AioContext *ctx;

GLOBAL_STATE_CODE();

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

return NULL;

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

}

/*

* Wrapper on bdrv_open_child() for most popular case: open primary child of bs.

*

+ * 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.

*/

2.41.0

The function can move the child node to a different AioContext. In this

case, it also must take the AioContext lock for the new context before

calling functions that require the caller to hold the AioContext for the

child node.

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

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

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

block.c | 21 ++++++++++++++++++++-

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

diff --git a/block.c b/block.c

--- a/block.c

+++ b/block.c

@@ -XXX,XX +XXX,XX @@ static TransactionActionDrv bdrv_attach_child_common_drv = {

* Function doesn't update permissions, caller is responsible for this.

*

* Returns new created child.

+ *

+ * The caller must hold the AioContext lock for @child_bs. Both @parent_bs and

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

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

*/

static BdrvChild *bdrv_attach_child_common(BlockDriverState *child_bs,

const char *child_name,

@@ -XXX,XX +XXX,XX @@ static BdrvChild *bdrv_attach_child_common(BlockDriverState *child_bs,

Transaction *tran, Error **errp)

{

BdrvChild *new_child;

- AioContext *parent_ctx;

+ AioContext *parent_ctx, *new_child_ctx;

AioContext *child_ctx = bdrv_get_aio_context(child_bs);

assert(child_class->get_parent_desc);

@@ -XXX,XX +XXX,XX @@ static BdrvChild *bdrv_attach_child_common(BlockDriverState *child_bs,

2.41.0

bdrv_set_file_or_backing_noperm() requires the caller to hold the

AioContext lock for the child node, but we hold the one for the parent

node in bdrv_reopen_parse_file_or_backing(). Take the other one

temporarily.

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

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

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

block.c | 35 +++++++++++++++++++++++++++++++++--

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

diff --git a/block.c b/block.c

--- a/block.c

+++ b/block.c

@@ -XXX,XX +XXX,XX @@ static BdrvChildRole bdrv_backing_role(BlockDriverState *bs)

* callers which don't need their own reference any more must call bdrv_unref().

*

* Function doesn't update permissions, caller is responsible for this.

+ *

+ * The caller must hold the AioContext lock for @child_bs. Both @parent_bs and

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

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

*/

static int bdrv_set_file_or_backing_noperm(BlockDriverState *parent_bs,

BlockDriverState *child_bs,

@@ -XXX,XX +XXX,XX @@ out:

return 0;

}

+/*

+ * The caller must hold the AioContext lock for @backing_hd. Both @bs and

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

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

+ */

static int bdrv_set_backing_noperm(BlockDriverState *bs,

BlockDriverState *backing_hd,

Transaction *tran, Error **errp)

@@ -XXX,XX +XXX,XX @@ int bdrv_reopen_set_read_only(BlockDriverState *bs, bool read_only,

* backing BlockDriverState (or NULL).

*

* Return 0 on success, otherwise return < 0 and set @errp.

+ *

+ * The caller must hold the AioContext lock of @reopen_state->bs.

+ * @reopen_state->bs can move to a different AioContext in this function.

+ * Callers must make sure that their AioContext locking is still correct after

+ * this.

*/

static int bdrv_reopen_parse_file_or_backing(BDRVReopenState *reopen_state,

bool is_backing, Transaction *tran,

@@ -XXX,XX +XXX,XX @@ static int bdrv_reopen_parse_file_or_backing(BDRVReopenState *reopen_state,

const char *child_name = is_backing ? "backing" : "file";

QObject *value;

const char *str;

+ AioContext *ctx, *old_ctx;

+ int ret;

GLOBAL_STATE_CODE();

@@ -XXX,XX +XXX,XX @@ static int bdrv_reopen_parse_file_or_backing(BDRVReopenState *reopen_state,

reopen_state->old_file_bs = old_child_bs;

- return bdrv_set_file_or_backing_noperm(bs, new_child_bs, is_backing,

- tran, errp);

+ old_ctx = bdrv_get_aio_context(bs);

+ ctx = bdrv_get_aio_context(new_child_bs);

+ if (old_ctx != ctx) {

+ aio_context_release(old_ctx);

+ aio_context_acquire(ctx);

+ }

+

+ ret = bdrv_set_file_or_backing_noperm(bs, new_child_bs, is_backing,

+ tran, errp);

+

+ if (old_ctx != ctx) {

+ aio_context_release(ctx);

+ aio_context_acquire(old_ctx);

+ }

+

+ return ret;

@@ -XXX,XX +XXX,XX @@ static int bdrv_reopen_parse_file_or_backing(BDRVReopenState *reopen_state,

* It is the responsibility of the caller to then call the abort() or

* commit() for any other BDS that have been left in a prepare() state

*

+ * The caller must hold the AioContext lock of @reopen_state->bs.

static int bdrv_reopen_prepare(BDRVReopenState *reopen_state,

BlockReopenQueue *queue,

2.41.0

bdrv_open_inherit() calls several functions for which it needs to hold

the AioContext lock, but currently doesn't. This includes calls in

bdrv_append_temp_snapshot(), for which bdrv_open_inherit() is the only

caller. Fix the locking in these places.

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

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

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

block.c | 25 ++++++++++++++++++++++++-

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

diff --git a/block.c b/block.c

--- a/block.c

+++ b/block.c

@@ -XXX,XX +XXX,XX @@ static BlockDriverState *bdrv_append_temp_snapshot(BlockDriverState *bs,

int64_t total_size;

QemuOpts *opts = NULL;

BlockDriverState *bs_snapshot = NULL;

+ AioContext *ctx = bdrv_get_aio_context(bs);

int ret;

GLOBAL_STATE_CODE();

@@ -XXX,XX +XXX,XX @@ static BlockDriverState *bdrv_append_temp_snapshot(BlockDriverState *bs,

instead of opening 'filename' directly */

/* Get the required size from the image */

+ aio_context_acquire(ctx);

total_size = bdrv_getlength(bs);

+ aio_context_release(ctx);

+

if (total_size < 0) {

error_setg_errno(errp, -total_size, "Could not get image size");

goto out;

@@ -XXX,XX +XXX,XX @@ static BlockDriverState *bdrv_append_temp_snapshot(BlockDriverState *bs,

goto out;

+ aio_context_acquire(ctx);

ret = bdrv_append(bs_snapshot, bs, errp);

+ aio_context_release(ctx);

+

if (ret < 0) {

bs_snapshot = NULL;

goto out;

@@ -XXX,XX +XXX,XX @@ bdrv_open_inherit(const char *filename, const char *reference, QDict *options,

QDict *snapshot_options = NULL;

int snapshot_flags = 0;

+ AioContext *ctx = qemu_get_aio_context();

assert(!child_class || !flags);

assert(!child_class == !parent);

@@ -XXX,XX +XXX,XX @@ bdrv_open_inherit(const char *filename, const char *reference, QDict *options,

/* Not requesting BLK_PERM_CONSISTENT_READ because we're only

* looking at the header to guess the image format. This works even

* in cases where a guest would not see a consistent state. */

- file = blk_new(bdrv_get_aio_context(file_bs), 0, BLK_PERM_ALL);

+ ctx = bdrv_get_aio_context(file_bs);

+ aio_context_acquire(ctx);

+ file = blk_new(ctx, 0, BLK_PERM_ALL);

blk_insert_bs(file, file_bs, &local_err);

bdrv_unref(file_bs);

+ aio_context_release(ctx);

+

if (local_err) {

goto fail;

}

@@ -XXX,XX +XXX,XX @@ bdrv_open_inherit(const char *filename, const char *reference, QDict *options,