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>

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

new file mode 100755

index XXXXXXX..XXXXXXX

--- /dev/null

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

@@ -XXX,XX +XXX,XX @@

+#!/usr/bin/env python3

+# group: rw quick auto

+#

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

+#

+# This program is free software; you can redistribute it and/or modify

+# it under the terms of the GNU General Public License as published by

+# the Free Software Foundation; either version 2 of the License, or

+# (at your option) any later version.

+#

+# This program is distributed in the hope that it will be useful,

+# but WITHOUT ANY WARRANTY; without even the implied warranty of

+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the

+# GNU General Public License for more details.

+#

+# You should have received a copy of the GNU General Public License

+# along with this program. If not, see <http://www.gnu.org/licenses/>.

+#

+# Creator/Owner: Kevin Wolf <kwolf@redhat.com>

+

+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

new file mode 100644

index XXXXXXX..XXXXXXX

--- /dev/null

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

@@ -XXX,XX +XXX,XX @@

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

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

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

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.

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

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

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

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

index XXXXXXX..XXXXXXX 100644

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

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

index XXXXXXX..XXXXXXX 100644

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

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

---

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

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

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

index XXXXXXX..XXXXXXX 100644

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

}

}

+ new_child_ctx = bdrv_get_aio_context(child_bs);

+ if (new_child_ctx != child_ctx) {

+ aio_context_release(child_ctx);

+ aio_context_acquire(new_child_ctx);

+ }

+

bdrv_ref(child_bs);

/*

* Let every new BdrvChild start with a drained parent. Inserting the child

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

};

tran_add(tran, &bdrv_attach_child_common_drv, s);

+ if (new_child_ctx != child_ctx) {

+ aio_context_release(new_child_ctx);

+ aio_context_acquire(child_ctx);

+ }

+

return new_child;

}

/*

* 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 BdrvChild *bdrv_attach_child_noperm(BlockDriverState *parent_bs,

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.

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

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

index XXXXXXX..XXXXXXX 100644

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

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

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

index XXXXXXX..XXXXXXX 100644

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

Error *local_err = NULL;

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,

goto fail;

}

+ /* The AioContext could have changed during bdrv_open_common() */

+ ctx = bdrv_get_aio_context(bs);

+

if (file) {

+ aio_context_acquire(ctx);

blk_unref(file);

+ aio_context_release(ctx);

file = NULL;

}

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

* (snapshot_bs); thus, we have to drop the strong reference to bs

* (which we obtained by calling bdrv_new()). bs will not be deleted,

* though, because the overlay still has a reference to it. */

+ aio_context_acquire(ctx);

bdrv_unref(bs);

+ aio_context_release(ctx);

bs = snapshot_bs;

}

return bs;

fail:

+ aio_context_acquire(ctx);

blk_unref(file);

qobject_unref(snapshot_options);

qobject_unref(bs->explicit_options);

@@ -XXX,XX +XXX,XX @@ fail:

bs->options = NULL;

bs->explicit_options = NULL;

bdrv_unref(bs);

+ aio_context_release(ctx);

error_propagate(errp, local_err);

return NULL;

close_and_fail:

+ aio_context_acquire(ctx);

bdrv_unref(bs);

+ aio_context_release(ctx);

qobject_unref(snapshot_options);

qobject_unref(options);

error_propagate(errp, local_err);

--

2.41.0

bdrv_set_backing() requires the caller to hold the AioContext lock for

@backing_hd. Take it in bdrv_open_backing_file() before calling the

function.

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

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

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

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

---

block.c | 5 +++++

1 file changed, 5 insertions(+)

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

index XXXXXXX..XXXXXXX 100644

--- a/block.c

+++ b/block.c

@@ -XXX,XX +XXX,XX @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *parent_options,

int ret = 0;

bool implicit_backing = false;

BlockDriverState *backing_hd;

+ AioContext *backing_hd_ctx;

QDict *options;

QDict *tmp_parent_options = NULL;

Error *local_err = NULL;

@@ -XXX,XX +XXX,XX @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *parent_options,

/* Hook up the backing file link; drop our reference, bs owns the

* backing_hd reference now */

+ backing_hd_ctx = bdrv_get_aio_context(backing_hd);

+ aio_context_acquire(backing_hd_ctx);

ret = bdrv_set_backing_hd(bs, backing_hd, errp);

bdrv_unref(backing_hd);

+ aio_context_release(backing_hd_ctx);

+

if (ret < 0) {

goto free_exit;

}

--

2.41.0

bdrv_root_attach_child() requires callers to hold the AioContext lock

for child_bs. Take it in block_job_add_bdrv() before calling the

function.

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

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

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

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

---

blockjob.c | 17 ++++++++++++-----

1 file changed, 12 insertions(+), 5 deletions(-)

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

index XXXXXXX..XXXXXXX 100644

--- a/blockjob.c

+++ b/blockjob.c

@@ -XXX,XX +XXX,XX @@ int block_job_add_bdrv(BlockJob *job, const char *name, BlockDriverState *bs,

uint64_t perm, uint64_t shared_perm, Error **errp)

{

BdrvChild *c;

+ AioContext *ctx = bdrv_get_aio_context(bs);

bool need_context_ops;

GLOBAL_STATE_CODE();

bdrv_ref(bs);

- need_context_ops = bdrv_get_aio_context(bs) != job->job.aio_context;

+ need_context_ops = ctx != job->job.aio_context;

- if (need_context_ops && job->job.aio_context != qemu_get_aio_context()) {

- aio_context_release(job->job.aio_context);

+ if (need_context_ops) {

+ if (job->job.aio_context != qemu_get_aio_context()) {

+ aio_context_release(job->job.aio_context);

+ }

+ aio_context_acquire(ctx);

}

c = bdrv_root_attach_child(bs, name, &child_job, 0, perm, shared_perm, job,

errp);

- if (need_context_ops && job->job.aio_context != qemu_get_aio_context()) {

- aio_context_acquire(job->job.aio_context);

+ if (need_context_ops) {

+ aio_context_release(ctx);

+ if (job->job.aio_context != qemu_get_aio_context()) {

+ aio_context_acquire(job->job.aio_context);

+ }

}

if (c == NULL) {

return -EPERM;

--

2.41.0

If the caller keeps the AioContext lock for a block node in an iothread,

polling in bdrv_graph_wrlock() deadlocks if the condition isn't

fulfilled immediately.

Now that all callers make sure to actually have the AioContext locked

when they call bdrv_replace_child_noperm() like they should, we can

change bdrv_graph_wrlock() to take a BlockDriverState whose AioContext

lock the caller holds (NULL if it doesn't) and unlock it temporarily

while polling.

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

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

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

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

---

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

block.c | 4 ++--

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

3 files changed, 28 insertions(+), 5 deletions(-)

diff --git a/include/block/graph-lock.h b/include/block/graph-lock.h

index XXXXXXX..XXXXXXX 100644

--- a/include/block/graph-lock.h

+++ b/include/block/graph-lock.h

@@ -XXX,XX +XXX,XX @@ void unregister_aiocontext(AioContext *ctx);

* The wrlock can only be taken from the main loop, with BQL held, as only the

* main loop is allowed to modify the graph.

*

+ * If @bs is non-NULL, its AioContext is temporarily released.

+ *

* This function polls. Callers must not hold the lock of any AioContext other

- * than the current one.

+ * than the current one and the one of @bs.

*/

-void bdrv_graph_wrlock(void) TSA_ACQUIRE(graph_lock) TSA_NO_TSA;

+void bdrv_graph_wrlock(BlockDriverState *bs) TSA_ACQUIRE(graph_lock) TSA_NO_TSA;

/*

* bdrv_graph_wrunlock:

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

index XXXXXXX..XXXXXXX 100644

--- a/block.c

+++ b/block.c

@@ -XXX,XX +XXX,XX @@ uint64_t bdrv_qapi_perm_to_blk_perm(BlockPermission qapi_perm)

* Replaces the node that a BdrvChild points to without updating permissions.

*

* If @new_bs is non-NULL, the parent of @child must already be drained through

- * @child.

+ * @child and the caller must hold the AioContext lock for @new_bs.

*/

static void bdrv_replace_child_noperm(BdrvChild *child,

BlockDriverState *new_bs)

@@ -XXX,XX +XXX,XX @@ static void bdrv_replace_child_noperm(BdrvChild *child,

}

/* TODO Pull this up into the callers to avoid polling here */

- bdrv_graph_wrlock();

+ bdrv_graph_wrlock(new_bs);

if (old_bs) {

if (child->klass->detach) {

child->klass->detach(child);

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

index XXXXXXX..XXXXXXX 100644

--- a/block/graph-lock.c

+++ b/block/graph-lock.c

@@ -XXX,XX +XXX,XX @@ static uint32_t reader_count(void)

}

#endif

-void bdrv_graph_wrlock(void)

+void bdrv_graph_wrlock(BlockDriverState *bs)

{

+ AioContext *ctx = NULL;

+

GLOBAL_STATE_CODE();

/*

* TODO Some callers hold an AioContext lock when this is called, which

@@ -XXX,XX +XXX,XX @@ void bdrv_graph_wrlock(void)

*/

#if 0

assert(!qatomic_read(&has_writer));

+#endif

+

+ /*

+ * Release only non-mainloop AioContext. The mainloop often relies on the

+ * BQL and doesn't lock the main AioContext before doing things.

+ */

+ if (bs) {

+ ctx = bdrv_get_aio_context(bs);

+ if (ctx != qemu_get_aio_context()) {

+ aio_context_release(ctx);

+ } else {

+ ctx = NULL;

+ }

+ }

+#if 0

/* Make sure that constantly arriving new I/O doesn't cause starvation */

bdrv_drain_all_begin_nopoll();

@@ -XXX,XX +XXX,XX @@ void bdrv_graph_wrlock(void)

bdrv_drain_all_end();

#endif

+

+ if (ctx) {

+ aio_context_acquire(bdrv_get_aio_context(bs));

+ }

}

void bdrv_graph_wrunlock(void)

--

2.41.0

Now that bdrv_graph_wrlock() temporarily drops the AioContext lock that

its caller holds, it can poll without causing deadlocks. We can now

re-enable graph locking.

This reverts commit ad128dff0bf4b6f971d05eb4335a627883a19c1d.

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

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

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

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

---

block/graph-lock.c | 26 --------------------------

1 file changed, 26 deletions(-)

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

index XXXXXXX..XXXXXXX 100644

--- a/block/graph-lock.c

+++ b/block/graph-lock.c

@@ -XXX,XX +XXX,XX @@ BdrvGraphLock graph_lock;

/* Protects the list of aiocontext and orphaned_reader_count */

static QemuMutex aio_context_list_lock;

-#if 0

/* Written and read with atomic operations. */

static int has_writer;

-#endif

/*

* A reader coroutine could move from an AioContext to another.

@@ -XXX,XX +XXX,XX @@ void unregister_aiocontext(AioContext *ctx)

g_free(ctx->bdrv_graph);

}

-#if 0

static uint32_t reader_count(void)

{

BdrvGraphRWlock *brdv_graph;

@@ -XXX,XX +XXX,XX @@ static uint32_t reader_count(void)

assert((int32_t)rd >= 0);

return rd;

}

-#endif

void bdrv_graph_wrlock(BlockDriverState *bs)

{

AioContext *ctx = NULL;

GLOBAL_STATE_CODE();

- /*

- * TODO Some callers hold an AioContext lock when this is called, which

- * causes deadlocks. Reenable once the AioContext locking is cleaned up (or

- * AioContext locks are gone).

- */

-#if 0

assert(!qatomic_read(&has_writer));

-#endif

/*

* Release only non-mainloop AioContext. The mainloop often relies on the

@@ -XXX,XX +XXX,XX @@ void bdrv_graph_wrlock(BlockDriverState *bs)

}

}

-#if 0

/* Make sure that constantly arriving new I/O doesn't cause starvation */

bdrv_drain_all_begin_nopoll();

@@ -XXX,XX +XXX,XX @@ void bdrv_graph_wrlock(BlockDriverState *bs)

} while (reader_count() >= 1);

bdrv_drain_all_end();

-#endif

if (ctx) {

aio_context_acquire(bdrv_get_aio_context(bs));

@@ -XXX,XX +XXX,XX @@ void bdrv_graph_wrlock(BlockDriverState *bs)

void bdrv_graph_wrunlock(void)

{

GLOBAL_STATE_CODE();

-#if 0

QEMU_LOCK_GUARD(&aio_context_list_lock);

assert(qatomic_read(&has_writer));

@@ -XXX,XX +XXX,XX @@ void bdrv_graph_wrunlock(void)

/* Wake up all coroutine that are waiting to read the graph */

qemu_co_enter_all(&reader_queue, &aio_context_list_lock);

-#endif

}

void coroutine_fn bdrv_graph_co_rdlock(void)

{

- /* TODO Reenable when wrlock is reenabled */

-#if 0

BdrvGraphRWlock *bdrv_graph;

bdrv_graph = qemu_get_current_aio_context()->bdrv_graph;

@@ -XXX,XX +XXX,XX @@ void coroutine_fn bdrv_graph_co_rdlock(void)

qemu_co_queue_wait(&reader_queue, &aio_context_list_lock);

}

}

-#endif

}

void coroutine_fn bdrv_graph_co_rdunlock(void)

{

-#if 0

BdrvGraphRWlock *bdrv_graph;

bdrv_graph = qemu_get_current_aio_context()->bdrv_graph;

@@ -XXX,XX +XXX,XX @@ void coroutine_fn bdrv_graph_co_rdunlock(void)

if (qatomic_read(&has_writer)) {

aio_wait_kick();

}

-#endif

}

void bdrv_graph_rdlock_main_loop(void)

@@ -XXX,XX +XXX,XX @@ void bdrv_graph_rdunlock_main_loop(void)

void assert_bdrv_graph_readable(void)

{

/* reader_count() is slow due to aio_context_list_lock lock contention */

- /* TODO Reenable when wrlock is reenabled */

-#if 0

#ifdef CONFIG_DEBUG_GRAPH_LOCK

assert(qemu_in_main_thread() || reader_count());

#endif

-#endif

}

void assert_bdrv_graph_writable(void)

{

assert(qemu_in_main_thread());

- /* TODO Reenable when wrlock is reenabled */

-#if 0

assert(qatomic_read(&has_writer));

-#endif

}

--

2.41.0

From: Paolo Bonzini <pbonzini@redhat.com>

raw_co_getlength is called by handle_aiocb_write_zeroes, which is not a coroutine

function. This is harmless because raw_co_getlength does not actually suspend,

but in the interest of clarity make it a non-coroutine_fn that is just wrapped

by the coroutine_fn raw_co_getlength. Likewise, check_cache_dropped was only

a coroutine_fn because it called raw_co_getlength, so it can be made non-coroutine

as well.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Message-ID: <20230601115145.196465-2-pbonzini@redhat.com>

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

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

---

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

1 file changed, 17 insertions(+), 12 deletions(-)

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

index XXXXXXX..XXXXXXX 100644

--- a/block/file-posix.c

+++ b/block/file-posix.c

@@ -XXX,XX +XXX,XX @@ static int fd_open(BlockDriverState *bs)

return -EIO;

}

-static int64_t coroutine_fn raw_co_getlength(BlockDriverState *bs);

+static int64_t raw_getlength(BlockDriverState *bs);

typedef struct RawPosixAIOData {

BlockDriverState *bs;

@@ -XXX,XX +XXX,XX @@ static int handle_aiocb_write_zeroes(void *opaque)

#ifdef CONFIG_FALLOCATE

/* Last resort: we are trying to extend the file with zeroed data. This

* can be done via fallocate(fd, 0) */

- len = raw_co_getlength(aiocb->bs);

+ len = raw_getlength(aiocb->bs);

if (s->has_fallocate && len >= 0 && aiocb->aio_offset >= len) {

int ret = do_fallocate(s->fd, 0, aiocb->aio_offset, aiocb->aio_nbytes);

if (ret == 0 || ret != -ENOTSUP) {

@@ -XXX,XX +XXX,XX @@ static int coroutine_fn raw_co_truncate(BlockDriverState *bs, int64_t offset,

}

if (S_ISCHR(st.st_mode) || S_ISBLK(st.st_mode)) {

- int64_t cur_length = raw_co_getlength(bs);

+ int64_t cur_length = raw_getlength(bs);

if (offset != cur_length && exact) {

error_setg(errp, "Cannot resize device files");

@@ -XXX,XX +XXX,XX @@ static int coroutine_fn raw_co_truncate(BlockDriverState *bs, int64_t offset,

}

#ifdef __OpenBSD__

-static int64_t coroutine_fn raw_co_getlength(BlockDriverState *bs)

+static int64_t raw_getlength(BlockDriverState *bs)

{

BDRVRawState *s = bs->opaque;

int fd = s->fd;

@@ -XXX,XX +XXX,XX @@ static int64_t coroutine_fn raw_co_getlength(BlockDriverState *bs)

return st.st_size;

}

#elif defined(__NetBSD__)

-static int64_t coroutine_fn raw_co_getlength(BlockDriverState *bs)

+static int64_t raw_getlength(BlockDriverState *bs)

{

BDRVRawState *s = bs->opaque;

int fd = s->fd;

@@ -XXX,XX +XXX,XX @@ static int64_t coroutine_fn raw_co_getlength(BlockDriverState *bs)

return st.st_size;

}

#elif defined(__sun__)

-static int64_t coroutine_fn raw_co_getlength(BlockDriverState *bs)

+static int64_t raw_getlength(BlockDriverState *bs)

{

BDRVRawState *s = bs->opaque;

struct dk_minfo minfo;

@@ -XXX,XX +XXX,XX @@ static int64_t coroutine_fn raw_co_getlength(BlockDriverState *bs)

return size;

}

#elif defined(CONFIG_BSD)

-static int64_t coroutine_fn raw_co_getlength(BlockDriverState *bs)

+static int64_t raw_getlength(BlockDriverState *bs)

{

BDRVRawState *s = bs->opaque;

int fd = s->fd;

@@ -XXX,XX +XXX,XX @@ again:

return size;

}

#else

-static int64_t coroutine_fn raw_co_getlength(BlockDriverState *bs)

+static int64_t raw_getlength(BlockDriverState *bs)

{

BDRVRawState *s = bs->opaque;

int ret;

@@ -XXX,XX +XXX,XX @@ static int64_t coroutine_fn raw_co_getlength(BlockDriverState *bs)

}

#endif

+static int64_t coroutine_fn raw_co_getlength(BlockDriverState *bs)

+{

+ return raw_getlength(bs);

+}

+

static int64_t coroutine_fn raw_co_get_allocated_file_size(BlockDriverState *bs)

{

struct stat st;

@@ -XXX,XX +XXX,XX @@ static int coroutine_fn raw_co_block_status(BlockDriverState *bs,

* round up if necessary.

*/

if (!QEMU_IS_ALIGNED(*pnum, bs->bl.request_alignment)) {

- int64_t file_length = raw_co_getlength(bs);

+ int64_t file_length = raw_getlength(bs);

if (file_length > 0) {

/* Ignore errors, this is just a safeguard */

assert(hole == file_length);

@@ -XXX,XX +XXX,XX @@ static int coroutine_fn raw_co_block_status(BlockDriverState *bs,

#if defined(__linux__)

/* Verify that the file is not in the page cache */

-static void coroutine_fn check_cache_dropped(BlockDriverState *bs, Error **errp)

+static void check_cache_dropped(BlockDriverState *bs, Error **errp)

{

const size_t window_size = 128 * 1024 * 1024;

BDRVRawState *s = bs->opaque;

@@ -XXX,XX +XXX,XX @@ static void coroutine_fn check_cache_dropped(BlockDriverState *bs, Error **errp)

page_size = sysconf(_SC_PAGESIZE);

vec = g_malloc(DIV_ROUND_UP(window_size, page_size));

- end = raw_co_getlength(bs);

+ end = raw_getlength(bs);

for (offset = 0; offset < end; offset += window_size) {

void *new_window;

@@ -XXX,XX +XXX,XX @@ static int cdrom_reopen(BlockDriverState *bs)

static bool coroutine_fn cdrom_co_is_inserted(BlockDriverState *bs)

{

- return raw_co_getlength(bs) > 0;

+ return raw_getlength(bs) > 0;

}

static void coroutine_fn cdrom_co_eject(BlockDriverState *bs, bool eject_flag)

--

2.41.0

From: Paolo Bonzini <pbonzini@redhat.com>

Mark functions as coroutine_fn when they are only called by other coroutine_fns

and they can suspend. Change calls to co_wrappers to use the non-wrapped

functions, which in turn requires adding GRAPH_RDLOCK annotations.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Message-ID: <20230601115145.196465-3-pbonzini@redhat.com>

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

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

---

block/qed-check.c | 5 +++--

block/qed.c | 7 ++++---

2 files changed, 7 insertions(+), 5 deletions(-)

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

index XXXXXXX..XXXXXXX 100644

--- a/block/qed-check.c

+++ b/block/qed-check.c

@@ -XXX,XX +XXX,XX @@ static void qed_check_for_leaks(QEDCheck *check)

/**

* Mark an image clean once it passes check or has been repaired

*/

-static void qed_check_mark_clean(BDRVQEDState *s, BdrvCheckResult *result)

+static void coroutine_fn GRAPH_RDLOCK

+qed_check_mark_clean(BDRVQEDState *s, BdrvCheckResult *result)

{

/* Skip if there were unfixable corruptions or I/O errors */

if (result->corruptions > 0 || result->check_errors > 0) {

@@ -XXX,XX +XXX,XX @@ static void qed_check_mark_clean(BDRVQEDState *s, BdrvCheckResult *result)

}

/* Ensure fixes reach storage before clearing check bit */

- bdrv_flush(s->bs);

+ bdrv_co_flush(s->bs);

s->header.features &= ~QED_F_NEED_CHECK;

qed_write_header_sync(s);

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

index XXXXXXX..XXXXXXX 100644

--- a/block/qed.c

+++ b/block/qed.c

@@ -XXX,XX +XXX,XX @@ static bool qed_is_image_size_valid(uint64_t image_size, uint32_t cluster_size,

*

* The string is NUL-terminated.

*/

-static int qed_read_string(BdrvChild *file, uint64_t offset, size_t n,

- char *buf, size_t buflen)

+static int coroutine_fn GRAPH_RDLOCK

+qed_read_string(BdrvChild *file, uint64_t offset,

+ size_t n, char *buf, size_t buflen)

{

int ret;

if (n >= buflen) {

return -EINVAL;

}

- ret = bdrv_pread(file, offset, n, buf, 0);

+ ret = bdrv_co_pread(file, offset, n, buf, 0);

if (ret < 0) {

return ret;

}

--

2.41.0

From: Paolo Bonzini <pbonzini@redhat.com>

Mark functions as coroutine_fn when they are only called by other coroutine_fns

and they can suspend. Change calls to co_wrappers to use the non-wrapped

functions, which in turn requires adding GRAPH_RDLOCK annotations.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Message-ID: <20230601115145.196465-4-pbonzini@redhat.com>

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

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

---

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

1 file changed, 26 insertions(+), 26 deletions(-)

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

index XXXXXXX..XXXXXXX 100644

--- a/block/vpc.c

+++ b/block/vpc.c

@@ -XXX,XX +XXX,XX @@ static int vpc_reopen_prepare(BDRVReopenState *state,

* operation (the block bitmaps is updated then), 0 otherwise.

* If write is true then err must not be NULL.

*/

-static inline int64_t get_image_offset(BlockDriverState *bs, uint64_t offset,

- bool write, int *err)

+static int64_t coroutine_fn GRAPH_RDLOCK

+get_image_offset(BlockDriverState *bs, uint64_t offset, bool write, int *err)

{

BDRVVPCState *s = bs->opaque;

uint64_t bitmap_offset, block_offset;

@@ -XXX,XX +XXX,XX @@ static inline int64_t get_image_offset(BlockDriverState *bs, uint64_t offset,

s->last_bitmap_offset = bitmap_offset;

memset(bitmap, 0xff, s->bitmap_size);

- r = bdrv_pwrite_sync(bs->file, bitmap_offset, s->bitmap_size, bitmap,

- 0);

+ r = bdrv_co_pwrite_sync(bs->file, bitmap_offset, s->bitmap_size, bitmap, 0);

if (r < 0) {

*err = r;

return -2;

@@ -XXX,XX +XXX,XX @@ static inline int64_t get_image_offset(BlockDriverState *bs, uint64_t offset,

*

* Returns 0 on success and < 0 on error

*/

-static int rewrite_footer(BlockDriverState *bs)