From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39618) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YJ3VG-00077z-Dh for qemu-devel@nongnu.org; Wed, 04 Feb 2015 12:12:40 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YJ3VC-00050E-9E for qemu-devel@nongnu.org; Wed, 04 Feb 2015 12:12:38 -0500 Received: from mx1.redhat.com ([209.132.183.28]:35783) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YJ3VC-000503-2k for qemu-devel@nongnu.org; Wed, 04 Feb 2015 12:12:34 -0500 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id t14HCW5d030116 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for ; Wed, 4 Feb 2015 12:12:32 -0500 Message-ID: <54D252FE.9030106@redhat.com> Date: Wed, 04 Feb 2015 12:12:30 -0500 From: Max Reitz MIME-Version: 1.0 References: <1418647857-3589-1-git-send-email-mreitz@redhat.com> <1418647857-3589-11-git-send-email-mreitz@redhat.com> <20150204160645.GF5641@noname.redhat.com> In-Reply-To: <20150204160645.GF5641@noname.redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v5 10/26] qcow2: Helper function for refcount modification 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-04 at 11:06, Kevin Wolf wrote: > Am 15.12.2014 um 13:50 hat Max Reitz geschrieben: >> Since refcounts do not always have to be a uint16_t, all refcount blocks >> and arrays in memory should not have a specific type (thus they become >> pointers to void) and for accessing them, two helper functions are used >> (a getter and a setter). Those functions are called indirectly through >> function pointers in the BDRVQcowState so they may later be exchanged >> for different refcount orders. >> >> With the check and repair functions using this function, the refcount >> array they are creating will be in big endian byte order; additionally, >> using realloc_refcount_array() makes the size of this refcount array >> always cluster-aligned. Both combined allow rebuild_refcount_structure() >> to drop the bounce buffer which was used to convert parts of the >> refcount array to big endian byte order and store them on disk. Instead, >> those parts can now be written directly. >> >> Signed-off-by: Max Reitz >> Reviewed-by: Eric Blake >> Reviewed-by: Stefan Hajnoczi >> --- >> block/qcow2-refcount.c | 122 ++++++++++++++++++++++++++++--------------------- >> block/qcow2.h | 8 ++++ >> 2 files changed, 79 insertions(+), 51 deletions(-) >> @@ -541,7 +561,7 @@ static int QEMU_WARN_UNUSED_RESULT update_refcount(BlockDriverState *bs, >> { >> BDRVQcowState *s = bs->opaque; >> int64_t start, last, cluster_offset; >> - uint16_t *refcount_block = NULL; >> + void *refcount_block = NULL; >> int64_t old_table_index = -1; >> int ret; >> >> @@ -593,7 +613,7 @@ static int QEMU_WARN_UNUSED_RESULT update_refcount(BlockDriverState *bs, >> /* we can update the count and save it */ >> block_index = cluster_index & (s->refcount_block_size - 1); >> >> - refcount = be16_to_cpu(refcount_block[block_index]); >> + refcount = s->get_refcount(refcount_block, block_index); >> if (decrease ? (refcount - addend > refcount) >> : (refcount + addend < refcount || >> refcount + addend > s->refcount_max)) >> @@ -609,7 +629,7 @@ static int QEMU_WARN_UNUSED_RESULT update_refcount(BlockDriverState *bs, >> if (refcount == 0 && cluster_index < s->free_cluster_index) { >> s->free_cluster_index = cluster_index; >> } >> - refcount_block[block_index] = cpu_to_be16(refcount); >> + s->set_refcount(refcount_block, block_index, refcount); >> >> if (refcount == 0 && s->discard_passthrough[type]) { >> update_refcount_discard(bs, cluster_offset, s->cluster_size); >> @@ -625,8 +645,7 @@ fail: >> /* Write last changed block to disk */ >> if (refcount_block) { >> int wret; >> - wret = qcow2_cache_put(bs, s->refcount_block_cache, >> - (void**) &refcount_block); >> + wret = qcow2_cache_put(bs, s->refcount_block_cache, &refcount_block); >> if (wret < 0) { >> return ret < 0 ? ret : wret; >> } > There is a (void**) cast left in the other qcow2_cache_put() call in > update_refcount(). Will remove. >> @@ -1883,7 +1907,7 @@ static int64_t alloc_clusters_imrt(BlockDriverState *bs, >> */ >> static int rebuild_refcount_structure(BlockDriverState *bs, >> BdrvCheckResult *res, >> - uint16_t **refcount_table, >> + void **refcount_table, >> int64_t *nb_clusters) >> { >> BDRVQcowState *s = bs->opaque; >> @@ -1891,8 +1915,8 @@ static int rebuild_refcount_structure(BlockDriverState *bs, >> int64_t refblock_offset, refblock_start, refblock_index; >> uint32_t reftable_size = 0; >> uint64_t *on_disk_reftable = NULL; >> - uint16_t *on_disk_refblock; >> - int i, ret = 0; >> + void *on_disk_refblock; >> + int ret = 0; >> struct { >> uint64_t reftable_offset; >> uint32_t reftable_clusters; >> @@ -1902,7 +1926,7 @@ static int rebuild_refcount_structure(BlockDriverState *bs, >> >> write_refblocks: >> for (; cluster < *nb_clusters; cluster++) { >> - if (!(*refcount_table)[cluster]) { >> + if (!s->get_refcount(*refcount_table, cluster)) { >> continue; >> } >> >> @@ -1975,17 +1999,13 @@ write_refblocks: >> goto fail; >> } >> >> - on_disk_refblock = qemu_blockalign0(bs->file, s->cluster_size); >> - for (i = 0; i < s->refcount_block_size && >> - refblock_start + i < *nb_clusters; i++) >> - { >> - on_disk_refblock[i] = >> - cpu_to_be16((*refcount_table)[refblock_start + i]); >> - } >> + /* The size of *refcount_table is always cluster-aligned, therefore the >> + * write operation will not overflow */ >> + on_disk_refblock = (void *)((uintptr_t)*refcount_table + >> + (refblock_index << s->refcount_block_bits)); > Are you sure about s->refcount_block_bits? You're now calculating in > bytes and not in entries any more like before with uint16_t*.So I think > this should be s->cluster_bits now (or refblock_index * s->cluster_size > if you don't want to confuse people more than necessary :-)). You're right. Now I'm wondering why it worked, though. Maybe the tests only covered the refblock_index == 0 case... Good catch, thanks! Max >> ret = bdrv_write(bs->file, refblock_offset / BDRV_SECTOR_SIZE, >> - (void *)on_disk_refblock, s->cluster_sectors); >> - qemu_vfree(on_disk_refblock); >> + on_disk_refblock, s->cluster_sectors); >> if (ret < 0) { >> fprintf(stderr, "ERROR writing refblock: %s\n", strerror(-ret)); >> goto fail; > Kevin