From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53499) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XEEeH-0005Mu-Tj for qemu-devel@nongnu.org; Mon, 04 Aug 2014 05:33:52 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XEEeB-0005AK-Nm for qemu-devel@nongnu.org; Mon, 04 Aug 2014 05:33:45 -0400 Received: from lputeaux-656-01-25-125.w80-12.abo.wanadoo.fr ([80.12.84.125]:44297 helo=paradis.irqsave.net) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XEEeB-0005AB-DP for qemu-devel@nongnu.org; Mon, 04 Aug 2014 05:33:39 -0400 Date: Mon, 4 Aug 2014 11:32:56 +0200 From: =?iso-8859-1?Q?Beno=EEt?= Canet Message-ID: <20140804093255.GE17835@irqsave.net> References: <1406936961-20356-1-git-send-email-mreitz@redhat.com> <1406936961-20356-7-git-send-email-mreitz@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <1406936961-20356-7-git-send-email-mreitz@redhat.com> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v2 6/7] block/qcow2: Simplify shared L2 handling in amend List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Max Reitz Cc: Kevin Wolf , qemu-devel@nongnu.org, Stefan Hajnoczi The Saturday 02 Aug 2014 =E0 01:49:20 (+0200), Max Reitz wrote : > Currently, we have a bitmap for keeping track of which clusters have > been created during the zero cluster expansion process. This was > necessary because we need to properly increase the refcount for shared > L2 tables. >=20 > However, now we can simply take the L2 refcount and use it for the > cluster allocated for expansion. This will be the correct refcount and > therefore we don't have to remember that cluster having been allocated > any more. >=20 > Signed-off-by: Max Reitz > --- > block/qcow2-cluster.c | 90 ++++++++++++++++---------------------------= -------- > 1 file changed, 28 insertions(+), 62 deletions(-) >=20 > diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c > index 2607715..7e65c13 100644 > --- a/block/qcow2-cluster.c > +++ b/block/qcow2-cluster.c > @@ -1543,20 +1543,12 @@ fail: > * Expands all zero clusters in a specific L1 table (or deallocates th= em, for > * non-backed non-pre-allocated zero clusters). > * > - * expanded_clusters is a bitmap where every bit corresponds to one cl= uster in > - * the image file; a bit gets set if the corresponding cluster has bee= n used for > - * zero expansion (i.e., has been filled with zeroes and is referenced= from an > - * L2 table). nb_clusters contains the total cluster count of the imag= e file, > - * i.e., the number of bits in expanded_clusters. > - * > * l1_entries and *visited_l1_entries are used to keep track of progre= ss for > * status_cb(). l1_entries contains the total number of L1 entries and > * *visited_l1_entries counts all visited L1 entries. > */ > static int expand_zero_clusters_in_l1(BlockDriverState *bs, uint64_t *= l1_table, > - int l1_size, uint8_t **expanded_= clusters, > - uint64_t *nb_clusters, > - int64_t *visited_l1_entries, > + int l1_size, int64_t *visited_l1= _entries, > int64_t l1_entries, > BlockDriverAmendStatusCB *status= _cb) > { > @@ -1575,6 +1567,7 @@ static int expand_zero_clusters_in_l1(BlockDriver= State *bs, uint64_t *l1_table, > for (i =3D 0; i < l1_size; i++) { > uint64_t l2_offset =3D l1_table[i] & L1E_OFFSET_MASK; > bool l2_dirty =3D false; > + int l2_refcount; > =20 > if (!l2_offset) { > /* unallocated */ > @@ -1598,33 +1591,19 @@ static int expand_zero_clusters_in_l1(BlockDriv= erState *bs, uint64_t *l1_table, > goto fail; > } > =20 > + l2_refcount =3D qcow2_get_refcount(bs, l2_offset >> s->cluster= _bits); > + if (l2_refcount < 0) { > + ret =3D l2_refcount; > + goto fail; > + } > + > for (j =3D 0; j < s->l2_size; j++) { > uint64_t l2_entry =3D be64_to_cpu(l2_table[j]); > - int64_t offset =3D l2_entry & L2E_OFFSET_MASK, cluster_ind= ex; > + int64_t offset =3D l2_entry & L2E_OFFSET_MASK; > int cluster_type =3D qcow2_get_cluster_type(l2_entry); > bool preallocated =3D offset !=3D 0; > =20 > - if (cluster_type =3D=3D QCOW2_CLUSTER_NORMAL) { > - cluster_index =3D offset >> s->cluster_bits; > - assert((cluster_index >=3D 0) && (cluster_index < *nb_= clusters)); > - if ((*expanded_clusters)[cluster_index / 8] & > - (1 << (cluster_index % 8))) { > - /* Probably a shared L2 table; this cluster was a = zero > - * cluster which has been expanded, its refcount > - * therefore most likely requires an update. */ > - ret =3D qcow2_update_cluster_refcount(bs, cluster_= index, 1, > - QCOW2_DISCARD_= NEVER); > - if (ret < 0) { > - goto fail; > - } > - /* Since we just increased the refcount, the COPIE= D flag may > - * no longer be set. */ > - l2_table[j] =3D cpu_to_be64(l2_entry & ~QCOW_OFLAG= _COPIED); > - l2_dirty =3D true; > - } > - continue; > - } > - else if (qcow2_get_cluster_type(l2_entry) !=3D QCOW2_CLUST= ER_ZERO) { > + if (cluster_type !=3D QCOW2_CLUSTER_ZERO) { > continue; > } > =20 > @@ -1642,6 +1621,19 @@ static int expand_zero_clusters_in_l1(BlockDrive= rState *bs, uint64_t *l1_table, > ret =3D offset; > goto fail; > } > + > + if (l2_refcount > 1) { > + /* For shared L2 tables, set the refcount accordin= gly (it is > + * already 1 and needs to be l2_refcount) */ > + ret =3D qcow2_update_cluster_refcount(bs, > + offset >> s->cluster_bits, l2_refcount - 1= , > + QCOW2_DISCARD_OTHER); > + if (ret < 0) { > + qcow2_free_clusters(bs, offset, s->cluster_siz= e, > + QCOW2_DISCARD_OTHER); > + goto fail; > + } > + } > } > =20 > ret =3D qcow2_pre_write_overlap_check(bs, 0, offset, s->cl= uster_size); > @@ -1663,29 +1655,12 @@ static int expand_zero_clusters_in_l1(BlockDriv= erState *bs, uint64_t *l1_table, > goto fail; > } > =20 > - l2_table[j] =3D cpu_to_be64(offset | QCOW_OFLAG_COPIED); > - l2_dirty =3D true; > - > - cluster_index =3D offset >> s->cluster_bits; > - > - if (cluster_index >=3D *nb_clusters) { > - uint64_t old_bitmap_size =3D (*nb_clusters + 7) / 8; > - uint64_t new_bitmap_size; > - /* The offset may lie beyond the old end of the underl= ying image > - * file for growable files only */ > - assert(bs->file->growable); > - *nb_clusters =3D size_to_clusters(s, bs->file->total_s= ectors * > - BDRV_SECTOR_SIZE); > - new_bitmap_size =3D (*nb_clusters + 7) / 8; > - *expanded_clusters =3D g_realloc(*expanded_clusters, > - new_bitmap_size); > - /* clear the newly allocated space */ > - memset(&(*expanded_clusters)[old_bitmap_size], 0, > - new_bitmap_size - old_bitmap_size); > + if (l2_refcount =3D=3D 1) { > + l2_table[j] =3D cpu_to_be64(offset | QCOW_OFLAG_COPIED= ); > + } else { > + l2_table[j] =3D cpu_to_be64(offset); > } > - > - assert((cluster_index >=3D 0) && (cluster_index < *nb_clus= ters)); > - (*expanded_clusters)[cluster_index / 8] |=3D 1 << (cluster= _index % 8); > + l2_dirty =3D true; > } > =20 > if (is_active_l1) { > @@ -1750,9 +1725,7 @@ int qcow2_expand_zero_clusters(BlockDriverState *= bs, > { > BDRVQcowState *s =3D bs->opaque; > uint64_t *l1_table =3D NULL; > - uint64_t nb_clusters; > int64_t l1_entries =3D 0, visited_l1_entries =3D 0; > - uint8_t *expanded_clusters; > int ret; > int i, j; > =20 > @@ -1763,12 +1736,7 @@ int qcow2_expand_zero_clusters(BlockDriverState = *bs, > } > } > =20 > - nb_clusters =3D size_to_clusters(s, bs->file->total_sectors * > - BDRV_SECTOR_SIZE); > - expanded_clusters =3D g_malloc0((nb_clusters + 7) / 8); > - > ret =3D expand_zero_clusters_in_l1(bs, s->l1_table, s->l1_size, > - &expanded_clusters, &nb_clusters, > &visited_l1_entries, l1_entries, > status_cb); > if (ret < 0) { > @@ -1804,7 +1772,6 @@ int qcow2_expand_zero_clusters(BlockDriverState *= bs, > } > =20 > ret =3D expand_zero_clusters_in_l1(bs, l1_table, s->snapshots[= i].l1_size, > - &expanded_clusters, &nb_clust= ers, > &visited_l1_entries, l1_entri= es, > status_cb); > if (ret < 0) { > @@ -1815,7 +1782,6 @@ int qcow2_expand_zero_clusters(BlockDriverState *= bs, > ret =3D 0; > =20 > fail: > - g_free(expanded_clusters); > g_free(l1_table); > return ret; > } > --=20 > 2.0.3 >=20 I don't understand this one very well so I will not Rev-By it.