Re: [PATCH 01/20] qcow2: Don't call bdrv_getlength() in coroutine_fns

[Kevin Wolf <kwolf@redhat.com>]

Am 25.04.2023 um 20:37 hat Eric Blake geschrieben:
> On Tue, Apr 25, 2023 at 07:31:39PM +0200, Kevin Wolf wrote:
> > There is a bdrv_co_getlength() now, which should be used in coroutine
> > context.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  block/qcow2.h          |  4 +++-
> >  block/qcow2-refcount.c |  2 +-
> >  block/qcow2.c          | 19 +++++++++----------
> >  3 files changed, 13 insertions(+), 12 deletions(-)
> 
> > 
> > diff --git a/block/qcow2.h b/block/qcow2.h
> > index c75decc38a..4f67eb912a 100644
> > --- a/block/qcow2.h
> > +++ b/block/qcow2.h
> > @@ -895,7 +895,9 @@ int qcow2_change_refcount_order(BlockDriverState *bs, int refcount_order,
> >                                  void *cb_opaque, Error **errp);
> >  int coroutine_fn GRAPH_RDLOCK qcow2_shrink_reftable(BlockDriverState *bs);
> >  int64_t coroutine_fn qcow2_get_last_cluster(BlockDriverState *bs, int64_t size);
> > -int coroutine_fn qcow2_detect_metadata_preallocation(BlockDriverState *bs);
> > +
> > +int coroutine_fn GRAPH_RDLOCK
> > +qcow2_detect_metadata_preallocation(BlockDriverState *bs);
> 
> > +++ b/block/qcow2.c
> > @@ -2089,11 +2089,10 @@ static void qcow2_join_options(QDict *options, QDict *old_options)
> >      }
> >  }
> >  
> > -static int coroutine_fn qcow2_co_block_status(BlockDriverState *bs,
> > -                                              bool want_zero,
> > -                                              int64_t offset, int64_t count,
> > -                                              int64_t *pnum, int64_t *map,
> > -                                              BlockDriverState **file)
> > +static int coroutine_fn GRAPH_RDLOCK
> > +qcow2_co_block_status(BlockDriverState *bs, bool want_zero, int64_t offset,
> > +                      int64_t count, int64_t *pnum, int64_t *map,
> > +                      BlockDriverState **file)
> >  {
> 
> Should the commit message also call out that you are using this
> opportunity to add GRAPH_RDLOCK annotations on affected functions?

This is not just some additional change I did on the side, but the patch
doesn't compile (on clang with TSA enabled) without it because
bdrv_co_getlength() is GRAPH_RDLOCK, so its callers already must hold
the lock.

I can be more explicit about it in this patch, though I expect that the
same situation might happen more often in the future, and I'm not sure
if we want to mention that in the commit message any more than why we're
passing through an Error pointer.

> Overall, all changes in this patch make sense, but I'm reluctant to
> add R-b unless you confirm whether this was a rebase mistake (where
> the annotations were intended to be added in a later patch).

No, it's intentional.

Kevin