virtio_blk: add block topology support

PATCH Applied Christoph Hellwig 3 participants 10 messages Jan 29, 2010 – Aug 1, 2011

Subsystems (3)

Block layer virtio core virtio block and SCSI drivers

lore

Share & tools

lore

Christoph Hellwig

author

2010-01-29 19:02 UTC #0 lore raw

PATCH 1/1 virtio_blk: add block topology support

Allow reading various alignment values from the config page. This allows the guest to much better align I/O requests depending on the storage topology.

Note that the formats for the config values appear a bit messed up, but we follow the formats used by ATA and SCSI so they are expected in the storage world.

Signed-off-by Christoph Hellwig <[email protected]>

drivers/block/virtio_blk.c +37 ~~-18~~

Index: linux-2.6/drivers/block/virtio_blk.c
===================================================================
@@ -243,10 +243,12 @@ static int index_to_minor(int index)
 static int __devinit virtblk_probe(struct virtio_device *vdev)
 {
 	struct virtio_blk *vblk;
+	struct request_queue *q;
 	int err;
 	u64 cap;
 	u32 v, blk_size, sg_elems, opt_io_size;
 	u16 min_io_size;
+	u8 physical_block_exp, alignment_offset;
 
 	if (index_to_minor(index) >= 1 << MINORBITS)
 		return -ENOSPC;

@@ -293,13 +295,13 @@ static int __devinit virtblk_probe(struc
 		goto out_mempool;
 	}
 
-	vblk->disk->queue = blk_init_queue(do_virtblk_request, &vblk->lock);
-	if (!vblk->disk->queue) {
+	q = vblk->disk->queue = blk_init_queue(do_virtblk_request, &vblk->lock);
+	if (!q) {
 		err = -ENOMEM;
 		goto out_put_disk;
 	}
 
-	vblk->disk->queue->queuedata = vblk;
+	q->queuedata = vblk;
 
 	if (index < 26) {
 		sprintf(vblk->disk->disk_name, "vd%c", 'a' + index % 26);

@@ -323,10 +325,10 @@ static int __devinit virtblk_probe(struc
 
 	/* If barriers are supported, tell block layer that queue is ordered */
 	if (virtio_has_feature(vdev, VIRTIO_BLK_F_FLUSH))
-		blk_queue_ordered(vblk->disk->queue, QUEUE_ORDERED_DRAIN_FLUSH,
+		blk_queue_ordered(q, QUEUE_ORDERED_DRAIN_FLUSH,
 				  virtblk_prepare_flush);
 	else if (virtio_has_feature(vdev, VIRTIO_BLK_F_BARRIER))
-		blk_queue_ordered(vblk->disk->queue, QUEUE_ORDERED_TAG, NULL);
+		blk_queue_ordered(q, QUEUE_ORDERED_TAG, NULL);
 
 	/* If disk is read-only in the host, the guest should obey */
 	if (virtio_has_feature(vdev, VIRTIO_BLK_F_RO))

@@ -345,14 +347,14 @@ static int __devinit virtblk_probe(struc
 	set_capacity(vblk->disk, cap);
 
 	/* We can handle whatever the host told us to handle. */
-	blk_queue_max_phys_segments(vblk->disk->queue, vblk->sg_elems-2);
-	blk_queue_max_hw_segments(vblk->disk->queue, vblk->sg_elems-2);
+	blk_queue_max_phys_segments(q, vblk->sg_elems-2);
+	blk_queue_max_hw_segments(q, vblk->sg_elems-2);
 
 	/* No need to bounce any requests */
-	blk_queue_bounce_limit(vblk->disk->queue, BLK_BOUNCE_ANY);
+	blk_queue_bounce_limit(q, BLK_BOUNCE_ANY);
 
 	/* No real sector limit. */
-	blk_queue_max_sectors(vblk->disk->queue, -1U);
+	blk_queue_max_sectors(q, -1U);
 
 	/* Host can optionally specify maximum segment size and number of
 	 * segments. */

@@ -360,29 +362,46 @@ static int __devinit virtblk_probe(struc
 				offsetof(struct virtio_blk_config, size_max),
 				&v);
 	if (!err)
-		blk_queue_max_segment_size(vblk->disk->queue, v);
+		blk_queue_max_segment_size(q, v);
 	else
-		blk_queue_max_segment_size(vblk->disk->queue, -1U);
+		blk_queue_max_segment_size(q, -1U);
 
 	/* Host can optionally specify the block size of the device */
 	err = virtio_config_val(vdev, VIRTIO_BLK_F_BLK_SIZE,
 				offsetof(struct virtio_blk_config, blk_size),
 				&blk_size);
 	if (!err)
-		blk_queue_logical_block_size(vblk->disk->queue, blk_size);
+		blk_queue_logical_block_size(q, blk_size);
 
 	/* Use topology information if available */
-	err = virtio_config_val(vdev, VIRTIO_BLK_F_BLK_TOPOLOGY,
+	err = virtio_config_val(vdev, VIRTIO_BLK_F_TOPOLOGY,
+			offsetof(struct virtio_blk_config, physical_block_exp),
+			&physical_block_exp);
+	if (!err && physical_block_exp) {
+		u16 physical_block_size =
+			queue_logical_block_size(q) * (1 << physical_block_exp);
+		blk_queue_physical_block_size(q, physical_block_size);
+	}
+
+	err = virtio_config_val(vdev, VIRTIO_BLK_F_TOPOLOGY,
+			offsetof(struct virtio_blk_config, alignment_offset),
+			&alignment_offset);
+	if (!err && alignment_offset) {
+		u16 alignment = queue_logical_block_size(q) * alignment_offset;
+		blk_queue_alignment_offset(q, alignment);
+	}
+
+	err = virtio_config_val(vdev, VIRTIO_BLK_F_TOPOLOGY,
 			offsetof(struct virtio_blk_config, min_io_size),
 			&min_io_size);
-	if (!err)
-		blk_queue_io_min(vblk->disk->queue, min_io_size);
+	if (!err && min_io_size)
+		blk_queue_io_min(q, min_io_size);
 
-	err = virtio_config_val(vdev, VIRTIO_BLK_F_BLK_TOPOLOGY,
+	err = virtio_config_val(vdev, VIRTIO_BLK_F_TOPOLOGY,
 			offsetof(struct virtio_blk_config, opt_io_size),
 			&opt_io_size);
-	if (!err)
-		blk_queue_io_opt(vblk->disk->queue, opt_io_size);
+	if (!err && opt_io_size)
+		blk_queue_io_opt(q, opt_io_size);
 
 
 	add_disk(vblk->disk);

include/linux/virtio_blk.h +13 -0

Index: linux-2.6/include/linux/virtio_blk.h
===================================================================
@@ -15,6 +15,7 @@
 #define VIRTIO_BLK_F_BLK_SIZE	6	/* Block size of disk is available*/
 #define VIRTIO_BLK_F_SCSI	7	/* Supports scsi command passthru */
 #define VIRTIO_BLK_F_FLUSH	9	/* Cache flush command support */
+#define VIRTIO_BLK_F_TOPOLOGY	10	/* Topology information is available */
 
 struct virtio_blk_config {
 	/* The capacity (in 512-byte sectors). */

@@ -29,8 +30,20 @@ struct virtio_blk_config {
 		__u8 heads;
 		__u8 sectors;
 	} geometry;
+
 	/* block size of device (if VIRTIO_BLK_F_BLK_SIZE) */
 	__u32 blk_size;
+
+	/* the next 4 entries are covered by VIRTIO_BLK_F_TOPOLOGY  */
+	/* exponent for physical block per logical block. */
+	__u8 physical_block_exp;
+	/* alignment offset in logical blocks. */
+	__u8 alignment_offset;
+	/* minimum I/O size without performance penalty in bytes. */
+	__u16 min_io_size;
+	/* optimal sustained I/O size. */
+	__u32 opt_io_size;
+
 } __attribute__((packed));
 
 /*

Christoph Hellwig

author

2010-01-30 20:19 UTC #2 lore raw

Quoted 1 line · Rusty Russell

I bow to your expertise on that. My only query is the __u16 for min_io_size; is that likely to restrict us?

Looks like you caught me there - I wrote the above odd format about the physical_block exponent, but scsi actually does the min_io and opt_io size in logical blocks, too. With that in account the u16 as in scsi is perfectly fine.

Quoted 1 line

Also, patch seems to be based on a prior one?

Yes, quilt once again totally messed up the patch, sorry.

New version below that uses block based values everywhere (but still the strange exponent for the physical block size) and actually contains all the needed hunks. I'll make sure qemu also advertizes the values in blocks. Btw, I remember you had a virtio protocol documentation somewhere - is that one only for the base virtio protocol or does it also include virtio-blk? I'll prepare some patches for it if it includes the latter.

---

From: Christoph Hellwig <[email protected]>
Subject: [PATCH] virtio_blk: add block topology support

Allow reading various alignment values from the config page. This allows the guest to much better align I/O requests depending on the storage topology.

Note that the formats for the config values appear a bit messed up, but we follow the formats used by ATA and SCSI so they are expected in the storage world.

Signed-off-by Christoph Hellwig <[email protected]>

drivers/block/virtio_blk.c +46 ~~-15~~

Index: linux-2.6/drivers/block/virtio_blk.c
===================================================================
@@ -243,10 +243,12 @@ static int index_to_minor(int index)
 static int __devinit virtblk_probe(struct virtio_device *vdev)
 {
 	struct virtio_blk *vblk;
+	struct request_queue *q;
 	int err;
 	u64 cap;
-	u32 v;
-	u32 blk_size, sg_elems;
+	u32 v, blk_size, sg_elems, opt_io_size;
+	u16 min_io_size;
+	u8 physical_block_exp, alignment_offset;
 
 	if (index_to_minor(index) >= 1 << MINORBITS)
 		return -ENOSPC;

@@ -293,13 +295,13 @@ static int __devinit virtblk_probe(struc
 		goto out_mempool;
 	}
 
-	vblk->disk->queue = blk_init_queue(do_virtblk_request, &vblk->lock);
-	if (!vblk->disk->queue) {
+	q = vblk->disk->queue = blk_init_queue(do_virtblk_request, &vblk->lock);
+	if (!q) {
 		err = -ENOMEM;
 		goto out_put_disk;
 	}
 
-	vblk->disk->queue->queuedata = vblk;
+	q->queuedata = vblk;
 
 	if (index < 26) {
 		sprintf(vblk->disk->disk_name, "vd%c", 'a' + index % 26);

@@ -323,10 +325,10 @@ static int __devinit virtblk_probe(struc
 
 	/* If barriers are supported, tell block layer that queue is ordered */
 	if (virtio_has_feature(vdev, VIRTIO_BLK_F_FLUSH))
-		blk_queue_ordered(vblk->disk->queue, QUEUE_ORDERED_DRAIN_FLUSH,
+		blk_queue_ordered(q, QUEUE_ORDERED_DRAIN_FLUSH,
 				  virtblk_prepare_flush);
 	else if (virtio_has_feature(vdev, VIRTIO_BLK_F_BARRIER))
-		blk_queue_ordered(vblk->disk->queue, QUEUE_ORDERED_TAG, NULL);
+		blk_queue_ordered(q, QUEUE_ORDERED_TAG, NULL);
 
 	/* If disk is read-only in the host, the guest should obey */
 	if (virtio_has_feature(vdev, VIRTIO_BLK_F_RO))

@@ -345,14 +347,14 @@ static int __devinit virtblk_probe(struc
 	set_capacity(vblk->disk, cap);
 
 	/* We can handle whatever the host told us to handle. */
-	blk_queue_max_phys_segments(vblk->disk->queue, vblk->sg_elems-2);
-	blk_queue_max_hw_segments(vblk->disk->queue, vblk->sg_elems-2);
+	blk_queue_max_phys_segments(q, vblk->sg_elems-2);
+	blk_queue_max_hw_segments(q, vblk->sg_elems-2);
 
 	/* No need to bounce any requests */
-	blk_queue_bounce_limit(vblk->disk->queue, BLK_BOUNCE_ANY);
+	blk_queue_bounce_limit(q, BLK_BOUNCE_ANY);
 
 	/* No real sector limit. */
-	blk_queue_max_sectors(vblk->disk->queue, -1U);
+	blk_queue_max_sectors(q, -1U);
 
 	/* Host can optionally specify maximum segment size and number of
 	 * segments. */

@@ -360,16 +362,45 @@ static int __devinit virtblk_probe(struc
 				offsetof(struct virtio_blk_config, size_max),
 				&v);
 	if (!err)
-		blk_queue_max_segment_size(vblk->disk->queue, v);
+		blk_queue_max_segment_size(q, v);
 	else
-		blk_queue_max_segment_size(vblk->disk->queue, -1U);
+		blk_queue_max_segment_size(q, -1U);
 
 	/* Host can optionally specify the block size of the device */
 	err = virtio_config_val(vdev, VIRTIO_BLK_F_BLK_SIZE,
 				offsetof(struct virtio_blk_config, blk_size),
 				&blk_size);
 	if (!err)
-		blk_queue_logical_block_size(vblk->disk->queue, blk_size);
+		blk_queue_logical_block_size(q, blk_size);
+	else
+		blk_size = queue_logical_block_size(q);
+
+	/* Use topology information if available */
+	err = virtio_config_val(vdev, VIRTIO_BLK_F_TOPOLOGY,
+			offsetof(struct virtio_blk_config, physical_block_exp),
+			&physical_block_exp);
+	if (!err && physical_block_exp)
+		blk_queue_physical_block_size(q,
+				blk_size * (1 << physical_block_exp));
+
+	err = virtio_config_val(vdev, VIRTIO_BLK_F_TOPOLOGY,
+			offsetof(struct virtio_blk_config, alignment_offset),
+			&alignment_offset);
+	if (!err && alignment_offset)
+		blk_queue_alignment_offset(q, blk_size * alignment_offset);
+
+	err = virtio_config_val(vdev, VIRTIO_BLK_F_TOPOLOGY,
+			offsetof(struct virtio_blk_config, min_io_size),
+			&min_io_size);
+	if (!err && min_io_size)
+		blk_queue_io_min(q, blk_size * min_io_size);
+
+	err = virtio_config_val(vdev, VIRTIO_BLK_F_TOPOLOGY,
+			offsetof(struct virtio_blk_config, opt_io_size),
+			&opt_io_size);
+	if (!err && opt_io_size)
+		blk_queue_io_opt(q, blk_size * opt_io_size);
+
 
 	add_disk(vblk->disk);
 	return 0;

@@ -412,7 +443,7 @@ static struct virtio_device_id id_table[
 static unsigned int features[] = {
 	VIRTIO_BLK_F_BARRIER, VIRTIO_BLK_F_SEG_MAX, VIRTIO_BLK_F_SIZE_MAX,
 	VIRTIO_BLK_F_GEOMETRY, VIRTIO_BLK_F_RO, VIRTIO_BLK_F_BLK_SIZE,
-	VIRTIO_BLK_F_SCSI, VIRTIO_BLK_F_FLUSH
+	VIRTIO_BLK_F_SCSI, VIRTIO_BLK_F_FLUSH, VIRTIO_BLK_F_TOPOLOGY
 };
 
 /*

include/linux/virtio_blk.h +13 -0

Index: linux-2.6/include/linux/virtio_blk.h
===================================================================
@@ -15,6 +15,7 @@
 #define VIRTIO_BLK_F_BLK_SIZE	6	/* Block size of disk is available*/
 #define VIRTIO_BLK_F_SCSI	7	/* Supports scsi command passthru */
 #define VIRTIO_BLK_F_FLUSH	9	/* Cache flush command support */
+#define VIRTIO_BLK_F_TOPOLOGY	10	/* Topology information is available */
 
 struct virtio_blk_config {
 	/* The capacity (in 512-byte sectors). */

@@ -29,8 +30,20 @@ struct virtio_blk_config {
 		__u8 heads;
 		__u8 sectors;
 	} geometry;
+
 	/* block size of device (if VIRTIO_BLK_F_BLK_SIZE) */
 	__u32 blk_size;
+
+	/* the next 4 entries are guarded by VIRTIO_BLK_F_TOPOLOGY  */
+	/* exponent for physical block per logical block. */
+	__u8 physical_block_exp;
+	/* alignment offset in logical blocks. */
+	__u8 alignment_offset;
+	/* minimum I/O size without performance penalty in logical blocks. */
+	__u16 min_io_size;
+	/* optimal sustained I/O size in logical blocks. */
+	__u32 opt_io_size;
+
 } __attribute__((packed));
 
 /*

Rusty Russell

Applied

2010-02-01 01:10 UTC #3 lore raw

Quoted 7 lines · Christoph Hellwig

Quoted 1 line · Rusty Russell
I bow to your expertise on that. My only query is the __u16 for min_io_size; is that likely to restrict us?
Looks like you caught me there - I wrote the above odd format about the physical_block exponent, but scsi actually does the min_io and opt_io size in logical blocks, too. With that in account the u16 as in scsi is perfectly fine.

Thanks, applied.

Rusty.

Alexey Zaytsev

2011-07-14 23:36 UTC #4 lore raw

Quoted 10 lines · Rusty Russell

Quoted 7 lines · Christoph Hellwig
Quoted 1 line · Rusty Russell
I bow to your expertise on that. My only query is the __u16 for min_io_size; is that likely to restrict us?
Looks like you caught me there - I wrote the above odd format about the physical_block exponent, but scsi actually does the min_io and opt_io size in logical blocks, too. With that in account the u16 as in scsi is perfectly fine.
Thanks, applied.

Ugh, guys. I know it's already applied long ago, but this kind of contradicts the virtio specification, doesn't it?

VIRTIO_BLK_F_SECTOR_MAX (10) Maximum total sectors in
an I/O.
[...]

4. If the VIRTIO_BLK_F_SECTOR_MAX feature is negotiated, the sec- tors_max eld should be read to determine the maximum I/O size for the driver to use. No requests should be submitted which go beyond this limit.

struct virtio_blk_config        {
        u64  capacity ;
        u32  si z e_ m ax ;
        u32  seg_max ;
        struct     virtio_blk_geometry {
                    u16   cylinders ;
                    u8  heads ;
                    u8  sectors ;
        } geometry ;
        u32  blk_size ;
        u32  sectors_max ;
};

Rusty Russell

2011-07-21 09:33 UTC #5 lore raw

Quoted 10 lines · Alexey Zaytsev

Quoted 9 lines · Rusty Russell
Quoted 7 lines · Christoph Hellwig
Quoted 1 line · Rusty Russell
I bow to your expertise on that. My only query is the __u16 for min_io_size; is that likely to restrict us?
Looks like you caught me there - I wrote the above odd format about the physical_block exponent, but scsi actually does the min_io and opt_io size in logical blocks, too. With that in account the u16 as in scsi is perfectly fine.

Quoted 4 lines

Quoted 1 line
Thanks, applied.
Ugh, guys. I know it's already applied long ago, but this kind of contradicts the virtio specification, doesn't it?

Ugh indeed! The same field is used by two places, as VIRTIO_BLK_F_SECTOR_MAX never made it into the linux headers.

Quoted 2 lines

VIRTIO_BLK_F_SECTOR_MAX (10) Maximum total sectors in
an I/O.

The patch made the spec, but as far as I can tell, no implementation.

If that's right, we just remove it from the spec. If it did make it in, we now have a very ugly case where the layout will have to vary depending on what options are negotiated.

Once we've resolved this, I'll update the spec...

Thanks,
Rusty.

Alexey Zaytsev

2011-07-29 19:09 UTC #6 lore raw

Quoted 19 lines · Rusty Russell

Quoted 10 lines · Alexey Zaytsev
Quoted 9 lines · Rusty Russell
Quoted 7 lines · Christoph Hellwig
Quoted 1 line · Rusty Russell
I bow to your expertise on that. My only query is the __u16 for min_io_size; is that likely to restrict us?
Looks like you caught me there - I wrote the above odd format about the physical_block exponent, but scsi actually does the min_io and opt_io size in logical blocks, too. With that in account the u16 as in scsi is perfectly fine.
Quoted 4 lines
Quoted 1 line
Thanks, applied.
Ugh, guys. I know it's already applied long ago, but this kind of contradicts the virtio specification, doesn't it?
Ugh indeed! The same field is used by two places, as VIRTIO_BLK_F_SECTOR_MAX never made it into the linux headers.

Quoted 8 lines

Quoted 2 lines
VIRTIO_BLK_F_SECTOR_MAX (10) Maximum total sectors in an I/O.
The patch made the spec, but as far as I can tell, no implementation.
If that's right, we just remove it from the spec. If it did make it in, we now have a very ugly case where the layout will have to vary depending on what options are negotiated.

VIRTIO_BLK_F_TOPOLOGY and VIRTIO_BLK_F_SECTOR_MAX features use the same bit, don't they?

I guess you could just remove VIRTIO_BLK_F_SECTOR_MAX from the spec. Linux did not implement it, and virtualbox does not have virtio block support at all. Are there any other known users, who might have implemented the VIRTIO_BLK_F_SECTOR_MAX feature?

Rusty Russell

2011-07-30 05:20 UTC #7 lore raw

Quoted 2 lines · Alexey Zaytsev

VIRTIO_BLK_F_TOPOLOGY and VIRTIO_BLK_F_SECTOR_MAX features use the same bit, don't they?

Yes, spec bug.

Quoted 4 lines

I guess you could just remove VIRTIO_BLK_F_SECTOR_MAX from the spec. Linux did not implement it, and virtualbox does not have virtio block support at all. Are there any other known users, who might have implemented the VIRTIO_BLK_F_SECTOR_MAX feature?

Did qemu implement it? If not, we'll simply kill it, and pretend it never happened...

Thanks,
Rusty.

Christoph Hellwig

author

2011-07-31 18:19 UTC #8 lore raw

Quoted 7 lines · Rusty Russell

Quoted 4 lines
I guess you could just remove VIRTIO_BLK_F_SECTOR_MAX from the spec. Linux did not implement it, and virtualbox does not have virtio block support at all. Are there any other known users, who might have implemented the VIRTIO_BLK_F_SECTOR_MAX feature?
Did qemu implement it? If not, we'll simply kill it, and pretend it never happened...

It was never added to either qemu or the kernel. It would really help if we had a copy of the spec in a readable format (that is plain text) in major implementations of virtio like qemu and the kernel.

Rusty Russell

2011-08-01 23:25 UTC #9 lore raw

Quoted 6 lines · Christoph Hellwig

Quoted 5 lines · Rusty Russell
Quoted 4 lines
I guess you could just remove VIRTIO_BLK_F_SECTOR_MAX from the spec. Linux did not implement it, and virtualbox does not have virtio block support at all. Are there any other known users, who might have implemented the VIRTIO_BLK_F_SECTOR_MAX feature?

Quoted 6 lines

Quoted 2 lines
Did qemu implement it? If not, we'll simply kill it, and pretend it never happened...
It was never added to either qemu or the kernel. It would really help if we had a copy of the spec in a readable format (that is plain text) in major implementations of virtio like qemu and the kernel.

That's a really good idea. I've prepared a patch which puts the latest version in the kernel source (Documentation/virtio/virtio-spec.txt).

Thanks!
Rusty.