[PATCH/RFC] virtio_blk: check for hardsector size from host

[PATCH/RFC] virtio_blk: check for hardsector size from host

[Christian Borntraeger]

Rusty, Jens,

I need your opinion on the following patch. It seems to work, but I would like 
to get some feedback if this patch is the right approach:

---

Currently virtio_blk assumes a 512 byte hard sector size. This can cause 
trouble / performance issues if the backing has a different block size
(like a file on an ext3 file system formatted with 4k block size or a dasd 
device).

Lets add a feature flag that tells the guest to use a different hard sector
size than 512 byte. The host can use stat->st_blksize to determine the right
hard sector size for a specific backing.

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 drivers/block/virtio_blk.c |   10 +++++++++-
 include/linux/virtio_blk.h |    3 +++
 2 files changed, 12 insertions(+), 1 deletion(-)

Index: kvm/drivers/block/virtio_blk.c
===================================================================
--- kvm.orig/drivers/block/virtio_blk.c
+++ kvm/drivers/block/virtio_blk.c
@@ -196,6 +196,7 @@ static int virtblk_probe(struct virtio_d
 	int err;
 	u64 cap;
 	u32 v;
+	u64 blk_size;
 
 	if (index_to_minor(index) >= 1 << MINORBITS)
 		return -ENOSPC;
@@ -290,6 +291,13 @@ static int virtblk_probe(struct virtio_d
 	if (!err)
 		blk_queue_max_hw_segments(vblk->disk->queue, v);
 
+	/* 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_hardsect_size(vblk->disk->queue, blk_size);
+
 	add_disk(vblk->disk);
 	return 0;
 
@@ -329,7 +337,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_GEOMETRY, VIRTIO_BLK_F_RO, VIRTIO_BLK_F_BLK_SIZE,
 };
 
 static struct virtio_driver virtio_blk = {
Index: kvm/include/linux/virtio_blk.h
===================================================================
--- kvm.orig/include/linux/virtio_blk.h
+++ kvm/include/linux/virtio_blk.h
@@ -11,6 +11,7 @@
 #define VIRTIO_BLK_F_SEG_MAX	2	/* Indicates maximum # of segments */
 #define VIRTIO_BLK_F_GEOMETRY	4	/* Legacy geometry available  */
 #define VIRTIO_BLK_F_RO		5	/* Disk is read-only */
+#define VIRTIO_BLK_F_BLK_SIZE	6	/* Block size of disk is available*/
 
 struct virtio_blk_config
 {
@@ -26,6 +27,8 @@ struct virtio_blk_config
 		__u8 heads;
 		__u8 sectors;
 	} geometry;
+	/* block size of device (if VIRTIO_BLK_F_BLK_SIZE) */
+	__u64 blk_size;
 } __attribute__((packed));
 
 /* These two define direction. */

Re: [PATCH/RFC] virtio_blk: check for hardsector size from host

[Rusty Russell]

On Tuesday 27 May 2008 19:04:59 Christian Borntraeger wrote:
> Rusty, Jens,
>
> I need your opinion on the following patch. It seems to work, but I would
> like to get some feedback if this patch is the right approach:

Looks like the right approach to me.  Don't know about the block side of it...

Just that u64 seems like overkill: u32?

Cheers,
Rusty.

Re: [PATCH/RFC] virtio_blk: check for hardsector size from host

[Rusty Russell]

On Tuesday 27 May 2008 19:04:59 Christian Borntraeger wrote:
> Rusty, Jens,
>
> I need your opinion on the following patch. It seems to work, but I would
> like to get some feedback if this patch is the right approach:

Looks like the right approach to me.  Don't know about the block side of it...

Just that u64 seems like overkill: u32?

Cheers,
Rusty.

Re: [PATCH/RFC] virtio_blk: check for hardsector size from host

[Christian Borntraeger]

Am Donnerstag, 29. Mai 2008 schrieb Rusty Russell:
> On Tuesday 27 May 2008 19:04:59 Christian Borntraeger wrote:
> > Rusty, Jens,
> >
> > I need your opinion on the following patch. It seems to work, but I would
> > like to get some feedback if this patch is the right approach:
> 
> Looks like the right approach to me.  Don't know about the block side of 
it...
> 
> Just that u64 seems like overkill: u32?

I looked at blksize_t, which seems to be long.  So I decided to use u64 which 
should be enough (but still overkill...) on all platforms.

While we are at it. Since we dont convert endianess any longer, should I 
change all the __le{..} definitions to __u{..} in virtio_blk.h ?

Christian

Re: [PATCH/RFC] virtio_blk: check for hardsector size from host

[Christian Borntraeger]

Am Donnerstag, 29. Mai 2008 schrieb Rusty Russell:
> On Tuesday 27 May 2008 19:04:59 Christian Borntraeger wrote:
> > Rusty, Jens,
> >
> > I need your opinion on the following patch. It seems to work, but I would
> > like to get some feedback if this patch is the right approach:
> 
> Looks like the right approach to me.  Don't know about the block side of 
it...
> 
> Just that u64 seems like overkill: u32?

I looked at blksize_t, which seems to be long.  So I decided to use u64 which 
should be enough (but still overkill...) on all platforms.

While we are at it. Since we dont convert endianess any longer, should I 
change all the __le{..} definitions to __u{..} in virtio_blk.h ?

Christian

Re: [PATCH/RFC] virtio_blk: check for hardsector size from host

[Jens Axboe]

On Thu, May 29 2008, Rusty Russell wrote:
> On Tuesday 27 May 2008 19:04:59 Christian Borntraeger wrote:
> > Rusty, Jens,
> >
> > I need your opinion on the following patch. It seems to work, but I would
> > like to get some feedback if this patch is the right approach:
> 
> Looks like the right approach to me.  Don't know about the block side
> of it...

Looks good to me, definitely an improvement for eg loop backed
storage.

> Just that u64 seems like overkill: u32?

Definitely, u32 would be just fine, u64 is way overkill :-)

