[PATCH v4 0/5] block: allow commit to unmap zero blocks

[PATCH v4 0/5] block: allow commit to unmap zero blocks

[Vincent Vanlaer]

This patch series adds support for zero blocks in non-active commits.
The first three patches in the series refactor the relevant code, patch
four makes the actual changes, and the last patch adds a test for the
new functionality.

---

Changes since v3:
- minor reformating based on checkpatch.pl
- moved tracepoint in commit_iteration before first possible return on
  error
- renamed the handle_error label in commit_iteration to fail and
  prevented the happy path from passing through this label
- moved test script to the tests/qemu-iotests/tests folder and named it
  commit-zero-blocks

Changes since v2:
- moved main loop of commit_run to a separate function and refactored
  the error handling.
- call blk_co_pwrite_zero even if the size of the zero region does not
  align with the sectors of the base image. This removes the need for
  the CommitMethod enum

Changes since v1:
- split up the implementation in three separate commits
- removed accidentally left over includes from testing

Vincent Vanlaer (5):
  block: get type of block allocation in commit_run
  block: move commit_run loop to separate function
  block: refactor error handling of commit_iteration
  block: allow commit to unmap zero blocks
  block: add test non-active commit with zeroed data

 block/commit.c                                | 116 +++++++++++++-----
 tests/qemu-iotests/tests/commit-zero-blocks   |  96 +++++++++++++++
 .../qemu-iotests/tests/commit-zero-blocks.out |  54 ++++++++
 3 files changed, 232 insertions(+), 34 deletions(-)
 create mode 100755 tests/qemu-iotests/tests/commit-zero-blocks
 create mode 100644 tests/qemu-iotests/tests/commit-zero-blocks.out

-- 
2.44.1

[PATCH v4 1/5] block: get type of block allocation in commit_run

[Vincent Vanlaer]

bdrv_co_common_block_status_above not only returns whether the block is
allocated, but also if it contains zeroes.

Signed-off-by: Vincent Vanlaer <libvirt-e6954efa@volkihar.be>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 block/commit.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/block/commit.c b/block/commit.c
index 7c3fdcb0ca..8dee25b313 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -15,6 +15,8 @@
 #include "qemu/osdep.h"
 #include "qemu/cutils.h"
 #include "trace.h"
+#include "block/block-common.h"
+#include "block/coroutines.h"
 #include "block/block_int.h"
 #include "block/blockjob_int.h"
 #include "qapi/error.h"
@@ -167,9 +169,13 @@ static int coroutine_fn commit_run(Job *job, Error **errp)
             break;
         }
         /* Copy if allocated above the base */
-        ret = blk_co_is_allocated_above(s->top, s->base_overlay, true,
-                                        offset, COMMIT_BUFFER_SIZE, &n);
-        copy = (ret > 0);
+        WITH_GRAPH_RDLOCK_GUARD() {
+            ret = bdrv_co_common_block_status_above(blk_bs(s->top),
+                s->base_overlay, true, true, offset, COMMIT_BUFFER_SIZE,
+                &n, NULL, NULL, NULL);
+        }
+
+        copy = (ret >= 0 && ret & BDRV_BLOCK_ALLOCATED);
         trace_commit_one_iteration(s, offset, n, ret);
         if (copy) {
             assert(n < SIZE_MAX);
-- 
2.44.1

[PATCH v4 2/5] block: move commit_run loop to separate function

[Vincent Vanlaer]

Signed-off-by: Vincent Vanlaer <libvirt-e6954efa@volkihar.be>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 block/commit.c | 89 +++++++++++++++++++++++++++++---------------------
 1 file changed, 52 insertions(+), 37 deletions(-)

diff --git a/block/commit.c b/block/commit.c
index 8dee25b313..078e54f51f 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -128,6 +128,55 @@ static void commit_clean(Job *job)
     blk_unref(s->top);
 }
 
