Re: [PATCH 2/3] qcow2: Assert that qcow2_cache_get() callers hold s->lock

[Kevin Wolf <kwolf@redhat.com>]

Am 24.10.2019 um 12:01 hat Denis Lunev geschrieben:
> On 10/23/19 6:26 PM, Kevin Wolf wrote:
> > qcow2_cache_do_get() requires that s->lock is locked because it can
> > yield between picking a cache entry and actually taking ownership of it
> > by setting offset and increasing the reference count.
> >
> > Add an assertion to make sure the caller really holds the lock. The
> > function can be called outside of coroutine context, where bdrv_pread
> > and flushes become synchronous operations. The lock cannot and need not
> > be taken in this case.
> >
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  block/qcow2-cache.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/block/qcow2-cache.c b/block/qcow2-cache.c
> > index d29b038a67..75b13dad99 100644
> > --- a/block/qcow2-cache.c
> > +++ b/block/qcow2-cache.c
> > @@ -327,6 +327,9 @@ static int qcow2_cache_do_get(BlockDriverState *bs, Qcow2Cache *c,
> >      int min_lru_index = -1;
> >  
> >      assert(offset != 0);
> > +    if (qemu_in_coroutine()) {
> > +        qemu_co_mutex_assert_locked(&s->lock);
> > +    }
> 
> that is looking not good to me. If this is really requires lock, we should
> check for the lock always. In the other hand we could face missed
> lock out of coroutine.

As the commit message explains, outside of coroutine context, we can't
yield and bdrv_pread and bdrv_flush become synchronous operations
instead, so there is nothing else that we need to protect against.

Kevin