09.08.2019 12:12, Vladimir Sementsov-Ogievskiy wrote:
> 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.
>>
>>>
>>> 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 <vsement...@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.
>
> Hmm, I remembered that I thought of it, but decided that it may be not
> efficient if
> bitmap fragmentation is higher than block-status fragmentation. So, if we
> don't know
> what is better, let's keep simpler code.
Hmm2, but I see a compromise (may be you meant exactly this): not using next
zero as limit
(but always use request_end), but before each iteration skip to next_dirty.
>
>>
>>>
>>>> + 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
