Re: [PATCH v3 4/4] block: Mark 'block_resize' as coroutine

From: Markus Armbruster <armbru@redhat.com>
To: Kevin Wolf <kwolf@redhat.com>
Cc: stefanha@redhat.com, marcandre.lureau@gmail.com,
	qemu-devel@nongnu.org, qemu-block@nongnu.org
Subject: Re: [PATCH v3 4/4] block: Mark 'block_resize' as coroutine
Date: Fri, 17 Jan 2020 06:44:53 +0100	[thread overview]
Message-ID: <871rryr60a.fsf@dusky.pond.sub.org> (raw)
In-Reply-To: <20200116152312.GI9470@linux.fritz.box> (Kevin Wolf's message of "Thu, 16 Jan 2020 16:23:12 +0100")

Kevin Wolf <kwolf@redhat.com> writes:

> Am 16.01.2020 um 16:13 hat Markus Armbruster geschrieben:
>> Kevin Wolf <kwolf@redhat.com> writes:
>> 
>> > Am 16.01.2020 um 10:45 hat Markus Armbruster geschrieben:
>> >> Kevin Wolf <kwolf@redhat.com> writes:
>> >> > block_resize is safe to run in a coroutine, so use it as an example for
>> >> > the new 'coroutine': true annotation in the QAPI schema.
>> >> >
>> >> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>> >> > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> >
>> >> > diff --git a/blockdev.c b/blockdev.c
>> >> > index 8e029e9c01..b5e5d1e072 100644
>> >> > --- a/blockdev.c
>> >> > +++ b/blockdev.c
>> >> > @@ -3161,9 +3161,9 @@ void hmp_drive_del(Monitor *mon, const QDict *qdict)
>> >> >      aio_context_release(aio_context);
>> >> >  }
>> >> >  
>> >> > -void qmp_block_resize(bool has_device, const char *device,
>> >> > -                      bool has_node_name, const char *node_name,
>> >> > -                      int64_t size, Error **errp)
>> >> > +void coroutine_fn qmp_block_resize(bool has_device, const char *device,
>> >> > +                                   bool has_node_name, const char *node_name,
>> >> > +                                   int64_t size, Error **errp)
>> >> >  {
>> >> >      Error *local_err = NULL;
>> >> >      BlockBackend *blk = NULL;
>> >> 
>> >> Pardon my ignorant question: what exactly makes a function a
>> >> coroutine_fn?
>> >
>> > When Stefan requested adding the coroutine_fn marker, it seemed to make
>> > sense to me because the QMP dispatcher will always call it from
>> > coroutine context now, and being always run in coroutine context makes a
>> > function a coroutine_fn.
>> >
>> > However, it's also called from hmp_block_resize(), so at least for now
>> > coroutine_fn is actually wrong.
>> 
>> This answers the question when we mark a function a coroutine_fn.  I
>> meant to ask what conditions the function itself must satisfy to be
>> eligible for this mark.
>
> The requirement is actually not about the function itself, it's about
> the callers, as stated above.
>
> But being a coroutine_fn allows the function to call other functions
> that only work in coroutine context (other coroutine_fns). In the end
> the reason why a function only works in coroutine context is usually
> that it (or any other coroutine_fns called by it) could yield, which
> obviously doesn't work outside of coroutine contest.

Thanks.

I think "being always run in coroutine context makes a function a
coroutine_fn" is inaccurate.  It's "calling a coroutine_fn without
switching to coroutine context first when not already in coroutine
context".  The induction terminates at basic coroutine_fn like
qemu_coroutine_yield().

Pertinent:

    /**
     * Return whether or not currently inside a coroutine
     *
     * This can be used to write functions that work both when in coroutine context
     * and when not in coroutine context.  Note that such functions cannot use the
     * coroutine_fn annotation since they work outside coroutine context.
     */
    bool qemu_in_coroutine(void);

For qmp_block_resize(), it's used like this, in bdrv_truncate():

    if (qemu_in_coroutine()) {
        /* Fast-path if already in coroutine context */
        bdrv_truncate_co_entry(&tco);
    } else {
        co = qemu_coroutine_create(bdrv_truncate_co_entry, &tco);
        bdrv_coroutine_enter(child->bs, co);
        BDRV_POLL_WHILE(child->bs, tco.ret == NOT_DONE);
    }

where bdrv_truncate_co_entry() is a coroutine_fn.