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

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.

Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 block/qcow2-cluster.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

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);

On 11/3/23 17:19, Hanna Czenczek wrote:
> 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.

You mean omitting the UNallocation on the discard path?

> 
> 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.
> 

Noted.

>>       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).
> 

Yes, this way we'd indeed be able to get rid of passing an additional
argument.

> 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
> 

Good question.  I also can't think of any issues when
zeroizing/discarding already unallocated clusters.

Essentially you're saying that zeroizing subclusters is equivalent to
discard_l2_subclusters(full_discard=false).  Then in
discard_l2_subclusters() implementation we may mark the subclusters as
zeroes regardless of their allocation status in case of
full_discard=false.  But when dealing with the whole clusters this logic
isn't quite applicable cause if the l2 entry isn't allocated at all
there's no point marking it as zero.  Correct?

> [...]