[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 3/6] block/block-copy: refactor copying
From: |
Max Reitz |
Subject: |
Re: [PATCH 3/6] block/block-copy: refactor copying |
Date: |
Mon, 7 Oct 2019 16:17:34 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.1.0 |
On 03.10.19 19:15, Vladimir Sementsov-Ogievskiy wrote:
> Merge copying code into one function block_copy_do_copy, which only
> calls bdrv_ io functions and don't do any synchronization (like dirty
> bitmap set/reset).
>
> Refactor block_copy() function so that it takes full decision about
> size of chunk to be copied and does all the synchronization (checking
> intersecting requests, set/reset dirty bitmaps).
>
> It will help:
> - introduce parallel processing of block_copy iterations: we need to
> calculate chunk size, start async chunk copying and go to the next
> iteration
> - simplify synchronization improvement (like memory limiting in
> further commit and reducing critical section (now we lock the whole
> requested range, when actually we need to lock only dirty region
> which we handle at the moment))
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> ---
> block/block-copy.c | 113 ++++++++++++++++++++-------------------------
> block/trace-events | 6 +--
> 2 files changed, 53 insertions(+), 66 deletions(-)
Looks good to me, just some clean-up path nit picks below.
> diff --git a/block/block-copy.c b/block/block-copy.c
> index 75287ce24d..cc49d2345d 100644
> --- a/block/block-copy.c
> +++ b/block/block-copy.c
> @@ -126,25 +126,43 @@ void block_copy_set_callbacks(
> }
>
> /*
> - * Copy range to target with a bounce buffer and return the bytes copied. If
> - * error occurred, return a negative error number
> + * block_copy_do_copy
> + *
> + * Do copy of cluser-aligned chunk. @end is allowed to exceed s->len only to
> + * cover last cluster when s->len is not aligned to clusters.
> + *
> + * No sync here: nor bitmap neighter intersecting requests handling, only
> copy.
> + *
> + * Returns 0 on success.
> */
> -static int coroutine_fn block_copy_with_bounce_buffer(BlockCopyState *s,
> - int64_t start,
> - int64_t end,
> - bool *error_is_read)
> +static int coroutine_fn block_copy_do_copy(BlockCopyState *s,
> + int64_t start, int64_t end,
> + bool *error_is_read)
> {
> int ret;
> - int nbytes;
> - void *bounce_buffer = qemu_blockalign(s->source->bs, s->cluster_size);
> + int nbytes = MIN(end, s->len) - start;
> + void *bounce_buffer = NULL;
>
> assert(QEMU_IS_ALIGNED(start, s->cluster_size));
> - bdrv_reset_dirty_bitmap(s->copy_bitmap, start, s->cluster_size);
> - nbytes = MIN(s->cluster_size, s->len - start);
> + assert(QEMU_IS_ALIGNED(end, s->cluster_size));
> + assert(end < s->len || end == QEMU_ALIGN_UP(s->len, s->cluster_size));
> +
> + if (s->use_copy_range) {
> + ret = bdrv_co_copy_range(s->source, start, s->target, start, nbytes,
> + 0, s->write_flags);
> + if (ret < 0) {
> + trace_block_copy_copy_range_fail(s, start, ret);
> + s->use_copy_range = false;
> + } else {
> + return ret;
Maybe the “fail” label should be called ”out” and then we could go there
from here. Doesn’t make much of a difference here (1 LoC either way),
but maybe it’s a bit cleaner to always use the clean-up path in this
function (even when there’s nothing to clean up).
*shrug*
> + }
> + }
> +
> + bounce_buffer = qemu_blockalign(s->source->bs, nbytes);
>
> ret = bdrv_co_pread(s->source, start, nbytes, bounce_buffer, 0);
> if (ret < 0) {
> - trace_block_copy_with_bounce_buffer_read_fail(s, start, ret);
> + trace_block_copy_read_fail(s, start, ret);
> if (error_is_read) {
> *error_is_read = true;
> }
[...]
> @@ -163,42 +181,12 @@ static int coroutine_fn
> block_copy_with_bounce_buffer(BlockCopyState *s,
>
> qemu_vfree(bounce_buffer);
>
> - return nbytes;
> + return 0;
> +
> fail:
> qemu_vfree(bounce_buffer);
> - bdrv_set_dirty_bitmap(s->copy_bitmap, start, s->cluster_size);
> - return ret;
> -
> -}
Wouldn’t it be simpler to drop the “qemu_vfree(bounce_buffer); return
0;” above the fail label and just fall through?
In any case:
Reviewed-by: Max Reitz <address@hidden>
signature.asc
Description: OpenPGP digital signature
- [PATCH 6/6] block/block-copy: increase buffered copy request, (continued)
- [PATCH 6/6] block/block-copy: increase buffered copy request, Vladimir Sementsov-Ogievskiy, 2019/10/03
- [PATCH 4/6] util: introduce co-shared-amount, Vladimir Sementsov-Ogievskiy, 2019/10/03
- [PATCH 2/6] block/block-copy: limit copy_range_size to 16 MiB, Vladimir Sementsov-Ogievskiy, 2019/10/03
- [PATCH 3/6] block/block-copy: refactor copying, Vladimir Sementsov-Ogievskiy, 2019/10/03
- Re: [PATCH 3/6] block/block-copy: refactor copying,
Max Reitz <=
- [PATCH 1/6] block/block-copy: allocate buffer in block_copy_with_bounce_buffer, Vladimir Sementsov-Ogievskiy, 2019/10/03
- [PATCH 5/6] block/block-copy: add memory limit, Vladimir Sementsov-Ogievskiy, 2019/10/03
- Re: [PATCH 5/6] block/block-copy: add memory limit, Max Reitz, 2019/10/07
- Re: [PATCH 5/6] block/block-copy: add memory limit, Vladimir Sementsov-Ogievskiy, 2019/10/07
- Re: [PATCH 5/6] block/block-copy: add memory limit, Max Reitz, 2019/10/08
- Re: [PATCH 5/6] block/block-copy: add memory limit, Vladimir Sementsov-Ogievskiy, 2019/10/08
- Re: [PATCH 5/6] block/block-copy: add memory limit, Vladimir Sementsov-Ogievskiy, 2019/10/08
- Re: [PATCH 5/6] block/block-copy: add memory limit, Max Reitz, 2019/10/08