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

Hi

On Fri, Jul 24, 2026 at 5:20 PM Kevin Wolf <[email protected]> wrote:
>
> 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 <[email protected]>
>
> > 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?
>

Indeed, that's a wrong annotation I added

> 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.

it calls qcow2_update_options_commit(), which itself calls coroutine
cache_clean_timer_co_locked_del_and_wait(bs).

But since s_locked is false, the function is safe it call in non-coroutine.

Given that there are 2 callers, I suggest we move the coroutine call
outside and drop the annotation.


>
> The other parts of the patch looks correct.
>
> Kevin
>
>