Re: [PATCH 1/3] qemu-img check: fixing duplicated clusters for parallels format

[Denis V. Lunev]

On 18.04.2022 14:04, Natalia Kuzmina wrote:
> Let qemu-img check fix corruption in the image file: two
> guest memory areas refer to the same host memory area
> (duplicated offsets in BAT).
The code below requires big fat comment, what is reversed BAT,
why it is needed and how it helps us to detect the corruption.
You have spent more than a month writing the code. It would
be very good to spend 1 hour to write detailed comment.

> Signed-off-by: Natalia Kuzmina <natalia.kuzmina@openvz.org>
> ---
>   block/parallels.c | 66 +++++++++++++++++++++++++++++++++++++++++++++--
>   1 file changed, 64 insertions(+), 2 deletions(-)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index 6ebad2a2bb..6a73933d45 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -418,9 +418,11 @@ static int coroutine_fn 
> parallels_co_check(BlockDriverState *bs,
>                                              BdrvCheckMode fix)
>   {
>       BDRVParallelsState *s = bs->opaque;
> -    int64_t size, prev_off, high_off;
> +    int64_t size, prev_off, high_off, idx_host, sector_num;
>       int ret;
>       uint32_t i;
> +    int64_t *buf;
> +    int *reversed_bat;
>       bool flush_bat = false;
>   
>       size = bdrv_getlength(bs->file->bs);
> @@ -442,8 +444,14 @@ static int coroutine_fn 
> parallels_co_check(BlockDriverState *bs,
>       }
>   
>       res->bfi.total_clusters = s->bat_size;
> +    res->bfi.allocated_clusters = 0;
>       res->bfi.compressed_clusters = 0; /* compression is not supported */
>   
> +    reversed_bat = g_malloc(s->bat_size * sizeof(int));
> +    for (i = 0; i < s->bat_size; i++) {
> +        reversed_bat[i] = -1;
> +    }
> +
>       high_off = 0;
>       prev_off = 0;
>       for (i = 0; i < s->bat_size; i++) {
> @@ -453,6 +461,59 @@ static int coroutine_fn 
> parallels_co_check(BlockDriverState *bs,
>               continue;
>           }
>   
> +        /* checking bat entry uniqueness */
> +        idx_host = (off - ((s->header->data_off) << BDRV_SECTOR_BITS))
> +            / (s->cluster_size);

> +        if (reversed_bat[idx_host] != -1) { /* duplicated cluster */
> +            fprintf(stderr, "%s cluster %u is duplicated (with cluster %u)\n",
> +                    fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR",
> +                    i, reversed_bat[idx_host]);
> +            res->corruptions++;
> +            res->bfi.allocated_clusters--; /* not to count this cluster twice 
> */
that seems wrong. Duplicated cluster after the fix would be present
twice, with one location and with another one.
If this is correct, detailed explanation is needed.

> +            if (fix & BDRV_FIX_ERRORS) {
> +                /* copy data to new cluster */
> +                sector_num = bat2sect(s, reversed_bat[idx_host]);
> +                buf = g_malloc(s->cluster_size);
why not to allocate this buffer once at the beginning of the function?
Error handling would be simplear or use approach below.

> +                ret = bdrv_pread(bs->file, sector_num << BDRV_SECTOR_BITS,
> +                                 buf, s->cluster_size);
> +                if (ret < 0) {
> +                    res->check_errors++;
> +                    g_free(buf);
> +                    goto out;
> +                }
> +

would it be sane to just zero BAT entry and call
                                  bdrv_pwritev(bs, sector_num << 
BDRV_SECTOR_BITS,  buf, s->cluster_size);
and avoid error prone dances with the internals?
Once the operation is done you could start the operation for the same 
block from the beginning.

> +                ret = bdrv_pwrite(bs->file, s->data_end << BDRV_SECTOR_BITS,
> +                                  buf, s->cluster_size);
> +                if (ret < 0) {
> +                    res->check_errors++;
> +                    g_free(buf);
> +                    goto out;
> +                }
> +
> +                s->bat_bitmap[i] = cpu_to_le32(s->data_end / 
> s->off_multiplier);
> +                s->data_end += s->tracks;
> +                bitmap_set(s->bat_dirty_bmap,
> +                           bat_entry_off(i) / s->bat_dirty_block, 1);
> +                g_free(buf);
> +
> +                res->corruptions_fixed++;
> +                flush_bat = true;
> +
> +                /* these values are invalid after repairing */
> +                off = bat2sect(s, i) << BDRV_SECTOR_BITS;
> +                idx_host = (off - ((s->header->data_off) << BDRV_SECTOR_BITS))
> +                    / (s->cluster_size);
> +                size = bdrv_getlength(bs->file->bs);
> +                if (size < 0) {
> +                    res->check_errors++;
> +                    ret = size;
> +                    goto out;
> +                }
> +            }
> +        }
> +
> +        reversed_bat[idx_host] = i;
> +
>           /* cluster outside the image */
>           if (off > size) {
>               fprintf(stderr, "%s cluster %u is outside image\n",
> @@ -472,7 +533,7 @@ static int coroutine_fn parallels_co_check(BlockDriverState 
> *bs,
>               high_off = off;
>           }
>   
> -        if (prev_off != 0 && (prev_off + s->cluster_size) != off) {
> +        if (prev_off != 0 && (off - prev_off) % s->cluster_size != 0) {
>               res->bfi.fragmented_clusters++;
>           }
for me this change is not a part of the commit. Should it be present here
or be moved into the separate commit?

>           prev_off = off;
> @@ -514,6 +575,7 @@ static int coroutine_fn parallels_co_check(BlockDriverState 
> *bs,
>       }
>   
>   out:
> +    g_free(reversed_bat);
>       qemu_co_mutex_unlock(&s->lock);
>       return ret;
>   }