Re: [Qemu-devel] [PATCH 1/2] virtio-block: support auto-sensing of block sizes

[Jens Freimann]

On Mon, Aug 13, 2012 at 07:50:39PM +0000, Blue Swirl wrote:
> On Mon, Aug 13, 2012 at 8:40 AM, Jens Freimann <address@hidden> wrote:
> > From: Einar Lueck <address@hidden>
> >
> > Virtio-blk does not impose fixed block sizes for access to
> > backing devices. This patch introduces support for auto
> > lookup of the block sizes of the backing block device. This
> > automatic lookup needs to be enabled explicitly. Users may
> > do this by specifying (physical|logical)_block_size=0.
> > Machine types may do this for their defaults, too.
> > To achieve this, a new function blkconf_blocksizes is
> > implemented. If physical or logical block size are zero
> > a corresponding ioctl tries to find an appropriate value.
> > If this does not work, 512 is used. blkconf_blocksizes
> > is therefore only called w/in the virtio-blk context.
> > For s390-virtio, this patch configures auto lookup as
> > default.
> >
> > Signed-off-by: Einar Lueck <address@hidden>
> > Signed-off-by: Jens Freimann <address@hidden>
> > ---
> >  hw/block-common.c    | 23 +++++++++++++++++++++++
> >  hw/block-common.h    | 12 +++++++++---
> >  hw/qdev-properties.c |  4 +++-
> >  hw/s390-virtio-bus.c |  2 +-
> >  hw/virtio-blk.c      |  1 +
> >  5 files changed, 37 insertions(+), 5 deletions(-)
> >
> > diff --git a/hw/block-common.c b/hw/block-common.c
> > index f0196d7..444edd2 100644
> > --- a/hw/block-common.c
> > +++ b/hw/block-common.c
> > @@ -10,6 +10,9 @@
> >  #include "blockdev.h"
> >  #include "hw/block-common.h"
> >  #include "qemu-error.h"
> > +#ifdef __linux__
> > +#include <linux/fs.h>
> > +#endif
> >
> >  void blkconf_serial(BlockConf *conf, char **serial)
> >  {
> > @@ -24,6 +27,26 @@ void blkconf_serial(BlockConf *conf, char **serial)
> >      }
> >  }
> >
> > +void blkconf_blocksizes(BlockConf *conf)
> > +{
> > +    int block_size;
> > +
> > +    if (!conf->physical_block_size) {
> > +        if (bdrv_ioctl(conf->bs, BLKPBSZGET, &block_size) == 0) {
> 
> This use of BLKPBSZGET is not protected by #ifdef __linux__ (or
> BLKPBSZGET), so this will probably fail when the host OS is not Linux
> and BLKPBSZGET is not defined.

Yes, you're right.

> > +            conf->physical_block_size = (uint16_t) block_size;
> > +        } else {
> > +            conf->physical_block_size = BLOCK_PROPERTY_STD_BLKSIZE;
> > +        }
> > +    }
> > +    if (!conf->logical_block_size) {
> > +        if (bdrv_ioctl(conf->bs, BLKSSZGET, &block_size) == 0) {
> 
> Also here.

Ok, will fix. Thanks for the review!

Jens

> > +            conf->logical_block_size = (uint16_t) block_size;
> > +        } else {
> > +            conf->logical_block_size = BLOCK_PROPERTY_STD_BLKSIZE;
> > +        }
> > +    }
> > +}
> > +
> >  int blkconf_geometry(BlockConf *conf, int *ptrans,
> >                       unsigned cyls_max, unsigned heads_max, unsigned 
> > secs_max)
> >  {
> > diff --git a/hw/block-common.h b/hw/block-common.h
> > index bb808f7..d593128 100644
> > --- a/hw/block-common.h
> > +++ b/hw/block-common.h
> > @@ -40,18 +40,23 @@ static inline unsigned int 
> > get_physical_block_exp(BlockConf *conf)
> >      return exp;
> >  }
> >
> > -#define DEFINE_BLOCK_PROPERTIES(_state, _conf)                          \
> > +#define BLOCK_PROPERTY_STD_BLKSIZE 512
> > +#define DEFINE_BLOCK_PROPERTIES_EXTENDED(_state, _conf, _blksize)       \
> >      DEFINE_PROP_DRIVE("drive", _state, _conf.bs),                       \
> >      DEFINE_PROP_BLOCKSIZE("logical_block_size", _state,                 \
> > -                          _conf.logical_block_size, 512),               \
> > +                          _conf.logical_block_size, _blksize),          \
> >      DEFINE_PROP_BLOCKSIZE("physical_block_size", _state,                \
> > -                          _conf.physical_block_size, 512),              \
> > +                          _conf.physical_block_size, _blksize),         \
> >      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_INT32("bootindex", _state, _conf.bootindex, -1),        \
> >      DEFINE_PROP_UINT32("discard_granularity", _state, \
> >                         _conf.discard_granularity, 0)
> >
> > +#define DEFINE_BLOCK_PROPERTIES(_state, _conf)                          \
> > +    DEFINE_BLOCK_PROPERTIES_EXTENDED(_state, _conf,                     \
> > +                                     BLOCK_PROPERTY_STD_BLKSIZE)
> > +
> >  #define DEFINE_BLOCK_CHS_PROPERTIES(_state, _conf)      \
> >      DEFINE_PROP_UINT32("cyls", _state, _conf.cyls, 0),  \
> >      DEFINE_PROP_UINT32("heads", _state, _conf.heads, 0), \
> > @@ -60,6 +65,7 @@ static inline unsigned int 
> > get_physical_block_exp(BlockConf *conf)
> >  /* Configuration helpers */
> >
> >  void blkconf_serial(BlockConf *conf, char **serial);
> > +void blkconf_blocksizes(BlockConf *conf);
> >  int blkconf_geometry(BlockConf *conf, int *trans,
> >                       unsigned cyls_max, unsigned heads_max, unsigned 
> > secs_max);
> >
> > diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
> > index 8aca0d4..e99dee4 100644
> > --- a/hw/qdev-properties.c
> > +++ b/hw/qdev-properties.c
> > @@ -904,7 +904,9 @@ static void set_blocksize(Object *obj, Visitor *v, void 
> > *opaque,
> >          error_propagate(errp, local_err);
> >          return;
> >      }
> > -    if (value < min || value > max) {
> > +
> > +    /* value == 0 indicates that block size should be sensed later on */
> > +    if ((value < min || value > max) && value > 0) {
> >          error_set(errp, QERR_PROPERTY_VALUE_OUT_OF_RANGE,
> >                    dev->id?:"", name, (int64_t)value, min, max);
> >          return;
> > diff --git a/hw/s390-virtio-bus.c b/hw/s390-virtio-bus.c
> > index a245684..562964e 100644
> > --- a/hw/s390-virtio-bus.c
> > +++ b/hw/s390-virtio-bus.c
> > @@ -401,7 +401,7 @@ static TypeInfo s390_virtio_net = {
> >  };
> >
> >  static Property s390_virtio_blk_properties[] = {
> > -    DEFINE_BLOCK_PROPERTIES(VirtIOS390Device, blk.conf),
> > +    DEFINE_BLOCK_PROPERTIES_EXTENDED(VirtIOS390Device, blk.conf, 0),
> >      DEFINE_BLOCK_CHS_PROPERTIES(VirtIOS390Device, blk.conf),
> >      DEFINE_PROP_STRING("serial", VirtIOS390Device, blk.serial),
> >  #ifdef __linux__
> > diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
> > index f21757e..7686433 100644
> > --- a/hw/virtio-blk.c
> > +++ b/hw/virtio-blk.c
> > @@ -600,6 +600,7 @@ VirtIODevice *virtio_blk_init(DeviceState *dev, 
> > VirtIOBlkConf *blk)
> >      }
> >
> >      blkconf_serial(&blk->conf, &blk->serial);
> > +    blkconf_blocksizes(&blk->conf);
> >      if (blkconf_geometry(&blk->conf, NULL, 65535, 255, 255) < 0) {
> >          return NULL;
> >      }
> > --
> > 1.7.11.4
> >
> >
> 

-- 
Mit freundlichen Grüßen / Kind regards 

Jens Freimann 

-- 
IBM Linux Technology Center / Boeblingen lab
IBM Systems &Technology Group, Systems Software Development
-------------------------------------------------------------
IBM Deutschland
Schoenaicher Str 220
71032 Boeblingen
Phone: +49-7031-16 x1122
E-Mail: address@hidden
-------------------------------------------------------------
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294