Re: [PATCH v2 05/20] block/block-copy: implement block_copy_async

[Vladimir Sementsov-Ogievskiy]

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.c
> > index 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