| Message ID | 20240526192918.3503128-2-libvirt-e6954efa@volkihar.be (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block: allow commit to unmap zero blocks | expand |
On 26.05.24 22:29, 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 oother arbitrary data. Hi Vincent! Sorry for long delay. Could you please split the commit into simpler parts? Something like this: 1. refactor to use direct bdrv_co_common_block_status_above() 2. refactor to use CommitMethod (with two possible modes) 3. add new mode (The idea is to split parts of no-logic-change refactoring from real logic changes and keep the latter smaller and clearer) > > Signed-off-by: Vincent Vanlaer <libvirt-e6954efa@volkihar.be> > --- > block/commit.c | 71 +++++++++++++++++++++++++++++++++++++++++--------- > 1 file changed, 58 insertions(+), 13 deletions(-) > > diff --git a/block/commit.c b/block/commit.c > index 7c3fdcb0ca..5bd97b5a74 100644 > --- a/block/commit.c > +++ b/block/commit.c > @@ -12,9 +12,13 @@ > * > */ > > +#include "bits/time.h" > #include "qemu/osdep.h" > #include "qemu/cutils.h" > +#include "time.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" > @@ -126,6 +130,12 @@ static void commit_clean(Job *job) > blk_unref(s->top); > } > > +typedef enum CommitMethod { > + COMMIT_METHOD_COPY, > + COMMIT_METHOD_ZERO, > + COMMIT_METHOD_IGNORE, > +} CommitMethod; > + > static int coroutine_fn commit_run(Job *job, Error **errp) > { > CommitBlockJob *s = container_of(job, CommitBlockJob, common.job); > @@ -156,8 +166,9 @@ 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; > + CommitMethod commit_method = COMMIT_METHOD_COPY; > + > > /* 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. > @@ -167,21 +178,54 @@ 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); > + } > + > 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 && !(ret & BDRV_BLOCK_ALLOCATED)) { > + commit_method = COMMIT_METHOD_IGNORE; > + } else if (ret >= 0 && 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; > + } > + } > + > + if (ret >= 0) { > + switch (commit_method) { > + case COMMIT_METHOD_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; > + } > } > + 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: > + abort(); > } > } > + > if (ret < 0) { > BlockErrorAction action = > block_job_error_action(&s->common, s->on_error, > @@ -193,10 +237,11 @@ static int coroutine_fn commit_run(Job *job, Error **errp) > continue; > } > } > + > /* Publish progress */ > job_progress_update(&s->common.job, n); > > - if (copy) { > + if (commit_method == COMMIT_METHOD_COPY) { > block_job_ratelimit_processed_bytes(&s->common, n); > } > }
diff --git a/block/commit.c b/block/commit.c index 7c3fdcb0ca..5bd97b5a74 100644 --- a/block/commit.c +++ b/block/commit.c @@ -12,9 +12,13 @@ * */ +#include "bits/time.h" #include "qemu/osdep.h" #include "qemu/cutils.h" +#include "time.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" @@ -126,6 +130,12 @@ static void commit_clean(Job *job) blk_unref(s->top); } +typedef enum CommitMethod { + COMMIT_METHOD_COPY, + COMMIT_METHOD_ZERO, + COMMIT_METHOD_IGNORE, +} CommitMethod; + static int coroutine_fn commit_run(Job *job, Error **errp) { CommitBlockJob *s = container_of(job, CommitBlockJob, common.job); @@ -156,8 +166,9 @@ 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; + CommitMethod commit_method = COMMIT_METHOD_COPY; + /* 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. @@ -167,21 +178,54 @@ 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); + } + 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 && !(ret & BDRV_BLOCK_ALLOCATED)) { + commit_method = COMMIT_METHOD_IGNORE; + } else if (ret >= 0 && 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; + } + } + + if (ret >= 0) { + switch (commit_method) { + case COMMIT_METHOD_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; + } } + 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: + abort(); } } + if (ret < 0) { BlockErrorAction action = block_job_error_action(&s->common, s->on_error, @@ -193,10 +237,11 @@ static int coroutine_fn commit_run(Job *job, Error **errp) continue; } } + /* Publish progress */ job_progress_update(&s->common.job, n); - if (copy) { + if (commit_method == COMMIT_METHOD_COPY) { block_job_ratelimit_processed_bytes(&s->common, 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 oother arbitrary data. Signed-off-by: Vincent Vanlaer <libvirt-e6954efa@volkihar.be> --- block/commit.c | 71 +++++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 58 insertions(+), 13 deletions(-)