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

[Emanuele Giuseppe Esposito]

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