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

[Vladimir Sementsov-Ogievskiy]

09.08.2019 10:50, Vladimir Sementsov-Ogievskiy wrote:
> 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.

oops, and seek lack the limit anyway.

But anyway, we have API with count limit and it seems natural to assume that 
some
driver may do more calculations for bigger request. So smaller request is good 
for
copy-before-write operation when we are in a harry to unfreeze guest write as 
soon
as possible.

Hmm, example of such drive may be NBD, which can concatenate block-status 
results of
exported disk.


> 
>>
>> 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 <address@hidden>
>>> ---
>>>   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