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 <[email protected]> >>>> --- >>>> 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
