Re: [PATCH 5/7] qcow2: zero_l2_subclusters: fall through to discard operation when requested

[Hanna Czenczek <hreitz@redhat.com>]

On 20.10.23 23:56, Andrey Drobyshev wrote:
> When zeroizing subclusters within single cluster, detect usage of the
> BDRV_REQ_MAY_UNMAP flag and fall through to the subcluster-based discard
> operation, much like it's done with the cluster-based discards.  That
> way subcluster-aligned operations "qemu-io -c 'write -z -u ...'" will
> lead to actual unmap.

Ever since the introduction of discard-no-unref, I wonder whether that 
is actually an advantage or not.  I can’t think of a reason why it’d be 
advantageous to drop the allocation.

On the other hand, zero_in_l2_slice() does it indeed, so consistency is 
a reasonable argument.

> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
> ---
>   block/qcow2-cluster.c | 17 ++++++++++++++---
>   1 file changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index cf40f2dc12..040251f2c3 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
> @@ -2242,7 +2242,7 @@ zero_l2_subclusters(BlockDriverState *bs, uint64_t offset,
>                       unsigned nb_subclusters, int flags)
>   {
>       BDRVQcow2State *s = bs->opaque;
> -    uint64_t new_l2_bitmap;
> +    uint64_t new_l2_bitmap, l2_bitmap_mask;
>       int ret, sc = offset_to_sc_index(s, offset);
>       SubClusterRangeInfo scri = { 0 };
>   
> @@ -2251,9 +2251,10 @@ zero_l2_subclusters(BlockDriverState *bs, uint64_t offset,
>           goto out;
>       }
>   
> +    l2_bitmap_mask = QCOW_OFLAG_SUB_ALLOC_RANGE(sc, sc + nb_subclusters);

“l2_bitmap_mask” already wasn’t a great name in patch 4 because it 
doesn’t say what mask it is, but in patch 4, there was at least only one 
relevant mask.  Here, we have two (ALLOC_RANGE and ZERO_RANGE), so the 
name should reflect what kind of mask it is.

>       new_l2_bitmap = scri.l2_bitmap;
> -    new_l2_bitmap |=  QCOW_OFLAG_SUB_ZERO_RANGE(sc, sc + nb_subclusters);
> -    new_l2_bitmap &= ~QCOW_OFLAG_SUB_ALLOC_RANGE(sc, sc + nb_subclusters);
> +    new_l2_bitmap |= QCOW_OFLAG_SUB_ZERO_RANGE(sc, sc + nb_subclusters);
> +    new_l2_bitmap &= ~l2_bitmap_mask;
>   
>       /*
>        * If there're no non-zero subclusters left, we might as well zeroize
> @@ -2266,6 +2267,16 @@ zero_l2_subclusters(BlockDriverState *bs, uint64_t offset,
>                                   1, flags);
>       }
>   
> +    /*
> +     * If the request allows discarding subclusters and they're actually
> +     * allocated, we go down the discard path since after the discard
> +     * operation the subclusters are going to be read as zeroes anyway.
> +     */
> +    if ((flags & BDRV_REQ_MAY_UNMAP) && (scri.l2_bitmap & l2_bitmap_mask)) {
> +        return discard_l2_subclusters(bs, offset, nb_subclusters,
> +                                      QCOW2_DISCARD_REQUEST, false, &scri);
> +    }

I wonder why it matters whether any subclusters are allocated, i.e. why 
can’t we just immediately use discard_l2_subclusters() whenever 
BDRV_REQ_MAY_UNMAP is set, without even fetching SCRI (that would also 
save us passing SCRI here, which I’ve already said on patch 4, I don’t 
find great).

Doesn’t discard_l2_subclusters() guarantee the clusters read as zero 
when full_discard=false?  There is this case where it won’t mark the 
subclusters as zero if there is no backing file, and none of the 
subclusters is allocated.  But still, (A) those subclusters will read as 
zero, and (B) if there is a problem there, why don’t we just always mark 
those subclusters as zero?  This optimization only has effect when the 
guest discards/zeroes subclusters (not whole clusters) that were already 
discarded, so sounds miniscule.

Hanna

> +
>       if (new_l2_bitmap != scri.l2_bitmap) {
>           set_l2_bitmap(s, scri.l2_slice, scri.l2_index, new_l2_bitmap);
>           qcow2_cache_entry_mark_dirty(s->l2_table_cache, scri.l2_slice);