[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v5 22/31] block_int-common.h: assertion in the callers of Blo
From: |
Emanuele Giuseppe Esposito |
Subject: |
Re: [PATCH v5 22/31] block_int-common.h: assertion in the callers of BlockDriver function pointers |
Date: |
Fri, 17 Dec 2021 16:53:34 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.2.0 |
On 16/12/2021 19:43, Hanna Reitz wrote:
On 24.11.21 07:44, Emanuele Giuseppe Esposito wrote:
Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
---
block.c | 18 ++++++++++++++++++
block/create.c | 10 ++++++++++
2 files changed, 28 insertions(+)
[...]
diff --git a/block/create.c b/block/create.c
index 89812669df..0167118579 100644
--- a/block/create.c
+++ b/block/create.c
@@ -42,6 +42,16 @@ static int coroutine_fn blockdev_create_run(Job
*job, Error **errp)
BlockdevCreateJob *s = container_of(job, BlockdevCreateJob,
common);
int ret;
+ /*
+ * Currently there is nothing preventing this
+ * function from being called in an iothread context.
+ * However, since it will crash anyways because of the
+ * aiocontext lock not taken, we might as well make it
+ * crash with a more meaningful error, by checking that
+ * we are in the main loop
+ */
+ assert(qemu_in_main_thread());
Mostly agreed. This function is always run in the main loop right now,
so this assertion will never fail.
But that’s the “mostly”: Adding this assertion won’t give a more
meaningful error, because the problem still remains that block drivers
do not error out when encountering (or correctly handle) BDSs in
non-main contexts, and so it remains a “qemu_mutex_unlock_impl:
Operation not permitted”.
Not trying to say that that’s your problem. It’s pre-existing, and this
assertion is good. Just wanting to clarify something about the comment
that seemed unclear to me (in that I found it implied that the
qemu_mutex_unlock_impl error would be replaced by this assertion failing).
You are right. Trying your example given in v4:
$ touch /tmp/iothread-create-test.qcow2
$ ./qemu-system-x86_64 -object iothread,id=iothr0 -qmp stdio <<EOF
{"execute": "qmp_capabilities"}
{"execute":"blockdev-add","arguments":{"node-name":"proto","driver":"file","filename":"/tmp/iothread-create-test.qcow2"}}
{"execute":"x-blockdev-set-iothread","arguments":{"node-name":"proto","iothread":"iothr0"}}
{"execute":"blockdev-create","arguments":{"job-id":"create","options":{"driver":"qcow2","file":"proto","size":0}}}
EOF
I still get "qemu_mutex_unlock_impl: Operation not permitted"
I will remove the comment above the assertion, makes no sense.
Or should I replace it with a TODO/FIXME explaining the above? Something
like:
/*
* TODO: it is currently possible to run a blockdev-create job in an
* I/O thread, for example by doing:
* [ command line above ]
* This should not be allowed.
*/
What do you think?
Emanuele
+
job_progress_set_remaining(&s->common, 1);
ret = s->drv->bdrv_co_create(s->opts, errp);
job_progress_update(&s->common, 1);