|
From: | Vladimir Sementsov-Ogievskiy |
Subject: | Re: [PATCH v2 05/20] block/block-copy: implement block_copy_async |
Date: | Fri, 17 Jul 2020 18:24:43 +0300 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0 |
17.07.2020 17:00, Max Reitz wrote:
On 01.06.20 20:11, Vladimir Sementsov-Ogievskiy wrote:We'll need async block-copy invocation to use in backup directly. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> --- include/block/block-copy.h | 13 +++++++++++++ block/block-copy.c | 40 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 53 insertions(+) diff --git a/include/block/block-copy.h b/include/block/block-copy.h index 6397505f30..ada0d99566 100644 --- a/include/block/block-copy.h +++ b/include/block/block-copy.h @@ -19,7 +19,10 @@ #include "qemu/co-shared-resource.h"typedef void (*ProgressBytesCallbackFunc)(int64_t bytes, void *opaque);+typedef void (*BlockCopyAsyncCallbackFunc)(int ret, bool error_is_read, + void *opaque); typedef struct BlockCopyState BlockCopyState; +typedef struct BlockCopyCallState BlockCopyCallState;BlockCopyState *block_copy_state_new(BdrvChild *source, BdrvChild *target,int64_t cluster_size, bool use_copy_range, @@ -41,6 +44,16 @@ int64_t block_copy_reset_unallocated(BlockCopyState *s, int coroutine_fn block_copy(BlockCopyState *s, int64_t offset, int64_t bytes, bool *error_is_read);+/*+ * Run block-copy in a coroutine, return state pointer. If finished early + * returns NULL (@cb is called anyway).Any special reason for doing so? Seems like the code would be a tad simpler if we just returned it either way. (And off the top of my head I’d guess it’d be easier for the caller if the returned value was always non-NULL, too.)
Sounds reasonable, will check
+ */ +BlockCopyCallState *block_copy_async(BlockCopyState *s, + int64_t offset, int64_t bytes, + bool ratelimit, int max_workers, + int64_t max_chunk, + BlockCopyAsyncCallbackFunc cb); + BdrvDirtyBitmap *block_copy_dirty_bitmap(BlockCopyState *s); void block_copy_set_skip_unallocated(BlockCopyState *s, bool skip);diff --git a/block/block-copy.c b/block/block-copy.cindex 75882a094c..a0477d90f3 100644 --- a/block/block-copy.c +++ b/block/block-copy.c @@ -34,9 +34,11 @@ typedef struct BlockCopyCallState { BlockCopyState *s; int64_t offset; int64_t bytes; + BlockCopyAsyncCallbackFunc cb;/* State */bool failed; + bool finished;/* OUT parameters */bool error_is_read; @@ -676,6 +678,13 @@ static int coroutine_fn block_copy_common(BlockCopyCallState *call_state) */ } while (ret > 0);+ if (call_state->cb) {+ call_state->cb(ret, call_state->error_is_read, + call_state->s->progress_opaque);I find it weird to pass progress_opaque here. Shouldn’t we just have a dedicated opaque object for this CB?
I remember, it should be refactored later. But seems strange here, better to change.
+ } + + call_state->finished = true; + return ret; }@@ -697,6 +706,37 @@ int coroutine_fn block_copy(BlockCopyState *s, int64_t start, int64_t bytes,return ret; }+static void coroutine_fn block_copy_async_co_entry(void *opaque)+{ + block_copy_common(opaque); +} + +BlockCopyCallState *block_copy_async(BlockCopyState *s, + int64_t offset, int64_t bytes, + bool ratelimit, int max_workers, + int64_t max_chunk, + BlockCopyAsyncCallbackFunc cb) +{ + BlockCopyCallState *call_state = g_new(BlockCopyCallState, 1); + Coroutine *co = qemu_coroutine_create(block_copy_async_co_entry, + call_state); + + *call_state = (BlockCopyCallState) { + .s = s, + .offset = offset, + .bytes = bytes, + .cb = cb, + }; + + qemu_coroutine_enter(co);Do we need/want any already-in-coroutine shenanigans here?
No: the aim of the function is to start a new coroutine in parallel, independently of are we already in some other coroutine or not.
+ + if (call_state->finished) { + g_free(call_state); + return NULL; + } + + return call_state; +} BdrvDirtyBitmap *block_copy_dirty_bitmap(BlockCopyState *s) { return s->copy_bitmap;
-- Best regards, Vladimir
[Prev in Thread] | Current Thread | [Next in Thread] |