Re: [Qemu-block] [PATCH 2/7] qcow2: bdrv_co_pwritev: move encryption code out of lock

[Max Reitz]

On 07.08.18 19:43, Vladimir Sementsov-Ogievskiy wrote:
> We don't need locking for encryption code.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> ---
>  block/qcow2.c | 20 ++++++++++++--------
>  1 file changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 41def07c67..65e3ca00e2 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -2039,6 +2039,15 @@ static coroutine_fn int 
> qcow2_co_pwritev(BlockDriverState *bs, uint64_t offset,
>  
>          assert((cluster_offset & 511) == 0);
>  
> +        ret = qcow2_pre_write_overlap_check(bs, 0,
> +                                            cluster_offset + 
> offset_in_cluster,
> +                                            cur_bytes);
> +        if (ret < 0) {
> +            goto fail;
> +        }
> +
> +        qemu_co_mutex_unlock(&s->lock);
> +
>          qemu_iovec_reset(&hd_qiov);
>          qemu_iovec_concat(&hd_qiov, qiov, bytes_done, cur_bytes);
>  
> @@ -2072,30 +2081,25 @@ static coroutine_fn int 
> qcow2_co_pwritev(BlockDriverState *bs, uint64_t offset,
>              qemu_iovec_add(&hd_qiov, cluster_data, cur_bytes);
>          }
>  
> -        ret = qcow2_pre_write_overlap_check(bs, 0,
> -                cluster_offset + offset_in_cluster, cur_bytes);
> -        if (ret < 0) {
> -            goto fail;
> -        }
> -
>          /* If we need to do COW, check if it's possible to merge the
>           * writing of the guest data together with that of the COW regions.
>           * If it's not possible (or not necessary) then write the
>           * guest data now. */
>          if (!merge_cow(offset, cur_bytes, &hd_qiov, l2meta)) {
> -            qemu_co_mutex_unlock(&s->lock);
>              BLKDBG_EVENT(bs->file, BLKDBG_WRITE_AIO);
>              trace_qcow2_writev_data(qemu_coroutine_self(),
>                                      cluster_offset + offset_in_cluster);
>              ret = bdrv_co_pwritev(bs->file,
>                                    cluster_offset + offset_in_cluster,
>                                    cur_bytes, &hd_qiov, 0);
> -            qemu_co_mutex_lock(&s->lock);
>              if (ret < 0) {
> +                qemu_co_mutex_lock(&s->lock);
>                  goto fail;

There are a couple more failure places in between that probably need a
re-locking.

Otherwise, the patch looks good.

Max

>              }
>          }
>  
> +        qemu_co_mutex_lock(&s->lock);
> +
>          ret = qcow2_handle_l2meta(bs, &l2meta, true);
>          if (ret) {
>              goto fail;
>

signature.asc
Description: OpenPGP digital signature