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

[Alberto Garcia]

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?

Berto