Ekaterina Tumanova <tuman...@linux.vnet.ibm.com> writes:
> On 11/28/2014 01:10 PM, Markus Armbruster wrote:
>> Ekaterina Tumanova <tuman...@linux.vnet.ibm.com> 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 <tuman...@linux.vnet.ibm.com>
>>> ---
>>> 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.
[...]
