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

[Eric Blake]

On Thu, Apr 27, 2023 at 01:12:43PM +0200, Kevin Wolf wrote:
> 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>
> > > ---

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

Okay, that makes sense.

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

Still, a commit message something like:

There is a bdrv_co_getlength() now, which should be used in coroutine
context, which in turn requires more accurate function annotations.

would have helped me review faster.

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

All right, you've answered my question.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org