Re: [Qemu-devel] [PATCH] block: don't probe zeroes in bs->file by default on block_status

[Vladimir Sementsov-Ogievskiy]

11.01.2019 16:15, Kevin Wolf wrote:
> Am 11.01.2019 um 13:59 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> 11.01.2019 15:21, Kevin Wolf wrote:
>>> Am 11.01.2019 um 12:40 hat Vladimir Sementsov-Ogievskiy geschrieben:
>>>> 11.01.2019 13:41, Kevin Wolf wrote:
>>>>> Am 10.01.2019 um 14:20 hat Vladimir Sementsov-Ogievskiy geschrieben:
>>>>>> drv_co_block_status digs bs->file for additional, more accurate search
>>>>>> for hole inside region, reported as DATA by bs since 5daa74a6ebc.
>>>>>>
>>>>>> This accuracy is not free: assume we have qcow2 disk. Actually, qcow2
>>>>>> knows, where are holes and where is data. But every block_status
>>>>>> request calls lseek additionally. Assume a big disk, full of
>>>>>> data, in any iterative copying block job (or img convert) we'll call
>>>>>> lseek(HOLE) on every iteration, and each of these lseeks will have to
>>>>>> iterate through all metadata up to the end of file. It's obviously
>>>>>> ineffective behavior. And for many scenarios we don't need this lseek
>>>>>> at all.
>>>>>>
>>>>>> So, let's "5daa74a6ebc" by default, leaving an option to return
>>>>>> previous behavior, which is needed for scenarios with preallocated
>>>>>> images.
>>>>>>
>>>>>> Add iotest illustrating new option semantics.
>>>>>>
>>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
>>>>>
>>>>> I still think that an option isn't a good solution and we should try use
>>>>> some heuristics instead.
>>>>
>>>> Do you think that heuristics would be better than fair cache for lseek
>>>> results?
>>>
>>> I don't think lseek() results are cachable, at least for images that can
>>> share BLK_PERM_WRITE between processes.
>>>
>>>> I don't see good heuristics, neither want to implement lseek optimizations.
>>>> In our cases we don't need lseek under qcow2 at all, and it's obviously 
>>>> better
>>>> just don't lseek in these cases.
>>>
>>> I suggested one: Pass large contiguous allocated ranges to the protocol
>>> driver, but just assume that the allocation status is correct in the
>>> format layer if they are small.
>>
>> So, for fully allocated image, we will call lseek always?
> 
> If they are fully contiguous, yes. But that's a single lseek() call per
> image then instead of an lseek() for every 64k, so not a big problem.

lseek is called on each mirror iteration, why one per image?

> 
> In the more realistic case, you will still call lseek() occasionally
> because you will have some fragmentation, but the fragments can be
> rather large. But it should still significantly reduce them compared to
> now because you skip it for those parts with small contiguous
> allocations where lseek() would be called a lot today.
> 
> Kevin
> 

Ok, you propose not to call lseek for small enough data regions reported by
format layer. And for images which are less fragmented, this helps less or don't
help.

Why do you think it is better?

For not preallocated images it is worse, as it covers less cases. So, for our
scenarios it is worse.

The only case, when heuristic works better, is when user have preallocated 
image,
but don't know about new option, which returns old behavior. We are not 
interested
in this case and can't go this way, as it doesn't guarantee, that some customer 
will
not come again with lseek-related problems.

Don't you like what Eric propose, about binding behavior switch to existing
detect-zeroes option?

Or, we can add an opposite option, to enable new behavior, keeping the old one 
by
default. So, all stays as is, and who need uses new option. Heuristic may be
implemented then too.

-- 
Best regards,
Vladimir