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

[Max Reitz <mreitz@redhat.com>]

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 <mreitz@redhat.com>
>> ---
>>   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