Re: [Qemu-devel] [PATCH v3 8/9] qcow2: bdrv_co_pwritev: move encryption code out of the lock

[Alberto Garcia]

On Fri 18 Jan 2019 12:37:42 PM CET, Vladimir Sementsov-Ogievskiy wrote:
> 18.01.2019 12:51, Alberto Garcia wrote:
>> On Tue 08 Jan 2019 06:06:54 PM CET, Vladimir Sementsov-Ogievskiy wrote:
>>> Encryption will be done in threads, to take benefit of it, we should
>>> move it out of the lock first.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
>>> ---
>>>   block/qcow2.c | 35 +++++++++++++++++++++--------------
>>>   1 file changed, 21 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/block/qcow2.c b/block/qcow2.c
>>> index d6ef606d89..76d3715350 100644
>>> --- a/block/qcow2.c
>>> +++ b/block/qcow2.c
>>> @@ -2077,11 +2077,20 @@ static coroutine_fn int 
>>> qcow2_co_pwritev(BlockDriverState *bs, uint64_t offset,
>>>           ret = qcow2_alloc_cluster_offset(bs, offset, &cur_bytes,
>>>                                            &cluster_offset, &l2meta);
>>>           if (ret < 0) {
>>> -            goto fail;
>>> +            goto out_locked;
>>>           }
>>>   
>>>           assert((cluster_offset & 511) == 0);
>>>   
>>> +        ret = qcow2_pre_write_overlap_check(bs, 0,
>>> +                                            cluster_offset + 
>>> offset_in_cluster,
>>> +                                            cur_bytes);
>>> +        if (ret < 0) {
>>> +            goto out_locked;
>>> +        }
>>> +
>>> +        qemu_co_mutex_unlock(&s->lock);
>> 
>> The usage of lock() and unlock() functions inside and outside of the
>> loop plus the two 'locked' and 'unlocked' exit paths is starting to make
>> things a bit more messy.
>> 
>> Having a look at the code it seems that there's only these three parts
>> in the whole function that need to have the lock held:
>> 
>> one:
>>     ret = qcow2_alloc_cluster_offset(bs, offset, &cur_bytes,
>>                                      &cluster_offset, &l2meta);
>>     /* ... */
>>     ret = qcow2_pre_write_overlap_check(bs, 0,
>>                                         cluster_offset +
>>                                         offset_in_cluster,
>>                                         cur_bytes);
>> 
>> two:
>> 
>>     ret = qcow2_handle_l2meta(bs, &l2meta, true);
>> 
>> 
>> three:
>>     qcow2_handle_l2meta(bs, &l2meta, false);
>> 
>> 
>> So I wonder if it's perhaps simpler to add lock/unlock calls around
>> those blocks?
>
> this means, that we'll unlock after qcow2_handle_l2meta and then
> immediately lock on next iteration before qcow2_alloc_cluster_offset,
> so current code avoids this extra unlock/lock..

I don't have a very strong opinion, but maybe it's worth having if it
makes the code easier to read and maintain.

Berto