-- 
Jens Axboe

Re: [PATCH/RFC] virtio_blk: check for hardsector size from host

[Jens Axboe]

On Thu, May 29 2008, Rusty Russell wrote:
> On Tuesday 27 May 2008 19:04:59 Christian Borntraeger wrote:
> > Rusty, Jens,
> >
> > I need your opinion on the following patch. It seems to work, but I would
> > like to get some feedback if this patch is the right approach:
> 
> Looks like the right approach to me.  Don't know about the block side
> of it...

Looks good to me, definitely an improvement for eg loop backed
storage.

> Just that u64 seems like overkill: u32?

Definitely, u32 would be just fine, u64 is way overkill :-)

-- 
Jens Axboe

Re: [PATCH/RFC] virtio_blk: check for hardsector size from host

[Jens Axboe]

On Thu, May 29 2008, Jens Axboe wrote:
> On Thu, May 29 2008, Rusty Russell wrote:
> > On Tuesday 27 May 2008 19:04:59 Christian Borntraeger wrote:
> > > Rusty, Jens,
> > >
> > > I need your opinion on the following patch. It seems to work, but I would
> > > like to get some feedback if this patch is the right approach:
> > 
> > Looks like the right approach to me.  Don't know about the block side
> > of it...
> 
> Looks good to me, definitely an improvement for eg loop backed
> storage.
> 
> > Just that u64 seems like overkill: u32?
> 
> Definitely, u32 would be just fine, u64 is way overkill :-)

Even u16 would work, the block layer doesn't use more than an unsigned
short for storing hardware sector size anyway.

-- 
Jens Axboe

Re: [PATCH/RFC] virtio_blk: check for hardsector size from host

[Jens Axboe]

On Thu, May 29 2008, Jens Axboe wrote:
> On Thu, May 29 2008, Rusty Russell wrote:
> > On Tuesday 27 May 2008 19:04:59 Christian Borntraeger wrote:
> > > Rusty, Jens,
> > >
> > > I need your opinion on the following patch. It seems to work, but I would
> > > like to get some feedback if this patch is the right approach:
> > 
> > Looks like the right approach to me.  Don't know about the block side
> > of it...
> 
> Looks good to me, definitely an improvement for eg loop backed
> storage.
> 
> > Just that u64 seems like overkill: u32?
> 
> Definitely, u32 would be just fine, u64 is way overkill :-)

