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>
---
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);
+ }
+
+ if (target_offset == offset &&
+ target_bytes == n) {
+ commit_method = COMMIT_METHOD_ZERO;
+ }
}
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;
}
}
+
/* Publish progress */
job_progress_update(&s->common.job, n);
--
2.44.1On 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-e6954efa@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
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-e6954efa@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);
>
© 2016 - 2024 Red Hat, Inc.