+static int commit_iteration(CommitBlockJob *s, int64_t offset,
+                            int64_t *n, void *buf)
+{
+    int ret = 0;
+    bool copy;
+    bool error_in_source = true;
+
+    /* Copy if allocated above the base */
+    WITH_GRAPH_RDLOCK_GUARD() {
+        ret = bdrv_co_common_block_status_above(blk_bs(s->top),
+            s->base_overlay, true, true, offset, COMMIT_BUFFER_SIZE,
+            n, NULL, NULL, NULL);
+    }
+
+    copy = (ret >= 0 && ret & BDRV_BLOCK_ALLOCATED);
+    trace_commit_one_iteration(s, offset, *n, ret);
+    if (copy) {
+        assert(*n < SIZE_MAX);
+
+        ret = blk_co_pread(s->top, offset, *n, buf, 0);
+        if (ret >= 0) {
+            ret = blk_co_pwrite(s->base, offset, *n, buf, 0);
+            if (ret < 0) {
+                error_in_source = false;
+            }
+        }
+    }
+    if (ret < 0) {
+        BlockErrorAction action = block_job_error_action(&s->common,
+                                                         s->on_error,
+                                                         error_in_source,
+                                                         -ret);
+        if (action == BLOCK_ERROR_ACTION_REPORT) {
+            return ret;
+        } else {
+            *n = 0;
+            return 0;
+        }
+    }
+    /* Publish progress */
+    job_progress_update(&s->common.job, *n);
+
+    if (copy) {
+        block_job_ratelimit_processed_bytes(&s->common, *n);
+    }
+
+    return 0;
+}
+
 static int coroutine_fn commit_run(Job *job, Error **errp)
 {
     CommitBlockJob *s = container_of(job, CommitBlockJob, common.job);
@@ -158,9 +207,6 @@ static int coroutine_fn commit_run(Job *job, Error **errp)
     buf = blk_blockalign(s->top, COMMIT_BUFFER_SIZE);
 
     for (offset = 0; offset < len; offset += n) {
-        bool copy;
-        bool error_in_source = true;
-
         /* Note that even when no rate limit is applied we need to yield
          * with no pending I/O here so that bdrv_drain_all() returns.
          */
@@ -168,42 +214,11 @@ static int coroutine_fn commit_run(Job *job, Error **errp)
         if (job_is_cancelled(&s->common.job)) {
             break;
         }
-        /* Copy if allocated above the base */
-        WITH_GRAPH_RDLOCK_GUARD() {
-            ret = bdrv_co_common_block_status_above(blk_bs(s->top),
-                s->base_overlay, true, true, offset, COMMIT_BUFFER_SIZE,
-                &n, NULL, NULL, NULL);
-        }
 
-        copy = (ret >= 0 && ret & BDRV_BLOCK_ALLOCATED);
-        trace_commit_one_iteration(s, offset, n, ret);
-        if (copy) {
-            assert(n < SIZE_MAX);
-
-            ret = blk_co_pread(s->top, offset, n, buf, 0);
-            if (ret >= 0) {
-                ret = blk_co_pwrite(s->base, offset, n, buf, 0);
-                if (ret < 0) {
-                    error_in_source = false;
-                }
-            }
-        }
-        if (ret < 0) {
-            BlockErrorAction action =
-                block_job_error_action(&s->common, s->on_error,
-                                       error_in_source, -ret);
-            if (action == BLOCK_ERROR_ACTION_REPORT) {
-                return ret;
-            } else {
-                n = 0;
-                continue;
-            }
-        }
-        /* Publish progress */
-        job_progress_update(&s->common.job, n);
+        ret = commit_iteration(s, offset, &n, buf);
 
-        if (copy) {
-            block_job_ratelimit_processed_bytes(&s->common, n);
+        if (ret < 0) {
+            return ret;
         }
     }
 
-- 
2.44.1

[PATCH v4 3/5] block: refactor error handling of commit_iteration

[Vincent Vanlaer]

Signed-off-by: Vincent Vanlaer <libvirt-e6954efa@volkihar.be>
---
 block/commit.c | 61 ++++++++++++++++++++++++++++----------------------
 1 file changed, 34 insertions(+), 27 deletions(-)

diff --git a/block/commit.c b/block/commit.c
index 078e54f51f..5c24c8b80a 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -129,51 +129,58 @@ static void commit_clean(Job *job)
 }
 
 static int commit_iteration(CommitBlockJob *s, int64_t offset,
-                            int64_t *n, void *buf)
+                            int64_t *requested_bytes, void *buf)
 {
+    int64_t bytes = *requested_bytes;
     int ret = 0;
-    bool copy;
     bool error_in_source = true;
 
     /* Copy if allocated above the base */
     WITH_GRAPH_RDLOCK_GUARD() {
         ret = bdrv_co_common_block_status_above(blk_bs(s->top),
             s->base_overlay, true, true, offset, COMMIT_BUFFER_SIZE,
-            n, NULL, NULL, NULL);
+            &bytes, NULL, NULL, NULL);
     }
 