Even u16 would work, the block layer doesn't use more than an unsigned
short for storing hardware sector size anyway.

-- 
Jens Axboe

Re: [PATCH/RFC] virtio_blk: check for hardsector size from host

[Christian Borntraeger]

Am Donnerstag, 29. Mai 2008 schrieb Jens Axboe:
> > > Just that u64 seems like overkill: u32?
> > 
> > Definitely, u32 would be just fine, u64 is way overkill :-)
> 
> Even u16 would work, the block layer doesn't use more than an unsigned
> short for storing hardware sector size anyway.

Thanks, good to know. Do you think, that could change in the future? The 
virtio definition is going to be a public interface, so if there is a chance 
that u16 is not enough in the future I would respin the patch with u32, 
otherwise u16.

Christian

Re: [PATCH/RFC] virtio_blk: check for hardsector size from host

[Jens Axboe]

On Thu, May 29 2008, Christian Borntraeger wrote:
> Am Donnerstag, 29. Mai 2008 schrieb Jens Axboe:
> > > > Just that u64 seems like overkill: u32?
> > > 
> > > Definitely, u32 would be just fine, u64 is way overkill :-)
> > 
> > Even u16 would work, the block layer doesn't use more than an unsigned
> > short for storing hardware sector size anyway.
> 
> Thanks, good to know. Do you think, that could change in the future?
> The virtio definition is going to be a public interface, so if there
> is a chance that u16 is not enough in the future I would respin the
> patch with u32, otherwise u16.

I'd say go with the u32, it's the safest option for an exported
interface.

-- 
Jens Axboe

[PATCH] virtio_config: fix len calculation of config elements

[Christian Borntraeger]

Rusty,

This patch is a prereq for the virtio_blk blocksize patch, please apply it 
first.

