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

[Andrey Drobyshev]

On 10/31/23 18:32, Hanna Czenczek wrote:
> 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
> 
> [...]
> 
>> +    if (scri.l2_bitmap != new_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);
>> +    }
>> +
>> +    if (s->discard_passthrough[type]) {
>> +        qcow2_queue_discard(bs, (scri.l2_entry & L2E_OFFSET_MASK) +
>> +                            offset_into_cluster(s, offset),
>> +                            nb_subclusters * s->subcluster_size);
> 
> Are we sure that the cluster is allocated, i.e. that scri.l2_entry &
> L2E_OFFSET_MASK != 0?
> 

I think it must be illegal to mark the entire cluster as unallocated and
yet mark some of the subclusters as allocated.  So in the case you
describe we should detect it earlier in the '!(new_l2_bitmap &
QCOW_L2_BITMAP_ALL_ALLOC)' condition and fall back to discard_in_l2_slice().

> As a side note, I guess discard_in_l2_slice() should also use
> qcow2_queue_discard() instead of bdrv_pdiscard() then.
> 

Yes, looks like it.  I'll  make it a separate patch then.

>> +    }
>> +
>> +    ret = 0;
>> +out:
>> +    qcow2_cache_put(s->l2_table_cache, (void **) &scri.l2_slice);
>> +
>> +    return ret;
>> +}
>> +
>>   int qcow2_cluster_discard(BlockDriverState *bs, uint64_t offset,
>>                             uint64_t bytes, enum qcow2_discard_type type,
>>                             bool full_discard)
>> @@ -2049,19 +2117,36 @@ int qcow2_cluster_discard(BlockDriverState
>> *bs, uint64_t offset,
>>       BDRVQcow2State *s = bs->opaque;
>>       uint64_t end_offset = offset + bytes;
>>       uint64_t nb_clusters;
>> +    unsigned head, tail;
>>       int64_t cleared;
>>       int ret;
>>         /* Caller must pass aligned values, except at image end */
>> -    assert(QEMU_IS_ALIGNED(offset, s->cluster_size));
>> -    assert(QEMU_IS_ALIGNED(end_offset, s->cluster_size) ||
>> +    assert(QEMU_IS_ALIGNED(offset, s->subcluster_size));
>> +    assert(QEMU_IS_ALIGNED(end_offset, s->subcluster_size) ||
>>              end_offset == bs->total_sectors << BDRV_SECTOR_BITS);
>>   -    nb_clusters = size_to_clusters(s, bytes);
>> +    head = MIN(end_offset, ROUND_UP(offset, s->cluster_size)) - offset;
>> +    offset += head;
>> +
>> +    tail = (end_offset >= bs->total_sectors << BDRV_SECTOR_BITS) ? 0 :
>> +           end_offset - MAX(offset, start_of_cluster(s, end_offset));
>> +    end_offset -= tail;
>>         s->cache_discards = true;
>>   +    if (head) {
>> +        ret = discard_l2_subclusters(bs, offset - head,
>> +                                     size_to_subclusters(s, head), type,
>> +                                     full_discard, NULL);
>> +        if (ret < 0) {
>> +            goto fail;
>> +        }
>> +    }
>> +
>>       /* Each L2 slice is handled by its own loop iteration */
>> +    nb_clusters = size_to_clusters(s, end_offset - offset);
>> +
>>       while (nb_clusters > 0) {
> 
> I think the comment should stay attached to the `while`.
> 

Agreed.

> Hanna
> 
>>           cleared = discard_in_l2_slice(bs, offset, nb_clusters, type,
>>                                         full_discard);
>