Re: [Qemu-devel] [PATCH v2 5/6] geometry: Call backend function to detect geometry and blocksize

[Markus Armbruster]

Ekaterina Tumanova <address@hidden> writes:

> On 11/28/2014 01:10 PM, Markus Armbruster wrote:
>> Ekaterina Tumanova <address@hidden> writes:
>>
>>> hd_geometry_guess function autodetects the drive geometry. This patch
>>> adds a block backend call, that probes the backing device geometry.
>>> If the inner driver method is implemented and succeeds (currently only 
>>> DASDs),
>>> the blkconf_geometry will pass-through the backing device geometry.
>>>
>>> Signed-off-by: Ekaterina Tumanova <address@hidden>
>>> ---
>>>   hw/block/block.c         | 11 +++++++++++
>>>   hw/block/hd-geometry.c   |  9 +++++++++
>>>   hw/block/virtio-blk.c    |  1 +
>>>   include/hw/block/block.h |  1 +
>>>   4 files changed, 22 insertions(+)
>>>
>>> diff --git a/hw/block/block.c b/hw/block/block.c
>>> index a625773..f1d29bc 100644
>>> --- a/hw/block/block.c
>>> +++ b/hw/block/block.c
>>> @@ -25,6 +25,17 @@ void blkconf_serial(BlockConf *conf, char **serial)
>>>       }
>>>   }
>>>
>>> +void blkconf_blocksizes(BlockConf *conf)
>>> +{
>>> +    BlockBackend *blk = conf->blk;
>>> +    struct ProbeBlockSize blocksize = blk_probe_blocksizes(blk);
>>> +
>>> +    if (blocksize.rc == 0) {
>>> +        conf->physical_block_size = blocksize.size.phys;
>>> +        conf->logical_block_size = blocksize.size.log;
>>> +    }
>>> +}
>>> +
>>
>> Uh, doesn't this overwrite the user's geometry?
>>
>> The existing blkconf_ functions do nothing when the user supplied the
>> configuration.  In other words, they only provide defaults.  Why should
>> this one be different?
>>
>
> this will be fixed in the next version
>
>>>   void blkconf_geometry(BlockConf *conf, int *ptrans,
>>>                         unsigned cyls_max, unsigned heads_max, unsigned 
>>> secs_max,
>>>                         Error **errp)
>>> diff --git a/hw/block/hd-geometry.c b/hw/block/hd-geometry.c
>>> index 6fcf74d..4972114 100644
>>> --- a/hw/block/hd-geometry.c
>>> +++ b/hw/block/hd-geometry.c
>>> @@ -121,6 +121,14 @@ void hd_geometry_guess(BlockBackend *blk,
>>>                          int *ptrans)
>>>   {
>>>       int cylinders, heads, secs, translation;
>>> +    struct ProbeGeometry geometry = blk_probe_geometry(blk);
>>> +
>>> +    if (geometry.rc == 0) {
>>> +        *pcyls = geometry.geo.cylinders;
>>> +        *psecs = geometry.geo.sectors;
>>> +        *pheads = geometry.geo.heads;
>>> +        goto done;
>>> +    }
>>>
>>
>> hd_geometry_guess() is called by blkconf_geometry() to conjure up a
>> default geometry when the user didn't pick one.  Fairly elaborate
>> guesswork.  blkconf_geometry() runs for IDE, SCSI and virtio-blk only.
>>
>> Your patch makes it pick the backend's geometry as reported by
>> blk_probe_geometry() before anything else.
>>
>> This is an incompatible change for backends where blk_probe_geometry()
>> succeeds.  Currently, it succeeds only for DASD, and there you *want*
>> the incompatible change.
>>
>> My point is: if we rely on blk_probe_geometry() failing except for DASD,
>> then we should call it something like blk_dasd_geometry() to make that
>> obvious.
>>
>
> This function itself is not DASD specific.

I agree, but...

>                                            It calls the inner method for
> "host_device" driver, which currently only succeeds for DASDs.
> But in future someone can define methods for other drivers or
> even modify the host_device driver.

... doing that will change the default geometry of the devices using the
backend.

At the very least, such action at a distance needs to be documented
prominently in the source.

[...]