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

[Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>]

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?

> [...]