From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39161) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1W4utb-0008L3-E5 for qemu-devel@nongnu.org; Sun, 19 Jan 2014 11:06:52 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1W4utW-0006wI-QU for qemu-devel@nongnu.org; Sun, 19 Jan 2014 11:06:47 -0500 Received: from mx1.redhat.com ([209.132.183.28]:55017) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1W4utW-0006wE-Hv for qemu-devel@nongnu.org; Sun, 19 Jan 2014 11:06:42 -0500 Message-ID: <52DBF87A.1050506@redhat.com> Date: Sun, 19 Jan 2014 17:08:26 +0100 From: Max Reitz MIME-Version: 1.0 References: <0214be9a80cce21c7e8b9d6b9a9c3c063473faa8.1388381026.git.hutao@cn.fujitsu.com> In-Reply-To: <0214be9a80cce21c7e8b9d6b9a9c3c063473faa8.1388381026.git.hutao@cn.fujitsu.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v1 1/3] qcow2: remove n_start and n_end of qcow2_alloc_cluster_offset() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Hu Tao , qemu-devel@nongnu.org Cc: Kevin Wolf , Stefan Hajnoczi On 30.12.2013 06:29, Hu Tao wrote: > n_start can be actually calculated from offset. The number of > sectors to be allocated(n_end - n_start) can be passed in in > num. By removing n_start and n_end, we can save two parameters. > > The side effect is there is a bug in qcow2.c:preallocate() that > passes incorrect n_start to qcow2_alloc_cluster_offset() is > fixed. The bug can be triggerred by a larger cluster size than > the default value(65536), for example: > > ./qemu-img create -f qcow2 \ > -o 'cluster_size=131072,preallocation=metadata' file.img 4G > > Signed-off-by: Hu Tao > --- > block/qcow2-cluster.c | 14 ++++++-------- > block/qcow2.c | 6 +++--- > block/qcow2.h | 2 +- > trace-events | 2 +- > 4 files changed, 11 insertions(+), 13 deletions(-) > > diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c > index 8534084..be33781 100644 > --- a/block/qcow2-cluster.c > +++ b/block/qcow2-cluster.c > @@ -1182,7 +1182,7 @@ fail: > * Return 0 on success and -errno in error cases > */ > int qcow2_alloc_cluster_offset(BlockDriverState *bs, uint64_t offset, > - int n_start, int n_end, int *num, uint64_t *host_offset, QCowL2Meta **m) > + int *num, uint64_t *host_offset, QCowL2Meta **m) > { > BDRVQcowState *s = bs->opaque; > uint64_t start, remaining; > @@ -1190,15 +1190,13 @@ int qcow2_alloc_cluster_offset(BlockDriverState *bs, uint64_t offset, > uint64_t cur_bytes; > int ret; > > - trace_qcow2_alloc_clusters_offset(qemu_coroutine_self(), offset, > - n_start, n_end); > + trace_qcow2_alloc_clusters_offset(qemu_coroutine_self(), offset, *num); > > - assert(n_start * BDRV_SECTOR_SIZE == offset_into_cluster(s, offset)); > - offset = start_of_cluster(s, offset); > + assert((offset & ~BDRV_SECTOR_MASK) == 0); > > again: > - start = offset + (n_start << BDRV_SECTOR_BITS); > - remaining = (n_end - n_start) << BDRV_SECTOR_BITS; > + start = offset; > + remaining = (*num) << BDRV_SECTOR_BITS; If you were to do a respin, the brackets here are unnecessary. > cluster_offset = 0; > *host_offset = 0; > cur_bytes = 0; > @@ -1284,7 +1282,7 @@ again: > } > } > > - *num = (n_end - n_start) - (remaining >> BDRV_SECTOR_BITS); > + *num = *num - (remaining >> BDRV_SECTOR_BITS); And this could be written as "*num -= remaining >> BDRV_SECTOR_BITS". But regardless: Reviewed-by: Max Reitz > assert(*num > 0); > assert(*host_offset != 0); > > diff --git a/block/qcow2.c b/block/qcow2.c > index f29aa88..46860d5 100644 > --- a/block/qcow2.c > +++ b/block/qcow2.c > @@ -1016,14 +1016,14 @@ static coroutine_fn int qcow2_co_writev(BlockDriverState *bs, > > trace_qcow2_writev_start_part(qemu_coroutine_self()); > index_in_cluster = sector_num & (s->cluster_sectors - 1); > - n_end = index_in_cluster + remaining_sectors; > + cur_nr_sectors = remaining_sectors; > if (s->crypt_method && > n_end > QCOW_MAX_CRYPT_CLUSTERS * s->cluster_sectors) { > n_end = QCOW_MAX_CRYPT_CLUSTERS * s->cluster_sectors; > } > > ret = qcow2_alloc_cluster_offset(bs, sector_num << 9, > - index_in_cluster, n_end, &cur_nr_sectors, &cluster_offset, &l2meta); > + &cur_nr_sectors, &cluster_offset, &l2meta); > if (ret < 0) { > goto fail; > } > @@ -1400,7 +1400,7 @@ static int preallocate(BlockDriverState *bs) > > while (nb_sectors) { > num = MIN(nb_sectors, INT_MAX >> 9); > - ret = qcow2_alloc_cluster_offset(bs, offset, 0, num, &num, > + ret = qcow2_alloc_cluster_offset(bs, offset, &num, > &host_offset, &meta); > if (ret < 0) { > return ret; > diff --git a/block/qcow2.h b/block/qcow2.h > index 303eb26..84e1344 100644 > --- a/block/qcow2.h > +++ b/block/qcow2.h > @@ -468,7 +468,7 @@ void qcow2_encrypt_sectors(BDRVQcowState *s, int64_t sector_num, > int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t offset, > int *num, uint64_t *cluster_offset); > int qcow2_alloc_cluster_offset(BlockDriverState *bs, uint64_t offset, > - int n_start, int n_end, int *num, uint64_t *host_offset, QCowL2Meta **m); > + int *num, uint64_t *host_offset, QCowL2Meta **m); > uint64_t qcow2_alloc_compressed_cluster_offset(BlockDriverState *bs, > uint64_t offset, > int compressed_size); > diff --git a/trace-events b/trace-events > index 9f4456a..9b4e586 100644 > --- a/trace-events > +++ b/trace-events > @@ -494,7 +494,7 @@ qcow2_writev_done_part(void *co, int cur_nr_sectors) "co %p cur_nr_sectors %d" > qcow2_writev_data(void *co, uint64_t offset) "co %p offset %" PRIx64 > > # block/qcow2-cluster.c > -qcow2_alloc_clusters_offset(void *co, uint64_t offset, int n_start, int n_end) "co %p offet %" PRIx64 " n_start %d n_end %d" > +qcow2_alloc_clusters_offset(void *co, uint64_t offset, int num) "co %p offet %" PRIx64 " num %d" > qcow2_handle_copied(void *co, uint64_t guest_offset, uint64_t host_offset, uint64_t bytes) "co %p guest_offet %" PRIx64 " host_offset %" PRIx64 " bytes %" PRIx64 > qcow2_handle_alloc(void *co, uint64_t guest_offset, uint64_t host_offset, uint64_t bytes) "co %p guest_offet %" PRIx64 " host_offset %" PRIx64 " bytes %" PRIx64 > qcow2_do_alloc_clusters_offset(void *co, uint64_t guest_offset, uint64_t host_offset, int nb_clusters) "co %p guest_offet %" PRIx64 " host_offset %" PRIx64 " nb_clusters %d"