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

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2838 bytes --]

On 29.04.20 13:54, Vladimir Sementsov-Ogievskiy wrote:
> 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 <vsementsov@virtuozzo.com>
>>> ---
>>>   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?

Oh, it absolutely is and personally I’m absolutely not against it, but
CODING_STYLE says:

> Mixed declarations (interleaving statements and declarations within
> blocks) are generally not allowed; declarations should be at the beginning                                                                                                                                                                   
> of blocks.

Max

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



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]