Adding an u32 value to the virtio_blk_config unconvered a small bug the config 
space defintions:
v is a pointer, to we have to use sizeof(*v) instead of sizeof(v).

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 include/linux/virtio_config.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: kvm/include/linux/virtio_config.h
===================================================================
--- kvm.orig/include/linux/virtio_config.h
+++ kvm/include/linux/virtio_config.h
@@ -99,7 +99,7 @@ static inline bool virtio_has_feature(co
  * The return value is -ENOENT if the feature doesn't exist.  Otherwise
  * the config value is copied into whatever is pointed to by v. */
 #define virtio_config_val(vdev, fbit, offset, v) \
-	virtio_config_buf((vdev), (fbit), (offset), (v), sizeof(v))
+	virtio_config_buf((vdev), (fbit), (offset), (v), sizeof(*v))
 
 static inline int virtio_config_buf(struct virtio_device *vdev,
 				    unsigned int fbit,

[PATCH] virtio_config: fix len calculation of config elements

[Christian Borntraeger]

Rusty,

This patch is a prereq for the virtio_blk blocksize patch, please apply it 
first.

Adding an u32 value to the virtio_blk_config unconvered a small bug the config 
space defintions:
v is a pointer, to we have to use sizeof(*v) instead of sizeof(v).

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 include/linux/virtio_config.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: kvm/include/linux/virtio_config.h
===================================================================
--- kvm.orig/include/linux/virtio_config.h
+++ kvm/include/linux/virtio_config.h
@@ -99,7 +99,7 @@ static inline bool virtio_has_feature(co
  * The return value is -ENOENT if the feature doesn't exist.  Otherwise
  * the config value is copied into whatever is pointed to by v. */
 #define virtio_config_val(vdev, fbit, offset, v) \
-	virtio_config_buf((vdev), (fbit), (offset), (v), sizeof(v))
+	virtio_config_buf((vdev), (fbit), (offset), (v), sizeof(*v))
 
 static inline int virtio_config_buf(struct virtio_device *vdev,
 				    unsigned int fbit,

Re: [PATCH] virtio_config: fix len calculation of config elements

[Rusty Russell]

On Thursday 29 May 2008 19:08:01 Christian Borntraeger wrote:
> v is a pointer, to we have to use sizeof(*v) instead of sizeof(v).

How embarassing.  Applied, thanks.

Rusty.

Re: [PATCH] virtio_config: fix len calculation of config elements

[Rusty Russell]

On Thursday 29 May 2008 19:08:01 Christian Borntraeger wrote:
> v is a pointer, to we have to use sizeof(*v) instead of sizeof(v).

How embarassing.  Applied, thanks.

Rusty.

[PATCH v2] virtio_blk: check for hardsector size from host

[Christian Borntraeger]

Currently virtio_blk assumes a 512 byte hard sector size. This can cause 
trouble / performance issues if the backing has a different block size
(like a file on an ext3 file system formatted with 4k block size or a dasd).

Lets add a feature flag that tells the guest to use a different hard sector
size than 512 byte.

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 drivers/block/virtio_blk.c |   10 +++++++++-
 include/linux/virtio_blk.h |    3 +++
 2 files changed, 12 insertions(+), 1 deletion(-)

Index: kvm/drivers/block/virtio_blk.c
===================================================================
--- kvm.orig/drivers/block/virtio_blk.c
+++ kvm/drivers/block/virtio_blk.c
@@ -196,6 +196,7 @@ static int virtblk_probe(struct virtio_d
 	int err;
 	u64 cap;
 	u32 v;
+	u32 blk_size;
 
 	if (index_to_minor(index) >= 1 << MINORBITS)
 		return -ENOSPC;
@@ -290,6 +291,13 @@ static int virtblk_probe(struct virtio_d
 	if (!err)
 		blk_queue_max_hw_segments(vblk->disk->queue, v);
 
+	/* 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_hardsect_size(vblk->disk->queue, blk_size);
+
 	add_disk(vblk->disk);
 	return 0;
 
@@ -329,7 +337,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_GEOMETRY, VIRTIO_BLK_F_RO, VIRTIO_BLK_F_BLK_SIZE,
 };
 
 static struct virtio_driver virtio_blk = {
Index: kvm/include/linux/virtio_blk.h
===================================================================
--- kvm.orig/include/linux/virtio_blk.h
+++ kvm/include/linux/virtio_blk.h
@@ -11,6 +11,7 @@
 #define VIRTIO_BLK_F_SEG_MAX	2	/* Indicates maximum # of segments */
 #define VIRTIO_BLK_F_GEOMETRY	4	/* Legacy geometry available  */
 #define VIRTIO_BLK_F_RO		5	/* Disk is read-only */
+#define VIRTIO_BLK_F_BLK_SIZE	6	/* Block size of disk is available*/
 
 struct virtio_blk_config
 {
@@ -26,6 +27,8 @@ struct virtio_blk_config
 		__u8 heads;
 		__u8 sectors;
 	} geometry;
+	/* block size of device (if VIRTIO_BLK_F_BLK_SIZE) */
+	__u32 blk_size;
 } __attribute__((packed));
 
 /* These two define direction. */

Re: [PATCH v2] virtio_blk: check for hardsector size from host

[Rusty Russell]

On Thursday 29 May 2008 19:08:26 Christian Borntraeger wrote:
> Currently virtio_blk assumes a 512 byte hard sector size. This can cause
> trouble / performance issues if the backing has a different block size
> (like a file on an ext3 file system formatted with 4k block size or a
> dasd).
>
> Lets add a feature flag that tells the guest to use a different hard sector
> size than 512 byte.
>
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>

Thanks.  I've queued this for next kernel (the other two will go 
striaght-away).

Cheers,
Rusty.

Re: [PATCH v2] virtio_blk: check for hardsector size from host

[Rusty Russell]

On Thursday 29 May 2008 19:08:26 Christian Borntraeger wrote:
> Currently virtio_blk assumes a 512 byte hard sector size. This can cause
> trouble / performance issues if the backing has a different block size
> (like a file on an ext3 file system formatted with 4k block size or a
> dasd).
>
> Lets add a feature flag that tells the guest to use a different hard sector
> size than 512 byte.
>
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>

Thanks.  I've queued this for next kernel (the other two will go 
striaght-away).

Cheers,
Rusty.

[PATCH v2] virtio_blk: check for hardsector size from host

[Christian Borntraeger]

Currently virtio_blk assumes a 512 byte hard sector size. This can cause 
trouble / performance issues if the backing has a different block size
(like a file on an ext3 file system formatted with 4k block size or a dasd).

Lets add a feature flag that tells the guest to use a different hard sector
size than 512 byte.

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 drivers/block/virtio_blk.c |   10 +++++++++-
 include/linux/virtio_blk.h |    3 +++
 2 files changed, 12 insertions(+), 1 deletion(-)

Index: kvm/drivers/block/virtio_blk.c
===================================================================
--- kvm.orig/drivers/block/virtio_blk.c
+++ kvm/drivers/block/virtio_blk.c
@@ -196,6 +196,7 @@ static int virtblk_probe(struct virtio_d
 	int err;
 	u64 cap;
 	u32 v;
+	u32 blk_size;
 
 	if (index_to_minor(index) >= 1 << MINORBITS)
 		return -ENOSPC;
@@ -290,6 +291,13 @@ static int virtblk_probe(struct virtio_d
 	if (!err)
 		blk_queue_max_hw_segments(vblk->disk->queue, v);
 
+	/* 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_hardsect_size(vblk->disk->queue, blk_size);
+
 	add_disk(vblk->disk);
 	return 0;
 
@@ -329,7 +337,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_GEOMETRY, VIRTIO_BLK_F_RO, VIRTIO_BLK_F_BLK_SIZE,
 };
 
 static struct virtio_driver virtio_blk = {
Index: kvm/include/linux/virtio_blk.h
===================================================================
--- kvm.orig/include/linux/virtio_blk.h
+++ kvm/include/linux/virtio_blk.h
@@ -11,6 +11,7 @@
 #define VIRTIO_BLK_F_SEG_MAX	2	/* Indicates maximum # of segments */
 #define VIRTIO_BLK_F_GEOMETRY	4	/* Legacy geometry available  */
 #define VIRTIO_BLK_F_RO		5	/* Disk is read-only */
+#define VIRTIO_BLK_F_BLK_SIZE	6	/* Block size of disk is available*/
 
 struct virtio_blk_config
 {
@@ -26,6 +27,8 @@ struct virtio_blk_config
 		__u8 heads;
 		__u8 sectors;
 	} geometry;
+	/* block size of device (if VIRTIO_BLK_F_BLK_SIZE) */
+	__u32 blk_size;
 } __attribute__((packed));
 
 /* These two define direction. */

Re: [PATCH/RFC] virtio_blk: check for hardsector size from host

[Jens Axboe]

On Thu, May 29 2008, Christian Borntraeger wrote:
> Am Donnerstag, 29. Mai 2008 schrieb Jens Axboe:
> > > > Just that u64 seems like overkill: u32?
> > > 
> > > Definitely, u32 would be just fine, u64 is way overkill :-)
> > 
> > Even u16 would work, the block layer doesn't use more than an unsigned
> > short for storing hardware sector size anyway.
> 
> Thanks, good to know. Do you think, that could change in the future?
> The virtio definition is going to be a public interface, so if there
> is a chance that u16 is not enough in the future I would respin the
> patch with u32, otherwise u16.

I'd say go with the u32, it's the safest option for an exported
interface.

-- 
Jens Axboe

Re: [PATCH/RFC] virtio_blk: check for hardsector size from host

[Christian Borntraeger]

Am Donnerstag, 29. Mai 2008 schrieb Jens Axboe:
> > > Just that u64 seems like overkill: u32?
> > 
> > Definitely, u32 would be just fine, u64 is way overkill :-)
> 
> Even u16 would work, the block layer doesn't use more than an unsigned
> short for storing hardware sector size anyway.

Thanks, good to know. Do you think, that could change in the future? The 
virtio definition is going to be a public interface, so if there is a chance 
that u16 is not enough in the future I would respin the patch with u32, 
otherwise u16.

Christian