[v3,4/4] block: Mark 'block_resize' as coroutine
diff mbox series

Message ID 20200115122326.26393-5-kwolf@redhat.com
State New
Headers show
Series
  • qmp: Optionally run handlers in coroutines
Related show

Commit Message

Kevin Wolf Jan. 15, 2020, 12:23 p.m. UTC
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>
---
 qapi/block-core.json | 3 ++-
 blockdev.c           | 6 +++---
 2 files changed, 5 insertions(+), 4 deletions(-)

Comments

Markus Armbruster Jan. 16, 2020, 9:45 a.m. UTC | #1
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>
> ---
>  qapi/block-core.json | 3 ++-
>  blockdev.c           | 6 +++---
>  2 files changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 7ff5e5edaf..1dbb2a9901 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1341,7 +1341,8 @@
>  { 'command': 'block_resize',
>    'data': { '*device': 'str',
>              '*node-name': 'str',
> -            'size': 'int' } }
> +            'size': 'int' },
> +  'coroutine': true }
>  
>  ##
>  # @NewImageMode:
> 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?
Kevin Wolf Jan. 16, 2020, 10:13 a.m. UTC | #2
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.

Kevin
Markus Armbruster Jan. 16, 2020, 3:13 p.m. UTC | #3
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.
Kevin Wolf Jan. 16, 2020, 3:23 p.m. UTC | #4
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.

Kevin
Markus Armbruster Jan. 17, 2020, 5:44 a.m. UTC | #5
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.
Markus Armbruster Jan. 17, 2020, 8:13 a.m. UTC | #6
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.

With the coroutine_fn dropped:
Reviewed-by: Markus Armbruster <armbru@redhat.com>

Any plans to make more QMP commands 'coroutine': true?
Kevin Wolf Jan. 17, 2020, 9:13 a.m. UTC | #7
Am 17.01.2020 um 09: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.
> 
> With the coroutine_fn dropped:
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> 
> Any plans to make more QMP commands 'coroutine': true?

No immediate plans from me. Myself, I was interested in block_resize
because without the conversion, Max wanted me to jump through some hoops
with a bdrv_truncate() fix to make sure that it wouldn't block the guest
during block_resize.

Of course, apart from that, there is Marc-André's long-standing
screendump bug that will finally have all of the required
infrastructure (and I think I saw a patch on the list already).

Kevin
Kevin Wolf Jan. 17, 2020, 9:24 a.m. UTC | #8
Am 17.01.2020 um 06:44 hat Markus Armbruster geschrieben:
> 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().

I think we would tend to mark things as coroutine_fn even if they don't
call other coroutine_fns (yet), but would be allowed to. But this is now
really splitting hairs.

Maybe I should just have referred to the documentation in the source:

/**
 * Mark a function that executes in coroutine context
 *
 * Functions that execute in coroutine context cannot be called directly from
 * normal functions.  In the future it would be nice to enable compiler or
 * static checker support for catching such errors.  This annotation might make
 * it possible and in the meantime it serves as documentation.
 *
 * For example:
 *
 *   static void coroutine_fn foo(void) {
 *       ....
 *   }
 */
#define coroutine_fn

Kevin
Markus Armbruster Jan. 17, 2020, 10:46 a.m. UTC | #9
Kevin Wolf <kwolf@redhat.com> writes:

> Am 17.01.2020 um 06:44 hat Markus Armbruster geschrieben:
>> 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:
[...]
>> >> >> 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().
>
> I think we would tend to mark things as coroutine_fn even if they don't
> call other coroutine_fns (yet), but would be allowed to. But this is now
> really splitting hairs.

Your hair-splitting is my education :)

> Maybe I should just have referred to the documentation in the source:
>
> /**
>  * Mark a function that executes in coroutine context
>  *
>  * Functions that execute in coroutine context cannot be called directly from
>  * normal functions.  In the future it would be nice to enable compiler or
>  * static checker support for catching such errors.  This annotation might make
>  * it possible and in the meantime it serves as documentation.
>  *
>  * For example:
>  *
>  *   static void coroutine_fn foo(void) {
>  *       ....
>  *   }
>  */
> #define coroutine_fn

I had read that, of course, but it didn't quite enlighten me, so I
asked.

Perhaps it would have if it said "Mark a function that expects to run in
coroutine context".

Patch
diff mbox series

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 7ff5e5edaf..1dbb2a9901 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1341,7 +1341,8 @@ 
 { 'command': 'block_resize',
   'data': { '*device': 'str',
             '*node-name': 'str',
-            'size': 'int' } }
+            'size': 'int' },
+  'coroutine': true }
 
 ##
 # @NewImageMode:
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;