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 <vsement...@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