| Message ID | 20240713215644.742244-4-libvirt-e6954efa@volkihar.be |
|---|---|
| State | New |
| Headers | show |
| Series | block: allow commit to unmap zero blocks | expand |
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. > + } > } > > 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); >
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); >
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);
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(+)