Patchwork [1/2] virtio-block: support auto-sensing of block sizes

login
register
mail settings
Submitter Jens Freimann
Date Aug. 13, 2012, 8:40 a.m.
Message ID <1344847212-5047-2-git-send-email-jfrei@de.ibm.com>
Download mbox | patch
Permalink /patch/176898/
State New
Headers show

Comments

Jens Freimann - Aug. 13, 2012, 8:40 a.m.
From: Einar Lueck <elelueck@linux.vnet.ibm.com>

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 <elelueck@linux.vnet.ibm.com>
Signed-off-by: Jens Freimann <jfrei@linux.vnet.ibm.com>
---
 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(-)
Blue Swirl - Aug. 13, 2012, 7:50 p.m.
On Mon, Aug 13, 2012 at 8:40 AM, Jens Freimann <jfrei@de.ibm.com> wrote:
> From: Einar Lueck <elelueck@linux.vnet.ibm.com>
>
> 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 <elelueck@linux.vnet.ibm.com>
> Signed-off-by: Jens Freimann <jfrei@linux.vnet.ibm.com>
> ---
>  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.

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

> +            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
>
>
Jens Freimann - Aug. 14, 2012, 6:40 a.m.
On Mon, Aug 13, 2012 at 07:50:39PM +0000, Blue Swirl wrote:
> On Mon, Aug 13, 2012 at 8:40 AM, Jens Freimann <jfrei@de.ibm.com> wrote:
> > From: Einar Lueck <elelueck@linux.vnet.ibm.com>
> >
> > 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 <elelueck@linux.vnet.ibm.com>
> > Signed-off-by: Jens Freimann <jfrei@linux.vnet.ibm.com>
> > ---
> >  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
> >
> >
>

Patch

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) {
+            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) {
+            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;
     }