Re: [PATCH 6/8] block: add missing coroutine_fn annotations

[Kevin Wolf <kwolf@redhat.com>]

Am 20.07.2026 um 09:55 hat Marc-André Lureau geschrieben:
> The functions call coroutine functions or are called by coroutine.

That it's called only by coroutines isn't a reason to make a function
coroutine_fn as long as it theoretically could be called by
non-coroutine code and it would still work fine (i.e. it doesn't yield).

> Add an assert() in qcow2_do_close() code path which calls a
> no_coroutine_fn bdrv_graph_wrlock_drained().
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

> diff --git a/block/parallels.c b/block/parallels.c
> index 7a90fb5220b..46a6e8f2743 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -142,8 +142,9 @@ static uint32_t host_cluster_index(BDRVParallelsState *s, int64_t off)
>      return off / s->cluster_size;
>  }
>  
> -static int64_t block_status(BDRVParallelsState *s, int64_t sector_num,
> -                            int nb_sectors, int *pnum)
> +static int64_t coroutine_fn
> +block_status(BDRVParallelsState *s, int64_t sector_num,
> +             int nb_sectors, int *pnum)
>  {
>      int64_t start_off = -2, prev_end_off = -2;
>  

Specifically this hunk is unclear to me.

As far as I can see, block_status() can't yield. Why did you add the
coroutine_fn marker here, but not e.g. for seek_to_sector() or
cluster_remainder(), which are only called by block_status() and
therefore also always only in coroutine context?

I believe the right way to stay consistent is to leave all of them
unmarked because there is no fundamental reason why they would only make
sense in coroutine context.

> diff --git a/block/qcow2.c b/block/qcow2.c
> index 19271b10a49..161626fa3cb 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1287,9 +1287,10 @@ fail:
>  }
>  
>  /* s_locked specifies whether s->lock is held or not */
> -static void qcow2_update_options_commit(BlockDriverState *bs,
> -                                        Qcow2ReopenState *r,
> -                                        bool s_locked)
> +static void coroutine_fn
> +qcow2_update_options_commit(BlockDriverState *bs,
> +                            Qcow2ReopenState *r,
> +                            bool s_locked)
>  {
>      BDRVQcow2State *s = bs->opaque;
>      int i;
> @@ -2126,7 +2127,8 @@ fail:
>      return ret;
>  }
>  
> -static void qcow2_reopen_commit(BDRVReopenState *state)
> +static void coroutine_fn
> +qcow2_reopen_commit(BDRVReopenState *state)
>  {
>      BDRVQcow2State *s = state->bs->opaque;
>  

This doesn't match the declaration of the .bdrv_reopen_commit callback
in BlockDriver, and as far as I can see, the callers are actually
guaranteed to _not_ be in coroutine context.

If this is true, qcow2_update_options_commit() should probably be marked
coroutine_mixed_fn and this one stay unannotated or no_coroutine_fn.

The other parts of the patch looks correct.

Kevin