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

login
register
mail settings
Submitter elelueck@linux.vnet.ibm.com
Date Feb. 8, 2013, 2:52 p.m.
Message ID <1360335155-39413-3-git-send-email-elelueck@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/219172/
State New
Headers show

Comments

elelueck@linux.vnet.ibm.com - Feb. 8, 2013, 2:52 p.m.
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. For virtio-ccw, this is already a default in
the existing upstream implementation.

Signed-off-by: Einar Lueck <elelueck@linux.vnet.ibm.com>
---
 hw/block-common.c          | 23 +++++++++++++++++++++++
 hw/block-common.h          | 12 +++++++++---
 hw/qdev-properties.c       |  4 +++-
 hw/s390x/s390-virtio-bus.c |  2 +-
 hw/virtio-blk.c            |  1 +
 5 files changed, 37 insertions(+), 5 deletions(-)
Stefan Hajnoczi - Feb. 12, 2013, 3:47 p.m.
On Fri, Feb 08, 2013 at 03:52:35PM +0100, Einar Lueck wrote:
> @@ -22,6 +25,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) {

#ifdef __linux__

> +           conf->physical_block_size = (uint16_t) block_size;

4 spaces for indentation.

> +        } 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)       \

Detecting the underlying block size is a generally useful configuration
option.  This should not be s390-specific, so no need to rename
DEFINE_BLOCK_PROPERTIES().

If you set the default to 0 then QEMU will do extra ioctls.  Perhaps
BlockDriverState needs a function to probe block sizes.  This way
block/raw-posix.c could do the probing only on hdev (host block
devices).

> diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
> index a8a31f5..73b6da0 100644
> --- a/hw/qdev-properties.c
> +++ b/hw/qdev-properties.c
> @@ -650,7 +650,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;

In the current patch this allows through the 0 value.  Since the patch
only adds a blkconf_blocksizes() call to hw/virtio-blk.c, who knows what
will happen to ide/scsi/etc -drives that are configured with 0.

We need to make sure that 0 is really handled for all emulated storage
controllers.

> diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
> index 34913ee..cd25712 100644
> --- a/hw/virtio-blk.c
> +++ b/hw/virtio-blk.c
> @@ -654,6 +654,7 @@ VirtIODevice *virtio_blk_init(DeviceState *dev, VirtIOBlkConf *blk)
>      }
>  
>      blkconf_serial(&blk->conf, &blk->serial);
> +    blkconf_blocksizes(&blk->conf);

Shouldn't be virtio-specific, all -drives should be able to autodetect
block size.

Patch

diff --git a/hw/block-common.c b/hw/block-common.c
index d21ec3a..7a040d1 100644
--- a/hw/block-common.c
+++ b/hw/block-common.c
@@ -10,6 +10,9 @@ 
 #include "sysemu/blockdev.h"
 #include "hw/block-common.h"
 #include "qemu/error-report.h"
+#ifdef __linux__
+#include <linux/fs.h>
+#endif
 
 void blkconf_serial(BlockConf *conf, char **serial)
 {
@@ -22,6 +25,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 a8a31f5..73b6da0 100644
--- a/hw/qdev-properties.c
+++ b/hw/qdev-properties.c
@@ -650,7 +650,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/s390x/s390-virtio-bus.c b/hw/s390x/s390-virtio-bus.c
index d467781..f403308 100644
--- a/hw/s390x/s390-virtio-bus.c
+++ b/hw/s390x/s390-virtio-bus.c
@@ -426,7 +426,7 @@  static const 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 34913ee..cd25712 100644
--- a/hw/virtio-blk.c
+++ b/hw/virtio-blk.c
@@ -654,6 +654,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;
     }