Re: [PATCH v2 3/4] block: allow commit to unmap zero blocks

[Vladimir Sementsov-Ogievskiy]

On 14.07.24 00:56, Vincent Vanlaer wrote:

Non-active block commits do not discard blocks only containing zeros,
causing images to lose sparseness after the commit. This commit fixes
that by writing zero blocks using blk_co_pwrite_zeroes rather than
writing them out as any other arbitrary data.

Signed-off-by: Vincent Vanlaer <libvirt-e6954...@volkihar.be>
---
  block/commit.c | 19 +++++++++++++++++++
  1 file changed, 19 insertions(+)

diff --git a/block/commit.c b/block/commit.c
index fb54fc9560..6ce30927ac 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -130,6 +130,7 @@ static void commit_clean(Job *job)

typedef enum CommitMethod {

      COMMIT_METHOD_COPY,
+    COMMIT_METHOD_ZERO,
      COMMIT_METHOD_IGNORE,
  } CommitMethod;

@@ -185,6 +186,18 @@ static int coroutine_fn commit_run(Job *job, Error **errp)

          if (ret >= 0) {
              if (!(ret & BDRV_BLOCK_ALLOCATED)) {
                  commit_method = COMMIT_METHOD_IGNORE;
+            } else if (ret & BDRV_BLOCK_ZERO) {
+                int64_t target_offset;
+                int64_t target_bytes;
+                WITH_GRAPH_RDLOCK_GUARD() {
+                    bdrv_round_to_subclusters(s->base_bs, offset, n,
+                                           &target_offset, &target_bytes);

indentation broken

+                }
+
+                if (target_offset == offset &&
+                    target_bytes == n) {
+                    commit_method = COMMIT_METHOD_ZERO;

Why this is needed? Could we blindly do write-zeroes at original (offset, n)? 
Underlying logic would use any possiblity to write zeroes effectively, and 
unaligned tails (if any) would be written as data.

+                }
              }

switch (commit_method) {

@@ -198,6 +211,11 @@ static int coroutine_fn commit_run(Job *job, Error **errp)
                      }
                  }
                  break;
+            case COMMIT_METHOD_ZERO:
+                ret = blk_co_pwrite_zeroes(s->base, offset, n,
+                    BDRV_REQ_MAY_UNMAP);
+                error_in_source = false;
+                break;
              case COMMIT_METHOD_IGNORE:
                  break;
              default:
@@ -216,6 +234,7 @@ static int coroutine_fn commit_run(Job *job, Error **errp)
                  continue;
              }
          }
+

extra unrelated hunk for style, I'd drop it

          /* Publish progress */
          job_progress_update(&s->common.job, n);

--
Best regards,
Vladimir