| Message ID | 20200601181118.579-9-vsementsov@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
| Series | backup performance: block_status + async | expand |
22.07.2020 14:28, Max Reitz wrote: > On 01.06.20 20:11, Vladimir Sementsov-Ogievskiy wrote: >> Add function to cancel running async block-copy call. It will be used >> in backup. >> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> --- >> include/block/block-copy.h | 7 +++++++ >> block/block-copy.c | 22 +++++++++++++++++++--- >> 2 files changed, 26 insertions(+), 3 deletions(-) >> >> diff --git a/include/block/block-copy.h b/include/block/block-copy.h >> index d40e691123..370a194d3c 100644 >> --- a/include/block/block-copy.h >> +++ b/include/block/block-copy.h >> @@ -67,6 +67,13 @@ BlockCopyCallState *block_copy_async(BlockCopyState *s, >> void block_copy_set_speed(BlockCopyState *s, BlockCopyCallState *call_state, >> uint64_t speed); >> >> +/* >> + * Cancel running block-copy call. >> + * Cancel leaves block-copy state valid: dirty bits are correct and you may use >> + * cancel + <run block_copy with same parameters> to emulate pause/resume. >> + */ >> +void block_copy_cancel(BlockCopyCallState *call_state); >> + >> 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.c >> index 851d9c8aaf..b551feb6c2 100644 >> --- a/block/block-copy.c >> +++ b/block/block-copy.c >> @@ -44,6 +44,8 @@ typedef struct BlockCopyCallState { >> bool failed; >> bool finished; >> QemuCoSleepState *sleep_state; >> + bool cancelled; >> + Coroutine *canceller; >> >> /* OUT parameters */ >> bool error_is_read; >> @@ -582,7 +584,7 @@ block_copy_dirty_clusters(BlockCopyCallState *call_state) >> assert(QEMU_IS_ALIGNED(offset, s->cluster_size)); >> assert(QEMU_IS_ALIGNED(bytes, s->cluster_size)); >> >> - while (bytes && aio_task_pool_status(aio) == 0) { >> + while (bytes && aio_task_pool_status(aio) == 0 && !call_state->cancelled) { >> BlockCopyTask *task; >> int64_t status_bytes; >> >> @@ -693,7 +695,7 @@ static int coroutine_fn block_copy_common(BlockCopyCallState *call_state) >> do { >> ret = block_copy_dirty_clusters(call_state); >> >> - if (ret == 0) { >> + if (ret == 0 && !call_state->cancelled) { >> ret = block_copy_wait_one(call_state->s, call_state->offset, >> call_state->bytes); >> } >> @@ -707,13 +709,18 @@ static int coroutine_fn block_copy_common(BlockCopyCallState *call_state) >> * 2. We have waited for some intersecting block-copy request >> * It may have failed and produced new dirty bits. >> */ >> - } while (ret > 0); >> + } while (ret > 0 && !call_state->cancelled); > > Would it be cleaner if block_copy_dirty_cluster() just returned > -ECANCELED? Or would that pose a problem for its callers or the async > callback? > I'd prefer not to merge io ret with block-copy logic: who knows what underlying operations may return.. Can't it be _another_ ECANCELED? And it would be just a sugar for block_copy_dirty_clusters() call, I'll have to check ->cancelled after block_copy_wait_one() anyway. Also, for the next version I try to make it more obvious that finished block-copy call is in one of thee states: - success - failed - cancelled Hmm. Also, cancelled should be OK for copy-on-write operations in filter, it just mean that we don't need to care anymore. >> if (call_state->cb) { >> call_state->cb(ret, call_state->error_is_read, >> call_state->s->progress_opaque); >> } >> >> + if (call_state->canceller) { >> + aio_co_wake(call_state->canceller); >> + call_state->canceller = NULL; >> + } >> + >> call_state->finished = true; >> >> return ret; >
22.10.2020 23:50, Vladimir Sementsov-Ogievskiy wrote: > 22.07.2020 14:28, Max Reitz wrote: >> On 01.06.20 20:11, Vladimir Sementsov-Ogievskiy wrote: >>> Add function to cancel running async block-copy call. It will be used >>> in backup. >>> >>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >>> --- >>> include/block/block-copy.h | 7 +++++++ >>> block/block-copy.c | 22 +++++++++++++++++++--- >>> 2 files changed, 26 insertions(+), 3 deletions(-) >>> >>> diff --git a/include/block/block-copy.h b/include/block/block-copy.h >>> index d40e691123..370a194d3c 100644 >>> --- a/include/block/block-copy.h >>> +++ b/include/block/block-copy.h >>> @@ -67,6 +67,13 @@ BlockCopyCallState *block_copy_async(BlockCopyState *s, >>> void block_copy_set_speed(BlockCopyState *s, BlockCopyCallState *call_state, >>> uint64_t speed); >>> +/* >>> + * Cancel running block-copy call. >>> + * Cancel leaves block-copy state valid: dirty bits are correct and you may use >>> + * cancel + <run block_copy with same parameters> to emulate pause/resume. >>> + */ >>> +void block_copy_cancel(BlockCopyCallState *call_state); >>> + >>> 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.c >>> index 851d9c8aaf..b551feb6c2 100644 >>> --- a/block/block-copy.c >>> +++ b/block/block-copy.c >>> @@ -44,6 +44,8 @@ typedef struct BlockCopyCallState { >>> bool failed; >>> bool finished; >>> QemuCoSleepState *sleep_state; >>> + bool cancelled; >>> + Coroutine *canceller; >>> /* OUT parameters */ >>> bool error_is_read; >>> @@ -582,7 +584,7 @@ block_copy_dirty_clusters(BlockCopyCallState *call_state) >>> assert(QEMU_IS_ALIGNED(offset, s->cluster_size)); >>> assert(QEMU_IS_ALIGNED(bytes, s->cluster_size)); >>> - while (bytes && aio_task_pool_status(aio) == 0) { >>> + while (bytes && aio_task_pool_status(aio) == 0 && !call_state->cancelled) { >>> BlockCopyTask *task; >>> int64_t status_bytes; >>> @@ -693,7 +695,7 @@ static int coroutine_fn block_copy_common(BlockCopyCallState *call_state) >>> do { >>> ret = block_copy_dirty_clusters(call_state); >>> - if (ret == 0) { >>> + if (ret == 0 && !call_state->cancelled) { >>> ret = block_copy_wait_one(call_state->s, call_state->offset, >>> call_state->bytes); >>> } >>> @@ -707,13 +709,18 @@ static int coroutine_fn block_copy_common(BlockCopyCallState *call_state) >>> * 2. We have waited for some intersecting block-copy request >>> * It may have failed and produced new dirty bits. >>> */ >>> - } while (ret > 0); >>> + } while (ret > 0 && !call_state->cancelled); >> >> Would it be cleaner if block_copy_dirty_cluster() just returned >> -ECANCELED? Or would that pose a problem for its callers or the async >> callback? >> > > I'd prefer not to merge io ret with block-copy logic: who knows what underlying operations may return.. Can't it be _another_ ECANCELED? > And it would be just a sugar for block_copy_dirty_clusters() call, I'll have to check ->cancelled after block_copy_wait_one() anyway. > Also, for the next version I try to make it more obvious that finished block-copy call is in one of thee states: > - success > - failed > - cancelled > > Hmm. Also, cancelled should be OK for copy-on-write operations in filter, it just mean that we don't need to care anymore. This is unrelated: actually only async block-copy call may be cancelled. > >>> if (call_state->cb) { >>> call_state->cb(ret, call_state->error_is_read, >>> call_state->s->progress_opaque); >>> } >>> + if (call_state->canceller) { >>> + aio_co_wake(call_state->canceller); >>> + call_state->canceller = NULL; >>> + } >>> + >>> call_state->finished = true; >>> return ret; >> > > -- Best regards, Vladimir
On 22.10.20 22:50, Vladimir Sementsov-Ogievskiy wrote: > 22.07.2020 14:28, Max Reitz wrote: >> On 01.06.20 20:11, Vladimir Sementsov-Ogievskiy wrote: >>> Add function to cancel running async block-copy call. It will be used >>> in backup. >>> >>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >>> --- >>> include/block/block-copy.h | 7 +++++++ >>> block/block-copy.c | 22 +++++++++++++++++++--- >>> 2 files changed, 26 insertions(+), 3 deletions(-) >>> >>> diff --git a/include/block/block-copy.h b/include/block/block-copy.h >>> index d40e691123..370a194d3c 100644 >>> --- a/include/block/block-copy.h >>> +++ b/include/block/block-copy.h >>> @@ -67,6 +67,13 @@ BlockCopyCallState >>> *block_copy_async(BlockCopyState *s, >>> void block_copy_set_speed(BlockCopyState *s, BlockCopyCallState >>> *call_state, >>> uint64_t speed); >>> +/* >>> + * Cancel running block-copy call. >>> + * Cancel leaves block-copy state valid: dirty bits are correct and >>> you may use >>> + * cancel + <run block_copy with same parameters> to emulate >>> pause/resume. >>> + */ >>> +void block_copy_cancel(BlockCopyCallState *call_state); >>> + >>> 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.c >>> index 851d9c8aaf..b551feb6c2 100644 >>> --- a/block/block-copy.c >>> +++ b/block/block-copy.c >>> @@ -44,6 +44,8 @@ typedef struct BlockCopyCallState { >>> bool failed; >>> bool finished; >>> QemuCoSleepState *sleep_state; >>> + bool cancelled; >>> + Coroutine *canceller; >>> /* OUT parameters */ >>> bool error_is_read; >>> @@ -582,7 +584,7 @@ block_copy_dirty_clusters(BlockCopyCallState >>> *call_state) >>> assert(QEMU_IS_ALIGNED(offset, s->cluster_size)); >>> assert(QEMU_IS_ALIGNED(bytes, s->cluster_size)); >>> - while (bytes && aio_task_pool_status(aio) == 0) { >>> + while (bytes && aio_task_pool_status(aio) == 0 && >>> !call_state->cancelled) { >>> BlockCopyTask *task; >>> int64_t status_bytes; >>> @@ -693,7 +695,7 @@ static int coroutine_fn >>> block_copy_common(BlockCopyCallState *call_state) >>> do { >>> ret = block_copy_dirty_clusters(call_state); >>> - if (ret == 0) { >>> + if (ret == 0 && !call_state->cancelled) { >>> ret = block_copy_wait_one(call_state->s, >>> call_state->offset, >>> call_state->bytes); >>> } >>> @@ -707,13 +709,18 @@ static int coroutine_fn >>> block_copy_common(BlockCopyCallState *call_state) >>> * 2. We have waited for some intersecting block-copy request >>> * It may have failed and produced new dirty bits. >>> */ >>> - } while (ret > 0); >>> + } while (ret > 0 && !call_state->cancelled); >> >> Would it be cleaner if block_copy_dirty_cluster() just returned >> -ECANCELED? Or would that pose a problem for its callers or the async >> callback? >> > > I'd prefer not to merge io ret with block-copy logic: who knows what > underlying operations may return.. Can't it be _another_ ECANCELED? > And it would be just a sugar for block_copy_dirty_clusters() call, I'll > have to check ->cancelled after block_copy_wait_one() anyway. > Also, for the next version I try to make it more obvious that finished > block-copy call is in one of thee states: > - success > - failed > - cancelled OK. Max
diff --git a/include/block/block-copy.h b/include/block/block-copy.h index d40e691123..370a194d3c 100644 --- a/include/block/block-copy.h +++ b/include/block/block-copy.h @@ -67,6 +67,13 @@ BlockCopyCallState *block_copy_async(BlockCopyState *s, void block_copy_set_speed(BlockCopyState *s, BlockCopyCallState *call_state, uint64_t speed); +/* + * Cancel running block-copy call. + * Cancel leaves block-copy state valid: dirty bits are correct and you may use + * cancel + <run block_copy with same parameters> to emulate pause/resume. + */ +void block_copy_cancel(BlockCopyCallState *call_state); + 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.c index 851d9c8aaf..b551feb6c2 100644 --- a/block/block-copy.c +++ b/block/block-copy.c @@ -44,6 +44,8 @@ typedef struct BlockCopyCallState { bool failed; bool finished; QemuCoSleepState *sleep_state; + bool cancelled; + Coroutine *canceller; /* OUT parameters */ bool error_is_read; @@ -582,7 +584,7 @@ block_copy_dirty_clusters(BlockCopyCallState *call_state) assert(QEMU_IS_ALIGNED(offset, s->cluster_size)); assert(QEMU_IS_ALIGNED(bytes, s->cluster_size)); - while (bytes && aio_task_pool_status(aio) == 0) { + while (bytes && aio_task_pool_status(aio) == 0 && !call_state->cancelled) { BlockCopyTask *task; int64_t status_bytes; @@ -693,7 +695,7 @@ static int coroutine_fn block_copy_common(BlockCopyCallState *call_state) do { ret = block_copy_dirty_clusters(call_state); - if (ret == 0) { + if (ret == 0 && !call_state->cancelled) { ret = block_copy_wait_one(call_state->s, call_state->offset, call_state->bytes); } @@ -707,13 +709,18 @@ static int coroutine_fn block_copy_common(BlockCopyCallState *call_state) * 2. We have waited for some intersecting block-copy request * It may have failed and produced new dirty bits. */ - } while (ret > 0); + } while (ret > 0 && !call_state->cancelled); if (call_state->cb) { call_state->cb(ret, call_state->error_is_read, call_state->s->progress_opaque); } + if (call_state->canceller) { + aio_co_wake(call_state->canceller); + call_state->canceller = NULL; + } + call_state->finished = true; return ret; @@ -772,6 +779,15 @@ BlockCopyCallState *block_copy_async(BlockCopyState *s, return call_state; } + +void block_copy_cancel(BlockCopyCallState *call_state) +{ + call_state->cancelled = true; + call_state->canceller = qemu_coroutine_self(); + block_copy_kick(call_state); + qemu_coroutine_yield(); +} + BdrvDirtyBitmap *block_copy_dirty_bitmap(BlockCopyState *s) { return s->copy_bitmap;
Add function to cancel running async block-copy call. It will be used in backup. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> --- include/block/block-copy.h | 7 +++++++ block/block-copy.c | 22 +++++++++++++++++++--- 2 files changed, 26 insertions(+), 3 deletions(-)