Re: [PATCH v5 22/31] block_int-common.h: assertion in the callers of BlockDriver function pointers

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

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