From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1Ky9ID-0001SC-AK for qemu-devel@nongnu.org; Thu, 06 Nov 2008 13:09:17 -0500 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1Ky9I9-0001RY-PP for qemu-devel@nongnu.org; Thu, 06 Nov 2008 13:09:16 -0500 Received: from [199.232.76.173] (port=33227 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Ky9I9-0001RV-JO for qemu-devel@nongnu.org; Thu, 06 Nov 2008 13:09:13 -0500 Received: from cantor.suse.de ([195.135.220.2]:45777 helo=mx1.suse.de) by monty-python.gnu.org with esmtps (TLS-1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.60) (envelope-from ) id 1Ky9I9-0000gu-Bw for qemu-devel@nongnu.org; Thu, 06 Nov 2008 13:09:13 -0500 Message-ID: <4913336E.3060309@suse.de> Date: Thu, 06 Nov 2008 19:11:58 +0100 From: Kevin Wolf MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH 2/4] qcow2: Allow update_cluster_refcount() to update several clusters refcount. References: <20081106165212.380421945@bull.net> <1225990558.6576.12.camel@frecb07144> In-Reply-To: <1225990558.6576.12.camel@frecb07144> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Reply-To: qemu-devel@nongnu.org List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org Cc: Laurent Vivier Laurent Vivier schrieb: > pi=C3=A8ce jointe document texte brut > (0002-Allow-update_cluster_refcount-to-update-several-cl.patch) > To improve performance when the qcow2 file is empty, this patch > allows update_cluster_refcount() to update refcount of > several clusters. >=20 > Signed-off-by: Laurent Vivier > --- > block-qcow2.c | 107 ++++++++++++++++++++++++++++++++++++-------------= --------- > 1 file changed, 68 insertions(+), 39 deletions(-) >=20 > Index: qemu/block-qcow2.c > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > --- qemu.orig/block-qcow2.c 2008-11-06 16:40:44.000000000 +0100 > +++ qemu/block-qcow2.c 2008-11-06 16:40:45.000000000 +0100 > @@ -159,6 +159,7 @@ static void refcount_close(BlockDriverSt > static int get_refcount(BlockDriverState *bs, int64_t cluster_index); > static int update_cluster_refcount(BlockDriverState *bs, > int64_t cluster_index, > + int nb_clusters, > int addend); > static void update_refcount(BlockDriverState *bs, > int64_t offset, int64_t length, > @@ -1711,7 +1712,7 @@ static int update_snapshot_refcount(Bloc > refcount =3D 2; > } else { > if (addend !=3D 0) { > - refcount =3D update_cluster_refcount(bs, o= ffset >> s->cluster_bits, addend); > + refcount =3D update_cluster_refcount(bs, o= ffset >> s->cluster_bits, 1, addend); > } else { > refcount =3D get_refcount(bs, offset >> s-= >cluster_bits); > } > @@ -1733,7 +1734,7 @@ static int update_snapshot_refcount(Bloc > } > =20 > if (addend !=3D 0) { > - refcount =3D update_cluster_refcount(bs, l2_offset >> = s->cluster_bits, addend); > + refcount =3D update_cluster_refcount(bs, l2_offset >> = s->cluster_bits, 1, addend); > } else { > refcount =3D get_refcount(bs, l2_offset >> s->cluster_= bits); > } > @@ -2292,14 +2293,14 @@ static int64_t alloc_bytes(BlockDriverSt > if (free_in_cluster =3D=3D 0) > s->free_byte_offset =3D 0; > if ((offset & (s->cluster_size - 1)) !=3D 0) > - update_cluster_refcount(bs, offset >> s->cluster_bits, 1); > + update_cluster_refcount(bs, offset >> s->cluster_bits, 1, = 1); > } else { > offset =3D alloc_clusters(bs, s->cluster_size); > cluster_offset =3D s->free_byte_offset & ~(s->cluster_size - 1= ); > if ((cluster_offset + s->cluster_size) =3D=3D offset) { > /* we are lucky: contiguous data */ > offset =3D s->free_byte_offset; > - update_cluster_refcount(bs, offset >> s->cluster_bits, 1); > + update_cluster_refcount(bs, offset >> s->cluster_bits, 1, = 1); > s->free_byte_offset +=3D size; > } else { > s->free_byte_offset =3D offset; > @@ -2389,46 +2390,77 @@ static int grow_refcount_table(BlockDriv > /* XXX: cache several refcount block clusters ? */ > static int update_cluster_refcount(BlockDriverState *bs, > int64_t cluster_index, > + int nb_clusters, > int addend) > { > BDRVQcowState *s =3D bs->opaque; > int64_t refcount_block_offset; > - int ret, refcount_table_index, block_index, refcount; > + int ret, refcount_table_index, refcount_table_last_index, block_in= dex, refcount; > + int nb_block_index; > + int refcount_cache_size; > =20 > - refcount_table_index =3D cluster_index >> (s->cluster_bits - REFCO= UNT_SHIFT); > - if (refcount_table_index >=3D s->refcount_table_size) { > + if (nb_clusters =3D=3D 0) > + return 0; > + > + refcount_table_last_index =3D (cluster_index + nb_clusters - 1) >> > + (s->cluster_bits - REFCOUNT_SHIFT); > + > + /* grow the refcount table if needed */ > + > + if (refcount_table_last_index >=3D s->refcount_table_size) { > if (addend < 0) > return -EINVAL; > - ret =3D grow_refcount_table(bs, refcount_table_index + 1); > + ret =3D grow_refcount_table(bs, refcount_table_last_index + 1)= ; > if (ret < 0) > return ret; > } > - refcount_block_offset =3D s->refcount_table[refcount_table_index]; > - if (!refcount_block_offset) { > - if (addend < 0) > - return -EINVAL; > - refcount_block_offset =3D alloc_refcount_block(bs, refcount_ta= ble_index); > - if (refcount_block_offset < 0) > - return -EINVAL; > - } else { > - if (load_refcount_block(bs, refcount_block_offset) < 0) > + > + while (nb_clusters) { > + refcount_table_index =3D cluster_index >> > + (s->cluster_bits - REFCOUNT_SHIFT); > + refcount_block_offset =3D s->refcount_table[refcount_table_ind= ex]; I guess (comment? ;-)) this outer loop is for handling requests which span multiple refcount blocks? If so, am I missing something or is refcount_block_offset the same for each iteration because cluster_index never changes? > + > + if (!refcount_block_offset) { > + if (addend < 0) > + return -EINVAL; > + refcount_block_offset =3D alloc_refcount_block(bs, refcoun= t_table_index); > + if (refcount_block_offset < 0) > + return -EINVAL; > + } else { > + if (load_refcount_block(bs, refcount_block_offset) < 0) > + return -EIO; > + } > + > + /* we can update the count and save it */ > + > + refcount_cache_size =3D 1 << (s->cluster_bits - REFCOUNT_SHIFT= ); > + nb_block_index =3D 0; I have to admit that nb_block_index is a name where I can't image what the variable might be for. Seems to be a counter for the changed refcounts in the current refcount block, and block_index seems to be the first index to be touched in this block. What about first_index and cur_refcount or something like that? (I don't like these suggestions too much, either. Maybe someone has better names.) > + block_index =3D cluster_index & (refcount_cache_size - 1); > + refcount =3D 0; > + while (nb_clusters && > + block_index + nb_block_index < refcount_cache_size) { > + > + refcount =3D be16_to_cpu( > + s->refcount_block_cache[block_index + nb_bloc= k_index]); > + refcount +=3D addend; > + if (refcount < 0 || refcount > 0xffff) > + return -EINVAL; Here we possibly abort in the middle of the operation. If it fails somewhere in the fifth refcount block, what happens with the four already updated blocks? > + if (refcount =3D=3D 0 && > + cluster_index + nb_block_index < s->free_cluster_index= ) { > + s->free_cluster_index =3D cluster_index + nb_block_ind= ex; > + } > + s->refcount_block_cache[block_index + nb_block_index] =3D > + cpu_to_be16(= refcount); > + nb_block_index++; > + nb_clusters--; > + } > + if (bdrv_pwrite(s->hd, > + refcount_block_offset + (block_index << REFCOU= NT_SHIFT), > + s->refcount_block_cache + block_index, > + nb_block_index * sizeof(uint16_t)) !=3D > + nb_block_index * sizeof(uint16_t)) > return -EIO; Same here. > } > - /* we can update the count and save it */ > - block_index =3D cluster_index & > - ((1 << (s->cluster_bits - REFCOUNT_SHIFT)) - 1); > - refcount =3D be16_to_cpu(s->refcount_block_cache[block_index]); > - refcount +=3D addend; > - if (refcount < 0 || refcount > 0xffff) > - return -EINVAL; > - if (refcount =3D=3D 0 && cluster_index < s->free_cluster_index) { > - s->free_cluster_index =3D cluster_index; > - } > - s->refcount_block_cache[block_index] =3D cpu_to_be16(refcount); > - if (bdrv_pwrite(s->hd, > - refcount_block_offset + (block_index << REFCOUNT_S= HIFT), > - &s->refcount_block_cache[block_index], 2) !=3D 2) > - return -EIO; > return refcount; > } > =20 > @@ -2437,7 +2469,7 @@ static void update_refcount(BlockDriverS > int addend) > { > BDRVQcowState *s =3D bs->opaque; > - int64_t start, last, cluster_offset; > + int64_t start, last; > =20 > #ifdef DEBUG_ALLOC2 > printf("update_refcount: offset=3D%lld size=3D%lld addend=3D%d\n", > @@ -2445,12 +2477,9 @@ static void update_refcount(BlockDriverS > #endif > if (length <=3D 0) > return; > - start =3D offset & ~(s->cluster_size - 1); > - last =3D (offset + length - 1) & ~(s->cluster_size - 1); > - for(cluster_offset =3D start; cluster_offset <=3D last; > - cluster_offset +=3D s->cluster_size) { > - update_cluster_refcount(bs, cluster_offset >> s->cluster_bits,= addend); > - } > + start =3D offset >> s->cluster_bits; > + last =3D (offset + length) >> s->cluster_bits; Off by one for length % cluster_size =3D=3D 0? > + update_cluster_refcount(bs, start, last - start + 1, addend); > } > =20 > #ifdef DEBUG_ALLOC >=20 Kevin