|
From: | Max Reitz |
Subject: | Re: [Qemu-devel] [PATCH v5 06/26] qcow2: Use 64 bits for refcount values |
Date: | Tue, 03 Feb 2015 14:48:35 -0500 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.4.0 |
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
[Prev in Thread] | Current Thread | [Next in Thread] |