Re: [PATCH v2 2/7] block/block-copy: use block_status

[Max Reitz <mreitz@redhat.com>]

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

On 08.02.20 13:25, Vladimir Sementsov-Ogievskiy wrote:
> 07.02.2020 20:46, Max Reitz wrote:
>> On 27.11.19 19:08, Vladimir Sementsov-Ogievskiy wrote:
>>> Use bdrv_block_status_above to chose effective chunk size and to handle
>>> zeroes effectively.
>>>
>>> This substitutes checking for just being allocated or not, and drops
>>> old code path for it. Assistance by backup job is dropped too, as
>>> caching block-status information is more difficult than just caching
>>> is-allocated information in our dirty bitmap, and backup job is not
>>> good place for this caching anyway.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> ---
>>>   block/block-copy.c | 67 +++++++++++++++++++++++++++++++++++++---------
>>>   block/trace-events |  1 +
>>>   2 files changed, 55 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/block/block-copy.c b/block/block-copy.c
>>> index 8602e2cae7..74295d93d5 100644
>>> --- a/block/block-copy.c
>>> +++ b/block/block-copy.c
>>
>> [...]
>>
>>> @@ -336,23 +375,25 @@ int coroutine_fn block_copy(BlockCopyState *s,
>>>               chunk_end = next_zero;
>>>           }
>>>   -        if (s->skip_unallocated) {
>>> -            ret = block_copy_reset_unallocated(s, start,
>>> &status_bytes);
>>> -            if (ret == 0) {
>>> -                trace_block_copy_skip_range(s, start, status_bytes);
>>> -                start += status_bytes;
>>> -                continue;
>>> -            }
>>> -            /* Clamp to known allocated region */
>>> -            chunk_end = MIN(chunk_end, start + status_bytes);
>>> +        ret = block_copy_block_status(s, start, chunk_end - start,
>>> +                                      &status_bytes);
>>> +        if (s->skip_unallocated && !(ret & BDRV_BLOCK_ALLOCATED)) {
>>> +            bdrv_reset_dirty_bitmap(s->copy_bitmap, start,
>>> status_bytes);
>>> +            s->progress_reset_callback(s->progress_opaque);
>>> +            trace_block_copy_skip_range(s, start, status_bytes);
>>> +            start += status_bytes;
>>> +            continue;
>>>           }
>>>   +        chunk_end = MIN(chunk_end, start + status_bytes);
>>
>> I’m not sure how much the old “Clamp to known allocated region” actually
>> helps, but I wouldn’t drop it anyway.
> 
> But it may be not allocated now. We just clamp to status_bytes.
> It's "known allocated" only if s->skip_unallocated is true.

Ah, yes, I suppose I was just thinking about that case when trying to
understand how the code changes.  So:

Reviewed-by: Max Reitz <mreitz@redhat.com>

Max


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