From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55064) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XDJI5-00061f-Tg for qemu-devel@nongnu.org; Fri, 01 Aug 2014 16:19:07 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XDJI0-0002Mh-Q6 for qemu-devel@nongnu.org; Fri, 01 Aug 2014 16:19:01 -0400 Received: from mx1.redhat.com ([209.132.183.28]:44672) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XDJI0-0002MT-Hv for qemu-devel@nongnu.org; Fri, 01 Aug 2014 16:18:56 -0400 Message-ID: <53DBF629.6000703@redhat.com> Date: Fri, 01 Aug 2014 22:18:49 +0200 From: Max Reitz MIME-Version: 1.0 References: <1406402531-9278-1-git-send-email-mreitz@redhat.com> <1406402531-9278-5-git-send-email-mreitz@redhat.com> <20140731080634.GH707@irqsave.net> In-Reply-To: <20140731080634.GH707@irqsave.net> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH alt 4/7] block/qcow2: Implement status CB for 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:06, Beno=EEt Canet wrote: > The Saturday 26 Jul 2014 =E0 21:22:08 (+0200), Max Reitz wrote : >> The only really time-consuming operation potentially performed by >> qcow2_amend_options() is zero cluster expansion when downgrading qcow2 >> images from compat=3D1.1 to compat=3D0.10, so report status of that >> operation and that operation only through the status CB. >> >> For this, approximate the progress as the number of L1 entries visited >> during the operation. >> >> Signed-off-by: Max Reitz >> --- >> block/qcow2-cluster.c | 36 ++++++++++++++++++++++++++++++++---- >> block/qcow2.c | 9 ++++----- >> block/qcow2.h | 3 ++- >> 3 files changed, 38 insertions(+), 10 deletions(-) >> >> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c >> index 4208dc0..f8bec6f 100644 >> --- a/block/qcow2-cluster.c >> +++ b/block/qcow2-cluster.c >> @@ -1548,10 +1548,17 @@ fail: >> * zero expansion (i.e., has been filled with zeroes and is referenc= ed from an >> * L2 table). nb_clusters contains the total cluster count of the im= age file, >> * i.e., the number of bits in expanded_clusters. >> + * >> + * l1_entries and *visited_l1_entries are ued to keep track of progre= ss for >> + * status_cb(). l1_entries contains the total number of L1 entries an= d >> + * *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 **expande= d_clusters, >> - uint64_t *nb_clusters) >> + uint64_t *nb_clusters, >> + int64_t *visited_l1_entries, >> + int64_t l1_entries, >> + BlockDriverAmendStatusCB *statu= s_cb) >> { >> BDRVQcowState *s =3D bs->opaque; >> bool is_active_l1 =3D (l1_table =3D=3D s->l1_table); >> @@ -1571,6 +1578,7 @@ static int expand_zero_clusters_in_l1(BlockDrive= rState *bs, uint64_t *l1_table, >> =20 >> if (!l2_offset) { >> /* unallocated */ >> + (*visited_l1_entries)++; >> continue; >> } >> =20 >> @@ -1703,6 +1711,13 @@ static int expand_zero_clusters_in_l1(BlockDriv= erState *bs, uint64_t *l1_table, >> } >> } >> } >> + >> + (*visited_l1_entries)++; >> + >> + if (status_cb) { >> + status_cb(bs, *visited_l1_entries << (s->l2_bits + s->clu= ster_bits), >> + l1_entries << (s->l2_bits + s->cluster_bits)); > Shifting is a multiplication so it keep proportionality intact. > So do we really need these shifts ? As of patch 1, the variables are defined as "offset" and "working_size"=20 which I meant to be byte ranges. We could indeed leave the unit for=20 BlockDriverAmendStatusCB's parameters undefined (and leave it up to the=20 block driver, only specifying that offset / working_size has to be the=20 progress ratio), but then we could just as well just give a floating=20 point percentage to that function. Bytes as a unit seemed safe to me, however, since all of qemu's code=20 assumes byte ranges to always fit in int64_t; and the reason I preferred=20 them over a percentage is that block jobs use bytes as well. A real reason not to use bytes would be that some driver is unable to=20 give a "byte" representation of its workload during the amend progress;=20 however, this seems unlikely to me (every large workload which should be=20 part of the progress report should be somehow representable as bytes). Max >> + } >> } >> =20 >> ret =3D 0; >> @@ -1729,21 +1744,32 @@ fail: >> * allocation for pre-allocated ones). This is important for downgra= ding to a >> * qcow2 version which doesn't yet support metadata zero clusters. >> */ >> -int qcow2_expand_zero_clusters(BlockDriverState *bs) >> +int qcow2_expand_zero_clusters(BlockDriverState *bs, >> + BlockDriverAmendStatusCB *status_cb) >> { >> 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 >> + if (status_cb) { >> + l1_entries =3D s->l1_size; >> + for (i =3D 0; i < s->nb_snapshots; i++) { >> + l1_entries +=3D s->snapshots[i].l1_size; >> + } >> + } >> + >> nb_clusters =3D size_to_clusters(s, bs->file->total_sectors * >> BDRV_SECTOR_SIZE); >> expanded_clusters =3D g_malloc0((nb_clusters + 7) / 8); >> =20 >> ret =3D expand_zero_clusters_in_l1(bs, s->l1_table, s->l1_size, >> - &expanded_clusters, &nb_clusters= ); >> + &expanded_clusters, &nb_clusters= , >> + &visited_l1_entries, l1_entries, >> + status_cb); >> if (ret < 0) { >> goto fail; >> } >> @@ -1777,7 +1803,9 @@ 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); >> + &expanded_clusters, &nb_clus= ters, >> + &visited_l1_entries, l1_entr= ies, >> + status_cb); >> if (ret < 0) { >> goto fail; >> } >> diff --git a/block/qcow2.c b/block/qcow2.c >> index 757f890..6e8c8ab 100644 >> --- a/block/qcow2.c >> +++ b/block/qcow2.c >> @@ -2147,7 +2147,8 @@ static int qcow2_load_vmstate(BlockDriverState *= bs, uint8_t *buf, >> * Downgrades an image's version. To achieve this, any incompatible = features >> * have to be removed. >> */ >> -static int qcow2_downgrade(BlockDriverState *bs, int target_version) >> +static int qcow2_downgrade(BlockDriverState *bs, int target_version, >> + BlockDriverAmendStatusCB *status_cb) >> { >> BDRVQcowState *s =3D bs->opaque; >> int current_version =3D s->qcow_version; >> @@ -2196,7 +2197,7 @@ static int qcow2_downgrade(BlockDriverState *bs,= int target_version) >> /* clearing autoclear features is trivial */ >> s->autoclear_features =3D 0; >> =20 >> - ret =3D qcow2_expand_zero_clusters(bs); >> + ret =3D qcow2_expand_zero_clusters(bs, status_cb); >> if (ret < 0) { >> return ret; >> } >> @@ -2224,8 +2225,6 @@ static int qcow2_amend_options(BlockDriverState = *bs, QemuOpts *opts, >> int ret; >> QemuOptDesc *desc =3D opts->list->desc; >> =20 >> - (void)status_cb; >> - >> while (desc && desc->name) { >> if (!qemu_opt_find(opts, desc->name)) { >> /* only change explicitly defined options */ >> @@ -2291,7 +2290,7 @@ static int qcow2_amend_options(BlockDriverState = *bs, QemuOpts *opts, >> return ret; >> } >> } else { >> - ret =3D qcow2_downgrade(bs, new_version); >> + ret =3D qcow2_downgrade(bs, new_version, status_cb); >> if (ret < 0) { >> return ret; >> } >> diff --git a/block/qcow2.h b/block/qcow2.h >> index b49424b..1c4f9bf 100644 >> --- a/block/qcow2.h >> +++ b/block/qcow2.h >> @@ -522,7 +522,8 @@ int qcow2_discard_clusters(BlockDriverState *bs, u= int64_t offset, >> int nb_sectors, enum qcow2_discard_type type); >> int qcow2_zero_clusters(BlockDriverState *bs, uint64_t offset, int n= b_sectors); >> =20 >> -int qcow2_expand_zero_clusters(BlockDriverState *bs); >> +int qcow2_expand_zero_clusters(BlockDriverState *bs, >> + BlockDriverAmendStatusCB *status_cb); >> =20 >> /* qcow2-snapshot.c functions */ >> int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn= _info); >> --=20 >> 2.0.3 >> >> > Reviewed-by: Benoit Canet