From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34362) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XOoJ1-0001uQ-IM for qemu-devel@nongnu.org; Tue, 02 Sep 2014 09:39:36 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XOoIw-0001sy-FP for qemu-devel@nongnu.org; Tue, 02 Sep 2014 09:39:31 -0400 Received: from lputeaux-656-01-25-125.w80-12.abo.wanadoo.fr ([80.12.84.125]:38357 helo=paradis.irqsave.net) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XOoIw-0001r8-41 for qemu-devel@nongnu.org; Tue, 02 Sep 2014 09:39:26 -0400 Date: Tue, 2 Sep 2014 15:38:37 +0200 From: =?iso-8859-1?Q?Beno=EEt?= Canet Message-ID: <20140902133837.GA28034@irqsave.net> References: <1409568798-2292-1-git-send-email-junmuzi@gmail.com> <20140901111119.GO15537@irqsave.net> <20140901160408.GA4193@localhost.localdomain> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <20140901160408.GA4193@localhost.localdomain> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v2] qcow2: add update refcount table realization for update_refcount List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jun Li Cc: =?iso-8859-1?Q?Beno=EEt?= Canet , kwolf@redhat.com, famz@redhat.com, juli@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com The Tuesday 02 Sep 2014 =E0 00:04:08 (+0800), Jun Li wrote : > On Mon, 09/01 13:11, Beno=EEt Canet wrote: > > The Monday 01 Sep 2014 =E0 18:52:48 (+0800), Jun Li wrote : > > > When every item of refcount block is NULL, free refcount block and = reset the > > > corresponding item of refcount table with NULL. > > >=20 > > > Signed-off-by: Jun Li > > > --- > > >=20 > > > The v2 do following change to modify some potential issue. > > >=20 > > > +------- Here should start from "0". > > > | > > > for (k =3D 0; k < refcount_block_entries; k++) { > > > if (refcount_block[k] !=3D cpu_to_be16(0)) { > > > ... | | > > > } | | > > > } | +---- Using "0" is mor= e safe. > > > | > > > +-------- This should be "k" not "++k". > > > --- > > > block/qcow2-refcount.c | 31 +++++++++++++++++++++++++++++++ > > > 1 file changed, 31 insertions(+) > > >=20 > > > diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c > > > index 43665b8..63f36e6 100644 > > > --- a/block/qcow2-refcount.c > > > +++ b/block/qcow2-refcount.c > > > @@ -586,6 +586,37 @@ static int QEMU_WARN_UNUSED_RESULT update_refc= ount(BlockDriverState *bs, > > > if (refcount =3D=3D 0 && s->discard_passthrough[type]) { > > > update_refcount_discard(bs, cluster_offset, s->cluster= _size); > > > } > > > + > > > + /* When refcount block is NULL, update refcount table */ > > > + if (block_index =3D=3D 0) { > > > + int k =3D block_index; > > > + int refcount_block_entries =3D s->cluster_size / sizeo= f(uint16_t); > > > + for (k =3D 0; k < refcount_block_entries; k++) { > > > + if (refcount_block[k] !=3D cpu_to_be16(0)) { > > > + break; > > > + } > > > + } > > > + > > > + if (k =3D=3D refcount_block_entries) { > > > + qemu_vfree(refcount_block); > > > + /* update refcount table */ > > > + unsigned int refcount_table_index; > > > + uint64_t data64 =3D cpu_to_be64(0); > > > + refcount_table_index =3D cluster_index >> (s->clus= ter_bits - > > > + REFCOUNT_SHIFT); > > > + ret =3D bdrv_pwrite_sync(bs->file, > > > + s->refcount_table_offset + > > > + refcount_table_index * > > > + sizeof(uint64_t), > > > + &data64, sizeof(data64)); > > > + if (ret < 0) { > > > + goto fail; > > > + } > > > + > >=20 > > > + s->refcount_table[refcount_table_index] =3D data64= ; > >=20 > > Shouldn't the in memory version be be in cpu order ? like > > s->refcount_table[refcount_table_index] =3D 0; >=20 > I don't think so. See following: >=20 > (gdb) p sizeof(s->refcount_table[0]) > $5 =3D 8 > (gdb) p sizeof(s->refcount_table[1]) > $6 =3D 8 > (gdb) p sizeof(0) > $7 =3D 4 There is two different thing here: endianness and type. For the endianess you can look at qcow2_refcount_init. The endianness of this in memory table is the one of the CPU. Here data64 is big endian and this is wrong. For the type integer promotion will take care of it. See http://www.tutorialspoint.com/cprogramming/c_type_casting.htm assigning zero means that the compiler will silently perform a cast to int64_t. Best regards Beno=EEt >=20 > So I think here is right. Thank you for sharing Max's patch(qcow2: Drop > REFCOUNT_SHIFT) with me. I find this patch has been reviewed, but it ha= s not > been merged. Maybe I will modify my realization after this patch merged= . >=20 > Thanks again. >=20 > Jun Li >=20 >=20 > >=20 > > Best regards > >=20 > > Beno=EEt=20 > > > + > > > + } > > > + } > > > } > > > =20 > > > ret =3D 0; > > > --=20 > > > 1.9.3 > > >=20 > > >=20 >=20