[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v4 19/25] block_int-common.h: split function pointers in Bloc

From: Emanuele Giuseppe Esposito
Subject: Re: [PATCH v4 19/25] block_int-common.h: split function pointers in BlockDriver
Date: Thu, 18 Nov 2021 13:42:42 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.2.0

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}}}
{"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.


reply via email to

[Prev in Thread] Current Thread [Next in Thread]