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

[Ekaterina Tumanova]

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. 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.

> The commit message needs to spell out the incompatible change.
>
> >       if (guess_disk_lchs(blk, &cylinders, &heads, &secs) < 0) {
> >           /* no LCHS guess: use a standard physical disk geometry  */
> > @@ -143,6 +151,7 @@ void hd_geometry_guess(BlockBackend *blk,
> >              the logical geometry */
> >           translation = BIOS_ATA_TRANSLATION_NONE;
> >       }
> > +done:
> >       if (ptrans) {
> >           *ptrans = translation;
> >       }
> > 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));
>
> Why only virtio-blk, and not the other device models sporting
> configurable block sizes (nvme, IDE, SCSI, usb-storage)?
>
> This is an incompatible change for backends where blk_probe_blocksizes()
> succeeds and returns something other than (512, 512).  Currently, it
> succeeds only for DASD.  Is that what we want?
>
> The commit message needs to spell out the incompatible change.
>
> Perhaps this patch should be split up into one for geometry and one for
> block sizes.
>
> > diff --git a/include/hw/block/block.h b/include/hw/block/block.h
> > index 0d0ce9a..856bf75 100644
> > --- a/include/hw/block/block.h
> > +++ b/include/hw/block/block.h
> > @@ -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 */