From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48702) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YIjSk-0004gq-Aa for qemu-devel@nongnu.org; Tue, 03 Feb 2015 14:48:43 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YIjSh-0001Kw-G4 for qemu-devel@nongnu.org; Tue, 03 Feb 2015 14:48:42 -0500 Received: from mx1.redhat.com ([209.132.183.28]:56238) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YIjSh-0001Ju-96 for qemu-devel@nongnu.org; Tue, 03 Feb 2015 14:48:39 -0500 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id t13JmbsV016983 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for ; Tue, 3 Feb 2015 14:48:37 -0500 Message-ID: <54D12613.8040606@redhat.com> Date: Tue, 03 Feb 2015 14:48:35 -0500 From: Max Reitz MIME-Version: 1.0 References: <1418647857-3589-1-git-send-email-mreitz@redhat.com> <1418647857-3589-7-git-send-email-mreitz@redhat.com> <20150203192632.GH4488@noname.redhat.com> In-Reply-To: <20150203192632.GH4488@noname.redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v5 06/26] qcow2: Use 64 bits for refcount values List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: qemu-devel@nongnu.org, Stefan Hajnoczi 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 >> --- >> 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