From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42136) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XgWEG-0008R5-GY for qemu-devel@nongnu.org; Tue, 21 Oct 2014 05:59:53 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XgWEB-0004qm-D4 for qemu-devel@nongnu.org; Tue, 21 Oct 2014 05:59:48 -0400 Received: from mx1.redhat.com ([209.132.183.28]:51935) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XgWEB-0004qa-5u for qemu-devel@nongnu.org; Tue, 21 Oct 2014 05:59:43 -0400 Date: Tue, 21 Oct 2014 11:59:37 +0200 From: Kevin Wolf Message-ID: <20141021095937.GF4409@noname.redhat.com> References: <1413815733-22829-1-git-send-email-mreitz@redhat.com> <1413815733-22829-10-git-send-email-mreitz@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <1413815733-22829-10-git-send-email-mreitz@redhat.com> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v6 09/11] qcow2: Clean up after refcount rebuild List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Max Reitz Cc: qemu-devel@nongnu.org, Stefan Hajnoczi , =?iso-8859-1?Q?Beno=EEt?= Canet Am 20.10.2014 um 16:35 hat Max Reitz geschrieben: > Because the old refcount structure will be leaked after having rebuilt > it, we need to recalculate the refcounts and run a leak-fixing operatio= n > afterwards (if leaks should be fixed at all). >=20 > Signed-off-by: Max Reitz > Reviewed-by: Beno=EEt Canet > --- > block/qcow2-refcount.c | 35 +++++++++++++++++++++++++++++++++++ > 1 file changed, 35 insertions(+) >=20 > diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c > index 75e726b..3730be2 100644 > --- a/block/qcow2-refcount.c > +++ b/block/qcow2-refcount.c > @@ -1956,12 +1956,47 @@ int qcow2_check_refcounts(BlockDriverState *bs,= BdrvCheckResult *res, > nb_clusters); > =20 > if (rebuild && (fix & BDRV_FIX_ERRORS)) { > + BdrvCheckResult old_res =3D *res; > + > fprintf(stderr, "Rebuilding refcount structure\n"); > ret =3D rebuild_refcount_structure(bs, res, &refcount_table, > &nb_clusters); > if (ret < 0) { > goto fail; > } > + > + res->corruptions =3D 0; > + res->leaks =3D 0; > + > + /* Because the old reftable has been exchanged for a new one t= he > + * references have to be recalculated */ > + rebuild =3D false; > + memset(refcount_table, 0, nb_clusters * sizeof(uint16_t)); > + ret =3D calculate_refcounts(bs, res, 0, &rebuild, &refcount_ta= ble, > + &nb_clusters); > + if (ret < 0) { > + goto fail; > + } > + > + if (fix & BDRV_FIX_LEAKS) { > + /* The old refcount structures are now leaked, fix it; the= result > + * can be ignored */ > + pre_compare_res =3D *res; I would prefer using another local variable here. At the first sight it's not quite clear which references to pre_compare_res correspond to which state. > + compare_refcounts(bs, res, BDRV_FIX_LEAKS, &rebuild, > + &highest_cluster, refcount_table, nb_clu= sters); > + if (rebuild) { > + fprintf(stderr, "ERROR rebuilt refcount structure is s= till " > + "broken\n"); > + } > + *res =3D pre_compare_res; > + } > + > + if (res->corruptions < old_res.corruptions) { > + res->corruptions_fixed +=3D old_res.corruptions - res->cor= ruptions; > + } > + if (res->leaks < old_res.leaks) { > + res->leaks_fixed +=3D old_res.leaks - res->leaks; > + } For these numbers to be accurate, don't we need to run compare_refcounts() unconditionally and only make BDRV_FIX_LEAKS conditional? Now that I've read the rest of the series, comparing res and old_res actually makes sense, so maybe it's not necessary to introduce a Qcow2CheckResult. > } else if (fix) { > if (rebuild) { > fprintf(stderr, "ERROR need to rebuild refcount structures= \n"); > --=20 > 2.1.2 Kevin