From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59519) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XDJnz-0007f8-Vx for qemu-devel@nongnu.org; Fri, 01 Aug 2014 16:52:06 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XDJnt-0004pc-PB for qemu-devel@nongnu.org; Fri, 01 Aug 2014 16:51:59 -0400 Received: from mx1.redhat.com ([209.132.183.28]:45098) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XDJnt-0004pQ-Gq for qemu-devel@nongnu.org; Fri, 01 Aug 2014 16:51:53 -0400 Message-ID: <53DBFDE3.7010402@redhat.com> Date: Fri, 01 Aug 2014 22:51:47 +0200 From: Max Reitz MIME-Version: 1.0 References: <1406402531-9278-1-git-send-email-mreitz@redhat.com> <1406402531-9278-7-git-send-email-mreitz@redhat.com> <20140731082420.GK707@irqsave.net> In-Reply-To: <20140731082420.GK707@irqsave.net> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH alt 6/7] block/qcow2: Simplify shared L2 handling in amend List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?windows-1252?Q?Beno=EEt_Canet?= Cc: Kevin Wolf , qemu-devel@nongnu.org, Stefan Hajnoczi On 31.07.2014 10:24, Beno=EEt Canet wrote: > The Saturday 26 Jul 2014 =E0 21:22:10 (+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. >> >> 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. >> >> Signed-off-by: Max Reitz >> --- >> block/qcow2-cluster.c | 90 ++++++++++++++++-------------------------= ---------- >> 1 file changed, 28 insertions(+), 62 deletions(-) >> >> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c >> index f8bec6f..e6bff40 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 = them, for >> * non-backed non-pre-allocated zero clusters). >> * >> - * expanded_clusters is a bitmap where every bit corresponds to one c= luster in >> - * the image file; a bit gets set if the corresponding cluster has be= en used for >> - * zero expansion (i.e., has been filled with zeroes and is reference= d from an >> - * L2 table). nb_clusters contains the total cluster count of the ima= ge file, >> - * i.e., the number of bits in expanded_clusters. >> - * >> * l1_entries and *visited_l1_entries are ued to keep track of progr= ess for >> * status_cb(). l1_entries contains the total number of L1 entries a= nd >> * *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_l= 1_entries, >> int64_t l1_entries, >> BlockDriverAmendStatusCB *stat= us_cb) >> { >> @@ -1575,6 +1567,7 @@ static int expand_zero_clusters_in_l1(BlockDrive= rState *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 */ >> @@ -1595,33 +1588,19 @@ static int expand_zero_clusters_in_l1(BlockDri= verState *bs, uint64_t *l1_table, >> goto fail; >> } >> =20 >> + l2_refcount =3D qcow2_get_refcount(bs, l2_offset >> s->cluste= r_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_in= dex; >> + 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 COPI= ED flag may >> - * no longer be set. */ >> - l2_table[j] =3D cpu_to_be64(l2_entry & ~QCOW_OFLA= G_COPIED); >> - l2_dirty =3D true; >> - } >> - continue; >> - } >> - else if (qcow2_get_cluster_type(l2_entry) !=3D QCOW2_CLUS= TER_ZERO) { >> + if (cluster_type !=3D QCOW2_CLUSTER_ZERO) { >> continue; >> } >> =20 >> @@ -1639,6 +1618,19 @@ static int expand_zero_clusters_in_l1(BlockDriv= erState *bs, uint64_t *l1_table, >> ret =3D offset; >> goto fail; >> } >> + >> + if (l2_refcount > 1) { >> + /* For shared L2 tables, set the refcount accordi= ngly (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); > This look like a wrong usage of qcow2_update_cluster_refcount: > > /* > * Increases or decreases the refcount of a given cluster by one. > * addend must be 1 or -1. > Here ^ As far as I can see, that comment no longer applies (anything in=20 update_refcount() very well allows arbitrary values). I'll remove it in v= 2. > * > * If the return value is non-negative, it is the new refcount of the = cluster. > * If it is negative, it is -errno and indicates an error. > */ > int qcow2_update_cluster_refcount(BlockDriverState *bs, > int64_t cluster_index, > int addend, > enum qcow2_discard_type type) > > Also this call is in a loop it would do l2_refcount - 1 * n increments = on the refcount. Hm? The cluster at "offset" is allocated directly before the call to=20 qcow2_update_cluster_refcount(). There is no loop which the latter call=20 is in, but the allocation is not. Max >> + if (ret < 0) { >> + qcow2_free_clusters(bs, offset, s->cluster_si= ze, >> + QCOW2_DISCARD_OTHER); >> + goto fail; >> + } >> + } > > > >> } >> =20 >> ret =3D qcow2_pre_write_overlap_check(bs, 0, offset, s->= cluster_size); >> @@ -1660,29 +1652,12 @@ static int expand_zero_clusters_in_l1(BlockDri= verState *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 under= lying image >> - * file for growable files only */ >> - assert(bs->file->growable); >> - *nb_clusters =3D size_to_clusters(s, bs->file->total_= sectors * >> - 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_COPIE= D); >> + } else { >> + l2_table[j] =3D cpu_to_be64(offset); >> } >> - >> - assert((cluster_index >=3D 0) && (cluster_index < *nb_clu= sters)); >> - (*expanded_clusters)[cluster_index / 8] |=3D 1 << (cluste= r_index % 8); >> + l2_dirty =3D true; >> } >> =20 >> if (is_active_l1) { >> @@ -1749,9 +1724,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 >> @@ -1762,12 +1735,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) { >> @@ -1803,7 +1771,6 @@ int qcow2_expand_zero_clusters(BlockDriverState = *bs, >> } >> =20 >> ret =3D expand_zero_clusters_in_l1(bs, l1_table, s->snapshot= s[i].l1_size, >> - &expanded_clusters, &nb_clus= ters, >> &visited_l1_entries, l1_ent= ries, >> status_cb); >> if (ret < 0) { >> @@ -1814,7 +1781,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 >> >>