From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:32906) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XctuJ-0006un-0j for qemu-devel@nongnu.org; Sat, 11 Oct 2014 06:28:24 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XctuC-0007Ob-9Y for qemu-devel@nongnu.org; Sat, 11 Oct 2014 06:28:14 -0400 Received: from mx1.redhat.com ([209.132.183.28]:53586) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XctuC-0007OR-02 for qemu-devel@nongnu.org; Sat, 11 Oct 2014 06:28:08 -0400 Message-ID: <5439062C.8070904@redhat.com> Date: Sat, 11 Oct 2014 12:27:56 +0200 From: Max Reitz MIME-Version: 1.0 References: <1409348463-16627-1-git-send-email-mreitz@redhat.com> <1409348463-16627-9-git-send-email-mreitz@redhat.com> <20141010124439.GE22893@nodalink.com> In-Reply-To: <20141010124439.GE22893@nodalink.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v5 08/11] qcow2: Rebuild refcount structure during check List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?windows-1252?Q?Beno=EEt_Canet?= Cc: Kevin Wolf , qemu-devel@nongnu.org, Stefan Hajnoczi Am 10.10.2014 um 14:44 schrieb Beno=EEt Canet: >> + *nb_clusters =3D cluster + cluster_count - contiguous_free_cl= usters; >> + *refcount_table =3D g_try_realloc(*refcount_table, >> + *nb_clusters * sizeof(uint16_= t)); > Something tells me that these sizeof(uint16_t) are connected to s->refc= ount_order > and indirectely to REFCOUNT_SHIFT and that this code could benefit from= this > relationship thus probably saving you work in the future. Yes, see the answer to Eric. I'm totally in favor of variable refcounts,=20 see my "qcow2: Drop REFCOUNT_SHIFT series". ;-) Once again, thank you for your reviews! Max >> + if (!*refcount_table) { >> + return -ENOMEM; >> + } >> + >> + memset(*refcount_table + old_nb_clusters, 0, >> + (*nb_clusters - old_nb_clusters) * sizeof(uint16_t)); >> + } >> + >> + /* Go back to the first free cluster */ >> + cluster -=3D contiguous_free_clusters; >> + for (i =3D 0; i < cluster_count; i++) { >> + (*refcount_table)[cluster + i] =3D 1; >> + } >> + >> + return cluster << s->cluster_bits; >> +} >> + >> +/* >> + * Creates a new refcount structure based solely on the in-memory inf= ormation >> + * given through *refcount_table. All necessary allocations will be r= eflected >> + * in that array. >> + * >> + * On success, the old refcount structure is leaked (it will be cover= ed by the >> + * new refcount structure). >> + */ >> +static int rebuild_refcount_structure(BlockDriverState *bs, >> + BdrvCheckResult *res, >> + uint16_t **refcount_table, >> + int64_t *nb_clusters) >> +{ >> + BDRVQcowState *s =3D bs->opaque; >> + int64_t first_free_cluster =3D 0, rt_ofs =3D -1, cluster =3D 0; >> + int64_t rb_ofs, rb_start, rb_index; >> + uint32_t reftable_size =3D 0; >> + uint64_t *reftable =3D NULL; >> + uint16_t *on_disk_rb; >> + int i, ret =3D 0; >> + struct { >> + uint64_t rt_offset; >> + uint32_t rt_clusters; >> + } QEMU_PACKED rt_offset_and_clusters; >> + >> + qcow2_cache_empty(bs, s->refcount_block_cache); >> + >> +write_refblocks: >> + for (; cluster < *nb_clusters; cluster++) { >> + if (!(*refcount_table)[cluster]) { >> + continue; >> + } >> + >> + rb_index =3D cluster >> s->refcount_block_bits; >> + rb_start =3D rb_index << s->refcount_block_bits; >> + >> + /* Don't allocate a cluster in a refblock already written to = disk */ >> + if (first_free_cluster < rb_start) { >> + first_free_cluster =3D rb_start; >> + } >> + rb_ofs =3D alloc_clusters_imrt(bs, 1, refcount_table, nb_clus= ters, >> + &first_free_cluster); >> + if (rb_ofs < 0) { >> + fprintf(stderr, "ERROR allocating refblock: %s\n", strerr= or(-ret)); >> + res->check_errors++; >> + ret =3D rb_ofs; >> + goto fail; >> + } >> + >> + if (reftable_size <=3D rb_index) { >> + uint32_t old_rt_size =3D reftable_size; >> + reftable_size =3D ROUND_UP((rb_index + 1) * sizeof(uint64= _t), >> + s->cluster_size) / sizeof(uint64= _t); >> + reftable =3D g_try_realloc(reftable, >> + reftable_size * sizeof(uint64_t)= ); >> + if (!reftable) { >> + res->check_errors++; >> + ret =3D -ENOMEM; >> + goto fail; >> + } >> + >> + memset(reftable + old_rt_size, 0, >> + (reftable_size - old_rt_size) * sizeof(uint64_t)); >> + >> + /* The offset we have for the reftable is now no longer v= alid; >> + * this will leak that range, but we can easily fix that = by running >> + * a leak-fixing check after this rebuild operation */ >> + rt_ofs =3D -1; >> + } >> + reftable[rb_index] =3D rb_ofs; >> + >> + /* If this is apparently the last refblock (for now), try to = squeeze the >> + * reftable in */ >> + if (rb_index =3D=3D (*nb_clusters - 1) >> s->refcount_block_b= its && >> + rt_ofs < 0) >> + { >> + rt_ofs =3D alloc_clusters_imrt(bs, size_to_clusters(s, re= ftable_size * >> + sizeof(= uint64_t)), >> + refcount_table, nb_clusters, >> + &first_free_cluster); >> + if (rt_ofs < 0) { >> + fprintf(stderr, "ERROR allocating reftable: %s\n", >> + strerror(-ret)); >> + res->check_errors++; >> + ret =3D rt_ofs; >> + goto fail; >> + } >> + } >> + >> + ret =3D qcow2_pre_write_overlap_check(bs, 0, rb_ofs, s->clust= er_size); >> + if (ret < 0) { >> + fprintf(stderr, "ERROR writing refblock: %s\n", strerror(= -ret)); >> + goto fail; >> + } >> + >> + on_disk_rb =3D g_malloc0(s->cluster_size); >> + for (i =3D 0; i < s->cluster_size / sizeof(uint16_t) && >> + rb_start + i < *nb_clusters; i++) >> + { >> + on_disk_rb[i] =3D cpu_to_be16((*refcount_table)[rb_start = + i]); >> + } >> + >> + ret =3D bdrv_write(bs->file, rb_ofs / BDRV_SECTOR_SIZE, >> + (void *)on_disk_rb, s->cluster_sectors); >> + g_free(on_disk_rb); >> + if (ret < 0) { >> + fprintf(stderr, "ERROR writing refblock: %s\n", strerror(= -ret)); >> + goto fail; >> + } >> + >> + /* Go to the end of this refblock */ >> + cluster =3D rb_start + s->cluster_size / sizeof(uint16_t) - 1= ; >> + } >> + >> + if (rt_ofs < 0) { >> + int64_t post_rb_start =3D ROUND_UP(*nb_clusters, >> + s->cluster_size / sizeof(uin= t16_t)); >> + >> + /* Not pretty but simple */ >> + if (first_free_cluster < post_rb_start) { >> + first_free_cluster =3D post_rb_start; >> + } >> + rt_ofs =3D alloc_clusters_imrt(bs, size_to_clusters(s, reftab= le_size * >> + sizeof(uint= 64_t)), >> + refcount_table, nb_clusters, >> + &first_free_cluster); >> + if (rt_ofs < 0) { >> + fprintf(stderr, "ERROR allocating reftable: %s\n", strerr= or(-ret)); >> + res->check_errors++; >> + ret =3D rt_ofs; >> + goto fail; >> + } >> + >> + goto write_refblocks; >> + } >> + >> + assert(reftable); >> + >> + for (rb_index =3D 0; rb_index < reftable_size; rb_index++) { >> + cpu_to_be64s(&reftable[rb_index]); >> + } >> + >> + ret =3D qcow2_pre_write_overlap_check(bs, 0, rt_ofs, >> + reftable_size * sizeof(uint64= _t)); >> + if (ret < 0) { >> + fprintf(stderr, "ERROR writing reftable: %s\n", strerror(-ret= )); >> + goto fail; >> + } >> + >> + ret =3D bdrv_write(bs->file, rt_ofs / BDRV_SECTOR_SIZE, (void *)r= eftable, >> + reftable_size * sizeof(uint64_t) / BDRV_SECTOR_S= IZE); >> + if (ret < 0) { >> + fprintf(stderr, "ERROR writing reftable: %s\n", strerror(-ret= )); >> + goto fail; >> + } >> + >> + /* Enter new reftable into the image header */ >> + cpu_to_be64w(&rt_offset_and_clusters.rt_offset, rt_ofs); >> + cpu_to_be32w(&rt_offset_and_clusters.rt_clusters, >> + size_to_clusters(s, reftable_size * sizeof(uint64_t)= )); >> + ret =3D bdrv_pwrite_sync(bs->file, offsetof(QCowHeader, >> + refcount_table_offset), >> + &rt_offset_and_clusters, >> + sizeof(rt_offset_and_clusters)); >> + if (ret < 0) { >> + fprintf(stderr, "ERROR setting reftable: %s\n", strerror(-ret= )); >> + goto fail; >> + } >> + >> + for (rb_index =3D 0; rb_index < reftable_size; rb_index++) { >> + be64_to_cpus(&reftable[rb_index]); >> + } >> + s->refcount_table =3D reftable; >> + s->refcount_table_offset =3D rt_ofs; >> + s->refcount_table_size =3D reftable_size; >> + >> + return 0; >> + >> +fail: >> + g_free(reftable); >> + return ret; >> +} >> + >> +/* >> * Checks an image for refcount consistency. >> * >> * Returns 0 if no errors are found, the number of errors in case th= e image is >> @@ -1612,6 +1872,7 @@ int qcow2_check_refcounts(BlockDriverState *bs, = BdrvCheckResult *res, >> BdrvCheckMode fix) >> { >> BDRVQcowState *s =3D bs->opaque; >> + BdrvCheckResult pre_compare_res; >> int64_t size, highest_cluster, nb_clusters; >> uint16_t *refcount_table =3D NULL; >> bool rebuild =3D false; >> @@ -1638,11 +1899,30 @@ int qcow2_check_refcounts(BlockDriverState *bs= , BdrvCheckResult *res, >> goto fail; >> } >> =20 >> - compare_refcounts(bs, res, fix, &rebuild, &highest_cluster, refco= unt_table, >> + /* In case we don't need to rebuild the refcount structure (but w= ant to fix >> + * something), this function is immediately called again, in whic= h case the >> + * result should be ignored */ >> + pre_compare_res =3D *res; >> + compare_refcounts(bs, res, 0, &rebuild, &highest_cluster, refcoun= t_table, >> nb_clusters); >> =20 >> - if (rebuild) { >> - fprintf(stderr, "ERROR need to rebuild refcount structures\n"= ); >> + if (rebuild && (fix & BDRV_FIX_ERRORS)) { >> + fprintf(stderr, "Rebuilding refcount structure\n"); >> + ret =3D rebuild_refcount_structure(bs, res, &refcount_table, >> + &nb_clusters); >> + if (ret < 0) { >> + goto fail; >> + } >> + } else if (fix) { >> + if (rebuild) { >> + fprintf(stderr, "ERROR need to rebuild refcount structure= s\n"); >> + } >> + >> + if (res->leaks || res->corruptions) { >> + *res =3D pre_compare_res; >> + compare_refcounts(bs, res, fix, &rebuild, &highest_cluste= r, >> + refcount_table, nb_clusters); >> + } >> } >> =20 >> /* check OFLAG_COPIED */ >> --=20 >> 2.1.0 >>