On 2/08/2024 12:58, Vladimir Sementsov-Ogievskiy wrote:
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.
This originates from the mirroring code. I did some testing and it
indeed is not necessary in this case. Letting it the underlying code
handle it also simplifies this code quite a bit.
+ }
}
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);
