From patchwork Mon Dec 15 12:50:36 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Max Reitz X-Patchwork-Id: 421175 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 8F4F21400D2 for ; Mon, 15 Dec 2014 23:54:11 +1100 (AEDT) Received: from localhost ([::1]:39315 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Y0VA9-0006e5-Qe for incoming@patchwork.ozlabs.org; Mon, 15 Dec 2014 07:54:09 -0500 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33739) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Y0V7S-0001XC-RC for qemu-devel@nongnu.org; Mon, 15 Dec 2014 07:51:29 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Y0V7H-0007lZ-MV for qemu-devel@nongnu.org; Mon, 15 Dec 2014 07:51:22 -0500 Received: from mx1.redhat.com ([209.132.183.28]:48906) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Y0V7H-0007lH-CK for qemu-devel@nongnu.org; Mon, 15 Dec 2014 07:51:11 -0500 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id sBFCpAv9016598 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for ; Mon, 15 Dec 2014 07:51:10 -0500 Received: from localhost (dhcp-192-247.str.redhat.com [10.33.192.247]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id sBFCp8uQ004235 (version=TLSv1/SSLv3 cipher=AES128-GCM-SHA256 bits=128 verify=NO); Mon, 15 Dec 2014 07:51:10 -0500 From: Max Reitz To: qemu-devel@nongnu.org Date: Mon, 15 Dec 2014 13:50:36 +0100 Message-Id: <1418647857-3589-6-git-send-email-mreitz@redhat.com> In-Reply-To: <1418647857-3589-1-git-send-email-mreitz@redhat.com> References: <1418647857-3589-1-git-send-email-mreitz@redhat.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.23 X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x X-Received-From: 209.132.183.28 Cc: Kevin Wolf , Stefan Hajnoczi , Max Reitz Subject: [Qemu-devel] [PATCH v5 05/26] qcow2: Use unsigned addend for update_refcount() X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org update_refcount() and qcow2_update_cluster_refcount() currently take a signed addend. At least one caller passes a value directly derived from an absolute refcount that should be reached ("l2_refcount - 1" in expand_zero_clusters_in_l1()). Therefore, the addend should be unsigned because unsigned overflow is well-defined in contrast to signed overflow. This will be especially important for 64 bit refcounts. Because update_refcount() then no longer knows whether the refcount should be increased or decreased (which is important for setting the refblock-L2-table cache dependency and for overflow/underflow checks), it now requires an additional flag which specified exactly that. The same applies to qcow2_update_cluster_refcount(). Signed-off-by: Max Reitz Reviewed-by: Eric Blake --- block/qcow2-cluster.c | 2 +- block/qcow2-refcount.c | 65 ++++++++++++++++++++++++++++++++------------------ block/qcow2.h | 3 ++- 3 files changed, 45 insertions(+), 25 deletions(-) diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c index 133aad3..b664d2f 100644 --- a/block/qcow2-cluster.c +++ b/block/qcow2-cluster.c @@ -1699,7 +1699,7 @@ static int expand_zero_clusters_in_l1(BlockDriverState *bs, uint64_t *l1_table, /* For shared L2 tables, set the refcount accordingly (it is * already 1 and needs to be l2_refcount) */ ret = qcow2_update_cluster_refcount(bs, - offset >> s->cluster_bits, l2_refcount - 1, + offset >> s->cluster_bits, l2_refcount - 1, false, QCOW2_DISCARD_OTHER); if (ret < 0) { qcow2_free_clusters(bs, offset, s->cluster_size, diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c index bd37b8d..99a3cce 100644 --- a/block/qcow2-refcount.c +++ b/block/qcow2-refcount.c @@ -29,8 +29,8 @@ static int64_t alloc_clusters_noref(BlockDriverState *bs, uint64_t size); static int QEMU_WARN_UNUSED_RESULT update_refcount(BlockDriverState *bs, - int64_t offset, int64_t length, - int addend, enum qcow2_discard_type type); + int64_t offset, int64_t length, uint16_t addend, + bool decrease, enum qcow2_discard_type type); /*********************************************************/ @@ -263,7 +263,7 @@ static int alloc_refcount_block(BlockDriverState *bs, } else { /* Described somewhere else. This can recurse at most twice before we * arrive at a block that describes itself. */ - ret = update_refcount(bs, new_block, s->cluster_size, 1, + ret = update_refcount(bs, new_block, s->cluster_size, 1, false, QCOW2_DISCARD_NEVER); if (ret < 0) { goto fail_block; @@ -530,8 +530,14 @@ found: } /* XXX: cache several refcount block clusters ? */ +/* @addend is the absolute value of the addend; if @decrease is set, @addend + * will be subtracted from the current refcount, otherwise it will be added */ static int QEMU_WARN_UNUSED_RESULT update_refcount(BlockDriverState *bs, - int64_t offset, int64_t length, int addend, enum qcow2_discard_type type) + int64_t offset, + int64_t length, + uint16_t addend, + bool decrease, + enum qcow2_discard_type type) { BDRVQcowState *s = bs->opaque; int64_t start, last, cluster_offset; @@ -540,8 +546,9 @@ static int QEMU_WARN_UNUSED_RESULT update_refcount(BlockDriverState *bs, int ret; #ifdef DEBUG_ALLOC2 - fprintf(stderr, "update_refcount: offset=%" PRId64 " size=%" PRId64 " addend=%d\n", - offset, length, addend); + fprintf(stderr, "update_refcount: offset=%" PRId64 " size=%" PRId64 + " addend=%s%" PRIu16 "\n", offset, length, decrease ? "-" : "", + addend); #endif if (length < 0) { return -EINVAL; @@ -549,7 +556,7 @@ static int QEMU_WARN_UNUSED_RESULT update_refcount(BlockDriverState *bs, return 0; } - if (addend < 0) { + if (decrease) { qcow2_cache_set_dependency(bs, s->refcount_block_cache, s->l2_table_cache); } @@ -559,7 +566,8 @@ static int QEMU_WARN_UNUSED_RESULT update_refcount(BlockDriverState *bs, for(cluster_offset = start; cluster_offset <= last; cluster_offset += s->cluster_size) { - int block_index, refcount; + int block_index; + uint16_t refcount; int64_t cluster_index = cluster_offset >> s->cluster_bits; int64_t table_index = cluster_index >> s->refcount_block_bits; @@ -586,11 +594,18 @@ static int QEMU_WARN_UNUSED_RESULT update_refcount(BlockDriverState *bs, block_index = cluster_index & (s->refcount_block_size - 1); refcount = be16_to_cpu(refcount_block[block_index]); - refcount += addend; - if (refcount < 0 || refcount > s->refcount_max) { + if (decrease ? ((uint16_t)(refcount - addend) > refcount) + : ((uint16_t)(refcount + addend) < refcount || + (uint16_t)(refcount + addend) > s->refcount_max)) + { ret = -EINVAL; goto fail; } + if (decrease) { + refcount -= addend; + } else { + refcount += addend; + } if (refcount == 0 && cluster_index < s->free_cluster_index) { s->free_cluster_index = cluster_index; } @@ -623,8 +638,8 @@ fail: */ if (ret < 0) { int dummy; - dummy = update_refcount(bs, offset, cluster_offset - offset, -addend, - QCOW2_DISCARD_NEVER); + dummy = update_refcount(bs, offset, cluster_offset - offset, addend, + !decrease, QCOW2_DISCARD_NEVER); (void)dummy; } @@ -634,18 +649,21 @@ fail: /* * Increases or decreases the refcount of a given cluster. * + * @addend is the absolute value of the addend; if @decrease is set, @addend + * will be subtracted from the current refcount, otherwise it will be added. + * * On success 0 is returned; on failure -errno is returned. */ int qcow2_update_cluster_refcount(BlockDriverState *bs, int64_t cluster_index, - int addend, + uint16_t addend, bool decrease, enum qcow2_discard_type type) { BDRVQcowState *s = bs->opaque; int ret; ret = update_refcount(bs, cluster_index << s->cluster_bits, 1, addend, - type); + decrease, type); if (ret < 0) { return ret; } @@ -709,7 +727,7 @@ int64_t qcow2_alloc_clusters(BlockDriverState *bs, uint64_t size) return offset; } - ret = update_refcount(bs, offset, size, 1, QCOW2_DISCARD_NEVER); + ret = update_refcount(bs, offset, size, 1, false, QCOW2_DISCARD_NEVER); } while (ret == -EAGAIN); if (ret < 0) { @@ -746,7 +764,7 @@ int qcow2_alloc_clusters_at(BlockDriverState *bs, uint64_t offset, } /* And then allocate them */ - ret = update_refcount(bs, offset, i << s->cluster_bits, 1, + ret = update_refcount(bs, offset, i << s->cluster_bits, 1, false, QCOW2_DISCARD_NEVER); } while (ret == -EAGAIN); @@ -786,7 +804,7 @@ int64_t qcow2_alloc_bytes(BlockDriverState *bs, int size) s->free_byte_offset = 0; if (offset_into_cluster(s, offset) != 0) qcow2_update_cluster_refcount(bs, offset >> s->cluster_bits, 1, - QCOW2_DISCARD_NEVER); + false, QCOW2_DISCARD_NEVER); } else { offset = qcow2_alloc_clusters(bs, s->cluster_size); if (offset < 0) { @@ -797,7 +815,7 @@ int64_t qcow2_alloc_bytes(BlockDriverState *bs, int size) /* we are lucky: contiguous data */ offset = s->free_byte_offset; qcow2_update_cluster_refcount(bs, offset >> s->cluster_bits, 1, - QCOW2_DISCARD_NEVER); + false, QCOW2_DISCARD_NEVER); s->free_byte_offset += size; } else { s->free_byte_offset = offset; @@ -820,7 +838,7 @@ void qcow2_free_clusters(BlockDriverState *bs, int ret; BLKDBG_EVENT(bs->file, BLKDBG_CLUSTER_FREE); - ret = update_refcount(bs, offset, size, -1, type); + ret = update_refcount(bs, offset, size, 1, true, type); if (ret < 0) { fprintf(stderr, "qcow2_free_clusters failed: %s\n", strerror(-ret)); /* TODO Remember the clusters to free them later and avoid leaking */ @@ -950,7 +968,7 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs, if (addend != 0) { ret = update_refcount(bs, (offset & s->cluster_offset_mask) & ~511, - nb_csectors * 512, addend, + nb_csectors * 512, abs(addend), addend < 0, QCOW2_DISCARD_SNAPSHOT); if (ret < 0) { goto fail; @@ -981,7 +999,7 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs, } if (addend != 0) { ret = qcow2_update_cluster_refcount(bs, - cluster_index, addend, + cluster_index, abs(addend), addend < 0, QCOW2_DISCARD_SNAPSHOT); if (ret < 0) { goto fail; @@ -1024,7 +1042,7 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs, if (addend != 0) { ret = qcow2_update_cluster_refcount(bs, l2_offset >> s->cluster_bits, - addend, + abs(addend), addend < 0, QCOW2_DISCARD_SNAPSHOT); if (ret < 0) { goto fail; @@ -1677,7 +1695,8 @@ static void compare_refcounts(BlockDriverState *bs, BdrvCheckResult *res, if (num_fixed) { ret = update_refcount(bs, i << s->cluster_bits, 1, - (int)refcount2 - (int)refcount1, + abs(refcount2 - refcount1), + refcount1 > refcount2, QCOW2_DISCARD_ALWAYS); if (ret >= 0) { (*num_fixed)++; diff --git a/block/qcow2.h b/block/qcow2.h index 1e59277..0240ee8 100644 --- a/block/qcow2.h +++ b/block/qcow2.h @@ -493,7 +493,8 @@ int qcow2_get_refcount(BlockDriverState *bs, int64_t cluster_index, uint16_t *refcount); int qcow2_update_cluster_refcount(BlockDriverState *bs, int64_t cluster_index, - int addend, enum qcow2_discard_type type); + uint16_t addend, bool decrease, + enum qcow2_discard_type type); int64_t qcow2_alloc_clusters(BlockDriverState *bs, uint64_t size); int qcow2_alloc_clusters_at(BlockDriverState *bs, uint64_t offset,