From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36882) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xgb6A-0005fE-6r for qemu-devel@nongnu.org; Tue, 21 Oct 2014 11:11:51 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Xgb65-00053Z-4E for qemu-devel@nongnu.org; Tue, 21 Oct 2014 11:11:46 -0400 Received: from mx1.redhat.com ([209.132.183.28]:2998) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xgb64-00053R-TZ for qemu-devel@nongnu.org; Tue, 21 Oct 2014 11:11:41 -0400 Date: Tue, 21 Oct 2014 17:11:34 +0200 From: Kevin Wolf Message-ID: <20141021151134.GL4409@noname.redhat.com> References: <1413815733-22829-1-git-send-email-mreitz@redhat.com> <1413815733-22829-10-git-send-email-mreitz@redhat.com> <20141021095937.GF4409@noname.redhat.com> <5446326E.7080500@redhat.com> <544673D2.2060209@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <544673D2.2060209@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 21.10.2014 um 16:55 hat Max Reitz geschrieben: > On 2014-10-21 at 12:16, Max Reitz wrote: > >On 2014-10-21 at 11:59, Kevin Wolf wrote: > >>Am 20.10.2014 um 16:35 hat Max Reitz geschrieben: > >>>Because the old refcount structure will be leaked after having rebui= lt > >>>it, we need to recalculate the refcounts and run a leak-fixing > >>>operation > >>>afterwards (if leaks should be fixed at all). > >>> > >>>Signed-off-by: Max Reitz > >>>Reviewed-by: Beno=EEt Canet > >>>--- > >>> block/qcow2-refcount.c | 35 +++++++++++++++++++++++++++++++++++ > >>> 1 file changed, 35 insertions(+) > >>> > >>>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); > >>> 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_tabl= e, > >>> &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 the > >>>+ * 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_table, > >>>+ &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 t= o > >>which state. > > > >Why not. > > > >>>+ compare_refcounts(bs, res, BDRV_FIX_LEAKS, &rebuild, > >>>+ &highest_cluster, > >>>refcount_table, nb_clusters); > >>>+ if (rebuild) { > >>>+ fprintf(stderr, "ERROR rebuilt refcount > >>>structure is still " > >>>+ "broken\n"); > >>>+ } > >>>+ *res =3D pre_compare_res; > >>>+ } > >>>+ > >>>+ if (res->corruptions < old_res.corruptions) { > >>>+ res->corruptions_fixed +=3D old_res.corruptions - > >>>res->corruptions; > >>>+ } > >>>+ 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? > > > >Actually, there is no difference, because at the point of this > >patch, you cannot use BDRV_FIX_ERRORS without BDRV_FIX_LEAKS. But > >it'd be more correct, right. >=20 > Wait, it would not be more correct. The result of the > compare_refcounts() call inside of the "if (fix & BDRV_FIX_LEAKS)" > conditional block is ignored, its only purpose is to fix leaks > resulting from rebuild_refcount_structure(). >=20 > So the question is whether we should discard the result of that > compare_refcounts() call. I think we should. Its sole purpose is to > fix leaks due to the rebuilt refcount structures, and qemu-img will > double check anyway. Right, the other leaks should have been fixed by rebuilding the refcount structures. So what you're saying is that we could do this: if (res->corruptions < old_res.corruptions) { res->corruptions_fixed +=3D old_res.corruptions - res->corruption= s; } assert(res->leaks =3D=3D 0); res->leaks_fixed =3D old_res.leaks; If this weren't true, we'd ignore leaked clusters even with BDRV_FIX_LEAKS set. Kevin