Re: [PATCH v2 03/11] qcow2: put discard requests in the common queue when discard-no-unref enabled

[Alexander Ivanov]

On 5/13/24 08:31, Andrey Drobyshev wrote:

Normally discard requests are stored in the queue attached to BDRVQcow2State
to be processed later at once.  Currently discard-no-unref option handling
causes these requests to be processed straight away.  Let's fix that.

Note that when doing regular discards qcow2_free_any_cluster() would check
for the presence of external data files for us and redirect request to
underlying data_file.  Here we want to do the same but avoid refcount updates,
thus we perform the same checks.

Suggested-by: Hanna Czenczek <hre...@redhat.com>
Signed-off-by: Andrey Drobyshev <andrey.drobys...@virtuozzo.com>
---
  block/qcow2-cluster.c | 39 +++++++++++++++++++++++++++++----------
  1 file changed, 29 insertions(+), 10 deletions(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 5f057ba2fd..7dff0bd5a1 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -1893,6 +1893,28 @@ again:
      return 0;
  }

+/*

+ * Helper for adding a discard request to the queue without any refcount
+ * modifications.  If external data file is used redirects the request to
+ * the corresponding BdrvChild.
+ */
+static inline void
+discard_no_unref_any_file(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 {
+            qcow2_queue_discard(bs, offset, length);
+        }
+    }
+}
+
  /*
   * This discards as many clusters of nb_clusters as possible at once (i.e.
   * all clusters in the same L2 slice) and returns the number of discarded
@@ -1974,12 +1996,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);
+            discard_no_unref_any_file(bs, old_l2_entry & L2E_OFFSET_MASK,
+                                      s->cluster_size, cluster_type, type);
          }
      }

@@ -2088,12 +2108,11 @@ 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);
+                discard_no_unref_any_file(bs, old_l2_entry & L2E_OFFSET_MASK,
+                                          s->cluster_size, type,
+                                          QCOW2_DISCARD_REQUEST);
              }
          }
      }

Reviewed-by: Alexander Ivanov <alexander.iva...@virtuozzo.com>

--
Best regards,
Alexander Ivanov