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

[Emanuele Giuseppe Esposito <eesposit@redhat.com>]

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