| Message ID | 20230424094309.197969-4-alexander.ivanov@virtuozzo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | parallels: Add duplication check, repair at open, fix bugs | expand |
On Mon, Apr 24, 2023 at 12:44 PM Alexander Ivanov <alexander.ivanov@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.ivanov@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; > +} > + I guess there should be: off -= le32_to_cpu(s->header->data_off) << BDRV_SECTOR_BITS Regards, Mike.
Good point. Thank you. Best regards, Alexander Ivanov On 4/26/23 23:56, Mike Maslenkin wrote: > On Mon, Apr 24, 2023 at 12:44 PM Alexander Ivanov > <alexander.ivanov@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.ivanov@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; >> +} >> + > I guess there should be: > off -= le32_to_cpu(s->header->data_off) << BDRV_SECTOR_BITS > > Regards, > Mike.
Sorry for the noise again , but I have another note On Mon, Apr 24, 2023 at 12:44 PM Alexander Ivanov <alexander.ivanov@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.ivanov@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.
On 4/27/23 15:46, Mike Maslenkin wrote: > Sorry for the noise again , but I have another note No, you don't need to apologize. You help me make code better. > > On Mon, Apr 24, 2023 at 12:44 PM Alexander Ivanov > <alexander.ivanov@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.ivanov@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. s->cluster_size is the size of cluster in bytes. We shift this value to BDRV_SECTOR_BITS and have the number of sectors. Pay attention that in allocate_clusters() we need not an offset in the image file, but an offset inside image. As for truncation, you are right, will add a cast to uint64_t.
There is another issue with host_cluster_index() function.
After this patchset applied `qemu-img check -f parallels some_disk`
aborts for empty (just created) disk image.
The problem is that host_cluster_index() returns 0 and then
bitmap_new(0) rises an abort.
For default empty disk s->header->data_off=2048, and
res->image_end_offset = 1048576, so:
static uint32_t host_cluster_index(BDRVParallelsState *s, int64_t 1048576)
{
off = 1048576 - le32_to_cpu(2048) << 9;
return 0 / 1048576;
}
Could you check this case?
Regards,
Mike.
On Thu, Apr 27, 2023 at 3:29 PM Alexander Ivanov
<alexander.ivanov@virtuozzo.com> wrote:
>
> Good point. Thank you.
>
> Best regards,
> Alexander Ivanov
>
> On 4/26/23 23:56, Mike Maslenkin wrote:
> > On Mon, Apr 24, 2023 at 12:44 PM Alexander Ivanov
> > <alexander.ivanov@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.ivanov@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;
> >> +}
> >> +
> > I guess there should be:
> > off -= le32_to_cpu(s->header->data_off) << BDRV_SECTOR_BITS
> >
> > Regards,
> > Mike.
>
Yes, there is a bug. Thank you. On 4/29/23 00:15, Mike Maslenkin wrote: > There is another issue with host_cluster_index() function. > After this patchset applied `qemu-img check -f parallels some_disk` > aborts for empty (just created) disk image. > The problem is that host_cluster_index() returns 0 and then > bitmap_new(0) rises an abort. > > For default empty disk s->header->data_off=2048, and > res->image_end_offset = 1048576, so: > static uint32_t host_cluster_index(BDRVParallelsState *s, int64_t 1048576) > { > off = 1048576 - le32_to_cpu(2048) << 9; > return 0 / 1048576; > } > > Could you check this case? > > Regards, > Mike. > > On Thu, Apr 27, 2023 at 3:29 PM Alexander Ivanov > <alexander.ivanov@virtuozzo.com> wrote: >> Good point. Thank you. >> >> Best regards, >> Alexander Ivanov >> >> On 4/26/23 23:56, Mike Maslenkin wrote: >>> On Mon, Apr 24, 2023 at 12:44 PM Alexander Ivanov >>> <alexander.ivanov@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.ivanov@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; >>>> +} >>>> + >>> I guess there should be: >>> off -= le32_to_cpu(s->header->data_off) << BDRV_SECTOR_BITS >>> >>> Regards, >>> Mike.
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; + } + off = sector << BDRV_SECTOR_BITS; + + ret = bdrv_co_pwritev(bs->file, off, s->cluster_size, &qiov, 0); + if (ret < 0) { + res->check_errors++; + goto out; + } + + if (off + s->cluster_size > res->image_end_offset) { + res->image_end_offset = off + s->cluster_size; + } + + /* + * In the future allocate_cluster() will reuse holed offsets + * inside the image. Keep the used clusters bitmap content + * consistent for the new allocated clusters too. + * + * Note, clusters allocated outside the current image are not + * considered, and the bitmap size doesn't change. + */ + cluster_index = host_cluster_index(s, off); + if (cluster_index < bitmap_size) { + bitmap_set(bitmap, cluster_index, 1); + } + + /* + * When new clusters are allocated, file size increases by + * 128 Mb blocks. We need to truncate the file to the right + * size. Let the leak fix code make its job. + */ + *fix |= BDRV_FIX_LEAKS; + res->corruptions_fixed++; + } + } else { + bitmap_set(bitmap, cluster_index, 1); + } + } + +out: + qemu_iovec_destroy(&qiov); + g_free(buf); + g_free(bitmap); + return ret; +} + static void parallels_collect_statistics(BlockDriverState *bs, BdrvCheckResult *res, BdrvCheckMode fix) @@ -611,7 +723,19 @@ parallels_co_check(BlockDriverState *bs, BdrvCheckResult *res, return ret; } + ret = parallels_check_duplicate(bs, res, &fix); + if (ret < 0) { + return ret; + } + parallels_collect_statistics(bs, res, fix); + + if (fix & BDRV_FIX_LEAKS) { + ret = parallels_fix_leak(bs, res); + if (ret < 0) { + return ret; + } + } } ret = bdrv_co_flush(bs);
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.ivanov@virtuozzo.com> --- block/parallels.c | 134 ++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 129 insertions(+), 5 deletions(-)