Sorry for the noise again , but I have another note On Mon, Apr 24, 2023 at 12:44 PM Alexander Ivanov <alexander.iva...@virtuozzo.com> wrote: > > Cluster offsets must be unique among all the BAT entries. Find duplicate > offsets in the BAT and fix it by copying the content of the relevant > cluster to a newly allocated cluster and set the new cluster offset to the > duplicated entry. > > Add host_cluster_index() helper to deduplicate the code. > > Move parallels_fix_leak() call to parallels_co_check() to fix both types > of leak: real corruption and a leak produced by allocate_clusters() > during deduplication. > > Signed-off-by: Alexander Ivanov <alexander.iva...@virtuozzo.com> > --- > block/parallels.c | 134 ++++++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 129 insertions(+), 5 deletions(-) > > diff --git a/block/parallels.c b/block/parallels.c > index ec89ed894b..3b992e8173 100644 > --- a/block/parallels.c > +++ b/block/parallels.c > @@ -136,6 +136,12 @@ static int cluster_remainder(BDRVParallelsState *s, > int64_t sector_num, > return MIN(nb_sectors, ret); > } > > +static uint32_t host_cluster_index(BDRVParallelsState *s, int64_t off) > +{ > + off -= s->header->data_off << BDRV_SECTOR_BITS; > + return off / s->cluster_size; > +} > + > static int64_t block_status(BDRVParallelsState *s, int64_t sector_num, > int nb_sectors, int *pnum) > { > @@ -533,7 +539,6 @@ parallels_check_leak(BlockDriverState *bs, > BdrvCheckResult *res, > { > BDRVParallelsState *s = bs->opaque; > int64_t count, leak_size; > - int ret; > > leak_size = parallels_get_leak_size(bs, res); > if (leak_size < 0) { > @@ -550,16 +555,123 @@ parallels_check_leak(BlockDriverState *bs, > BdrvCheckResult *res, > fix & BDRV_FIX_LEAKS ? "Repairing" : "ERROR", leak_size); > > if (fix & BDRV_FIX_LEAKS) { > - ret = parallels_fix_leak(bs, res); > - if (ret < 0) { > - return ret; > - } > res->leaks_fixed += count; > } > > return 0; > } > > +static int parallels_check_duplicate(BlockDriverState *bs, > + BdrvCheckResult *res, > + BdrvCheckMode *fix) > +{ > + BDRVParallelsState *s = bs->opaque; > + QEMUIOVector qiov; > + int64_t off, sector; > + unsigned long *bitmap; > + uint32_t i, bitmap_size, cluster_index; > + int n, ret = 0; > + uint64_t *buf = NULL; > + > + /* > + * Create a bitmap of used clusters. > + * If a bit is set, there is a BAT entry pointing to this cluster. > + * Loop through the BAT entries, check bits relevant to an entry offset. > + * If bit is set, this entry is duplicated. Otherwise set the bit. > + * > + * We shouldn't worry about newly allocated clusters outside the image > + * because they are created higher then any existing cluster pointed by > + * a BAT entry. > + */ > + bitmap_size = host_cluster_index(s, res->image_end_offset); > + bitmap = bitmap_new(bitmap_size); > + > + buf = qemu_memalign(4096, s->cluster_size); > + qemu_iovec_init(&qiov, 0); > + qemu_iovec_add(&qiov, buf, s->cluster_size); > + > + for (i = 0; i < s->bat_size; i++) { > + off = bat2sect(s, i) << BDRV_SECTOR_BITS; > + if (off == 0) { > + continue; > + } > + > + cluster_index = host_cluster_index(s, off); > + if (test_bit(cluster_index, bitmap)) { > + /* this cluster duplicates another one */ > + fprintf(stderr, > + "%s duplicate offset in BAT entry %u\n", > + *fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR", i); > + > + res->corruptions++; > + > + if (*fix & BDRV_FIX_ERRORS) { > + /* > + * Reset the entry and allocate a new cluster > + * for the relevant guest offset. In this way we let > + * the lower layer to place the new cluster properly. > + * Copy the original cluster to the allocated one. > + */ > + parallels_set_bat_entry(s, i, 0); > + > + ret = bdrv_co_pread(bs->file, off, s->cluster_size, buf, 0); > + if (ret < 0) { > + res->check_errors++; > + goto out; > + } > + > + sector = (i * s->cluster_size) >> BDRV_SECTOR_BITS; > + sector = allocate_clusters(bs, sector, s->tracks, &n); > + if (sector < 0) { > + res->check_errors++; > + ret = sector; > + goto out; > + }
I can not understand how index in a BAT table related to s->cluster_size. Probably there should be "cluster_index" used? Anyway, looks like both cause uint32 truncation as result of ({i,cluster_index} * s->cluster_size) Regards, Mike.