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.drobys...@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?
> [...]
