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