-    copy = (ret >= 0 && ret & BDRV_BLOCK_ALLOCATED);
-    trace_commit_one_iteration(s, offset, *n, ret);
-    if (copy) {
-        assert(*n < SIZE_MAX);
+    trace_commit_one_iteration(s, offset, bytes, ret);
 
-        ret = blk_co_pread(s->top, offset, *n, buf, 0);
-        if (ret >= 0) {
-            ret = blk_co_pwrite(s->base, offset, *n, buf, 0);
-            if (ret < 0) {
-                error_in_source = false;
-            }
-        }
-    }
     if (ret < 0) {
-        BlockErrorAction action = block_job_error_action(&s->common,
-                                                         s->on_error,
-                                                         error_in_source,
-                                                         -ret);
-        if (action == BLOCK_ERROR_ACTION_REPORT) {
-            return ret;
-        } else {
-            *n = 0;
-            return 0;
+        goto fail;
+    }
+
+    if (ret & BDRV_BLOCK_ALLOCATED) {
+        assert(bytes < SIZE_MAX);
+
+        ret = blk_co_pread(s->top, offset, bytes, buf, 0);
+        if (ret < 0) {
+            goto fail;
         }
+
+        ret = blk_co_pwrite(s->base, offset, bytes, buf, 0);
+        if (ret < 0) {
+            error_in_source = false;
+            goto fail;
+        }
+
+        block_job_ratelimit_processed_bytes(&s->common, bytes);
     }
+
     /* Publish progress */
-    job_progress_update(&s->common.job, *n);
 
-    if (copy) {
-        block_job_ratelimit_processed_bytes(&s->common, *n);
+    job_progress_update(&s->common.job, bytes);
+
+    *requested_bytes = bytes;
+
+    return 0;
+fail:;
+    BlockErrorAction action = block_job_error_action(&s->common, s->on_error,
+                                                     error_in_source, -ret);
+    if (action == BLOCK_ERROR_ACTION_REPORT) {
+        return ret;
     }
 
+    *requested_bytes = 0;
+
     return 0;
 }
 
-- 
2.44.1

[PATCH v4 4/5] block: allow commit to unmap zero blocks

[Vincent Vanlaer]

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-e6954efa@volkihar.be>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 block/commit.c | 38 +++++++++++++++++++++++++++++---------
 1 file changed, 29 insertions(+), 9 deletions(-)

diff --git a/block/commit.c b/block/commit.c
index 5c24c8b80a..ca1a367b41 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -149,19 +149,39 @@ static int commit_iteration(CommitBlockJob *s, int64_t offset,
     }
 
     if (ret & BDRV_BLOCK_ALLOCATED) {
-        assert(bytes < SIZE_MAX);
+        if (ret & BDRV_BLOCK_ZERO) {
+            /*
+             * If the top (sub)clusters are smaller than the base
+             * (sub)clusters, this will not unmap unless the underlying device
+             * does some tracking of these requests. Ideally, we would find
+             * the maximal extent of the zero clusters.
+             */
+            ret = blk_co_pwrite_zeroes(s->base, offset, bytes,
+                                       BDRV_REQ_MAY_UNMAP);
+            if (ret < 0) {
+                error_in_source = false;
+                goto fail;
+            }
+        } else {
+            assert(bytes < SIZE_MAX);
 
-        ret = blk_co_pread(s->top, offset, bytes, buf, 0);
-        if (ret < 0) {
-            goto fail;
-        }
+            ret = blk_co_pread(s->top, offset, bytes, buf, 0);
+            if (ret < 0) {
+                goto fail;
+            }
 
-        ret = blk_co_pwrite(s->base, offset, bytes, buf, 0);
-        if (ret < 0) {
-            error_in_source = false;
-            goto fail;
+            ret = blk_co_pwrite(s->base, offset, bytes, buf, 0);
+            if (ret < 0) {
+                error_in_source = false;
+                goto fail;
+            }
         }
 
+        /*
+         * Whether zeroes actually end up on disk depends on the details of
+         * the underlying driver. Therefore, this might rate limit more than
+         * is necessary.
+         */
         block_job_ratelimit_processed_bytes(&s->common, bytes);
     }
 
-- 
2.44.1

[PATCH v4 5/5] block: add test non-active commit with zeroed data

[Vincent Vanlaer]

Signed-off-by: Vincent Vanlaer <libvirt-e6954efa@volkihar.be>
Tested-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 tests/qemu-iotests/tests/commit-zero-blocks   | 96 +++++++++++++++++++
 .../qemu-iotests/tests/commit-zero-blocks.out | 54 +++++++++++
 2 files changed, 150 insertions(+)
 create mode 100755 tests/qemu-iotests/tests/commit-zero-blocks
 create mode 100644 tests/qemu-iotests/tests/commit-zero-blocks.out

diff --git a/tests/qemu-iotests/tests/commit-zero-blocks b/tests/qemu-iotests/tests/commit-zero-blocks
new file mode 100755
index 0000000000..de00273e72
--- /dev/null
+++ b/tests/qemu-iotests/tests/commit-zero-blocks
@@ -0,0 +1,96 @@
+#!/usr/bin/env bash
+# group: rw quick
+#
+# Test for commit of discarded blocks
+#
+# This tests committing a live snapshot where some of the blocks that
+# are present in the base image are discarded in the intermediate image.
+# This intends to check that these blocks are also discarded in the base
+# image after the commit.
+#
+# Copyright (C) 2024 Vincent Vanlaer.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+# creator
+owner=libvirt-e6954efa@volkihar.be
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+status=1	# failure is the default!
+
+_cleanup()
+{
+    _cleanup_qemu
+    _rm_test_img "${TEST_IMG}.base"
+    _rm_test_img "${TEST_IMG}.mid"
+    _cleanup_test_img
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+cd ..
+. ./common.rc
+. ./common.filter
+. ./common.qemu
+
+_supported_fmt qcow2
+_supported_proto file
+
+size="1M"
+
+TEST_IMG="$TEST_IMG.base" _make_test_img $size
+TEST_IMG="$TEST_IMG.mid" _make_test_img -b "$TEST_IMG.base" -F $IMGFMT $size
+_make_test_img -b "${TEST_IMG}.mid" -F $IMGFMT $size
+
+$QEMU_IO -c "write -P 0x01 64k 128k" "$TEST_IMG.base" | _filter_qemu_io
+$QEMU_IO -c "discard 64k 64k" "$TEST_IMG.mid" | _filter_qemu_io
+
+echo
+echo "=== Base image info before commit ==="
+TEST_IMG="${TEST_IMG}.base" _img_info | _filter_img_info
+$QEMU_IMG map --output=json "$TEST_IMG.base" | _filter_qemu_img_map
+
+echo
+echo "=== Middle image info before commit ==="
+TEST_IMG="${TEST_IMG}.mid" _img_info | _filter_img_info
+$QEMU_IMG map --output=json "$TEST_IMG.mid" | _filter_qemu_img_map
+
+echo
+echo === Running QEMU Live Commit Test ===
+echo
+
+qemu_comm_method="qmp"
+_launch_qemu -drive file="${TEST_IMG}",if=virtio,id=test
+h=$QEMU_HANDLE
+
+_send_qemu_cmd $h "{ 'execute': 'qmp_capabilities' }" "return"
+
+_send_qemu_cmd $h "{ 'execute': 'block-commit',
+                                 'arguments': { 'device': 'test',
+                                 'top': '"${TEST_IMG}.mid"',
+                                 'base': '"${TEST_IMG}.base"'} }" '"status": "null"'
+
+_cleanup_qemu
+
+echo
+echo "=== Base image info after commit ==="
+TEST_IMG="${TEST_IMG}.base" _img_info | _filter_img_info
+$QEMU_IMG map --output=json "$TEST_IMG.base" | _filter_qemu_img_map
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/tests/commit-zero-blocks.out b/tests/qemu-iotests/tests/commit-zero-blocks.out
new file mode 100644
index 0000000000..85bdc46aaf
--- /dev/null
+++ b/tests/qemu-iotests/tests/commit-zero-blocks.out
@@ -0,0 +1,54 @@
+QA output created by commit-zero-blocks
+Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=1048576
+Formatting 'TEST_DIR/t.IMGFMT.mid', fmt=IMGFMT size=1048576 backing_file=TEST_DIR/t.IMGFMT.base backing_fmt=IMGFMT
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576 backing_file=TEST_DIR/t.IMGFMT.mid backing_fmt=IMGFMT
+wrote 131072/131072 bytes at offset 65536
+128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+discard 65536/65536 bytes at offset 65536
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+=== Base image info before commit ===
+image: TEST_DIR/t.IMGFMT.base
+file format: IMGFMT
+virtual size: 1 MiB (1048576 bytes)
+[{ "start": 0, "length": 65536, "depth": 0, "present": false, "zero": true, "data": false, "compressed": false},
+{ "start": 65536, "length": 131072, "depth": 0, "present": true, "zero": false, "data": true, "compressed": false, "offset": OFFSET},
+{ "start": 196608, "length": 851968, "depth": 0, "present": false, "zero": true, "data": false, "compressed": false}]
+
+=== Middle image info before commit ===
+image: TEST_DIR/t.IMGFMT.mid
+file format: IMGFMT
+virtual size: 1 MiB (1048576 bytes)
+backing file: TEST_DIR/t.IMGFMT.base
+backing file format: IMGFMT
+[{ "start": 0, "length": 65536, "depth": 1, "present": false, "zero": true, "data": false, "compressed": false},
+{ "start": 65536, "length": 65536, "depth": 0, "present": true, "zero": true, "data": false, "compressed": false},
+{ "start": 131072, "length": 65536, "depth": 1, "present": true, "zero": false, "data": true, "compressed": false, "offset": OFFSET},
+{ "start": 196608, "length": 851968, "depth": 1, "present": false, "zero": true, "data": false, "compressed": false}]
+
+=== Running QEMU Live Commit Test ===
+
+{ 'execute': 'qmp_capabilities' }
+{"return": {}}
+{ 'execute': 'block-commit',
+                                 'arguments': { 'device': 'test',
+                                 'top': 'TEST_DIR/t.IMGFMT.mid',
+                                 'base': 'TEST_DIR/t.IMGFMT.base'} }
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "created", "id": "test"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "running", "id": "test"}}
+{"return": {}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "waiting", "id": "test"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "pending", "id": "test"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "test", "len": 1048576, "offset": 1048576, "speed": 0, "type": "commit"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "concluded", "id": "test"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "null", "id": "test"}}
+
+=== Base image info after commit ===
+image: TEST_DIR/t.IMGFMT.base
+file format: IMGFMT
+virtual size: 1 MiB (1048576 bytes)
+[{ "start": 0, "length": 65536, "depth": 0, "present": false, "zero": true, "data": false, "compressed": false},
+{ "start": 65536, "length": 65536, "depth": 0, "present": true, "zero": true, "data": false, "compressed": false},
+{ "start": 131072, "length": 65536, "depth": 0, "present": true, "zero": false, "data": true, "compressed": false, "offset": OFFSET},
+{ "start": 196608, "length": 851968, "depth": 0, "present": false, "zero": true, "data": false, "compressed": false}]
+*** done
-- 
2.44.1

