Re: [Qemu-devel] [PATCH 3/5] block: convert bdrv_invalidate_cache callback to coroutine_fn

[Kevin Wolf]

Am 11.07.2017 um 10:33 hat Paolo Bonzini geschrieben:
> On 11/07/2017 10:24, Kevin Wolf wrote:
> > Am 10.07.2017 um 18:58 hat Paolo Bonzini geschrieben:
> >> QED's bdrv_invalidate_cache implementation would like to reuse functions
> >> that acquire/release the metadata locks.  Call it from coroutine context
> >> to simplify the logic.
> >>
> >> Signed-off-by: Paolo Bonzini <address@hidden>
> > 
> >> diff --git a/block/qcow2.c b/block/qcow2.c
> >> index b5de67d113..4ff7e89009 100644
> >> --- a/block/qcow2.c
> >> +++ b/block/qcow2.c
> >> @@ -1854,7 +1854,8 @@ static void qcow2_close(BlockDriverState *bs)
> >>      qcow2_free_snapshots(bs);
> >>  }
> >>  
> >> -static void qcow2_invalidate_cache(BlockDriverState *bs, Error **errp)
> >> +static void coroutine_fn qcow2_co_invalidate_cache(BlockDriverState *bs,
> >> +                                                   Error **errp)
> >>  {
> >>      BDRVQcow2State *s = bs->opaque;
> >>      int flags = s->flags;
> >> @@ -1877,7 +1878,9 @@ static void qcow2_invalidate_cache(BlockDriverState 
> >> *bs, Error **errp)
> >>      options = qdict_clone_shallow(bs->options);
> >>  
> >>      flags &= ~BDRV_O_INACTIVE;
> >> +    qemu_co_mutex_lock(&s->lock);
> >>      ret = qcow2_do_open(bs, options, flags, &local_err);
> >> +    qemu_co_mutex_unlock(&s->lock);
> >>      QDECREF(options);
> >>      if (local_err) {
> >>          error_propagate(errp, local_err);
> > 
> > How useful is this lock really? If we expect any requests while this
> > function is running (we don't, it would break horribly), it should
> > probably cover qcow2_close(), too.
> 
> In the case of QED, this was needed because qed_read_table drops
> s->table_lock.  Here I just copied the idiom.

Ok, makes sense.

> However, I think it's cleaner to define which functions are called
> with which lock held.  Sometimes it leads to taking a lock
> unnecessarily, but it's clearer and it can also help once we annotate
> the source and let the compiler do static analysis (see clang's
> -Wthread-safety).

Can you just add a comment to this effect to qcow2/qed_do_open() then?

> > Or maybe the cleaner way would be bdrv_drained_begin/end() around
> > everything directly in bdrv_co_invalidate_cache().

Should we do this anyway? Or assert that no requests are in flight?

Kevin