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

[Max Reitz]

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?

Max

>          if (ret < 0) {
> +            qemu_co_mutex_unlock(&s->lock);
>              goto fail;
>          }
>

signature.asc
Description: OpenPGP digital signature