Re: [PATCH v2 10/11] qcow2: zero_l2_subclusters: fall through to discard operation when requested

[Alexander Ivanov]

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

When zeroizing subclusters within single cluster, detect usage of the
BDRV_REQ_MAY_UNMAP flag and fall through to the subcluster-based discard
operation, much like it's done with the cluster-based discards.  That
way subcluster-aligned operations "qemu-io -c 'write -z -u ...'" will
lead to actual unmap.

Signed-off-by: Andrey Drobyshev <andrey.drobys...@virtuozzo.com>
---
  block/qcow2-cluster.c | 26 ++++++++++++++++++++------
  1 file changed, 20 insertions(+), 6 deletions(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 3c134a7e80..53e04eff93 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -2107,15 +2107,16 @@ discard_in_l2_slice(BlockDriverState *bs, uint64_t 
offset, uint64_t 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)
+                       uint64_t nb_subclusters, enum qcow2_discard_type type,
+                       bool full_discard, bool uncond_zeroize)
  {
      BDRVQcow2State *s = bs->opaque;
      uint64_t new_l2_bitmap, bitmap_alloc_mask, bitmap_zero_mask;
      int ret, sc = offset_to_sc_index(s, offset);
      g_auto(SubClusterRangeInfo) scri = { 0 };

+ assert(!(full_discard && uncond_zeroize));

+
      ret = get_sc_range_info(bs, offset, nb_subclusters, &scri);
      if (ret < 0) {
          return ret;
@@ -2140,7 +2141,8 @@ discard_l2_subclusters(BlockDriverState *bs, uint64_t 
offset,
       */
      if (full_discard) {
          new_l2_bitmap &= ~bitmap_zero_mask;
-    } else if (bs->backing || scri.l2_bitmap & bitmap_alloc_mask) {
+    } else if (uncond_zeroize || bs->backing ||
+               scri.l2_bitmap & bitmap_alloc_mask) {
          new_l2_bitmap |= bitmap_zero_mask;
      }

@@ -2197,7 +2199,7 @@ int qcow2_subcluster_discard(BlockDriverState *bs, uint64_t offset,

      if (head) {
          ret = discard_l2_subclusters(bs, offset - head,
                                       size_to_subclusters(s, head), type,
-                                     full_discard);
+                                     full_discard, false);
          if (ret < 0) {
              goto fail;
          }
@@ -2221,7 +2223,7 @@ int qcow2_subcluster_discard(BlockDriverState *bs, 
uint64_t offset,
      if (tail) {
          ret = discard_l2_subclusters(bs, end_offset,
                                       size_to_subclusters(s, tail), type,
-                                     full_discard);
+                                     full_discard, false);
          if (ret < 0) {
              goto fail;
          }
@@ -2318,6 +2320,18 @@ zero_l2_subclusters(BlockDriverState *bs, uint64_t 
offset,
      int ret, sc = offset_to_sc_index(s, offset);
      g_auto(SubClusterRangeInfo) scri = { 0 };

+ /*

+     * If the request allows discarding subclusters we go down the discard
+     * path regardless of their allocation status.  After the discard
+     * operation with full_discard=false subclusters are going to be read as
+     * zeroes anyway.  But we make sure that subclusters are explicitly
+     * zeroed anyway with uncond_zeroize=true.
+     */
+    if (flags & BDRV_REQ_MAY_UNMAP) {
+        return discard_l2_subclusters(bs, offset, nb_subclusters,
+                                      QCOW2_DISCARD_REQUEST, false, true);
+    }
+
      ret = get_sc_range_info(bs, offset, nb_subclusters, &scri);
      if (ret < 0) {
          return ret;

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

--
Best regards,
Alexander Ivanov