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

[Vladimir Sementsov-Ogievskiy]

18.01.2019 17:39, Alberto Garcia wrote:
> 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.
> 

Intuitively I think, that locks optimization worth this difficulty,
so I'd prefer to leave this logic as is, at least in these series.


-- 
Best regards,
Vladimir