Re: [PATCH v4 14/30] qcow2: Add cluster type parameter to qcow2_get_host_offset()

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

17.03.2020 21:16, Alberto Garcia wrote:
> This function returns an integer that can be either an error code or a
> cluster type (a value from the QCow2ClusterType enum).
> 
> We are going to start using subcluster types instead of cluster types
> in some functions so it's better to use the exact data types instead
> of integers for clarity and in order to detect errors more easily.
> 
> This patch makes qcow2_get_host_offset() return 0 on success and
> puts the returned cluster type in a separate parameter. There are no
> semantic changes.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>   block/qcow2.h         |  3 ++-
>   block/qcow2-cluster.c | 11 +++++++----
>   block/qcow2.c         | 37 ++++++++++++++++++++++---------------
>   3 files changed, 31 insertions(+), 20 deletions(-)
> 
> diff --git a/block/qcow2.h b/block/qcow2.h
> index 52865787ee..6b7b286b91 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h


[..]

> @@ -3716,6 +3719,7 @@ static coroutine_fn int qcow2_co_pwrite_zeroes(BlockDriverState *bs,
>       if (head || tail) {
>           uint64_t off;
>           unsigned int nr;
> +        QCow2ClusterType type;
>   
>           assert(head + bytes <= s->cluster_size);
>   
> @@ -3731,10 +3735,11 @@ static coroutine_fn int qcow2_co_pwrite_zeroes(BlockDriverState *bs,
>           offset = QEMU_ALIGN_DOWN(offset, s->cluster_size);
>           bytes = s->cluster_size;
>           nr = s->cluster_size;
> -        ret = qcow2_get_host_offset(bs, offset, &nr, &off);
> -        if (ret != QCOW2_CLUSTER_UNALLOCATED &&
> -            ret != QCOW2_CLUSTER_ZERO_PLAIN &&
> -            ret != QCOW2_CLUSTER_ZERO_ALLOC) {
> +        ret = qcow2_get_host_offset(bs, offset, &nr, &off, &type);

pre-patch, but probably better to return original errno on qcow2_get_host_offset failure, instead of masking it.

> +        if (ret < 0 ||
> +            (type != QCOW2_CLUSTER_UNALLOCATED &&
> +             type != QCOW2_CLUSTER_ZERO_PLAIN &&
> +             type != QCOW2_CLUSTER_ZERO_ALLOC)) {
>               qemu_co_mutex_unlock(&s->lock);
>               return -ENOTSUP;
>           }
> @@ -3792,16 +3797,18 @@ qcow2_co_copy_range_from(BlockDriverState *bs,
>   
>       while (bytes != 0) {
>           uint64_t copy_offset = 0;
> +        QCow2ClusterType type;
>           /* prepare next request */
>           cur_bytes = MIN(bytes, INT_MAX);
>           cur_write_flags = write_flags;
>   
> -        ret = qcow2_get_host_offset(bs, src_offset, &cur_bytes, &copy_offset);
> +        ret = qcow2_get_host_offset(bs, src_offset, &cur_bytes,
> +                                    &copy_offset, &type);
>           if (ret < 0) {
>               goto out;
>           }
>   
> -        switch (ret) {
> +        switch (type) {
>           case QCOW2_CLUSTER_UNALLOCATED:
>               if (bs->backing && bs->backing->bs) {
>                   int64_t backing_length = bdrv_getlength(bs->backing->bs);
> 
Hmm, just noted that in case of bdrv_co_copy_range_from failure below, we do mutex lock/unlock for nothing.

I think, we want mutex lock/unlock just around qcow2_co_preadv_part() call, like in  qcow2_co_preadv_part above().

I can send a refactoring patch..

Anyway, patch itself is OK:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir