* [PATCH v4 1/5] block: get type of block allocation in commit_run
2024-10-26 16:30 [PATCH v4 0/5] block: allow commit to unmap zero blocks Vincent Vanlaer
@ 2024-10-26 16:30 ` Vincent Vanlaer
2024-10-26 16:30 ` [PATCH v4 2/5] block: move commit_run loop to separate function Vincent Vanlaer
` (4 subsequent siblings)
5 siblings, 0 replies; 9+ messages in thread
From: Vincent Vanlaer @ 2024-10-26 16:30 UTC (permalink / raw)
To: qemu-devel
Cc: John Snow, Vladimir Sementsov-Ogievskiy, Kevin Wolf, qemu-block,
Hanna Reitz, 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
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH v4 2/5] block: move commit_run loop to separate function
2024-10-26 16:30 [PATCH v4 0/5] block: allow commit to unmap zero blocks Vincent Vanlaer
2024-10-26 16:30 ` [PATCH v4 1/5] block: get type of block allocation in commit_run Vincent Vanlaer
@ 2024-10-26 16:30 ` Vincent Vanlaer
2024-10-26 16:30 ` [PATCH v4 3/5] block: refactor error handling of commit_iteration Vincent Vanlaer
` (3 subsequent siblings)
5 siblings, 0 replies; 9+ messages in thread
From: Vincent Vanlaer @ 2024-10-26 16:30 UTC (permalink / raw)
To: qemu-devel
Cc: John Snow, Vladimir Sementsov-Ogievskiy, Kevin Wolf, qemu-block,
Hanna Reitz, 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
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH v4 3/5] block: refactor error handling of commit_iteration
2024-10-26 16:30 [PATCH v4 0/5] block: allow commit to unmap zero blocks Vincent Vanlaer
2024-10-26 16:30 ` [PATCH v4 1/5] block: get type of block allocation in commit_run Vincent Vanlaer
2024-10-26 16:30 ` [PATCH v4 2/5] block: move commit_run loop to separate function Vincent Vanlaer
@ 2024-10-26 16:30 ` Vincent Vanlaer
2024-11-18 7:37 ` Vladimir Sementsov-Ogievskiy
2024-10-26 16:30 ` [PATCH v4 4/5] block: allow commit to unmap zero blocks Vincent Vanlaer
` (2 subsequent siblings)
5 siblings, 1 reply; 9+ messages in thread
From: Vincent Vanlaer @ 2024-10-26 16:30 UTC (permalink / raw)
To: qemu-devel
Cc: John Snow, Vladimir Sementsov-Ogievskiy, Kevin Wolf, qemu-block,
Hanna Reitz, 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
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH v4 3/5] block: refactor error handling of commit_iteration
2024-10-26 16:30 ` [PATCH v4 3/5] block: refactor error handling of commit_iteration Vincent Vanlaer
@ 2024-11-18 7:37 ` Vladimir Sementsov-Ogievskiy
2024-11-18 7:47 ` Vladimir Sementsov-Ogievskiy
0 siblings, 1 reply; 9+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2024-11-18 7:37 UTC (permalink / raw)
To: Vincent Vanlaer, qemu-devel
Cc: John Snow, Kevin Wolf, qemu-block, Hanna Reitz
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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v4 3/5] block: refactor error handling of commit_iteration
2024-11-18 7:37 ` Vladimir Sementsov-Ogievskiy
@ 2024-11-18 7:47 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 9+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2024-11-18 7:47 UTC (permalink / raw)
To: Vincent Vanlaer, qemu-devel
Cc: John Snow, Kevin Wolf, qemu-block, Hanna Reitz
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v4 4/5] block: allow commit to unmap zero blocks
2024-10-26 16:30 [PATCH v4 0/5] block: allow commit to unmap zero blocks Vincent Vanlaer
` (2 preceding siblings ...)
2024-10-26 16:30 ` [PATCH v4 3/5] block: refactor error handling of commit_iteration Vincent Vanlaer
@ 2024-10-26 16:30 ` Vincent Vanlaer
2024-10-26 16:30 ` [PATCH v4 5/5] block: add test non-active commit with zeroed data Vincent Vanlaer
2024-11-18 7:57 ` [PATCH v4 0/5] block: allow commit to unmap zero blocks Vladimir Sementsov-Ogievskiy
5 siblings, 0 replies; 9+ messages in thread
From: Vincent Vanlaer @ 2024-10-26 16:30 UTC (permalink / raw)
To: qemu-devel
Cc: John Snow, Vladimir Sementsov-Ogievskiy, Kevin Wolf, qemu-block,
Hanna Reitz, 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
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH v4 5/5] block: add test non-active commit with zeroed data
2024-10-26 16:30 [PATCH v4 0/5] block: allow commit to unmap zero blocks Vincent Vanlaer
` (3 preceding siblings ...)
2024-10-26 16:30 ` [PATCH v4 4/5] block: allow commit to unmap zero blocks Vincent Vanlaer
@ 2024-10-26 16:30 ` Vincent Vanlaer
2024-11-18 7:57 ` [PATCH v4 0/5] block: allow commit to unmap zero blocks Vladimir Sementsov-Ogievskiy
5 siblings, 0 replies; 9+ messages in thread
From: Vincent Vanlaer @ 2024-10-26 16:30 UTC (permalink / raw)
To: qemu-devel
Cc: John Snow, Vladimir Sementsov-Ogievskiy, Kevin Wolf, qemu-block,
Hanna Reitz, 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
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH v4 0/5] block: allow commit to unmap zero blocks
2024-10-26 16:30 [PATCH v4 0/5] block: allow commit to unmap zero blocks Vincent Vanlaer
` (4 preceding siblings ...)
2024-10-26 16:30 ` [PATCH v4 5/5] block: add test non-active commit with zeroed data Vincent Vanlaer
@ 2024-11-18 7:57 ` Vladimir Sementsov-Ogievskiy
5 siblings, 0 replies; 9+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2024-11-18 7:57 UTC (permalink / raw)
To: Vincent Vanlaer, qemu-devel
Cc: John Snow, Kevin Wolf, qemu-block, Hanna Reitz
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
^ permalink raw reply [flat|nested] 9+ messages in thread