Re: [PATCH v3 4/5] block/block-copy: refactor task creation

[Vladimir Sementsov-Ogievskiy]

29.04.2020 14:38, Max Reitz wrote:
> On 29.04.20 08:10, Vladimir Sementsov-Ogievskiy wrote:
> > Instead of just relying on the comment "Called only on full-dirty
> > region" in block_copy_task_create() let's move initial dirty area
> > search directly to block_copy_task_create(). Let's also use effective
> > bdrv_dirty_bitmap_next_dirty_area instead of looping through all
> > non-dirty clusters.
> >
> > Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> > ---
> >   block/block-copy.c | 78 ++++++++++++++++++++++++++--------------------
> >   1 file changed, 44 insertions(+), 34 deletions(-)
> >
> > diff --git a/block/block-copy.c b/block/block-copy.c
> > index 35ff9cc3ef..5cf032c4d8 100644
> > --- a/block/block-copy.c
> > +++ b/block/block-copy.c
>
> [...]
>
> > @@ -106,17 +111,27 @@ static bool coroutine_fn 
> > block_copy_wait_one(BlockCopyState *s, int64_t offset,
> >       return true;
> >   }
> >   
> > -/* Called only on full-dirty region */
> > +/*
> > + * Search for the first dirty area in offset/bytes range and create task at
> > + * the beginning of it.
>
> Oh, that’s even better.
>
> > + */
> >   static BlockCopyTask *block_copy_task_create(BlockCopyState *s,
> >                                                int64_t offset, int64_t bytes)
> >   {
> > -    BlockCopyTask *task = g_new(BlockCopyTask, 1);
> > +    if (!bdrv_dirty_bitmap_next_dirty_area(s->copy_bitmap,
> > +                                           offset, offset + bytes,
> > +                                           s->copy_size, &offset, &bytes))
> > +    {
> > +        return NULL;
> > +    }
> >   
> > +    /* region is dirty, so no existent tasks possible in it */
> >       assert(!find_conflicting_task(s, offset, bytes));
> >   
> >       bdrv_reset_dirty_bitmap(s->copy_bitmap, offset, bytes);
> >       s->in_flight_bytes += bytes;
> >   
> > +    BlockCopyTask *task = g_new(BlockCopyTask, 1);
>
> This should be declared at the top of the function.

I just thought, why not to try another style? Are you against? Requirement to 
declare variables at start of block is obsolete, isn't it?

> Reviewed-by: Max Reitz <address@hidden>
>
> >       *task = (BlockCopyTask) {
> >           .s = s,
> >           .offset = offset,


--
Best regards,
Vladimir