Re: [PATCH 3/6] parallels: Add checking and repairing duplicate offsets in BAT

[Denis V. Lunev]

On 9/2/22 10:52, Alexander Ivanov wrote:
> Cluster offsets must be unique among all BAT entries.
> Find duplicate offsets in the BAT.
>
> If a duplicated offset is found fix it by copying the content
> of the relevant cluster to a new allocated cluster and
> set the new cluster offset to the duplicated entry.
>
> Add host_cluster_index() helper to deduplicate the code.
> Add highest_offset() helper. It will be used for code deduplication
> in the next patch.
>
> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
> ---
>   block/parallels.c | 136 ++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 136 insertions(+)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index dbcaf5d310..339ce45634 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -136,6 +136,26 @@ 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;
not properly aligned cluster is a problem by itself
This is possible for older image format and I believe that
we should run "leak check" even for it and note that
in "not aligned" case one should call 'qemu-img convert"
and copy what can be saved. The error is really hard to
recover.

> +}
> +
> +static int64_t highest_offset(BDRVParallelsState *s)
> +{
> +    int64_t off, high_off = 0;
> +    int i;
> +
> +    for (i = 0; i < s->bat_size; i++) {
> +        off = bat2sect(s, i) << BDRV_SECTOR_BITS;
> +        if (off > high_off) {
> +            high_off = off;
> +        }
> +    }
> +    return high_off;
> +}
> +
>   static int64_t block_status(BDRVParallelsState *s, int64_t sector_num,
>                               int nb_sectors, int *pnum)
>   {
> @@ -547,6 +567,114 @@ static int parallels_check_leak(BlockDriverState *bs,
>       return 0;
>   }
>   
> +static int parallels_check_duplicate(BlockDriverState *bs,
> +                                     BdrvCheckResult *res,
> +                                     BdrvCheckMode fix)
> +{
> +    BDRVParallelsState *s = bs->opaque;
> +    QEMUIOVector qiov;
> +    int64_t off, high_off, sector;
> +    unsigned long *bitmap;
> +    uint32_t i, bitmap_size, cluster_index;
> +    int n, ret = 0;
> +    uint64_t *buf = NULL;
> +    bool new_allocations = false;
> +
> +    high_off = highest_offset(s);
> +    if (high_off == 0) {
> +        return 0;
> +    }
> +
> +    /*
> +     * Create a bitmap of used clusters.
> +     * If a bit is set, there is a BAT entry pointing to this cluster.
> +     * Loop through the BAT entrues, check bits relevant to an entry offset.
s/entrues/entries/
> +     * If bit is set, this entry is duplicated. Otherwise set the bit.
> +     */
> +    bitmap_size = host_cluster_index(s, high_off) + 1;
> +    bitmap = bitmap_new(bitmap_size);
this is wrong de-facto. once you deduplicate, you will have clusters
outside the bitmap. We should use MAX(high_off, virtual_size(bds))
OK, may be this is correct, see my note below, but that's needs
a comment.

> +
> +    buf = g_malloc(s->cluster_size);
nope-nope. you should use memalign as you could have
O_DIRECT IO later on with this buffer

> +    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_pread(bs->file, off, s->cluster_size, buf, 0);
I believe that we are for sure in co-routine context thus
bdrv_co_pread would be better


> +                if (ret < 0) {
> +                    res->check_errors++;
> +                    goto out;
> +                }
> +
> +                sector = (i * s->cluster_size) >> BDRV_SECTOR_BITS;
> +                off = allocate_clusters(bs, sector, s->tracks, &n);
no way. Please do not keep bytes and sectors in the same variable
That makes code unreadable

> +                if (off < 0) {
> +                    res->check_errors++;
> +                    ret = off;
> +                    goto out;
> +                }
> +                off <<= BDRV_SECTOR_BITS;
> +                if (off > high_off) {
> +                    high_off = off;
> +                }
> +
> +                ret = bdrv_co_pwritev(bs->file, off, s->cluster_size, &qiov, 
> 0);
> +                if (ret < 0) {
> +                    res->check_errors++;
> +                    goto out;
> +                }
> +
> +                new_allocations = true;
> +                res->corruptions_fixed++;
We are not setting the bit for the new cluster offset and that requires
a note, that all out of image clusters are already cleared and we
could safely use this position.

Anyway, this is potentially not enough. allocate_cluster() in the
further iterations will reuse holed offsets inside the image and
this potentially puts us into the trouble. This should be noted.

> +            }
> +
> +        } else {
> +            bitmap_set(bitmap, cluster_index, 1);
> +        }
> +    }
> +
> +    if (new_allocations) {
> +        /*
> +         * When new clusters are allocated, file size increases
> +         * by 128 Mb blocks. We need to truncate the file to the
> +         * right size.
> +         */
> +        ret = parallels_handle_leak(bs, res, high_off, true);
> +        if (ret < 0) {
> +            res->check_errors++;
> +            goto out;
> +        }
> +    }
This is redundant I believe. parallels_close should do the job well.
More over, we will have leak detector triggered and thus false
error printed.


> +
> +out:
> +    qemu_iovec_destroy(&qiov);
> +    g_free(buf);
> +    g_free(bitmap);
> +    return ret;
> +}
> +
>   static void parallels_collect_statistics(BlockDriverState *bs,
>                                            BdrvCheckResult *res,
>                                            BdrvCheckMode fix)
> @@ -595,6 +723,14 @@ static int coroutine_fn 
> parallels_co_check(BlockDriverState *bs,
>               return ret;
>           }
>   
> +        /* This checks only for "WithouFreSpacExt" format */
> +        if (!memcmp(s->header->magic, HEADER_MAGIC2, 16)) {
That is I am seriously against. Why only newer format supported?

> +            ret = parallels_check_duplicate(bs, res, fix);
> +            if (ret < 0) {
> +                return ret;
> +            }
> +        }
> +
>           parallels_collect_statistics(bs, res, fix);
>       }
>