Re: [Qemu-devel] [PATCH 4/8] block/backup: improve unallocated clusters skipping

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

07.08.2019 21:01, Max Reitz wrote:
> On 07.08.19 10:07, Vladimir Sementsov-Ogievskiy wrote:
>> Limit block_status querying to request bounds on write notifier to
>> avoid extra seeking.
> 
> I don’t understand this reasoning.  Checking whether something is
> allocated for qcow2 should just mean an L2 cache lookup.  Which we have
> to do anyway when we try to copy data off the source.

But for raw it's seeking. I think we just shouldn't do any unnecessary operations
in copy-before-write handling. So instead of calling
block_status(request_start, disk_end) I think it's better to call
block_status(request_start, request_end). And it is more applicable for reusing this
code too.

> 
> I could understand saying this makes the code easier, but I actually
> don’t think it does.  If you implemented checking the allocation status
> only for areas where the bitmap is reset (which I think this patch
> should), then it’d just duplicate the existing loop.
> 
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   block/backup.c | 38 +++++++++++++++++++++-----------------
>>   1 file changed, 21 insertions(+), 17 deletions(-)
>>
>> diff --git a/block/backup.c b/block/backup.c
>> index 11e27c844d..a4d37d2d62 100644
>> --- a/block/backup.c
>> +++ b/block/backup.c
> 
> [...]
> 
>> @@ -267,6 +267,18 @@ static int coroutine_fn backup_do_cow(BackupBlockJob *job,
>>       wait_for_overlapping_requests(job, start, end);
>>       cow_request_begin(&cow_request, job, start, end);
>>   
>> +    if (job->initializing_bitmap) {
>> +        int64_t off, chunk;
>> +
>> +        for (off = offset; offset < end; offset += chunk) {
> 
> This is what I’m referring to, I think this loop should skip areas that
> are clean.

Agree, will do.

> 
>> +            ret = backup_bitmap_reset_unallocated(job, off, end - off, &chunk);
>> +            if (ret < 0) {
>> +                chunk = job->cluster_size;
>> +            }
>> +        }
>> +    }
>> +    ret = 0;
>> +
>>       while (start < end) {
>>           int64_t dirty_end;
>>   
>> @@ -276,15 +288,6 @@ static int coroutine_fn backup_do_cow(BackupBlockJob *job,
>>               continue; /* already copied */
>>           }
>>   
>> -        if (job->initializing_bitmap) {
>> -            ret = backup_bitmap_reset_unallocated(job, start, &skip_bytes);
>> -            if (ret == 0) {
>> -                trace_backup_do_cow_skip_range(job, start, skip_bytes);
>> -                start += skip_bytes;
>> -                continue;
>> -            }
>> -        }
>> -
>>           dirty_end = bdrv_dirty_bitmap_next_zero(job->copy_bitmap, start,
>>                                                   end - start);
>>           if (dirty_end < 0) {
> 
> Hm, you resolved that conflict differently from me.
> 
> I decided the bdrv_dirty_bitmap_next_zero() call should go before the
> backup_bitmap_reset_unallocated() call so that we can then do a
> 
>    dirty_end = MIN(dirty_end, start + skip_bytes);
> 
> because we probably don’t want to copy anything past what
> backup_bitmap_reset_unallocated() has inquired.
> 
> 
> But then again this patch eliminates the difference anyway...
>  >
>> @@ -546,7 +549,8 @@ static int coroutine_fn backup_run(Job *job, Error **errp)
>>                   goto out;
>>               }
>>   
>> -            ret = backup_bitmap_reset_unallocated(s, offset, &count);
>> +            ret = backup_bitmap_reset_unallocated(s, offset, s->len - offset,
>> +                                                  &count);
>>               if (ret < 0) {
>>                   goto out;
>>               }
>>
> 
> 


-- 
Best regards,
Vladimir