Re: [PATCH 4/7] qcow2: make subclusters discardable

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

On 10/31/23 18:33, Hanna Czenczek wrote:
> (Sorry, opened another reply window, forgot I already had one open...)
> 
> On 20.10.23 23:56, Andrey Drobyshev wrote:
>> This commit makes the discard operation work on the subcluster level
>> rather than cluster level.  It introduces discard_l2_subclusters()
>> function and makes use of it in qcow2 discard implementation, much like
>> it's done with zero_in_l2_slice() / zero_l2_subclusters().  It also
>> changes the qcow2 driver pdiscard_alignment to subcluster_size.  That
>> way subcluster-aligned discards lead to actual fallocate(PUNCH_HOLE)
>> operation and free host disk space.
>>
>> This feature will let us gain additional disk space on guest
>> TRIM/discard requests, especially when using large enough clusters
>> (1M, 2M) with subclusters enabled.
>>
>> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
>> ---
>>   block/qcow2-cluster.c | 100 ++++++++++++++++++++++++++++++++++++++++--
>>   block/qcow2.c         |   8 ++--
>>   2 files changed, 101 insertions(+), 7 deletions(-)
>>
>> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
>> index 7c6fa5524c..cf40f2dc12 100644
>> --- a/block/qcow2-cluster.c
>> +++ b/block/qcow2-cluster.c
>> @@ -2042,6 +2042,74 @@ discard_in_l2_slice(BlockDriverState *bs,
>> uint64_t offset, uint64_t nb_clusters,
>>       return nb_clusters;
>>   }
>>   +static int coroutine_fn GRAPH_RDLOCK
>> +discard_l2_subclusters(BlockDriverState *bs, uint64_t offset,
>> +                       uint64_t nb_subclusters,
>> +                       enum qcow2_discard_type type,
>> +                       bool full_discard,
>> +                       SubClusterRangeInfo *pscri)
>> +{
>> +    BDRVQcow2State *s = bs->opaque;
>> +    uint64_t new_l2_bitmap, l2_bitmap_mask;
>> +    int ret, sc = offset_to_sc_index(s, offset);
>> +    SubClusterRangeInfo scri = { 0 };
>> +
>> +    if (!pscri) {
>> +        ret = get_sc_range_info(bs, offset, nb_subclusters, &scri);
>> +        if (ret < 0) {
>> +            goto out;
>> +        }
>> +    } else {
>> +        scri = *pscri;
> 
> Allowing to takes this from the caller sounds dangerous, considering we
> need to track who takes care of freeing scri.l2_slice.
> 
>> +    }
>> +
>> +    l2_bitmap_mask = QCOW_OFLAG_SUB_ALLOC_RANGE(sc, sc +
>> nb_subclusters);
>> +    new_l2_bitmap = scri.l2_bitmap;
>> +    new_l2_bitmap &= ~l2_bitmap_mask;
>> +
>> +    /*
>> +     * If there're no allocated subclusters left, we might as well
>> discard
>> +     * the entire cluster.  That way we'd also update the refcount
>> table.
>> +     */
>> +    if (!(new_l2_bitmap & QCOW_L2_BITMAP_ALL_ALLOC)) {
> 
> What if there are subclusters in the cluster that are marked as zero,
> outside of the discarded range?  It sounds wrong to apply a discard with
> either full_discard set or cleared to them.
> 

Hmmm, then I think before falling to this path we should either:

1. Make sure full_discard=false.  That is directly implied from what you
said in your other mail: discarding with full_discard=false is
equivalent to zeroizing;
2. Check that no subclusters within this cluster are explicitly marked
as zeroes.
3. Check that either 1) or 2) is true (if ((1) || (2))).

For now I like option 3) better.

>> +        return discard_in_l2_slice(bs,
>> +                                   QEMU_ALIGN_DOWN(offset,
>> s->cluster_size),
>> +                                   1, type, full_discard);
>> +    }
>> +
>> +    /*
>> +     * Full discard means we fall through to the backing file, thus
>> we only
>> +     * need to mark the subclusters as deallocated.
> 
> I think it also means we need to clear the zero bits.
>

Yes, that seems right.

> Hanna
> 
> [...]