From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52607) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XgaqO-0005zR-Sf for qemu-devel@nongnu.org; Tue, 21 Oct 2014 10:55:33 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XgaqJ-0003Wp-44 for qemu-devel@nongnu.org; Tue, 21 Oct 2014 10:55:28 -0400 Received: from mx1.redhat.com ([209.132.183.28]:7115) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XgaqI-0003WI-Ja for qemu-devel@nongnu.org; Tue, 21 Oct 2014 10:55:22 -0400 Message-ID: <544673D2.2060209@redhat.com> Date: Tue, 21 Oct 2014 16:55:14 +0200 From: Max Reitz MIME-Version: 1.0 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> In-Reply-To: <5446326E.7080500@redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed 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: Kevin Wolf Cc: qemu-devel@nongnu.org, Stefan Hajnoczi , =?windows-1252?Q?Beno=EEt_Canet?= 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 rebuil= t >>> it, we need to recalculate the refcounts and run a leak-fixing=20 >>> 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=20 >>> *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_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=20 >>> 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,=20 >>> &refcount_table, >>> + &nb_clusters); >>> + if (ret < 0) { >>> + goto fail; >>> + } >>> + >>> + if (fix & BDRV_FIX_LEAKS) { >>> + /* The old refcount structures are now leaked, fix it;=20 >>> 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. > > Why not. > >>> + compare_refcounts(bs, res, BDRV_FIX_LEAKS, &rebuild, >>> + &highest_cluster, refcount_table,=20 >>> nb_clusters); >>> + if (rebuild) { >>> + fprintf(stderr, "ERROR rebuilt refcount structure=20 >>> is still " >>> + "broken\n"); >>> + } >>> + *res =3D pre_compare_res; >>> + } >>> + >>> + if (res->corruptions < old_res.corruptions) { >>> + res->corruptions_fixed +=3D old_res.corruptions -=20 >>> 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,=20 > you cannot use BDRV_FIX_ERRORS without BDRV_FIX_LEAKS. But it'd be=20 > more correct, right. Wait, it would not be more correct. The result of the=20 compare_refcounts() call inside of the "if (fix & BDRV_FIX_LEAKS)"=20 conditional block is ignored, its only purpose is to fix leaks resulting=20 from rebuild_refcount_structure(). So the question is whether we should discard the result of that=20 compare_refcounts() call. I think we should. Its sole purpose is to fix=20 leaks due to the rebuilt refcount structures, and qemu-img will double=20 check anyway. Max > Thanks, > > Max > >> 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=20 >>> structures\n"); >>> --=20 >>> 2.1.2 >> Kevin >