| Message ID | 20230203091854.2221397-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 03.02.23 10:18, 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 | 81 ++++++++++++++++++++++++++++++----------------- > 1 file changed, 52 insertions(+), 29 deletions(-) > > diff --git a/block/parallels.c b/block/parallels.c > index 02fbaee1f2..f9acee1fa8 100644 > --- a/block/parallels.c > +++ b/block/parallels.c > @@ -438,14 +438,13 @@ static void parallels_check_unclean(BlockDriverState *bs, > } > } > > -static int coroutine_fn parallels_co_check(BlockDriverState *bs, > - BdrvCheckResult *res, > - BdrvCheckMode fix) > +static int parallels_check_outside_image(BlockDriverState *bs, > + BdrvCheckResult *res, > + BdrvCheckMode fix) I wonder, should we mark this function coroutine_fn? And with the graph lock changes that happened in the meantime, probably also GRAPH_RDLOCK (because it’s calling bdrv_getlength()). Hanna
On 3/7/23 13:17, Hanna Czenczek wrote: > On 03.02.23 10:18, 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 | 81 ++++++++++++++++++++++++++++++----------------- >> 1 file changed, 52 insertions(+), 29 deletions(-) >> >> diff --git a/block/parallels.c b/block/parallels.c >> index 02fbaee1f2..f9acee1fa8 100644 >> --- a/block/parallels.c >> +++ b/block/parallels.c >> @@ -438,14 +438,13 @@ static void >> parallels_check_unclean(BlockDriverState *bs, >> } >> } >> -static int coroutine_fn parallels_co_check(BlockDriverState *bs, >> - BdrvCheckResult *res, >> - BdrvCheckMode fix) >> +static int parallels_check_outside_image(BlockDriverState *bs, >> + BdrvCheckResult *res, >> + BdrvCheckMode fix) > > I wonder, should we mark this function coroutine_fn? And with the > graph lock changes that happened in the meantime, probably also > GRAPH_RDLOCK (because it’s calling bdrv_getlength()). > > Hanna > Thank you for your review. It seems, parallels_collect_statistics(), parallels_check_unclean() and parallels_set_bat_entry() also should be marked as coroutine_fn, shouldn't they?
On 10.03.23 12:08, Alexander Ivanov wrote: > > > On 3/7/23 13:17, Hanna Czenczek wrote: >> On 03.02.23 10:18, 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 | 81 >>> ++++++++++++++++++++++++++++++----------------- >>> 1 file changed, 52 insertions(+), 29 deletions(-) >>> >>> diff --git a/block/parallels.c b/block/parallels.c >>> index 02fbaee1f2..f9acee1fa8 100644 >>> --- a/block/parallels.c >>> +++ b/block/parallels.c >>> @@ -438,14 +438,13 @@ static void >>> parallels_check_unclean(BlockDriverState *bs, >>> } >>> } >>> -static int coroutine_fn parallels_co_check(BlockDriverState *bs, >>> - BdrvCheckResult *res, >>> - BdrvCheckMode fix) >>> +static int parallels_check_outside_image(BlockDriverState *bs, >>> + BdrvCheckResult *res, >>> + BdrvCheckMode fix) >> >> I wonder, should we mark this function coroutine_fn? And with the >> graph lock changes that happened in the meantime, probably also >> GRAPH_RDLOCK (because it’s calling bdrv_getlength()). >> >> Hanna >> > Thank you for your review. > > It seems, parallels_collect_statistics(), parallels_check_unclean() > and parallels_set_bat_entry() also should be marked as coroutine_fn, > shouldn't they? There should be no harm in doing so. I wasn’t mentioning it just because those functions don’t call other coroutine functions themselves, so I think it doesn’t really matter. Hanna
diff --git a/block/parallels.c b/block/parallels.c index 02fbaee1f2..f9acee1fa8 100644 --- a/block/parallels.c +++ b/block/parallels.c @@ -438,14 +438,13 @@ static void parallels_check_unclean(BlockDriverState *bs, } } -static int coroutine_fn parallels_co_check(BlockDriverState *bs, - BdrvCheckResult *res, - BdrvCheckMode fix) +static int parallels_check_outside_image(BlockDriverState *bs, + BdrvCheckResult *res, + BdrvCheckMode fix) { BDRVParallelsState *s = bs->opaque; - int64_t size, prev_off, high_off; - int ret = 0; uint32_t i; + int64_t off, high_off, size; size = bdrv_getlength(bs->file->bs); if (size < 0) { @@ -453,23 +452,9 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs, return size; } - qemu_co_mutex_lock(&s->lock); - - parallels_check_unclean(bs, res, fix); - - res->bfi.total_clusters = s->bat_size; - res->bfi.compressed_clusters = 0; /* compression is not supported */ - high_off = 0; - prev_off = 0; for (i = 0; i < s->bat_size; i++) { - int64_t off = bat2sect(s, i) << BDRV_SECTOR_BITS; - if (off == 0) { - prev_off = 0; - continue; - } - - /* cluster outside the image */ + 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); @@ -478,19 +463,11 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs, parallels_set_bat_entry(s, i, 0); res->corruptions_fixed++; } - prev_off = 0; continue; } - - res->bfi.allocated_clusters++; - if (off > high_off) { + if (high_off < off) { high_off = off; } - - if (prev_off != 0 && (prev_off + s->cluster_size) != off) { - res->bfi.fragmented_clusters++; - } - prev_off = off; } if (high_off == 0) { @@ -500,6 +477,52 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs, s->data_end = res->image_end_offset >> 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; + int ret; + uint32_t i; + + size = bdrv_getlength(bs->file->bs); + if (size < 0) { + res->check_errors++; + return size; + } + + qemu_co_mutex_lock(&s->lock); + + 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 */ + + prev_off = 0; + for (i = 0; i < s->bat_size; i++) { + int64_t off = bat2sect(s, i) << BDRV_SECTOR_BITS; + if (off == 0) { + prev_off = 0; + continue; + } + + res->bfi.allocated_clusters++; + + if (prev_off != 0 && (prev_off + s->cluster_size) != off) { + res->bfi.fragmented_clusters++; + } + prev_off = off; + } + if (size > res->image_end_offset) { int64_t count; count = DIV_ROUND_UP(size - res->image_end_offset, s->cluster_size);