Re: [Qemu-devel] [PATCH v5 06/26] qcow2: Use 64 bits for refcount values

[Kevin Wolf]

Am 15.12.2014 um 13:50 hat Max Reitz geschrieben:
> Refcounts may have a width of up to 64 bits, so qemu should use the same
> width to represent refcount values internally.
> 
> Signed-off-by: Max Reitz <address@hidden>
> ---
>  block/qcow2-cluster.c  |  2 +-
>  block/qcow2-refcount.c | 46 ++++++++++++++++++++++------------------------
>  block/qcow2.h          |  4 ++--
>  3 files changed, 25 insertions(+), 27 deletions(-)

> @@ -897,11 +895,10 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs,
>      int64_t l1_table_offset, int l1_size, int addend)
>  {

Your leaving addend an int here...

>      BDRVQcowState *s = bs->opaque;
> -    uint64_t *l1_table, *l2_table, l2_offset, offset, l1_size2;
> +    uint64_t *l1_table, *l2_table, l2_offset, offset, l1_size2, refcount;
>      bool l1_allocated = false;
>      int64_t old_offset, old_l2_offset;
>      int i, j, l1_modified = 0, nb_csectors;
> -    uint16_t refcount;
>      int ret;
>  
>      l2_table = NULL;
> @@ -968,7 +965,7 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs,
>                          if (addend != 0) {
>                              ret = update_refcount(bs,
>                                  (offset & s->cluster_offset_mask) & ~511,
> -                                nb_csectors * 512, abs(addend), addend < 0,
> +                                nb_csectors * 512, imaxabs(addend), addend < 
> 0,
>                                  QCOW2_DISCARD_SNAPSHOT);
>                              if (ret < 0) {
>                                  goto fail;
> @@ -999,7 +996,7 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs,
>                          }
>                          if (addend != 0) {
>                              ret = qcow2_update_cluster_refcount(bs,
> -                                    cluster_index, abs(addend), addend < 0,
> +                                    cluster_index, imaxabs(addend), addend < 
> 0,
>                                      QCOW2_DISCARD_SNAPSHOT);
>                              if (ret < 0) {
>                                  goto fail;
> @@ -1042,7 +1039,7 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs,
>              if (addend != 0) {
>                  ret = qcow2_update_cluster_refcount(bs, l2_offset >>
>                                                          s->cluster_bits,
> -                                                    abs(addend), addend < 0,
> +                                                    imaxabs(addend), addend 
> < 0,
>                                                      QCOW2_DISCARD_SNAPSHOT);
>                  if (ret < 0) {
>                      goto fail;

...but still replace abs() by imaxabs(). Did you intend to convert
addend or why this change?

> @@ -1658,7 +1655,7 @@ static void compare_refcounts(BlockDriverState *bs, 
> BdrvCheckResult *res,
>  {
>      BDRVQcowState *s = bs->opaque;
>      int64_t i;
> -    uint16_t refcount1, refcount2;
> +    uint64_t refcount1, refcount2;
>      int ret;
>  
>      for (i = 0, *highest_cluster = 0; i < nb_clusters; i++) {
> @@ -1687,7 +1684,8 @@ static void compare_refcounts(BlockDriverState *bs, 
> BdrvCheckResult *res,
>                  num_fixed = &res->corruptions_fixed;
>              }
>  
> -            fprintf(stderr, "%s cluster %" PRId64 " refcount=%d 
> reference=%d\n",
> +            fprintf(stderr, "%s cluster %" PRId64 " refcount=%" PRIu64
> +                    " reference=%" PRIu64 "\n",
>                     num_fixed != NULL     ? "Repairing" :
>                     refcount1 < refcount2 ? "ERROR" :
>                                             "Leaked",
> @@ -1695,7 +1693,7 @@ static void compare_refcounts(BlockDriverState *bs, 
> BdrvCheckResult *res,
>  
>              if (num_fixed) {
>                  ret = update_refcount(bs, i << s->cluster_bits, 1,
> -                                      abs(refcount2 - refcount1),
> +                                      imaxabs(refcount2 - refcount1),
>                                        refcount1 > refcount2,

Hope I got that right. Here's my analysis:

Before: refcount{1,2} were both uint16_t. Promoted to int for the
subtraction. Therefore a negative result could occur. abs() takes the
absolute value and the sign is passed separately.

After: refcount{1,2} are both uint64_t. No integer promotion happens, we
perform an unsigned subtraction. The separate passed sign is okay. For
the absolute value, there are two cases:

1. refcount2 >= refcount1: No overflow occurs, everything fine.

2. refcount2 < refcount1: (refcount2 - refcount1) wraps around, but is
   still an uint64_t. imaxabs() takes an intmax_t, which is signed. The
   conversion is implementation defined, but let's assume the obvious
   one. imaxabs() has two cases again:

   diff := refcount2 - refcount1 + UINT64_MAX

   a. diff > INTMAX_MAX:
      We get diff converted back to signed, which undoes the wraparound.
      The absolute value of the signed difference is:

        -(refcount2 - refcount1) = refcount1 - refcount2

      This is what we wanted. Good.

   b. diff <= INTMAX_MAX:
      diff is again converted back to signed, however its value is
      unchanged because diff can be represented by intmax_t. This is a
      positive value, so taking the absolute value changes nothing.

      This is _not_ refcount1 - refcount2!

I suggest using a function that calculates the absolute value of the
difference of two unsigned values the naive way with an if statement.
Gets us rid of the implementation defined conversion, too.

>                                        QCOW2_DISCARD_ALWAYS);
>                  if (ret >= 0) {

Kevin