Re: [PATCH v2 08/20] block/block-copy: add block_copy_cancel

[Vladimir Sementsov-Ogievskiy]

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