From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42330) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VFKNw-0008AF-0w for qemu-devel@nongnu.org; Fri, 30 Aug 2013 04:48:58 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VFKNp-0007iL-5B for qemu-devel@nongnu.org; Fri, 30 Aug 2013 04:48:51 -0400 Received: from mx1.redhat.com ([209.132.183.28]:10243) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VFKNo-0007iG-SH for qemu-devel@nongnu.org; Fri, 30 Aug 2013 04:48:45 -0400 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r7U8miCQ013050 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Fri, 30 Aug 2013 04:48:44 -0400 Date: Fri, 30 Aug 2013 10:48:42 +0200 From: Kevin Wolf Message-ID: <20130830084842.GA2840@dhcp-200-207.str.redhat.com> References: <1377848818-2623-1-git-send-email-mreitz@redhat.com> <1377848818-2623-5-git-send-email-mreitz@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1377848818-2623-5-git-send-email-mreitz@redhat.com> 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: Max Reitz Cc: qemu-devel@nongnu.org, Stefan Hajnoczi 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 the > 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 = bs->opaque; > uint64_t *l2_table, l2_entry; > uint64_t next_contiguous_offset = 0; > - int i, l2_size, nb_csectors, refcount; > + int i, l2_size, nb_csectors; > > /* Read L2 table from disk */ > l2_size = s->l2_size * sizeof(uint64_t); > @@ -1087,23 +1087,8 @@ static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res, > > case QCOW2_CLUSTER_NORMAL: > { > - /* QCOW_OFLAG_COPIED must be set iff refcount == 1 */ > uint64_t offset = l2_entry & L2E_OFFSET_MASK; > > - if (flags & CHECK_OFLAG_COPIED) { > - refcount = get_refcount(bs, offset >> s->cluster_bits); > - if (refcount < 0) { > - fprintf(stderr, "Can't get refcount for offset %" > - PRIx64 ": %s\n", l2_entry, strerror(-refcount)); > - goto fail; > - } > - if ((refcount == 1) != ((l2_entry & QCOW_OFLAG_COPIED) != 0)) { > - fprintf(stderr, "ERROR OFLAG_COPIED: offset=%" > - PRIx64 " refcount=%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 = bs->opaque; > uint64_t *l1_table, l2_offset, l1_size2; > - int i, refcount, ret; > + int i, ret; > > l1_size2 = l1_size * sizeof(uint64_t); > > @@ -1184,22 +1169,6 @@ static int check_refcounts_l1(BlockDriverState *bs, > for(i = 0; i < l1_size; i++) { > l2_offset = l1_table[i]; > if (l2_offset) { > - /* QCOW_OFLAG_COPIED must be set iff refcount == 1 */ > - if (flags & CHECK_OFLAG_COPIED) { > - refcount = get_refcount(bs, (l2_offset & ~QCOW_OFLAG_COPIED) > - >> s->cluster_bits); > - if (refcount < 0) { > - fprintf(stderr, "Can't get refcount for l2_offset %" > - PRIx64 ": %s\n", l2_offset, strerror(-refcount)); > - goto fail; > - } > - if ((refcount == 1) != ((l2_offset & QCOW_OFLAG_COPIED) != 0)) { > - fprintf(stderr, "ERROR OFLAG_COPIED: l2_offset=%" PRIx64 > - " refcount=%d\n", l2_offset, refcount); > - res->corruptions++; > - } > - } > - > /* Mark L2 table as used */ > l2_offset &= L1E_OFFSET_MASK; > inc_refcounts(bs, res, refcount_table, refcount_table_size, > @@ -1241,7 +1210,8 @@ int qcow2_check_refcounts(BlockDriverState *bs, BdrvCheckResult *res, > { > BDRVQcowState *s = bs->opaque; > int64_t size, i, highest_cluster; > - int nb_clusters, refcount1, refcount2; > + uint64_t *l2_table = 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, > } > } > > + l2_table = qemu_blockalign(bs, s->cluster_size); > + > + /* check OFLAG_COPIED */ > + for (i = 0; i < s->l1_size; i++) { > + uint64_t l1_entry = s->l1_table[i]; > + uint64_t l2_offset = l1_entry & L1E_OFFSET_MASK; > + int refcount; > + > + if (!l2_offset) { > + continue; > + } > + > + refcount = 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 == 1) != ((l1_entry & QCOW_OFLAG_COPIED) != 0)) { > + fprintf(stderr, "ERROR OFLAG_COPIED L2 cluster: l1_entry=%" PRIx64 > + " refcount=%d\n", > + l1_entry, refcount); In theory, code motion patches shouldn't change much else, but as you're already changing the message here, I wouldn't mind if you also sneaked in the L1 index. ;-) > + res->corruptions++; > + } > + > + ret = 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 = 0; j < s->l2_size; j++) { > + uint64_t l2_entry = be64_to_cpu(l2_table[j]); > + uint64_t data_offset; > + > + if (qcow2_get_cluster_type(l2_entry) != QCOW2_CLUSTER_NORMAL) { > + continue; > + } You're removing the check for QCOW2_CLUSTER_ZERO, which was a fall-through case in the original code. > + > + data_offset = l2_entry & L2E_OFFSET_MASK; > + > + refcount = get_refcount(bs, data_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 == 1) != ((l2_entry & QCOW_OFLAG_COPIED) != 0)) { > + fprintf(stderr, "ERROR OFLAG_COPIED data cluster: l2_entry=%" > + PRIx64 " refcount=%d\n", > + l2_entry, refcount); > + res->corruptions++; > + } > + } > + } I think it's long enough that it deserves a separate function. > + > res->image_end_offset = (highest_cluster + 1) * s->cluster_size; > ret = 0; > > fail: > + qemu_vfree(l2_table); > g_free(refcount_table); > > return ret; Kevin