Re: [PATCH v3 06/10] qcow2-refcount: check_refcounts_l2(): check l2_bitmap

From: Hanna Reitz <hreitz@redhat.com>
To: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>,
	qemu-block@nongnu.org
Cc: kwolf@redhat.com, berto@igalia.com, qemu-devel@nongnu.org,
	mreitz@redhat.com, ktkhai@virtuozzo.com, den@openvz.org,
	eblake@redhat.com
Subject: Re: [PATCH v3 06/10] qcow2-refcount: check_refcounts_l2(): check l2_bitmap
Date: Tue, 14 Sep 2021 10:54:28 +0200	[thread overview]
Message-ID: <704ed8a0-387a-d2e7-827f-39957f67b892@redhat.com> (raw)
In-Reply-To: <20210524142031.142109-7-vsementsov@virtuozzo.com>

On 24.05.21 16:20, Vladimir Sementsov-Ogievskiy wrote:
> Check subcluster bitmap of the l2 entry for different types of
> clusters:
>
>   - for compressed it must be zero
>   - for allocated check consistency of two parts of the bitmap
>   - for unallocated all subclusters should be unallocated
>     (or zero-plain)
>
> For unallocated clusters we can safely fix the entry by making it
> zero-plain.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Tested-by: Kirill Tkhai <ktkhai@virtuozzo.com>
> ---
>   block/qcow2-refcount.c | 30 +++++++++++++++++++++++++++++-
>   1 file changed, 29 insertions(+), 1 deletion(-)
>
> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
> index f48c5e1b5d..062ec48a15 100644
> --- a/block/qcow2-refcount.c
> +++ b/block/qcow2-refcount.c
> @@ -1681,6 +1681,7 @@ static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res,
>           uint64_t coffset;
>           int csize;
>           l2_entry = get_l2_entry(s, l2_table, i);
> +        uint64_t l2_bitmap = get_l2_bitmap(s, l2_table, i);

This is a declaration after a statement.  (Easily fixable by moving the 
l2_entry declaration here, though.  Or by putting the l2_bitmap 
declaration where l2_entry is declared.)

[...]

> @@ -1800,6 +1815,19 @@ static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res,
>   
>           case QCOW2_CLUSTER_ZERO_PLAIN:
>           case QCOW2_CLUSTER_UNALLOCATED:
> +            if (l2_bitmap & QCOW_L2_BITMAP_ALL_ALLOC) {
> +                res->corruptions++;
> +                fprintf(stderr, "%s: Unallocated "
> +                        "cluster has non-zero subcluster allocation map\n",
> +                        fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR");
> +                if (fix & BDRV_FIX_ERRORS) {
> +                    ret = fix_l2_entry_by_zero(bs, res, l2_offset, l2_table, i,
> +                                               active, &metadata_overlap);

I believe this is indeed the correct repair method for 
QCOW2_CLUSTER_ZERO_PLAIN, but I’m not so sure for 
QCOW2_CLUSTER_UNALLOCATED.  As far as I can tell, 
qcow2_get_subcluster_type() will return QCOW2_SUBCLUSTER_INVALID for 
this case, and so trying to read from this clusters will produce I/O 
errors.  But still, shouldn’t we rather make such a cluster unallocated 
rather than zero then?

And as for QCOW2_CLUSTER_ZERO_PLAIN, I believe qcow2_get_cluster_type() 
will never return it when subclusters are enabled.  So this repair path 
will never happen with a cluster type of ZERO_PLAIN, but only for 
UNALLOCATED.

Hanna

> +                    if (metadata_overlap) {
> +                        return ret;
> +                    }
> +                }
> +            }
>               break;
>   
>           default: