On 15/11/2021 13:00, Hanna Reitz wrote:
+
+ /*
+ * I/O API functions. These functions are thread-safe.
+ *
+ * See include/block/block-io.h for more information about
+ * the I/O API.
+ */
+
+ int coroutine_fn (*bdrv_co_create)(BlockdevCreateOptions *opts,
+ Error **errp);
+ int coroutine_fn (*bdrv_co_create_opts)(BlockDriver *drv,
+ const char *filename,
+ QemuOpts *opts,
+ Error **errp);
Now this is really interesting. Technically I suppose these should work
in any thread, but trying to do so results in:
$ 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
{"QMP": {"version": {"qemu": {"micro": 90, "minor": 1, "major": 6},
"package": "v6.2.0-rc0-40-gd02d5fe5fb-dirty"}, "capabilities": ["oob"]}}
{"return": {}}
{"return": {}}
{"return": {}}
{"timestamp": {"seconds": 1636973542, "microseconds": 338117}, "event":
"JOB_STATUS_CHANGE", "data": {"status": "created", "id": "create"}}
{"timestamp": {"seconds": 1636973542, "microseconds": 338197}, "event":
"JOB_STATUS_CHANGE", "data": {"status": "running", "id": "create"}}
{"return": {}}
qemu: qemu_mutex_unlock_impl: Operation not permitted
[1] 86154 IOT instruction (core dumped) ./qemu-system-x86_64 -object
iothread,id=iothr0 -qmp stdio <<<''
So something’s fishy and perhaps we should investigate this... I mean,
I can’t really imagine a case where someone would need to run a
blockdev-create job in an I/O thread, but right now the interface allows
for it.
And then bdrv_create() is classified as global state, and also
bdrv_co_create_opts_simple(), which is supposed to be a drop-in function
for this .bdrv_co_create_opts function. So that can’t work.
Also, I believe there might have been some functions you classified as
GS that are called from .create* implementations. I accepted that,
given the abort I sketched above. However, if we classify image
creation as I/O, then those would need to be re-evaluated. For example,
qcow2_co_create_opts() calls bdrv_create_file(), which is a GS function.
Some of this issues could be addressed by making .bdrv_co_create_opts a
GS function and .bdrv_co_create an I/O function. I believe that would
be the ideal split, even though as shown above .bdrv_co_create doesn’t
work in an I/O thread, and then you have the issue of probably all
format drivers’ .bdrv_co_create implementations calling
bdrv_open_blockdev_ref(), which is a GS function.
(VMDK even calls blk_new_open(), blk_new_with_bs(), and blk_unref(),
none of which can ever be I/O functions, I think.)
I believe in practice the best is to for now classify all create-related
functions as GS functions. This is supported by the fact that
qmp_blockdev_create() specifically creates the create job in the main
context (with a TODO comment) and requires block drivers to error out
when they encounter a node in a different AioContext.
Ok after better reviewing this I agree with you:
- .bdrv_co_create_opts is for sure a GS function. It is called by
bdrv_create and it is asserted to be under BQL.
- .bdrv_co_create should also be a GS, and the easiest thing to do would
be to follow the existing TODO and make sure we cannot run it outside
the main loop. I think that I will put it as GS, and add the BQL
assertion to blockdev_create_run, so that if for some reasons someone
tries to do what you did above, will crash because of the assertion, and
not because of the aiocontext lock missing.
Emanuele
