On Thursday 17 April 2008 04:56:37 Ryan Harper wrote:> From: Ryan Harper <ryanh at us.ibm.com>
>
> Rather than faking up some geometry, allow the backend to push the disk
> geometry via virtio pci config option. Keep the old geo code around for
> compatibility.
Hi Ryan,
Looks good! Some brief review below. Mainly just "how I would have done
things" stuff. BTW, does this help in real life? I assume something in
userspace wants it?
> + int err = 0;
> +
> + /* see if the host passed in geometry config */
> + err = virtio_config_val(vblk->vdev, VIRTIO_BLK_F_GEOMETRY,
> + offsetof(struct virtio_blk_config, cylinders),
> + &geo->cylinders);
Unnecessary err initialization. Sometimes gcc catches bugs when you defer
initializations of err to as late as possible (ie. paths where err isn't
set properly), so I tend to do it.
> + /* if host sets geo flag, all 3 values must be present */
> + if (!err) {
> + __virtio_config_val(vblk->vdev,
> + offsetof(struct virtio_blk_config, heads),
> + &geo->heads);
> + __virtio_config_val(vblk->vdev,
> + offsetof(struct virtio_blk_config, sectors),
> + &geo->sectors);
Kind of ugly; we can represent this in the data structure explicitly tho...
> @@ -18,6 +19,12 @@ struct virtio_blk_config
> __le32 size_max;
> /* The maximum number of segments (if VIRTIO_BLK_F_SEG_MAX) */
> __le32 seg_max;
> + /* cylinders of the device (if VIRTIO_BLK_F_GEOMETRY) */
> + __le16 cylinders;
> + /* heads of the device (if VIRTIO_BLK_F_GEOMETRY) */
> + __u8 heads;
> + /* sectors of the device (if VIRTIO_BLK_F_GEOMETRY) */
> + __u8 sectors;
> } __attribute__((packed));
... using a struct-within-a-struct.
Here's the result:
Subject: add virtio disk geometry feature
Date: Wed, 16 Apr 2008 13:56:37 -0500
From: Ryan Harper <ryanh at us.ibm.com>
Rather than faking up some geometry, allow the backend to push the disk
geometry via virtio pci config option. Keep the old geo code around for
compatibility.
Signed-off-by: Ryan Harper <ryanh at us.ibm.com>
Reviewed-by: Anthony Liguori <aliguori at us.ibm.com>
Signed-off-by: Rusty Russell <rusty at rustcorp.com.au> (modified to
single struct)
diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -157,10 +157,25 @@ static int virtblk_ioctl(struct inode *i
/* We provide getgeo only to please some old bootloader/partitioning tools */
static int virtblk_getgeo(struct block_device *bd, struct hd_geometry *geo)
{
- /* some standard values, similar to sd */
- geo->heads = 1 << 6;
- geo->sectors = 1 << 5;
- geo->cylinders = get_capacity(bd->bd_disk) >> 11;
+ struct virtio_blk *vblk = bd->bd_disk->private_data;
+ struct virtio_blk_geometry vgeo;
+ int err;
+
+ /* see if the host passed in geometry config */
+ err = virtio_config_val(vblk->vdev, VIRTIO_BLK_F_GEOMETRY,
+ offsetof(struct virtio_blk_config, geometry),
+ &vgeo);
+
+ if (!err) {
+ geo->heads = vgeo.heads;
+ geo->sectors = vgeo.sectors;
+ geo->cylinders = vgeo.cylinders;
+ } else {
+ /* some standard values, similar to sd */
+ geo->heads = 1 << 6;
+ geo->sectors = 1 << 5;
+ geo->cylinders = get_capacity(bd->bd_disk) >> 11;
+ }
return 0;
}
diff --git a/include/linux/virtio_blk.h b/include/linux/virtio_blk.h
--- a/include/linux/virtio_blk.h
+++ b/include/linux/virtio_blk.h
@@ -9,6 +9,7 @@
#define VIRTIO_BLK_F_BARRIER 0 /* Does host support barriers? */
#define VIRTIO_BLK_F_SIZE_MAX 1 /* Indicates maximum segment size */
#define VIRTIO_BLK_F_SEG_MAX 2 /* Indicates maximum # of segments */
+#define VIRTIO_BLK_F_GEOMETRY 4 /* Legacy geometry available */
struct virtio_blk_config
{
@@ -18,6 +19,12 @@ struct virtio_blk_config
__le32 size_max;
/* The maximum number of segments (if VIRTIO_BLK_F_SEG_MAX) */
__le32 seg_max;
+ /* geometry the device (if VIRTIO_BLK_F_GEOMETRY) */
+ struct virtio_blk_geometry {
+ __le16 cylinders;
+ __u8 heads;
+ __u8 sectors;
+ } geometry;
} __attribute__((packed));
/* These two define direction. */