From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56122) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VFLAl-0000qB-VR for qemu-devel@nongnu.org; Fri, 30 Aug 2013 05:39:26 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VFLAf-0008Pp-G1 for qemu-devel@nongnu.org; Fri, 30 Aug 2013 05:39:19 -0400 Received: from mx1.redhat.com ([209.132.183.28]:59282) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VFLAf-0008Pj-8I for qemu-devel@nongnu.org; Fri, 30 Aug 2013 05:39:13 -0400 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r7U9dCfX027864 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Fri, 30 Aug 2013 05:39:12 -0400 Message-ID: <5220683E.8020808@redhat.com> Date: Fri, 30 Aug 2013 11:39:10 +0200 From: Max Reitz MIME-Version: 1.0 References: <1377848818-2623-1-git-send-email-mreitz@redhat.com> <1377848818-2623-5-git-send-email-mreitz@redhat.com> <20130830084842.GA2840@dhcp-200-207.str.redhat.com> In-Reply-To: <20130830084842.GA2840@dhcp-200-207.str.redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v3 4/8] qcow2-refcount: Move OFLAG_COPIED checks List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: qemu-devel@nongnu.org, Stefan Hajnoczi Am 30.08.2013 10:48, schrieb Kevin Wolf: > Am 30.08.2013 um 09:46 hat Max Reitz geschrieben: >> Move the OFLAG_COPIED checks out of check_refcounts_l1 and >> check_refcounts_l2 and after the actual refcount checks/fixes (since t= he >> refcounts might actually change there). >> >> Signed-off-by: Max Reitz >> --- >> block/qcow2-refcount.c | 99 +++++++++++++++++++++++++++++++++-------= ---------- >> 1 file changed, 65 insertions(+), 34 deletions(-) >> >> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c >> index 310efcc..fdc0f86 100644 >> --- a/block/qcow2-refcount.c >> +++ b/block/qcow2-refcount.c >> @@ -1035,7 +1035,7 @@ static int check_refcounts_l2(BlockDriverState *= bs, BdrvCheckResult *res, >> BDRVQcowState *s =3D bs->opaque; >> uint64_t *l2_table, l2_entry; >> uint64_t next_contiguous_offset =3D 0; >> - int i, l2_size, nb_csectors, refcount; >> + int i, l2_size, nb_csectors; >> =20 >> /* Read L2 table from disk */ >> l2_size =3D s->l2_size * sizeof(uint64_t); >> @@ -1087,23 +1087,8 @@ static int check_refcounts_l2(BlockDriverState = *bs, BdrvCheckResult *res, >> =20 >> case QCOW2_CLUSTER_NORMAL: >> { >> - /* QCOW_OFLAG_COPIED must be set iff refcount =3D=3D 1 */ >> uint64_t offset =3D l2_entry & L2E_OFFSET_MASK; >> =20 >> - if (flags & CHECK_OFLAG_COPIED) { >> - refcount =3D get_refcount(bs, offset >> s->cluster_bi= ts); >> - if (refcount < 0) { >> - fprintf(stderr, "Can't get refcount for offset %" >> - PRIx64 ": %s\n", l2_entry, strerror(-refcount= )); >> - goto fail; >> - } >> - if ((refcount =3D=3D 1) !=3D ((l2_entry & QCOW_OFLAG_= COPIED) !=3D 0)) { >> - fprintf(stderr, "ERROR OFLAG_COPIED: offset=3D%" >> - PRIx64 " refcount=3D%d\n", l2_entry, refcount= ); >> - res->corruptions++; >> - } >> - } >> - >> if (flags & CHECK_FRAG_INFO) { >> res->bfi.allocated_clusters++; >> if (next_contiguous_offset && >> @@ -1160,7 +1145,7 @@ static int check_refcounts_l1(BlockDriverState *= bs, >> { >> BDRVQcowState *s =3D bs->opaque; >> uint64_t *l1_table, l2_offset, l1_size2; >> - int i, refcount, ret; >> + int i, ret; >> =20 >> l1_size2 =3D l1_size * sizeof(uint64_t); >> =20 >> @@ -1184,22 +1169,6 @@ static int check_refcounts_l1(BlockDriverState = *bs, >> for(i =3D 0; i < l1_size; i++) { >> l2_offset =3D l1_table[i]; >> if (l2_offset) { >> - /* QCOW_OFLAG_COPIED must be set iff refcount =3D=3D 1 */ >> - if (flags & CHECK_OFLAG_COPIED) { >> - refcount =3D get_refcount(bs, (l2_offset & ~QCOW_OFLA= G_COPIED) >> - >> s->cluster_bits); >> - if (refcount < 0) { >> - fprintf(stderr, "Can't get refcount for l2_offset= %" >> - PRIx64 ": %s\n", l2_offset, strerror(-refcoun= t)); >> - goto fail; >> - } >> - if ((refcount =3D=3D 1) !=3D ((l2_offset & QCOW_OFLAG= _COPIED) !=3D 0)) { >> - fprintf(stderr, "ERROR OFLAG_COPIED: l2_offset=3D= %" PRIx64 >> - " refcount=3D%d\n", l2_offset, refcount); >> - res->corruptions++; >> - } >> - } >> - >> /* Mark L2 table as used */ >> l2_offset &=3D L1E_OFFSET_MASK; >> inc_refcounts(bs, res, refcount_table, refcount_table_si= ze, >> @@ -1241,7 +1210,8 @@ int qcow2_check_refcounts(BlockDriverState *bs, = BdrvCheckResult *res, >> { >> BDRVQcowState *s =3D bs->opaque; >> int64_t size, i, highest_cluster; >> - int nb_clusters, refcount1, refcount2; >> + uint64_t *l2_table =3D NULL; >> + int nb_clusters, refcount1, refcount2, j; >> QCowSnapshot *sn; >> uint16_t *refcount_table; >> int ret; >> @@ -1365,10 +1335,71 @@ int qcow2_check_refcounts(BlockDriverState *bs= , BdrvCheckResult *res, >> } >> } >> =20 >> + l2_table =3D qemu_blockalign(bs, s->cluster_size); >> + >> + /* check OFLAG_COPIED */ >> + for (i =3D 0; i < s->l1_size; i++) { >> + uint64_t l1_entry =3D s->l1_table[i]; >> + uint64_t l2_offset =3D l1_entry & L1E_OFFSET_MASK; >> + int refcount; >> + >> + if (!l2_offset) { >> + continue; >> + } >> + >> + refcount =3D get_refcount(bs, l2_offset >> s->cluster_bits); >> + if (refcount < 0) { >> + /* don't print message nor increment check_errors, since = the above >> + * loop will have done this already */ >> + continue; >> + } >> + if ((refcount =3D=3D 1) !=3D ((l1_entry & QCOW_OFLAG_COPIED) = !=3D 0)) { >> + fprintf(stderr, "ERROR OFLAG_COPIED L2 cluster: l1_entry=3D= %" PRIx64 >> + " refcount=3D%d\n", >> + l1_entry, refcount); > In theory, code motion patches shouldn't change much else, but as you'r= e > already changing the message here, I wouldn't mind if you also sneaked > in the L1 index. ;-) Okay, I'll do that. >> + res->corruptions++; >> + } >> + >> + ret =3D bdrv_pread(bs->file, l2_offset, l2_table, >> + s->l2_size * sizeof(uint64_t)); > Indentation: s would usually be aligned to the same column as bs. > >> + if (ret < 0) { >> + fprintf(stderr, "ERROR: Could not read L2 table: %s\n", >> + strerror(-ret)); >> + res->check_errors++; >> + goto fail; >> + } >> + >> + for (j =3D 0; j < s->l2_size; j++) { >> + uint64_t l2_entry =3D be64_to_cpu(l2_table[j]); >> + uint64_t data_offset; >> + >> + if (qcow2_get_cluster_type(l2_entry) !=3D QCOW2_CLUSTER_N= ORMAL) { >> + continue; >> + } > You're removing the check for QCOW2_CLUSTER_ZERO, which was a > fall-through case in the original code. Oh, right. I keep forgetting preallocations=85 :-/ >> + >> + data_offset =3D l2_entry & L2E_OFFSET_MASK; >> + >> + refcount =3D get_refcount(bs, data_offset >> s->cluster_b= its); >> + if (refcount < 0) { >> + /* don't print message nor increment check_errors, si= nce the >> + * above loop will have done this already */ >> + continue; >> + } >> + if ((refcount =3D=3D 1) !=3D ((l2_entry & QCOW_OFLAG_COPI= ED) !=3D 0)) { >> + fprintf(stderr, "ERROR OFLAG_COPIED data cluster: l2_= entry=3D%" >> + PRIx64 " refcount=3D%d\n", >> + l2_entry, refcount); >> + res->corruptions++; >> + } >> + } >> + } > I think it's long enough that it deserves a separate function. Okay. >> + >> res->image_end_offset =3D (highest_cluster + 1) * s->cluster_siz= e; >> ret =3D 0; >> =20 >> fail: >> + qemu_vfree(l2_table); >> g_free(refcount_table); >> =20 >> return ret; > Kevin Max