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

[Kevin Wolf]

Am 24.10.2019 um 13:14 hat Denis Lunev geschrieben:
> On 10/24/19 1:57 PM, Kevin Wolf wrote:
> > 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 <address@hidden>
> >>> ---
> >>>  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.
> >
> Hmm. It seems I was not careful enough with reading entire message.
> I am fine with this though it looks a bit tricky to me as such things
> can change in the future.

In which way do you think this could change? It's a pretty fundamental
fact about non-coroutine code that it can't yield.

What could change, of course, is that some code switches from being
synchronous to using a coroutine. The assertion would automatically
apply then and catch the bug if adding proper locking is forgotten.

> Anyway, you could consider this as
> 
> Reviewed-by: Denis V. Lunev <address@hidden>

Thanks!

Kevin