From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53710) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1W6iba-0004YR-A8 for qemu-devel@nongnu.org; Fri, 24 Jan 2014 10:23:42 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1W6ibP-0003Af-SW for qemu-devel@nongnu.org; Fri, 24 Jan 2014 10:23:38 -0500 Received: from paradis.irqsave.net ([62.212.105.220]:48252) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1W6ibP-0003AJ-Fh for qemu-devel@nongnu.org; Fri, 24 Jan 2014 10:23:27 -0500 Date: Fri, 24 Jan 2014 16:23:26 +0100 From: =?iso-8859-1?Q?Beno=EEt?= Canet Message-ID: <20140124152326.GB3087@irqsave.net> References: <3a0b13997dc9ec9525f00f8d02defe6a8c52dc66.1390445921.git.hutao@cn.fujitsu.com> <20140123170208.GC3519@irqsave.net> <20140124093240.GC23175@G08FNSTD100614.fnst.cn.fujitsu.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <20140124093240.GC23175@G08FNSTD100614.fnst.cn.fujitsu.com> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v4 1/4] 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 Cc: =?iso-8859-1?Q?Beno=EEt?= Canet , Kevin Wolf , qemu-devel@nongnu.org Le Friday 24 Jan 2014 =E0 17:32:40 (+0800), Hu Tao a =E9crit : > On Thu, Jan 23, 2014 at 06:02:08PM +0100, Beno=EEt Canet wrote: > > Le Thursday 23 Jan 2014 =E0 11:04:05 (+0800), Hu Tao a =E9crit : > > > 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. > > >=20 > > > 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: > > >=20 > > > ./qemu-img create -f qcow2 \ > > > -o 'cluster_size=3D131072,preallocation=3Dmetadata' file.img 4G > > >=20 > > > Reviewed-by: Max Reitz > > > Signed-off-by: Hu Tao > > > --- > > > block/qcow2-cluster.c | 14 ++++++-------- > > > block/qcow2.c | 11 +++-------- > > > block/qcow2.h | 2 +- > > > trace-events | 2 +- > > > 4 files changed, 11 insertions(+), 18 deletions(-) > > >=20 > > > diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c > > > index 8534084..c57f39d 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 offs= et, > > > - int n_start, int n_end, int *num, uint64_t *host_offset, QCowL= 2Meta **m) > > > + int *num, uint64_t *host_offset, QCowL2Meta **m) > > > { > > > BDRVQcowState *s =3D bs->opaque; > > > uint64_t start, remaining; > > > @@ -1190,15 +1190,13 @@ int qcow2_alloc_cluster_offset(BlockDriverS= tate *bs, uint64_t offset, > > > uint64_t cur_bytes; > > > int ret; > > > =20 > > > - trace_qcow2_alloc_clusters_offset(qemu_coroutine_self(), offse= t, > > > - n_start, n_end); > > > + trace_qcow2_alloc_clusters_offset(qemu_coroutine_self(), offse= t, *num); > > > =20 > > > - assert(n_start * BDRV_SECTOR_SIZE =3D=3D offset_into_cluster(s= , offset)); > > > - offset =3D start_of_cluster(s, offset); > > > + assert((offset & ~BDRV_SECTOR_MASK) =3D=3D 0); > >=20 > > Why replace something that would round gently an unaligned offset > > (start_of_cluster) by an assert that would make QEMU exit ? >=20 > It is equivalent to the removed assert(). Oh sorry I didn't see the removed assert() when reviewing :( >=20 >=20