Re: [Qemu-block] [PATCH v12 04/10] qcow2: Make distinction between zero cluster types obvious

[Max Reitz]

On 04.05.2017 05:07, Eric Blake wrote:
> Treat plain zero clusters differently from allocated ones, so that
> we can simplify the logic of checking whether an offset is present.
> Do this by splitting QCOW2_CLUSTER_ZERO into two new enums,
> QCOW2_CLUSTER_ZERO_PLAIN and QCOW2_CLUSTER_ZERO_ALLOC.
> 
> I tried to arrange the enum so that we could use
> 'ret <= QCOW2_CLUSTER_ZERO_PLAIN' for all unallocated types, and
> 'ret >= QCOW2_CLUSTER_ZERO_ALLOC' for allocated types, although
> I didn't actually end up taking advantage of the layout.
> 
> In many cases, this leads to simpler code, by properly combining
> cases (sometimes, both zero types pair together, other times,
> plain zero is more like unallocated while allocated zero is more
> like normal).
> 
> Signed-off-by: Eric Blake <address@hidden>
> 
> ---
> v12: new patch
> ---
>  block/qcow2.h          |  8 +++++--
>  block/qcow2-cluster.c  | 65 
> ++++++++++++++++++--------------------------------
>  block/qcow2-refcount.c | 40 +++++++++++++------------------
>  block/qcow2.c          |  9 ++++---
>  4 files changed, 51 insertions(+), 71 deletions(-)

I have to admit I was rather skeptic of this idea at first (because I
thought we would have more places which treat both ZERO types the same
than those that separate it), but you have comprehensively proven me wrong.

Some nit picks below, I'll leave it completely up to you whether you
want to address them:

Reviewed-by: Max Reitz <address@hidden>

[...]

> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index f3bfce6..14e2086 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c

[...]

> @@ -558,52 +557,32 @@ int qcow2_get_cluster_offset(BlockDriverState *bs, 
> uint64_t offset,
>      assert(nb_clusters <= INT_MAX);
> 
>      ret = qcow2_get_cluster_type(*cluster_offset);
> +    if (s->qcow_version < 3 && (ret == QCOW2_CLUSTER_ZERO_PLAIN ||
> +                                ret == QCOW2_CLUSTER_ZERO_ALLOC)) {
> +        qcow2_signal_corruption(bs, true, -1, -1, "Zero cluster entry found"
> +                                " in pre-v3 image (L2 offset: %#" PRIx64
> +                                ", L2 index: %#x)", l2_offset, l2_index);
> +        ret = -EIO;
> +        goto fail;
> +    }
>      switch (ret) {
>      case QCOW2_CLUSTER_COMPRESSED:
>          /* Compressed clusters can only be processed one by one */
>          c = 1;
>          *cluster_offset &= L2E_COMPRESSED_OFFSET_SIZE_MASK;
>          break;
> -    case QCOW2_CLUSTER_ZERO:
> -        if (s->qcow_version < 3) {
> -            qcow2_signal_corruption(bs, true, -1, -1, "Zero cluster entry 
> found"
> -                                    " in pre-v3 image (L2 offset: %#" PRIx64
> -                                    ", L2 index: %#x)", l2_offset, l2_index);
> -            ret = -EIO;
> -            goto fail;
> -        }
> -        /* Distinguish between pure zero clusters and pre-allocated ones */
> -        if (*cluster_offset & L2E_OFFSET_MASK) {
> -            c = count_contiguous_clusters(nb_clusters, s->cluster_size,
> -                                          &l2_table[l2_index], 
> QCOW_OFLAG_ZERO);
> -            *cluster_offset &= L2E_OFFSET_MASK;
> -            if (offset_into_cluster(s, *cluster_offset)) {
> -                qcow2_signal_corruption(bs, true, -1, -1,
> -                                        "Preallocated zero cluster offset %#"
> -                                        PRIx64 " unaligned (L2 offset: %#"
> -                                        PRIx64 ", L2 index: %#x)",
> -                                        *cluster_offset, l2_offset, 
> l2_index);
> -                ret = -EIO;
> -                goto fail;
> -            }
> -        } else {
> -            c = count_contiguous_clusters_unallocated(nb_clusters,
> -                                                      &l2_table[l2_index],
> -                                                      QCOW2_CLUSTER_ZERO);
> -            *cluster_offset = 0;
> -        }
> -        break;
> +    case QCOW2_CLUSTER_ZERO_PLAIN:
>      case QCOW2_CLUSTER_UNALLOCATED:
>          /* how many empty clusters ? */
>          c = count_contiguous_clusters_unallocated(nb_clusters,
> -                                                  &l2_table[l2_index],
> -                                                  QCOW2_CLUSTER_UNALLOCATED);
> +                                                  &l2_table[l2_index], ret);

Nit pick: Using ret here is a bit weird (because it's such a meaningless
name). It would be good if we had a separate cluster_type variable.

>          *cluster_offset = 0;
>          break;
> +    case QCOW2_CLUSTER_ZERO_ALLOC:
>      case QCOW2_CLUSTER_NORMAL:
>          /* how many allocated clusters ? */
>          c = count_contiguous_clusters(nb_clusters, s->cluster_size,
> -                &l2_table[l2_index], QCOW_OFLAG_ZERO);
> +                                      &l2_table[l2_index], QCOW_OFLAG_ZERO);
>          *cluster_offset &= L2E_OFFSET_MASK;
>          if (offset_into_cluster(s, *cluster_offset)) {
>              qcow2_signal_corruption(bs, true, -1, -1, "Data cluster offset 
> %#"

Well, preallocated zero clusters are not exactly data clusters... Not
that any user cared, but s/Data cluster/Cluster allocation/ would be
more correct.

By the way, allow me to state just how much I love this hunk: Very much.
Looks great! It gets a place on my list of favorite hunks of this year
at least.

[...]

> @@ -1760,7 +1740,8 @@ static int expand_zero_clusters_in_l1(BlockDriverState 
> *bs, uint64_t *l1_table,
>              int cluster_type = qcow2_get_cluster_type(l2_entry);>            
>   bool preallocated = offset != 0;

I could get behind removing this variable and replacing all
"if (!preallocated)" instances by
"if (cluster_type == QCOW2_CLUSTER_ZERO_PLAIN)". Up to you, though.

Max

> 
> -            if (cluster_type != QCOW2_CLUSTER_ZERO) {
> +            if (cluster_type != QCOW2_CLUSTER_ZERO_PLAIN &&
> +                cluster_type != QCOW2_CLUSTER_ZERO_ALLOC) {
>                  continue;
>              }
>

signature.asc
Description: OpenPGP digital signature