[RFC,v2,14/26] qcow2: Add subcluster support to qcow2_get_cluster_offset()

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 <berto@igalia.com>
---
 block/qcow2-cluster.c | 111 ++++++++++++++++++++----------------------
 1 file changed, 52 insertions(+), 59 deletions(-)

On 26.10.19 23:25, 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 <berto@igalia.com>
> ---
>  block/qcow2-cluster.c | 111 ++++++++++++++++++++----------------------
>  1 file changed, 52 insertions(+), 59 deletions(-)
> 
> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index 990bc070af..e67559152f 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
> @@ -372,66 +372,51 @@ fail:
>  }
>  
>  /*
> - * Checks how many clusters in a given L2 slice are contiguous in the image
> - * file. As soon as one of the flags in the bitmask stop_flags changes compared
> - * to the first cluster, the search is stopped and the cluster is not counted
> - * as contiguous. (This allows it, for example, to stop at the first compressed
> - * cluster which may require a different handling)
> + * Return the number of contiguous subclusters of the exact same type
> + * in a given L2 slice, starting from cluster @l2_index, subcluster
> + * @sc_index. At most @nb_clusters are checked. Allocated clusters are

I’d add a note that reassures that @nb_clusters really is a number of
clusters, not subclusters.  (Because if the variable were named “x”
(i.e., it’d be “At most @x are checked”), you’d assume it refers to a
number of subclusters.)

OTOH, what I don’t like so far about this series is that the “cluster
logic” is still everywhere when I think it should just be about
subclusters now.  (Except in few places where it must be about clusters
as in something that can have a distinct host offset and/or has an own
L2 entry.)  So maybe the parameter should really be @nb_subclusters.

But I’m not sure.  For how this function is written right now, it makes
sense for it to be @nb_clusters, but I think it could be changed so it
would work with @nb_subclusters, too.  (Just a single loop over the
subclusters and then loading l2_entry+l2_bitmap and checking the offset
at every cluster boundary.)

> + * also required to be contiguous in the image file.
>   */
> -static int count_contiguous_clusters(BlockDriverState *bs, int nb_clusters,
> -        int cluster_size, uint64_t *l2_slice, int l2_index, uint64_t stop_flags)
> +static int count_contiguous_subclusters(BlockDriverState *bs, int nb_clusters,
> +                                        unsigned sc_index, uint64_t *l2_slice,
> +                                        int l2_index)
>  {
>      BDRVQcow2State *s = bs->opaque;
> -    int i;
> -    QCow2ClusterType first_cluster_type;
> -    uint64_t mask = stop_flags | L2E_OFFSET_MASK | QCOW_OFLAG_COMPRESSED;
> -    uint64_t first_entry = get_l2_entry(s, l2_slice, l2_index);
> -    uint64_t offset = first_entry & mask;
> -
> -    first_cluster_type = qcow2_get_cluster_type(bs, first_entry);
> -    if (first_cluster_type == QCOW2_CLUSTER_UNALLOCATED) {
> -        return 0;
> +    int i, j, count = 0;
> +    uint64_t l2_entry = get_l2_entry(s, l2_slice, l2_index);
> +    uint64_t l2_bitmap = get_l2_bitmap(s, l2_slice, l2_index);
> +    uint64_t expected_offset = l2_entry & L2E_OFFSET_MASK;
> +    bool check_offset = true;
> +    QCow2ClusterType type =
> +        qcow2_get_subcluster_type(bs, l2_entry, l2_bitmap, sc_index);
> +
> +    assert(type != QCOW2_CLUSTER_INVALID); /* The caller should check this */
> +
> +    if (type == QCOW2_CLUSTER_COMPRESSED) {
> +        return 1; /* Compressed clusters are always counted one by one */

Hm, yes, but cluster by cluster, not subcluster by subcluster, so this
should be s->subclusters_per_cluster, and perhaps sc_index should be
asserted to be 0.  (Or it should be s->subclusters_per_cluster - sc_index.)

[...]

> @@ -514,8 +499,8 @@ int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t offset,

I suppose this is get_subcluster_offset now.

[...]

> @@ -587,6 +574,13 @@ int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t offset,
>          goto fail;
>      }
>      switch (type) {
> +    case QCOW2_CLUSTER_INVALID:
> +        qcow2_signal_corruption(bs, true, -1, -1, "Invalid cluster entry found "
> +                                " (L2 offset: %#" PRIx64 ", L2 index: %#x)",
> +                                l2_offset, l2_index);
> +        ret = -EIO;
> +        goto fail;
> +        break;

No need to break here.

>      case QCOW2_CLUSTER_COMPRESSED:
>          if (has_data_file(bs)) {
>              qcow2_signal_corruption(bs, true, -1, -1, "Compressed cluster "
> @@ -602,16 +596,15 @@ int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t offset,
>          break;

Here (QCOW2_CLUSTER_COMPRESSED), c is 1, even though it should count
subclusters.

Max

>      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);
> +        c = count_contiguous_subclusters(bs, nb_clusters, sc_index,
> +                                         l2_slice, l2_index);
>          *cluster_offset = 0;
>          break;

On Mon 04 Nov 2019 03:58:57 PM CET, Max Reitz wrote:
> OTOH, what I don’t like so far about this series is that the “cluster
> logic” is still everywhere when I think it should just be about
> subclusters now.  (Except in few places where it must be about
> clusters as in something that can have a distinct host offset and/or
> has an own L2 entry.)  So maybe the parameter should really be
> @nb_subclusters.

> But I’m not sure.  For how this function is written right now, it
> makes sense for it to be @nb_clusters, but I think it could be changed
> so it would work with @nb_subclusters, too.

I'm still reviewing your (much appreciated) feedback, but one thing I
can tell you is that my initial versions were doing everything with
subclusters because of the reasons you mention (i.e. there was
@nb_subclusters and all that).

Later when I started to tidy things up I realized that most of those
places only needed the number of clusters after all, and in some cases
the necessary changes were really minimal (like in handle_copied()).

>> +static int count_contiguous_subclusters(BlockDriverState *bs, int nb_clusters,
>> +                                        unsigned sc_index, uint64_t *l2_slice,
>> +                                        int l2_index)
>>  {
   /* ... */
>> +    if (type == QCOW2_CLUSTER_COMPRESSED) {
>> +        return 1; /* Compressed clusters are always counted one by one */
>
> Hm, yes, but cluster by cluster, not subcluster by subcluster, so this
> should be s->subclusters_per_cluster, and perhaps sc_index should be
> asserted to be 0.  (Or it should be s->subclusters_per_cluster -
> sc_index.)

Right, that's a bug, it forces the caller to decompress the cluster 32
times in order to read it completely! Thanks!

(in reality this is not used because this function doesn't get called
for compressed clusters but the same problem happens in the calling
function, as you correctly point out. Maybe I should assert here
instead)

>> @@ -514,8 +499,8 @@ int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t offset,
>
> I suppose this is get_subcluster_offset now.

Hmmm no, this returns the actual host cluster offset, then the caller
uses offset_into_cluster() to get the final value (which doesn't need to
be subcluster-aligned anyway).

Berto

On 08.11.19 16:42, Alberto Garcia wrote:
> On Mon 04 Nov 2019 03:58:57 PM CET, Max Reitz wrote:

[...]

>>> @@ -514,8 +499,8 @@ int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t offset,
>>
>> I suppose this is get_subcluster_offset now.
> 
> Hmmm no, this returns the actual host cluster offset, then the caller
> uses offset_into_cluster() to get the final value (which doesn't need to
> be subcluster-aligned anyway).

It still returns the subcluster type, though.  Which makes the whole
thing kind of a weird chimera...  Maybe it would work better if the
subcluster type would be stored in a pointer parameter, so it’d be clear?

But does it have actually have any advantages to on one hand return a
cluster-aligned host offset and on the other return a bytes count that
starts at the mid-cluster offset?  That’s weird already.  Maybe the
offset that’s returned should just not be cluster-aligned and the whole
function be called qcow2_get_host_offset().

I suppose one reason (maybe the only one?) to return aligned offsets is
for compressed clusters.  It would be another kind of magic to return
aligned offsets only for them.  But maybe it’s worth it?  (Judging from
a quick glance, it doesn’t look too difficult to me to modify the
callers to accept a mid-cluster host offset.)

(Another thing I just noticed is that the comment above
qcow2_get_cluster_offset() needs some fixing, because it still refers to
whole clusters.)

Max