Re: [Qemu-block] [RFC 1/3] block: Add ImageRotationalInfo

[Max Reitz]

On 27.05.19 15:44, Anton Nefedov wrote:
> On 27/5/2019 3:57 PM, Max Reitz wrote:
>> On 27.05.19 14:37, Alberto Garcia wrote:
>>> On Mon 27 May 2019 02:16:53 PM CEST, Max Reitz wrote:
>>>> On 26.05.19 17:08, Alberto Garcia wrote:
>>>>> On Fri 24 May 2019 07:28:10 PM CEST, Max Reitz <address@hidden> wrote:
>>>>>> +##
>>>>>> +# @ImageRotationalInfo:
>>>>>> +#
>>>>>> +# Indicates whether an image is stored on a rotating disk or not.
>>>>>> +#
>>>>>> +# @solid-state: Image is stored on a solid-state drive
>>>>>> +#
>>>>>> +# @rotating:    Image is stored on a rotating disk
>>>>>
>>>>> What happens when you cannot tell? You assume it's solid-state?
>>>>
>>>> When *I* cannot tell?  This field is generally optional, so in that case
>>>> it just will not be present.
>>>>
>>>> (When Linux cannot tell?  I don’t know :-))
>>>>
> 
> Linux defaults to rotational == 1 unless the driver sets
> QUEUE_FLAG_NONROT.
> 
> By the way as far as I can tell, qemu does not report this flag unless
> explicitly set in a device property.
> 
>      DEFINE_PROP_UINT16("rotation_rate", IDEDrive, dev.rotation_rate, 0),
> and
>      DEFINE_PROP_UINT16("rotation_rate", SCSIDiskState, rotation_rate, 0),
> 
>>>> Do you think there should be an explicit value for that?
>>>
>>> I'll try to rephrase:
>>>
>>> we have a new optimization that improves performance on SSDs but reduces
>>> performance on HDDs, so this series would detect where an image is
>>> stored in order to enable the faster code path for each case.
>>>
>>> What happens if QEMU cannot detect if we have a solid drive or a
>>> rotational drive? (e.g. a remote storage backend). Will it default to
>>> enabling preallocation using write_zeroes()?
>>
>> In this series, yes.  That is the default I chose.
>>
>> We have to make a separate decision for each case.  In the case of
>> filling newly allocated areas with zeroes, I think the performance gain
>> for SSDs is more important than the performance loss for HDDs.  That is
>> what I wrote in my response to Anton’s series.  So I took the series
>> even without it being able to distinguish both cases at all.
>> Consequentially, I believe it is reasonable for that to be the default
>> behavior if we cannot tell.
>>
>> I think in general optimizing for SSDs should probably be the default.
>> HDDs are slow anyway, so whoever uses them probably doesn’t care about
>> performance too much anyway...?  Whereas people using SSDs probably do.
>>   But as I said, we can and should always make a separate decision for
>> each case.
>>
> 
> Overall it looks good to me but I wonder how do we ensure both variants
> are test covered? Need a blockdev option to enforce the mode?

That’s a good point.  Yes, file-posix should probably take an option to
override the mode.  Actually, that may be a useful option in general (if
the file is on some file system where we cannot query this information
(like glusterfs), the user may want to manually provide it).

Max

signature.asc
Description: OpenPGP digital signature