When discard-no-unref is enabled, discards are not queued like it
should.
This was broken since discard-no-unref was added.
Add some helper function qcow2_discard_cluster which handles some common
checks and calls the queue_discards function if needed to add the
discard request to the queue.
Signed-off-by: Jean-Louis Dupond <jean-louis@dupond.be>
---
block/qcow2-cluster.c | 16 ++++++----------
block/qcow2-refcount.c | 19 ++++++++++++++++++-
block/qcow2.h | 4 ++++
3 files changed, 28 insertions(+), 11 deletions(-)
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index ce8c0076b3..c655bf6df4 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -1978,12 +1978,10 @@ discard_in_l2_slice(BlockDriverState *bs, uint64_t offset, uint64_t nb_clusters,
if (!keep_reference) {
/* Then decrease the refcount */
qcow2_free_any_cluster(bs, old_l2_entry, type);
- } else if (s->discard_passthrough[type] &&
- (cluster_type == QCOW2_CLUSTER_NORMAL ||
- cluster_type == QCOW2_CLUSTER_ZERO_ALLOC)) {
+ } else {
/* If we keep the reference, pass on the discard still */
- bdrv_pdiscard(s->data_file, old_l2_entry & L2E_OFFSET_MASK,
- s->cluster_size);
+ qcow2_discard_cluster(bs, old_l2_entry & L2E_OFFSET_MASK,
+ s->cluster_size, cluster_type, type);
}
}
@@ -2092,12 +2090,10 @@ zero_in_l2_slice(BlockDriverState *bs, uint64_t offset,
if (!keep_reference) {
/* Then decrease the refcount */
qcow2_free_any_cluster(bs, old_l2_entry, QCOW2_DISCARD_REQUEST);
- } else if (s->discard_passthrough[QCOW2_DISCARD_REQUEST] &&
- (type == QCOW2_CLUSTER_NORMAL ||
- type == QCOW2_CLUSTER_ZERO_ALLOC)) {
+ } else {
/* If we keep the reference, pass on the discard still */
- bdrv_pdiscard(s->data_file, old_l2_entry & L2E_OFFSET_MASK,
- s->cluster_size);
+ qcow2_discard_cluster(bs, old_l2_entry & L2E_OFFSET_MASK,
+ s->cluster_size, type, QCOW2_DISCARD_REQUEST);
}
}
}
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index d796018970..e1f830504d 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -1205,6 +1205,23 @@ void qcow2_free_any_cluster(BlockDriverState *bs, uint64_t l2_entry,
}
}
+void qcow2_discard_cluster(BlockDriverState *bs, uint64_t offset,
+ uint64_t length, QCow2ClusterType ctype,
+ enum qcow2_discard_type dtype) {
+
+ BDRVQcow2State *s = bs->opaque;
+
+ if (s->discard_passthrough[dtype] &&
+ (ctype == QCOW2_CLUSTER_NORMAL ||
+ ctype == QCOW2_CLUSTER_ZERO_ALLOC)) {
+ if (has_data_file(bs)) {
+ bdrv_pdiscard(s->data_file, offset, length);
+ } else {
+ queue_discard(bs, offset, length);
+ }
+ }
+}
+
int qcow2_write_caches(BlockDriverState *bs)
{
BDRVQcow2State *s = bs->opaque;
@@ -3619,7 +3636,7 @@ qcow2_discard_refcount_block(BlockDriverState *bs, uint64_t discard_block_offs)
/* discard refblock from the cache if refblock is cached */
qcow2_cache_discard(s->refcount_block_cache, refblock);
}
- update_refcount_discard(bs, discard_block_offs, s->cluster_size);
+ queue_discard(bs, discard_block_offs, s->cluster_size);
return 0;
}
diff --git a/block/qcow2.h b/block/qcow2.h
index a9e3481c6e..547bb2b814 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -880,6 +880,10 @@ void GRAPH_RDLOCK qcow2_free_clusters(BlockDriverState *bs,
void GRAPH_RDLOCK
qcow2_free_any_cluster(BlockDriverState *bs, uint64_t l2_entry,
enum qcow2_discard_type type);
+void GRAPH_RDLOCK
+qcow2_discard_cluster(BlockDriverState *bs, uint64_t offset,
+ uint64_t length, QCow2ClusterType ctype,
+ enum qcow2_discard_type dtype);
int GRAPH_RDLOCK
qcow2_update_snapshot_refcount(BlockDriverState *bs, int64_t l1_table_offset,
--
2.49.0On 29.04.25 12:31, Jean-Louis Dupond wrote:
> When discard-no-unref is enabled, discards are not queued like it
> should.
> This was broken since discard-no-unref was added.
>
> Add some helper function qcow2_discard_cluster which handles some common
> checks and calls the queue_discards function if needed to add the
> discard request to the queue.
>
> Signed-off-by: Jean-Louis Dupond <jean-louis@dupond.be>
> ---
> block/qcow2-cluster.c | 16 ++++++----------
> block/qcow2-refcount.c | 19 ++++++++++++++++++-
> block/qcow2.h | 4 ++++
> 3 files changed, 28 insertions(+), 11 deletions(-)
>
> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index ce8c0076b3..c655bf6df4 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
> @@ -1978,12 +1978,10 @@ discard_in_l2_slice(BlockDriverState *bs, uint64_t offset, uint64_t nb_clusters,
> if (!keep_reference) {
> /* Then decrease the refcount */
> qcow2_free_any_cluster(bs, old_l2_entry, type);
> - } else if (s->discard_passthrough[type] &&
> - (cluster_type == QCOW2_CLUSTER_NORMAL ||
> - cluster_type == QCOW2_CLUSTER_ZERO_ALLOC)) {
> + } else {
> /* If we keep the reference, pass on the discard still */
> - bdrv_pdiscard(s->data_file, old_l2_entry & L2E_OFFSET_MASK,
> - s->cluster_size);
> + qcow2_discard_cluster(bs, old_l2_entry & L2E_OFFSET_MASK,
> + s->cluster_size, cluster_type, type);
> }
> }
>
> @@ -2092,12 +2090,10 @@ zero_in_l2_slice(BlockDriverState *bs, uint64_t offset,
> if (!keep_reference) {
> /* Then decrease the refcount */
> qcow2_free_any_cluster(bs, old_l2_entry, QCOW2_DISCARD_REQUEST);
> - } else if (s->discard_passthrough[QCOW2_DISCARD_REQUEST] &&
> - (type == QCOW2_CLUSTER_NORMAL ||
> - type == QCOW2_CLUSTER_ZERO_ALLOC)) {
> + } else {
> /* If we keep the reference, pass on the discard still */
> - bdrv_pdiscard(s->data_file, old_l2_entry & L2E_OFFSET_MASK,
> - s->cluster_size);
> + qcow2_discard_cluster(bs, old_l2_entry & L2E_OFFSET_MASK,
> + s->cluster_size, type, QCOW2_DISCARD_REQUEST);
> }
> }
> }
> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
> index d796018970..e1f830504d 100644
> --- a/block/qcow2-refcount.c
> +++ b/block/qcow2-refcount.c
> @@ -1205,6 +1205,23 @@ void qcow2_free_any_cluster(BlockDriverState *bs, uint64_t l2_entry,
> }
> }
>
> +void qcow2_discard_cluster(BlockDriverState *bs, uint64_t offset,
> + uint64_t length, QCow2ClusterType ctype,
> + enum qcow2_discard_type dtype) {
> +
QEMU coding style requires putting the { on a separate line.
> + BDRVQcow2State *s = bs->opaque;
> +
> + if (s->discard_passthrough[dtype] &&
> + (ctype == QCOW2_CLUSTER_NORMAL ||
> + ctype == QCOW2_CLUSTER_ZERO_ALLOC)) {
> + if (has_data_file(bs)) {
> + bdrv_pdiscard(s->data_file, offset, length);
> + } else {
> + queue_discard(bs, offset, length);
> + }
> + }
> +}
> +
> int qcow2_write_caches(BlockDriverState *bs)
> {
> BDRVQcow2State *s = bs->opaque;
> @@ -3619,7 +3636,7 @@ qcow2_discard_refcount_block(BlockDriverState *bs, uint64_t discard_block_offs)
> /* discard refblock from the cache if refblock is cached */
> qcow2_cache_discard(s->refcount_block_cache, refblock);
> }
> - update_refcount_discard(bs, discard_block_offs, s->cluster_size);
> + queue_discard(bs, discard_block_offs, s->cluster_size);
>
> return 0;
> }
This hunk should go into the previous patch.
> diff --git a/block/qcow2.h b/block/qcow2.h
> index a9e3481c6e..547bb2b814 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -880,6 +880,10 @@ void GRAPH_RDLOCK qcow2_free_clusters(BlockDriverState *bs,
> void GRAPH_RDLOCK
> qcow2_free_any_cluster(BlockDriverState *bs, uint64_t l2_entry,
> enum qcow2_discard_type type);
> +void GRAPH_RDLOCK
> +qcow2_discard_cluster(BlockDriverState *bs, uint64_t offset,
> + uint64_t length, QCow2ClusterType ctype,
> + enum qcow2_discard_type dtype);
>
> int GRAPH_RDLOCK
> qcow2_update_snapshot_refcount(BlockDriverState *bs, int64_t l1_table_offset,
With the above done, for both patch 1 and 2:
Reviewed-by: Hanna Czenczek <hreitz@redhat.com>
On 5/12/25 14:28, Hanna Czenczek wrote:
> On 29.04.25 12:31, Jean-Louis Dupond wrote:
>> When discard-no-unref is enabled, discards are not queued like it
>> should.
>> This was broken since discard-no-unref was added.
>>
>> Add some helper function qcow2_discard_cluster which handles some common
>> checks and calls the queue_discards function if needed to add the
>> discard request to the queue.
>>
>> Signed-off-by: Jean-Louis Dupond <jean-louis@dupond.be>
>> ---
>> block/qcow2-cluster.c | 16 ++++++----------
>> block/qcow2-refcount.c | 19 ++++++++++++++++++-
>> block/qcow2.h | 4 ++++
>> 3 files changed, 28 insertions(+), 11 deletions(-)
>>
>> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
>> index ce8c0076b3..c655bf6df4 100644
>> --- a/block/qcow2-cluster.c
>> +++ b/block/qcow2-cluster.c
>> @@ -1978,12 +1978,10 @@ discard_in_l2_slice(BlockDriverState *bs,
>> uint64_t offset, uint64_t nb_clusters,
>> if (!keep_reference) {
>> /* Then decrease the refcount */
>> qcow2_free_any_cluster(bs, old_l2_entry, type);
>> - } else if (s->discard_passthrough[type] &&
>> - (cluster_type == QCOW2_CLUSTER_NORMAL ||
>> - cluster_type == QCOW2_CLUSTER_ZERO_ALLOC)) {
>> + } else {
>> /* If we keep the reference, pass on the discard still */
>> - bdrv_pdiscard(s->data_file, old_l2_entry & L2E_OFFSET_MASK,
>> - s->cluster_size);
>> + qcow2_discard_cluster(bs, old_l2_entry & L2E_OFFSET_MASK,
>> + s->cluster_size, cluster_type, type);
>> }
>> }
>> @@ -2092,12 +2090,10 @@ zero_in_l2_slice(BlockDriverState *bs,
>> uint64_t offset,
>> if (!keep_reference) {
>> /* Then decrease the refcount */
>> qcow2_free_any_cluster(bs, old_l2_entry,
>> QCOW2_DISCARD_REQUEST);
>> - } else if (s->discard_passthrough[QCOW2_DISCARD_REQUEST] &&
>> - (type == QCOW2_CLUSTER_NORMAL ||
>> - type == QCOW2_CLUSTER_ZERO_ALLOC)) {
>> + } else {
>> /* If we keep the reference, pass on the discard
>> still */
>> - bdrv_pdiscard(s->data_file, old_l2_entry &
>> L2E_OFFSET_MASK,
>> - s->cluster_size);
>> + qcow2_discard_cluster(bs, old_l2_entry &
>> L2E_OFFSET_MASK,
>> + s->cluster_size, type,
>> QCOW2_DISCARD_REQUEST);
>> }
>> }
>> }
>> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
>> index d796018970..e1f830504d 100644
>> --- a/block/qcow2-refcount.c
>> +++ b/block/qcow2-refcount.c
>> @@ -1205,6 +1205,23 @@ void qcow2_free_any_cluster(BlockDriverState
>> *bs, uint64_t l2_entry,
>> }
>> }
>> +void qcow2_discard_cluster(BlockDriverState *bs, uint64_t offset,
>> + uint64_t length, QCow2ClusterType ctype,
>> + enum qcow2_discard_type dtype) {
>> +
>
> QEMU coding style requires putting the { on a separate line.
>
Fixed
>> + BDRVQcow2State *s = bs->opaque;
>> +
>> + if (s->discard_passthrough[dtype] &&
>> + (ctype == QCOW2_CLUSTER_NORMAL ||
>> + ctype == QCOW2_CLUSTER_ZERO_ALLOC)) {
>> + if (has_data_file(bs)) {
>> + bdrv_pdiscard(s->data_file, offset, length);
>> + } else {
>> + queue_discard(bs, offset, length);
>> + }
>> + }
>> +}
>> +
>> int qcow2_write_caches(BlockDriverState *bs)
>> {
>> BDRVQcow2State *s = bs->opaque;
>> @@ -3619,7 +3636,7 @@ qcow2_discard_refcount_block(BlockDriverState
>> *bs, uint64_t discard_block_offs)
>> /* discard refblock from the cache if refblock is cached */
>> qcow2_cache_discard(s->refcount_block_cache, refblock);
>> }
>> - update_refcount_discard(bs, discard_block_offs, s->cluster_size);
>> + queue_discard(bs, discard_block_offs, s->cluster_size);
>> return 0;
>> }
>
> This hunk should go into the previous patch.
>
Fixed
>> diff --git a/block/qcow2.h b/block/qcow2.h
>> index a9e3481c6e..547bb2b814 100644
>> --- a/block/qcow2.h
>> +++ b/block/qcow2.h
>> @@ -880,6 +880,10 @@ void GRAPH_RDLOCK
>> qcow2_free_clusters(BlockDriverState *bs,
>> void GRAPH_RDLOCK
>> qcow2_free_any_cluster(BlockDriverState *bs, uint64_t l2_entry,
>> enum qcow2_discard_type type);
>> +void GRAPH_RDLOCK
>> +qcow2_discard_cluster(BlockDriverState *bs, uint64_t offset,
>> + uint64_t length, QCow2ClusterType ctype,
>> + enum qcow2_discard_type dtype);
>> int GRAPH_RDLOCK
>> qcow2_update_snapshot_refcount(BlockDriverState *bs, int64_t
>> l1_table_offset,
>
> With the above done, for both patch 1 and 2:
>
> Reviewed-by: Hanna Czenczek <hreitz@redhat.com>
>
Send v2 patch with comments addressed.
Thanks
Jean-Louis
© 2016 - 2025 Red Hat, Inc.