Re: [PATCH v4 18/30] qcow2: Add subcluster support to qcow2_get_host_offset()

[Vladimir Sementsov-Ogievskiy]

17.03.2020 21:16, Alberto Garcia wrote:
> The logic of this function remains pretty much the same, except that
> it uses count_contiguous_subclusters(), which combines the logic of
> count_contiguous_clusters() / count_contiguous_clusters_unallocated()
> and checks individual subclusters.
>
> Signed-off-by: Alberto Garcia <address@hidden>

Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>

> ---

[..]

> +static int count_contiguous_subclusters(BlockDriverState *bs, int nb_clusters,
> +                                        unsigned sc_index, uint64_t *l2_slice,
> +                                        int l2_index)
>   {
>       BDRVQcow2State *s = bs->opaque;

preexist, but, worth asserting that nb_clusters are all in this l2_slice?

[..]

> +        for (j = (i == 0) ? sc_index : 0; j < s->subclusters_per_cluster; j++) 
> {
> +            if (qcow2_get_subcluster_type(bs, l2_entry, l2_bitmap, j) != type) 
> {
> +                goto out;

why not just return count from here? And then you don't need goto at all. Hmm, 
may be out: code will be extended in further patches..

> +            }
> +            count++;
>           }
> +        expected_offset += s->cluster_size;
>       }
>   
> -    return i;
> +out:
> +    return count;
>   }
>   

[..]

> @@ -607,21 +607,20 @@ int qcow2_get_host_offset(BlockDriverState *bs, uint64_t 
> offset,
>               goto fail;
>           }
>           /* Compressed clusters can only be processed one by one */
> -        c = 1;
> +        sc = s->subclusters_per_cluster - sc_index;

should not we assert here that sc_index == 0? Otherwise the caller definitely 
doing something wrong.

>           *host_offset = l2_entry & L2E_COMPRESSED_OFFSET_SIZE_MASK;
>           break;
> -    case QCOW2_CLUSTER_ZERO_PLAIN:
> -    case QCOW2_CLUSTER_UNALLOCATED:
> -        /* how many empty clusters ? */
> -        c = count_contiguous_clusters_unallocated(bs, nb_clusters,
> -                                                  l2_slice, l2_index, type);
> +    case QCOW2_SUBCLUSTER_ZERO_PLAIN:
> +    case QCOW2_SUBCLUSTER_UNALLOCATED_PLAIN:
> +        sc = count_contiguous_subclusters(bs, nb_clusters, sc_index,
> +                                          l2_slice, l2_index);
>           *host_offset = 0;
>           break;
> -    case QCOW2_CLUSTER_ZERO_ALLOC:
> -    case QCOW2_CLUSTER_NORMAL:
> -        /* how many allocated clusters ? */
> -        c = count_contiguous_clusters(bs, nb_clusters, s->cluster_size,
> -                                      l2_slice, l2_index, QCOW_OFLAG_ZERO);
> +    case QCOW2_SUBCLUSTER_ZERO_ALLOC:
> +    case QCOW2_SUBCLUSTER_NORMAL:
> +    case QCOW2_SUBCLUSTER_UNALLOCATED_ALLOC:
> +        sc = count_contiguous_subclusters(bs, nb_clusters, sc_index,
> +                                          l2_slice, l2_index);
>           *host_offset = l2_entry & L2E_OFFSET_MASK;
>           if (offset_into_cluster(s, *host_offset)) {

Hmm, you may move "sc = count_contiguous_subclusters" to be after the 
switch-block, as it is universal now. And keep only offset calculation and error checking 
in the switch-block.

>               qcow2_signal_corruption(bs, true, -1, -1,
> @@ -651,7 +650,7 @@ int qcow2_get_host_offset(BlockDriverState *bs, uint64_t 
> offset,
>   
>       qcow2_cache_put(s->l2_table_cache, (void **) &l2_slice);
>   
> -    bytes_available = (int64_t)c * s->cluster_size;
> +    bytes_available = ((int64_t)sc + sc_index) << s->subcluster_bits;
>   
>   out:
>       if (bytes_available > bytes_needed) {
> @@ -664,7 +663,7 @@ out:
>       assert(bytes_available - offset_in_cluster <= UINT_MAX);
>       *bytes = bytes_available - offset_in_cluster;
>   
> -    *subcluster_type = qcow2_cluster_to_subcluster_type(type);
> +    *subcluster_type = type;
>   
>       return 0;
>   


--
Best regards,
Vladimir