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

[Denis Lunev]

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

Anyway, you could consider this as

Reviewed-by: Denis V. Lunev <address@hidden>

Den