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

[Max Reitz]

On 2015-02-03 at 14:26, Kevin Wolf wrote:
> 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?

Mechanical replacement of every abs(addend) most likely.

Considering that qcow2_update_snapshot_refcount() is only called with 
@addend \in { -1, 0, 1 }, it doesn't seem to make any technical sense to 
convert @addend to something else than an int; and thus it doesn't make 
any sense to use imaxabs() instead of abs() (although it doesn't hurt, 
it just looks bad). Also, if I were to convert 
qcow2_update_snapshot_refcount() to the "full 64 bit difference 
interface", I'd need an additional argument for the sign of the addend.

Therefore, I'll drop the imaxabs() hunks for 
qcow2_update_snapshot_refcount(), if you're fine with that.

> > @@ -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

Actually it's + UINT64_MAX + 1, but it doesn't matter for the point 
you're making.

>     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!

You're completely right. Actually, I won absolutely nothing by 
separating the sign if using imaxabs() because the latter will only 
return values in 0 .. 2^63 - 1, which makes it (using imaxabs()) a very 
bad idea in the first place.

> 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.

Indeed, will do. Thanks!

Max

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