| Message ID | 20230115155821.1534598-8-alexander.ivanov@virtuozzo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | parallels: Refactor the code of images checks and fix a bug | expand |
On 15.01.23 16:58, Alexander Ivanov wrote: > We will add more and more checks so we need a better code structure > in parallels_co_check. Let each check performs in a separate loop > in a separate helper. > > Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com> > Reviewed-by: Denis V. Lunev <den@openvz.org> > --- > block/parallels.c | 59 ++++++++++++++++++++++++++++++++++------------- > 1 file changed, 43 insertions(+), 16 deletions(-) > > diff --git a/block/parallels.c b/block/parallels.c > index d48b447cca..3d06623355 100644 > --- a/block/parallels.c > +++ b/block/parallels.c [...] > @@ -469,19 +511,6 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs, > continue; > } > > - /* cluster outside the image */ > - if (off > size) { > - fprintf(stderr, "%s cluster %u is outside image\n", > - fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR", i); > - res->corruptions++; > - if (fix & BDRV_FIX_ERRORS) { > - parallels_set_bat_entry(s, i, 0); > - res->corruptions_fixed++; > - } > - prev_off = 0; > - continue; > - } > - > res->bfi.allocated_clusters++; > if (off > high_off) { > high_off = off; parallels_co_check() keeps the `high_off` variable, and now it is also bumped for clusters that are outside the image. This seems to go against patch 2’s intentions. Consider an image whose file length is larger than all of its clusters need (i.e. there’s leaked space), except for one cluster, which is beyond the EOF. This one cluster is considered an error (because it’s outside the image). Before this patch, we would have truncated the image’s file length to match all the other non-error clusters (and drop the leaked space). With this patch, the error cluster, if it wasn’t fixed by parallels_check_outside_image(), the image’s file length is no longer truncated. Basically, this seems to restore the behavior from before patch 2. Was this intentional? Hanna
On 18.01.2023 15:45, Hanna Czenczek wrote: > On 15.01.23 16:58, Alexander Ivanov wrote: >> We will add more and more checks so we need a better code structure >> in parallels_co_check. Let each check performs in a separate loop >> in a separate helper. >> >> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com> >> Reviewed-by: Denis V. Lunev <den@openvz.org> >> --- >> block/parallels.c | 59 ++++++++++++++++++++++++++++++++++------------- >> 1 file changed, 43 insertions(+), 16 deletions(-) >> >> diff --git a/block/parallels.c b/block/parallels.c >> index d48b447cca..3d06623355 100644 >> --- a/block/parallels.c >> +++ b/block/parallels.c > > [...] > >> @@ -469,19 +511,6 @@ static int coroutine_fn >> parallels_co_check(BlockDriverState *bs, >> continue; >> } >> - /* cluster outside the image */ >> - if (off > size) { >> - fprintf(stderr, "%s cluster %u is outside image\n", >> - fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR", i); >> - res->corruptions++; >> - if (fix & BDRV_FIX_ERRORS) { >> - parallels_set_bat_entry(s, i, 0); >> - res->corruptions_fixed++; >> - } >> - prev_off = 0; >> - continue; >> - } >> - >> res->bfi.allocated_clusters++; >> if (off > high_off) { >> high_off = off; > > parallels_co_check() keeps the `high_off` variable, and now it is also > bumped for clusters that are outside the image. This seems to go > against patch 2’s intentions. > > Consider an image whose file length is larger than all of its clusters > need (i.e. there’s leaked space), except for one cluster, which is > beyond the EOF. This one cluster is considered an error (because it’s > outside the image). Before this patch, we would have truncated the > image’s file length to match all the other non-error clusters (and > drop the leaked space). With this patch, the error cluster, if it > wasn’t fixed by parallels_check_outside_image(), the image’s file > length is no longer truncated. Basically, this seems to restore the > behavior from before patch 2. > > Was this intentional? > > Hanna > Good point! I have missed the case with !BDRV_FIX_ERRORS. Thank you!
diff --git a/block/parallels.c b/block/parallels.c index d48b447cca..3d06623355 100644 --- a/block/parallels.c +++ b/block/parallels.c @@ -438,13 +438,50 @@ static void parallels_check_unclean(BlockDriverState *bs, } } +static int parallels_check_outside_image(BlockDriverState *bs, + BdrvCheckResult *res, + BdrvCheckMode fix) +{ + BDRVParallelsState *s = bs->opaque; + uint32_t i; + int64_t off, high_off, size; + + size = bdrv_getlength(bs->file->bs); + if (size < 0) { + res->check_errors++; + return size; + } + + high_off = 0; + for (i = 0; i < s->bat_size; i++) { + off = bat2sect(s, i) << BDRV_SECTOR_BITS; + if (off > size) { + fprintf(stderr, "%s cluster %u is outside image\n", + fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR", i); + res->corruptions++; + if (fix & BDRV_FIX_ERRORS) { + parallels_set_bat_entry(s, i, 0); + res->corruptions_fixed++; + } + continue; + } + if (high_off < off) { + high_off = off; + } + } + + s->data_end = (high_off + s->cluster_size) >> BDRV_SECTOR_BITS; + + return 0; +} + static int coroutine_fn parallels_co_check(BlockDriverState *bs, BdrvCheckResult *res, BdrvCheckMode fix) { BDRVParallelsState *s = bs->opaque; int64_t size, prev_off, high_off; - int ret = 0; + int ret; uint32_t i; size = bdrv_getlength(bs->file->bs); @@ -457,6 +494,11 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs, parallels_check_unclean(bs, res, fix); + ret = parallels_check_outside_image(bs, res, fix); + if (ret < 0) { + goto out; + } + res->bfi.total_clusters = s->bat_size; res->bfi.compressed_clusters = 0; /* compression is not supported */ @@ -469,19 +511,6 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs, continue; } - /* cluster outside the image */ - if (off > size) { - fprintf(stderr, "%s cluster %u is outside image\n", - fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR", i); - res->corruptions++; - if (fix & BDRV_FIX_ERRORS) { - parallels_set_bat_entry(s, i, 0); - res->corruptions_fixed++; - } - prev_off = 0; - continue; - } - res->bfi.allocated_clusters++; if (off > high_off) { high_off = off; @@ -519,8 +548,6 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs, } } - s->data_end = res->image_end_offset >> BDRV_SECTOR_BITS; - out: qemu_co_mutex_unlock(&s->lock);