[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-block] [PATCH v5 07/10] qcow2: qcow2_co_preadv: improve lockin

From: Max Reitz
Subject: Re: [Qemu-block] [PATCH v5 07/10] qcow2: qcow2_co_preadv: improve locking
Date: Fri, 3 May 2019 16:08:49 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.6.1

On 30.04.19 11:44, Vladimir Sementsov-Ogievskiy wrote:
> 30.04.2019 11:38, Vladimir Sementsov-Ogievskiy wrote:
>> 29.04.2019 19:37, Max Reitz wrote:
>>> On 02.04.19 17:37, Vladimir Sementsov-Ogievskiy wrote:
>>>> Background: decryption will be done in threads, to take benefit of it,
>>>> we should move it out of the lock first.
>>> ...which is safe after your commit c972fa123c73501b4, I presume.
>>> (At first glance, the patched looked a bit weird to me because it
>>> doesn't give a reason why dropping the lock around
>>> qcrypto_block_decrypt() would be OK.)
>>>> But let's go further: it turns out, that for locking around switch
>>>> cases we have only two variants: when we just do memset(0) not
>>>> releasing the lock (it is useless) and when we actually can handle the
>>>> whole case out of the lock. So, refactor the whole thing to reduce
>>>> locked code region and make it clean.
>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
>>>> Reviewed-by: Alberto Garcia <address@hidden>
>>>> ---
>>>>   block/qcow2.c | 46 ++++++++++++++++++++++------------------------
>>>>   1 file changed, 22 insertions(+), 24 deletions(-)
>>>> diff --git a/block/qcow2.c b/block/qcow2.c
>>>> index 46e8e39da5..fcf92a7eb6 100644
>>>> --- a/block/qcow2.c
>>>> +++ b/block/qcow2.c
>>>> @@ -1983,6 +1983,7 @@ static coroutine_fn int 
>>>> qcow2_co_preadv(BlockDriverState *bs, uint64_t offset,
>>>>           ret = qcow2_get_cluster_offset(bs, offset, &cur_bytes, 
>>>> &cluster_offset);
>>> Isn't this the only function in the loop that actually needs the lock?
>>> Wouldn't it make more sense to just take it around this call?
>> Hmm, looks correct, I'll resend.
> Or not, actually, we may have several qcow2_get_data_offset calls under one 
> lock,
> if clusters are different kinds of ZERO. So, I think better to keep it as is 
> for now.

Hm, but how is this relevant?  For one thing, if that was a problem if
some other party concurrently changes the image, then that would be a
problem in general.  Keeping the lock would hide it for different kinds
of read-as-zero clusters, but it would still appear if data clusters and
other clusters are interleaved, wouldn’t it?

Also, this is a coroutine.  As long as nothing yields, nothing gets
concurrent access.  I don’t see anything outside of
qcow2_get_cluster_offset() that could yield as long as we only see
read-as-zero clusters.


Attachment: signature.asc
Description: OpenPGP digital signature

reply via email to

[Prev in Thread] Current Thread [Next in Thread]