Re: [PATCH v4 3/5] block: refactor error handling of commit_iteration

[Vladimir Sementsov-Ogievskiy]

On 26.10.24 19:30, Vincent Vanlaer wrote:
> Signed-off-by: Vincent Vanlaer <libvirt-e6954efa@volkihar.be>
> ---
>   block/commit.c | 61 ++++++++++++++++++++++++++++----------------------
>   1 file changed, 34 insertions(+), 27 deletions(-)
> 
> diff --git a/block/commit.c b/block/commit.c
> index 078e54f51f..5c24c8b80a 100644
> --- a/block/commit.c
> +++ b/block/commit.c
> @@ -129,51 +129,58 @@ static void commit_clean(Job *job)
>   }
>   
>   static int commit_iteration(CommitBlockJob *s, int64_t offset,
> -                            int64_t *n, void *buf)
> +                            int64_t *requested_bytes, void *buf)
>   {
> +    int64_t bytes = *requested_bytes;
>       int ret = 0;
> -    bool copy;
>       bool error_in_source = true;
>   
>       /* Copy if allocated above the base */
>       WITH_GRAPH_RDLOCK_GUARD() {
>           ret = bdrv_co_common_block_status_above(blk_bs(s->top),
>               s->base_overlay, true, true, offset, COMMIT_BUFFER_SIZE,
> -            n, NULL, NULL, NULL);
> +            &bytes, NULL, NULL, NULL);
>       }
>   
> -    copy = (ret >= 0 && ret & BDRV_BLOCK_ALLOCATED);
> -    trace_commit_one_iteration(s, offset, *n, ret);
> -    if (copy) {
> -        assert(*n < SIZE_MAX);
> +    trace_commit_one_iteration(s, offset, bytes, ret);
>   
> -        ret = blk_co_pread(s->top, offset, *n, buf, 0);
> -        if (ret >= 0) {
> -            ret = blk_co_pwrite(s->base, offset, *n, buf, 0);
> -            if (ret < 0) {
> -                error_in_source = false;
> -            }
> -        }
> -    }
>       if (ret < 0) {
> -        BlockErrorAction action = block_job_error_action(&s->common,
> -                                                         s->on_error,
> -                                                         error_in_source,
> -                                                         -ret);
> -        if (action == BLOCK_ERROR_ACTION_REPORT) {
> -            return ret;
> -        } else {
> -            *n = 0;
> -            return 0;
> +        goto fail;
> +    }
> +
> +    if (ret & BDRV_BLOCK_ALLOCATED) {
> +        assert(bytes < SIZE_MAX);
> +
> +        ret = blk_co_pread(s->top, offset, bytes, buf, 0);
> +        if (ret < 0) {
> +            goto fail;
>           }
> +
> +        ret = blk_co_pwrite(s->base, offset, bytes, buf, 0);
> +        if (ret < 0) {
> +            error_in_source = false;
> +            goto fail;
> +        }
> +
> +        block_job_ratelimit_processed_bytes(&s->common, bytes);
>       }
> +
>       /* Publish progress */
> -    job_progress_update(&s->common.job, *n);
>   
> -    if (copy) {
> -        block_job_ratelimit_processed_bytes(&s->common, *n);
> +    job_progress_update(&s->common.job, bytes);
> +
> +    *requested_bytes = bytes;
> +
> +    return 0;

With this extra semicolon removed:

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

I'd also add an empty line before "fail:".


> +fail:;
> +    BlockErrorAction action = block_job_error_action(&s->common, s->on_error,
> +                                                     error_in_source, -ret);
> +    if (action == BLOCK_ERROR_ACTION_REPORT) {
> +        return ret;
>       }
>   
> +    *requested_bytes = 0;
> +
>       return 0;
>   }
>   

-- 
Best regards,
Vladimir

Re: [PATCH v4 3/5] block: refactor error handling of commit_iteration

[Vladimir Sementsov-Ogievskiy]

On 18.11.24 10:37, Vladimir Sementsov-Ogievskiy wrote:
> On 26.10.24 19:30, Vincent Vanlaer wrote:
>> Signed-off-by: Vincent Vanlaer <libvirt-e6954efa@volkihar.be>
>> ---
>>   block/commit.c | 61 ++++++++++++++++++++++++++++----------------------
>>   1 file changed, 34 insertions(+), 27 deletions(-)
>>
>> diff --git a/block/commit.c b/block/commit.c
>> index 078e54f51f..5c24c8b80a 100644
>> --- a/block/commit.c
>> +++ b/block/commit.c
>> @@ -129,51 +129,58 @@ static void commit_clean(Job *job)
>>   }
>>   static int commit_iteration(CommitBlockJob *s, int64_t offset,
>> -                            int64_t *n, void *buf)
>> +                            int64_t *requested_bytes, void *buf)
>>   {
>> +    int64_t bytes = *requested_bytes;
>>       int ret = 0;
>> -    bool copy;
>>       bool error_in_source = true;
>>       /* Copy if allocated above the base */
>>       WITH_GRAPH_RDLOCK_GUARD() {
>>           ret = bdrv_co_common_block_status_above(blk_bs(s->top),
>>               s->base_overlay, true, true, offset, COMMIT_BUFFER_SIZE,
>> -            n, NULL, NULL, NULL);
>> +            &bytes, NULL, NULL, NULL);
>>       }
>> -    copy = (ret >= 0 && ret & BDRV_BLOCK_ALLOCATED);
>> -    trace_commit_one_iteration(s, offset, *n, ret);
>> -    if (copy) {
>> -        assert(*n < SIZE_MAX);
>> +    trace_commit_one_iteration(s, offset, bytes, ret);
>> -        ret = blk_co_pread(s->top, offset, *n, buf, 0);
>> -        if (ret >= 0) {
>> -            ret = blk_co_pwrite(s->base, offset, *n, buf, 0);
>> -            if (ret < 0) {
>> -                error_in_source = false;
>> -            }
>> -        }
>> -    }
>>       if (ret < 0) {
>> -        BlockErrorAction action = block_job_error_action(&s->common,
>> -                                                         s->on_error,
>> -                                                         error_in_source,
>> -                                                         -ret);
>> -        if (action == BLOCK_ERROR_ACTION_REPORT) {
>> -            return ret;
>> -        } else {
>> -            *n = 0;
>> -            return 0;
>> +        goto fail;
>> +    }
>> +
>> +    if (ret & BDRV_BLOCK_ALLOCATED) {
>> +        assert(bytes < SIZE_MAX);
>> +
>> +        ret = blk_co_pread(s->top, offset, bytes, buf, 0);
>> +        if (ret < 0) {
>> +            goto fail;
>>           }
>> +
>> +        ret = blk_co_pwrite(s->base, offset, bytes, buf, 0);
>> +        if (ret < 0) {
>> +            error_in_source = false;
>> +            goto fail;
>> +        }
>> +
>> +        block_job_ratelimit_processed_bytes(&s->common, bytes);
>>       }
>> +
>>       /* Publish progress */
>> -    job_progress_update(&s->common.job, *n);
>> -    if (copy) {
>> -        block_job_ratelimit_processed_bytes(&s->common, *n);
>> +    job_progress_update(&s->common.job, bytes);
>> +
>> +    *requested_bytes = bytes;
>> +
>> +    return 0;
> 
> With this extra semicolon removed:

I meant "fail:;"

I'll will touch this up when applying.

> 
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> 
> I'd also add an empty line before "fail:".
> 
> 
>> +fail:;
>> +    BlockErrorAction action = block_job_error_action(&s->common, s->on_error,
>> +                                                     error_in_source, -ret);
>> +    if (action == BLOCK_ERROR_ACTION_REPORT) {
>> +        return ret;
>>       }
>> +    *requested_bytes = 0;
>> +
>>       return 0;
>>   }
> 

-- 
Best regards,
Vladimir

Re: [PATCH v4 0/5] block: allow commit to unmap zero blocks

[Vladimir Sementsov-Ogievskiy]

On 26.10.24 19:30, Vincent Vanlaer wrote:
> This patch series adds support for zero blocks in non-active commits.
> The first three patches in the series refactor the relevant code, patch
> four makes the actual changes, and the last patch adds a test for the
> new functionality.
> 
> ---
> 
> Changes since v3:
> - minor reformating based on checkpatch.pl
> - moved tracepoint in commit_iteration before first possible return on
>    error
> - renamed the handle_error label in commit_iteration to fail and
>    prevented the happy path from passing through this label
> - moved test script to the tests/qemu-iotests/tests folder and named it
>    commit-zero-blocks
> 
> Changes since v2:
> - moved main loop of commit_run to a separate function and refactored
>    the error handling.
> - call blk_co_pwrite_zero even if the size of the zero region does not
>    align with the sectors of the base image. This removes the need for
>    the CommitMethod enum
> 
> Changes since v1:
> - split up the implementation in three separate commits
> - removed accidentally left over includes from testing
> 
> Vincent Vanlaer (5):
>    block: get type of block allocation in commit_run
>    block: move commit_run loop to separate function
>    block: refactor error handling of commit_iteration
>    block: allow commit to unmap zero blocks
>    block: add test non-active commit with zeroed data
> 
>   block/commit.c                                | 116 +++++++++++++-----
>   tests/qemu-iotests/tests/commit-zero-blocks   |  96 +++++++++++++++
>   .../qemu-iotests/tests/commit-zero-blocks.out |  54 ++++++++
>   3 files changed, 232 insertions(+), 34 deletions(-)
>   create mode 100755 tests/qemu-iotests/tests/commit-zero-blocks
>   create mode 100644 tests/qemu-iotests/tests/commit-zero-blocks.out
> 

Thanks, applied to my block branch,

with

     diff --git a/block/commit.c b/block/commit.c
     index 5c24c8b80a..bfd969b13f 100644
     --- a/block/commit.c
     +++ b/block/commit.c
     @@ -172,7 +172,8 @@ static int commit_iteration(CommitBlockJob *s, int64_t offset,
          *requested_bytes = bytes;

          return 0;
     -fail:;
     +
     +fail:
          BlockErrorAction action = block_job_error_action(&s->common, s->on_error,
                                                           error_in_source, -ret);
          if (action == BLOCK_ERROR_ACTION_REPORT) {

change to patch 3.


Unfortunately, I've missed soft-freeze on 05.11. Will send PR when 10.0 development phase opens.

-- 
Best regards,
Vladimir