| Message ID | 20230529151503.34006-6-alexander.ivanov@virtuozzo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | parallels: Add duplication check, repair at open, fix bugs | expand |
On 29.05.23 17:15, Alexander Ivanov wrote: > Repair an image at opening if the image is unclean or out-of-image > corruption was detected. > > Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com> > --- > block/parallels.c | 65 +++++++++++++++++++++++++---------------------- > 1 file changed, 35 insertions(+), 30 deletions(-) > > diff --git a/block/parallels.c b/block/parallels.c > index d64e8007d5..7bbd5cb112 100644 > --- a/block/parallels.c > +++ b/block/parallels.c [...] > @@ -1130,6 +1101,40 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags, > goto fail; > } > qemu_co_mutex_init(&s->lock); > + > + if (le32_to_cpu(ph.inuse) == HEADER_INUSE_MAGIC) { > + s->header_unclean = true; > + } > + > + for (i = 0; i < s->bat_size; i++) { > + sector = bat2sect(s, i); > + if (sector + s->tracks > s->data_end) { > + s->data_end = sector + s->tracks; > + } > + } > + > + /* > + * We don't repair the image here if it's opened for checks. Also we don't > + * want to change inactive images and can't change readonly images. > + */ > + if ((flags & (BDRV_O_CHECK | BDRV_O_INACTIVE)) || !(flags & BDRV_O_RDWR)) { > + return 0; > + } > + > + /* > + * Repair the image if it's dirty or > + * out-of-image corruption was detected. > + */ > + if (s->data_end > file_nb_sectors || s->header_unclean) { > + BdrvCheckResult res; > + ret = bdrv_check(bs, &res, BDRV_FIX_ERRORS | BDRV_FIX_LEAKS); > + if (ret < 0) { Should we also verify that res->corruptions == res->corruptions_fixed && res->check_errors == 0? > + error_free(s->migration_blocker); I’d move this clean-up to a new error path below, then we could even reuse that where migrate_add_blocker() fails. Anyway, not wrong as-is, just suggestion, so: Reviewed-by: Hanna Czenczek <hreitz@redhat.com> > + error_setg_errno(errp, -ret, "Could not repair corrupted image"); > + goto fail; > + } > + } > + > return 0; > > fail_format:
On 6/2/23 16:59, Hanna Czenczek wrote: > On 29.05.23 17:15, Alexander Ivanov wrote: >> Repair an image at opening if the image is unclean or out-of-image >> corruption was detected. >> >> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com> >> --- >> block/parallels.c | 65 +++++++++++++++++++++++++---------------------- >> 1 file changed, 35 insertions(+), 30 deletions(-) >> >> diff --git a/block/parallels.c b/block/parallels.c >> index d64e8007d5..7bbd5cb112 100644 >> --- a/block/parallels.c >> +++ b/block/parallels.c > > [...] > >> @@ -1130,6 +1101,40 @@ static int parallels_open(BlockDriverState >> *bs, QDict *options, int flags, >> goto fail; >> } >> qemu_co_mutex_init(&s->lock); >> + >> + if (le32_to_cpu(ph.inuse) == HEADER_INUSE_MAGIC) { >> + s->header_unclean = true; >> + } >> + >> + for (i = 0; i < s->bat_size; i++) { >> + sector = bat2sect(s, i); >> + if (sector + s->tracks > s->data_end) { >> + s->data_end = sector + s->tracks; >> + } >> + } >> + >> + /* >> + * We don't repair the image here if it's opened for checks. >> Also we don't >> + * want to change inactive images and can't change readonly images. >> + */ >> + if ((flags & (BDRV_O_CHECK | BDRV_O_INACTIVE)) || !(flags & >> BDRV_O_RDWR)) { >> + return 0; >> + } >> + >> + /* >> + * Repair the image if it's dirty or >> + * out-of-image corruption was detected. >> + */ >> + if (s->data_end > file_nb_sectors || s->header_unclean) { >> + BdrvCheckResult res; >> + ret = bdrv_check(bs, &res, BDRV_FIX_ERRORS | BDRV_FIX_LEAKS); >> + if (ret < 0) { > > Should we also verify that res->corruptions == res->corruptions_fixed > && res->check_errors == 0? If ret == 0 there must be res->check_errors == 0 and res->corruptions == res->corruptions_fixed. > >> + error_free(s->migration_blocker); > > I’d move this clean-up to a new error path below, then we could even > reuse that where migrate_add_blocker() fails. Is this guaranteed that s->migration_blocker is NULL at the function parallels_open() beginning? If so it could be easy to move the clean-up, otherwise it could lead to code complication. > > Anyway, not wrong as-is, just suggestion, so: > > Reviewed-by: Hanna Czenczek <hreitz@redhat.com> > >> + error_setg_errno(errp, -ret, "Could not repair corrupted >> image"); >> + goto fail; >> + } >> + } >> + >> return 0; >> fail_format: >
On 09.06.23 15:21, Alexander Ivanov wrote: > > > On 6/2/23 16:59, Hanna Czenczek wrote: >> On 29.05.23 17:15, Alexander Ivanov wrote: >>> Repair an image at opening if the image is unclean or out-of-image >>> corruption was detected. >>> >>> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com> >>> --- >>> block/parallels.c | 65 >>> +++++++++++++++++++++++++---------------------- >>> 1 file changed, 35 insertions(+), 30 deletions(-) >>> >>> diff --git a/block/parallels.c b/block/parallels.c >>> index d64e8007d5..7bbd5cb112 100644 >>> --- a/block/parallels.c >>> +++ b/block/parallels.c >> >> [...] >> >>> @@ -1130,6 +1101,40 @@ static int parallels_open(BlockDriverState >>> *bs, QDict *options, int flags, >>> goto fail; >>> } >>> qemu_co_mutex_init(&s->lock); >>> + >>> + if (le32_to_cpu(ph.inuse) == HEADER_INUSE_MAGIC) { >>> + s->header_unclean = true; >>> + } >>> + >>> + for (i = 0; i < s->bat_size; i++) { >>> + sector = bat2sect(s, i); >>> + if (sector + s->tracks > s->data_end) { >>> + s->data_end = sector + s->tracks; >>> + } >>> + } >>> + >>> + /* >>> + * We don't repair the image here if it's opened for checks. >>> Also we don't >>> + * want to change inactive images and can't change readonly >>> images. >>> + */ >>> + if ((flags & (BDRV_O_CHECK | BDRV_O_INACTIVE)) || !(flags & >>> BDRV_O_RDWR)) { >>> + return 0; >>> + } >>> + >>> + /* >>> + * Repair the image if it's dirty or >>> + * out-of-image corruption was detected. >>> + */ >>> + if (s->data_end > file_nb_sectors || s->header_unclean) { >>> + BdrvCheckResult res; >>> + ret = bdrv_check(bs, &res, BDRV_FIX_ERRORS | BDRV_FIX_LEAKS); >>> + if (ret < 0) { >> >> Should we also verify that res->corruptions == res->corruptions_fixed >> && res->check_errors == 0? > If ret == 0 there must be res->check_errors == 0 and res->corruptions > == res->corruptions_fixed. OK. >> >>> + error_free(s->migration_blocker); >> >> I’d move this clean-up to a new error path below, then we could even >> reuse that where migrate_add_blocker() fails. > Is this guaranteed that s->migration_blocker is NULL at the function > parallels_open() beginning? If so it could be easy to move the clean-up, > otherwise it could lead to code complication. Three answers here: First, I just realized that we probably need to undo the migrate_add_blocker() call, too, i.e. call migrate_del_blocker() here. Second, I’m pretty sure that s->migration_blocker must be NULL before the error_setg(&s->migration_blocker) call, because error_setg() asserts that the *errp passed to it is NULL. Third, I meant to add a new path e.g.: ``` fail_blocker: error_free(s->migration_blocker); fail_format: [...] ``` And then use `goto fail_blocker;` here and in the migrate_add_blocker() error path, so it shouldn’t really matter whether s->migration_blocker is NULL before the error_setg() call. But then again, I think the probably necessary migrate_del_blocker() call complicates things further. Hanna >> >> Anyway, not wrong as-is, just suggestion, so: >> >> Reviewed-by: Hanna Czenczek <hreitz@redhat.com> >> >>> + error_setg_errno(errp, -ret, "Could not repair >>> corrupted image"); >>> + goto fail; >>> + } >>> + } >>> + >>> return 0; >>> fail_format: >> >
On 6/9/23 15:41, Hanna Czenczek wrote: > On 09.06.23 15:21, Alexander Ivanov wrote: >> >> >> On 6/2/23 16:59, Hanna Czenczek wrote: >>> On 29.05.23 17:15, Alexander Ivanov wrote: >>>> Repair an image at opening if the image is unclean or out-of-image >>>> corruption was detected. >>>> >>>> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com> >>>> --- >>>> block/parallels.c | 65 >>>> +++++++++++++++++++++++++---------------------- >>>> 1 file changed, 35 insertions(+), 30 deletions(-) >>>> >>>> diff --git a/block/parallels.c b/block/parallels.c >>>> index d64e8007d5..7bbd5cb112 100644 >>>> --- a/block/parallels.c >>>> +++ b/block/parallels.c >>> >>> [...] >>> >>>> @@ -1130,6 +1101,40 @@ static int parallels_open(BlockDriverState >>>> *bs, QDict *options, int flags, >>>> goto fail; >>>> } >>>> qemu_co_mutex_init(&s->lock); >>>> + >>>> + if (le32_to_cpu(ph.inuse) == HEADER_INUSE_MAGIC) { >>>> + s->header_unclean = true; >>>> + } >>>> + >>>> + for (i = 0; i < s->bat_size; i++) { >>>> + sector = bat2sect(s, i); >>>> + if (sector + s->tracks > s->data_end) { >>>> + s->data_end = sector + s->tracks; >>>> + } >>>> + } >>>> + >>>> + /* >>>> + * We don't repair the image here if it's opened for checks. >>>> Also we don't >>>> + * want to change inactive images and can't change readonly >>>> images. >>>> + */ >>>> + if ((flags & (BDRV_O_CHECK | BDRV_O_INACTIVE)) || !(flags & >>>> BDRV_O_RDWR)) { >>>> + return 0; >>>> + } >>>> + >>>> + /* >>>> + * Repair the image if it's dirty or >>>> + * out-of-image corruption was detected. >>>> + */ >>>> + if (s->data_end > file_nb_sectors || s->header_unclean) { >>>> + BdrvCheckResult res; >>>> + ret = bdrv_check(bs, &res, BDRV_FIX_ERRORS | BDRV_FIX_LEAKS); >>>> + if (ret < 0) { >>> >>> Should we also verify that res->corruptions == >>> res->corruptions_fixed && res->check_errors == 0? >> If ret == 0 there must be res->check_errors == 0 and res->corruptions >> == res->corruptions_fixed. > > OK. > >>> >>>> + error_free(s->migration_blocker); >>> >>> I’d move this clean-up to a new error path below, then we could even >>> reuse that where migrate_add_blocker() fails. >> Is this guaranteed that s->migration_blocker is NULL at the function >> parallels_open() beginning? If so it could be easy to move the clean-up, >> otherwise it could lead to code complication. > > Three answers here: > > First, I just realized that we probably need to undo the > migrate_add_blocker() call, too, i.e. call migrate_del_blocker() here. > > Second, I’m pretty sure that s->migration_blocker must be NULL before > the error_setg(&s->migration_blocker) call, because error_setg() > asserts that the *errp passed to it is NULL. > > Third, I meant to add a new path e.g.: > > ``` > fail_blocker: > error_free(s->migration_blocker); > fail_format: > [...] > ``` > > And then use `goto fail_blocker;` here and in the > migrate_add_blocker() error path, so it shouldn’t really matter > whether s->migration_blocker is NULL before the error_setg() call. > But then again, I think the probably necessary migrate_del_blocker() > call complicates things further. > > Hanna Do we need to run the rest part of the parallels_close() code? if ((bs->open_flags & BDRV_O_RDWR) && !(bs->open_flags & BDRV_O_INACTIVE)) { s->header->inuse = 0; parallels_update_header(bs); /* errors are ignored, so we might as well pass exact=true */ bdrv_truncate(bs->file, s->data_end << BDRV_SECTOR_BITS, true, PREALLOC_MODE_OFF, 0, NULL); } g_free(s->bat_dirty_bmap); If so, maybe it would be better to call parallels_close()? >>> >>> Anyway, not wrong as-is, just suggestion, so: >>> >>> Reviewed-by: Hanna Czenczek <hreitz@redhat.com> >>> >>>> + error_setg_errno(errp, -ret, "Could not repair >>>> corrupted image"); >>>> + goto fail; >>>> + } >>>> + } >>>> + >>>> return 0; >>>> fail_format: >>> >> >
diff --git a/block/parallels.c b/block/parallels.c index d64e8007d5..7bbd5cb112 100644 --- a/block/parallels.c +++ b/block/parallels.c @@ -962,7 +962,7 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags, BDRVParallelsState *s = bs->opaque; ParallelsHeader ph; int ret, size, i; - int64_t file_nb_sectors; + int64_t file_nb_sectors, sector; QemuOpts *opts = NULL; Error *local_err = NULL; char *buf; @@ -1039,35 +1039,6 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags, } s->bat_bitmap = (uint32_t *)(s->header + 1); - for (i = 0; i < s->bat_size; i++) { - int64_t off = bat2sect(s, i); - if (off >= file_nb_sectors) { - if (flags & BDRV_O_CHECK) { - continue; - } - error_setg(errp, "parallels: Offset %" PRIi64 " in BAT[%d] entry " - "is larger than file size (%" PRIi64 ")", - off << BDRV_SECTOR_BITS, i, - file_nb_sectors << BDRV_SECTOR_BITS); - ret = -EINVAL; - goto fail; - } - if (off >= s->data_end) { - s->data_end = off + s->tracks; - } - } - - if (le32_to_cpu(ph.inuse) == HEADER_INUSE_MAGIC) { - /* Image was not closed correctly. The check is mandatory */ - s->header_unclean = true; - if ((flags & BDRV_O_RDWR) && !(flags & BDRV_O_CHECK)) { - error_setg(errp, "parallels: Image was not closed correctly; " - "cannot be opened read/write"); - ret = -EACCES; - goto fail; - } - } - opts = qemu_opts_create(¶llels_runtime_opts, NULL, 0, errp); if (!opts) { goto fail_options; @@ -1130,6 +1101,40 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags, goto fail; } qemu_co_mutex_init(&s->lock); + + if (le32_to_cpu(ph.inuse) == HEADER_INUSE_MAGIC) { + s->header_unclean = true; + } + + for (i = 0; i < s->bat_size; i++) { + sector = bat2sect(s, i); + if (sector + s->tracks > s->data_end) { + s->data_end = sector + s->tracks; + } + } + + /* + * We don't repair the image here if it's opened for checks. Also we don't + * want to change inactive images and can't change readonly images. + */ + if ((flags & (BDRV_O_CHECK | BDRV_O_INACTIVE)) || !(flags & BDRV_O_RDWR)) { + return 0; + } + + /* + * Repair the image if it's dirty or + * out-of-image corruption was detected. + */ + if (s->data_end > file_nb_sectors || s->header_unclean) { + BdrvCheckResult res; + ret = bdrv_check(bs, &res, BDRV_FIX_ERRORS | BDRV_FIX_LEAKS); + if (ret < 0) { + error_free(s->migration_blocker); + error_setg_errno(errp, -ret, "Could not repair corrupted image"); + goto fail; + } + } + return 0; fail_format:
Repair an image at opening if the image is unclean or out-of-image corruption was detected. Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com> --- block/parallels.c | 65 +++++++++++++++++++++++++---------------------- 1 file changed, 35 insertions(+), 30 deletions(-)