[PATCH 0/2] qcow2: queue discards when discard-no-unref enabled

[PATCH 0/2] qcow2: queue discards when discard-no-unref enabled

[Jean-Louis Dupond]

Partially based on the proposal of Andrey in
https://patchew.org/QEMU/20240913163942.423050-1-andrey.drobyshev@virtuozzo.com/
Split up this from the rest might get it merged a bit quicker hopefully :)

Since the implementation of discard-no-unref, we did not queue the discards correctly
when discard-no-unref was enabled.

Jean-Louis Dupond (2):
  qcow2: rename update_refcount_discard to queue_discard
  qcow2: put discards in discard queue when discard-no-unref is enabled

 block/qcow2-cluster.c  | 16 ++++++----------
 block/qcow2-refcount.c | 25 +++++++++++++++++++++----
 block/qcow2.h          |  4 ++++
 3 files changed, 31 insertions(+), 14 deletions(-)

-- 
2.49.0

[PATCH 1/2] qcow2: rename update_refcount_discard to queue_discard

[Jean-Louis Dupond]

The function just queues discards, and doesn't do any refcount change.
So let's change the function name to align with its function.

Signed-off-by: Jean-Louis Dupond <jean-louis@dupond.be>
---
 block/qcow2-refcount.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 0266542cee..d796018970 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -754,8 +754,8 @@ void qcow2_process_discards(BlockDriverState *bs, int ret)
     }
 }
 
-static void update_refcount_discard(BlockDriverState *bs,
-                                    uint64_t offset, uint64_t length)
+static void queue_discard(BlockDriverState *bs,
+                          uint64_t offset, uint64_t length)
 {
     BDRVQcow2State *s = bs->opaque;
     Qcow2DiscardRegion *d, *p, *next;
@@ -902,7 +902,7 @@ update_refcount(BlockDriverState *bs, int64_t offset, int64_t length,
             }
 
             if (s->discard_passthrough[type]) {
-                update_refcount_discard(bs, cluster_offset, s->cluster_size);
+                queue_discard(bs, cluster_offset, s->cluster_size);
             }
         }
     }
-- 
2.49.0

[PATCH 2/2] qcow2: put discards in discard queue when discard-no-unref is enabled

[Jean-Louis Dupond]

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.0

Re: [PATCH 2/2] qcow2: put discards in discard queue when discard-no-unref is enabled

[Hanna Czenczek]

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.

> +    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>

Re: [PATCH 2/2] qcow2: put discards in discard queue when discard-no-unref is enabled

[Jean-Louis Dupond]

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