Re: [Qemu-devel] [PATCH v3 5/5] BlockConf: Call backend functions to detect geometry and blocksizes

[Ekaterina Tumanova <tumanova@linux.vnet.ibm.com>]

On 12/15/2014 04:50 PM, Markus Armbruster wrote:
> Ekaterina Tumanova <tumanova@linux.vnet.ibm.com> writes:
>
>> geometry: 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 for DASDs), the blkconf_geometry will pass-through
>> the backing device geometry. Otherwise will fallback to old logic.
>>
>> blocksize: This patch initializes blocksize properties to 0.
>> In order to set the properly a blkconf_blocksizes was introduced.
>> If user didn't set physical or logical blocksize, it will
>> retrieve it's value from a driver (which will return a default 512 for
>> any backend but DASD).
>>
>> The blkconf_blocksizes call was added to all users of BlkConf.
>>
>> Signed-off-by: Ekaterina Tumanova <tumanova@linux.vnet.ibm.com>
>> ---
>>   hw/block/block.c          | 18 ++++++++++++++++++
>>   hw/block/hd-geometry.c    | 12 ++++++++++++
>>   hw/block/nvme.c           |  1 +
>>   hw/block/virtio-blk.c     |  1 +
>>   hw/core/qdev-properties.c |  3 ++-
>>   hw/ide/qdev.c             |  1 +
>>   hw/scsi/scsi-disk.c       |  1 +
>>   hw/usb/dev-storage.c      |  1 +
>>   include/hw/block/block.h  |  5 +++--
>>   9 files changed, 40 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/block/block.c b/hw/block/block.c
>> index a625773..9c07516 100644
>> --- a/hw/block/block.c
>> +++ b/hw/block/block.c
>> @@ -25,6 +25,24 @@ void blkconf_serial(BlockConf *conf, char **serial)
>>       }
>>   }
>>
>> +void blkconf_blocksizes(BlockConf *conf)
>> +{
>> +    BlockBackend *blk = conf->blk;
>> +    BlockSizes blocksizes;
>> +
>> +    if (conf->physical_block_size && conf->logical_block_size) {
>> +        return;
>> +    }
>
> This conditional isn't necessary for correctness.  Feel free to drop it.
>

But this allows to avoid the ioctl call when user has specified both
values. Remove anyway?

>> +    blk_probe_blocksizes(blk, &blocksizes);
>> +
>> +    if (!conf->physical_block_size) {
>> +        conf->physical_block_size = blocksizes.phys;
>> +    }
>> +    if (!conf->logical_block_size) {
>> +        conf->logical_block_size = blocksizes.log;
>> +    }
>

I'll add a comment to make it apparent.

> This works because you change the default block size to 0 (second to
> last hunk).
>
>> +}
>> +
>>   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..fbd602d 100644
>> --- a/hw/block/hd-geometry.c
>> +++ b/hw/block/hd-geometry.c
>> @@ -121,7 +121,17 @@ void hd_geometry_guess(BlockBackend *blk,
>>                          int *ptrans)
>>   {
>>       int cylinders, heads, secs, translation;
>> +    hdGeometry geo;
>>
>> +    /* Try to probe the backing device geometry, otherwise fallback
>> +       to the old logic. (as of 12/2014 probing only succeeds on DASDs) */
>> +    if (blk_probe_geometry(blk, &geo) == 0) {
>> +        *pcyls = geo.cylinders;
>> +        *psecs = geo.sectors;
>> +        *pheads = geo.heads;
>> +        translation = BIOS_ATA_TRANSLATION_NONE;
>> +        goto done;
>> +    }
>
> "else if" instead of goto, please.
>
>>       if (guess_disk_lchs(blk, &cylinders, &heads, &secs) < 0) {
>>           /* no LCHS guess: use a standard physical disk geometry  */
>>           guess_chs_for_size(blk, pcyls, pheads, psecs);
>> @@ -142,7 +152,9 @@ void hd_geometry_guess(BlockBackend *blk,
>>           /* disable any translation to be in sync with
>>              the logical geometry */
>>           translation = BIOS_ATA_TRANSLATION_NONE;
>> +
>>       }
>
> Humor me: put the empty line behind the brace instead of before.
>
>> +done:
>>       if (ptrans) {
>>           *ptrans = translation;
>>       }
>
> Much easier to gauge than v2.  Geometry change is a compatibility
> problem.  You change it only when blk_probe_geometry() succeeds.  It
> succeeds only for DASD.  Mission accomplished.
>
> Recommend to add a hint to the function contract of the
> bdrv_probe_geometry() callback in block_int.h:
>
>      /**
>       * Try to get @bs's geometry (cyls, heads, sectos)
>       * On success, store them in @geo and return 0.
>       * On failure return -errno.
>       * Only drivers that want to override guest geometry implement this
>       * callback; see hd_geometry_guess().
>       */
>      int (*bdrv_probe_geometry)(BlockDriverState *bs, hdGeometry *geo);
>
>> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
>> index 1327658..244e382 100644
>> --- a/hw/block/nvme.c
>> +++ b/hw/block/nvme.c
>> @@ -763,6 +763,7 @@ static int nvme_init(PCIDevice *pci_dev)
>>       if (!n->serial) {
>>           return -1;
>>       }
>> +    blkconf_blocksizes(&n->conf);
>>
>>       pci_conf = pci_dev->config;
>>       pci_conf[PCI_INTERRUPT_PIN] = 1;
>> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
>> index b19b102..6f01565 100644
>> --- a/hw/block/virtio-blk.c
>> +++ b/hw/block/virtio-blk.c
>> @@ -745,6 +745,7 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
>>           error_propagate(errp, err);
>>           return;
>>       }
>> +    blkconf_blocksizes(&conf->conf);
>>
>>       virtio_init(vdev, "virtio-blk", VIRTIO_ID_BLOCK,
>>                   sizeof(struct virtio_blk_config));
>> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
>> index 2e47f70..ba81709 100644
>> --- a/hw/core/qdev-properties.c
>> +++ b/hw/core/qdev-properties.c
>> @@ -580,7 +580,8 @@ static void set_blocksize(Object *obj, Visitor *v, void *opaque,
>>           error_propagate(errp, local_err);
>>           return;
>>       }
>> -    if (value < min || value > max) {
>> +    /* value of 0 means "unset" */
>> +    if (value && (value < min || value > max)) {
>>           error_set(errp, QERR_PROPERTY_VALUE_OUT_OF_RANGE,
>>                     dev->id?:"", name, (int64_t)value, min, max);
>>           return;
>> diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
>> index b4f096e..e71ff7f 100644
>> --- a/hw/ide/qdev.c
>> +++ b/hw/ide/qdev.c
>> @@ -164,6 +164,7 @@ static int ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind)
>>       }
>>
>>       blkconf_serial(&dev->conf, &dev->serial);
>> +    blkconf_blocksizes(&dev->conf);
>>       if (kind != IDE_CD) {
>>           blkconf_geometry(&dev->conf, &dev->chs_trans, 65536, 16, 255, &err);
>>           if (err) {
>> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
>> index 2f75d7d..5288129 100644
>> --- a/hw/scsi/scsi-disk.c
>> +++ b/hw/scsi/scsi-disk.c
>> @@ -2230,6 +2230,7 @@ static void scsi_realize(SCSIDevice *dev, Error **errp)
>>       }
>>
>>       blkconf_serial(&s->qdev.conf, &s->serial);
>> +    blkconf_blocksizes(&s->qdev.conf);
>>       if (dev->type == TYPE_DISK) {
>>           blkconf_geometry(&dev->conf, NULL, 65535, 255, 255, &err);
>>           if (err) {
>> diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
>> index 4539733..cc02dfd 100644
>> --- a/hw/usb/dev-storage.c
>> +++ b/hw/usb/dev-storage.c
>> @@ -611,6 +611,7 @@ static void usb_msd_realize_storage(USBDevice *dev, Error **errp)
>>       }
>>
>>       blkconf_serial(&s->conf, &dev->serial);
>> +    blkconf_blocksizes(&s->conf);
>>
>>       /*
>>        * Hack alert: this pretends to be a block device, but it's really
>> diff --git a/include/hw/block/block.h b/include/hw/block/block.h
>> index 0d0ce9a..3e502a8 100644
>> --- a/include/hw/block/block.h
>> +++ b/include/hw/block/block.h
>> @@ -44,9 +44,9 @@ static inline unsigned int get_physical_block_exp(BlockConf *conf)
>>   #define DEFINE_BLOCK_PROPERTIES(_state, _conf)                          \
>>       DEFINE_PROP_DRIVE("drive", _state, _conf.blk),                      \
>>       DEFINE_PROP_BLOCKSIZE("logical_block_size", _state,                 \
>> -                          _conf.logical_block_size, 512),               \
>> +                          _conf.logical_block_size, 0),               \
>>       DEFINE_PROP_BLOCKSIZE("physical_block_size", _state,                \
>> -                          _conf.physical_block_size, 512),              \
>> +                          _conf.physical_block_size, 0),              \
>>       DEFINE_PROP_UINT16("min_io_size", _state, _conf.min_io_size, 0),  \
>>       DEFINE_PROP_UINT32("opt_io_size", _state, _conf.opt_io_size, 0),    \
>>       DEFINE_PROP_UINT32("discard_granularity", _state, \
>> @@ -63,6 +63,7 @@ void blkconf_serial(BlockConf *conf, char **serial);
>>   void blkconf_geometry(BlockConf *conf, int *trans,
>>                         unsigned cyls_max, unsigned heads_max, unsigned secs_max,
>>                         Error **errp);
>> +void blkconf_blocksizes(BlockConf *conf);
>>
>>   /* Hard disk geometry */
>