[PATCH] virtio_blk: allow re-reading config space at runtime

[PATCH] virtio_blk: allow re-reading config space at runtime

[Christoph Hellwig]

Wire up the virtio_driver config_changed method to get notified about
config changes raised by the host.  For now we just re-read the device
size to support online resizing of devices, but once we add more
attributes that might be changeable they could be added as well.

Note that the config_changed method is called from irq context, so
we'll have to use the workqueue infrastructure to provide us a proper
user context for our changes.

Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: xfs/drivers/block/virtio_blk.c
===================================================================
--- xfs.orig/drivers/block/virtio_blk.c	2011-01-13 18:17:23.730254665 +0100
+++ xfs/drivers/block/virtio_blk.c	2011-01-14 16:57:50.572032906 +0100
@@ -6,10 +6,12 @@
 #include <linux/virtio.h>
 #include <linux/virtio_blk.h>
 #include <linux/scatterlist.h>
+#include <linux/string_helpers.h>
 
 #define PART_BITS 4
 
 static int major, index;
+struct workqueue_struct *virtblk_wq;
 
 struct virtio_blk
 {
@@ -42,6 +44,11 @@ struct virtblk_req
 	u8 status;
 };
 
+struct virtblk_config_change {
+	struct virtio_device *vdev;
+	struct work_struct work;
+};
+
 static void blk_done(struct virtqueue *vq)
 {
 	struct virtio_blk *vblk = vq->vdev->priv;
@@ -291,6 +298,57 @@ static ssize_t virtblk_serial_show(struc
 }
 DEVICE_ATTR(serial, S_IRUGO, virtblk_serial_show, NULL);
 
+static void virtblk_config_changed_work(struct work_struct *work)
+{
+	struct virtblk_config_change *cfg =
+		container_of(work, struct virtblk_config_change, work);
+	struct virtio_device *vdev = cfg->vdev;
+	struct virtio_blk *vblk = vdev->priv;
+	struct request_queue *q = vblk->disk->queue;
+	char cap_str_2[10], cap_str_10[10];
+	u64 capacity, size;
+
+	/* Host must always specify the capacity. */
+	vdev->config->get(vdev, offsetof(struct virtio_blk_config, capacity),
+			  &capacity, sizeof(capacity));
+
+	/* If capacity is too big, truncate with warning. */
+	if ((sector_t)capacity != capacity) {
+		dev_warn(&vdev->dev, "Capacity %llu too large: truncating\n",
+			 (unsigned long long)capacity);
+		capacity = (sector_t)-1;
+	}
+
+	size = capacity * queue_logical_block_size(q);
+	string_get_size(size, STRING_UNITS_2, cap_str_2, sizeof(cap_str_2));
+	string_get_size(size, STRING_UNITS_10, cap_str_10, sizeof(cap_str_10));
+
+	dev_notice(&vdev->dev,
+		  "new size: %llu %d-byte logical blocks (%s/%s)\n",
+		  (unsigned long long)capacity,
+		  queue_logical_block_size(q),
+		  cap_str_10, cap_str_2);
+
+	set_capacity(vblk->disk, capacity);
+
+}
+
+static void virtblk_config_changed(struct virtio_device *vdev)
+{
+	struct virtblk_config_change *cfg;
+
+	cfg = kmalloc(sizeof(*cfg), GFP_ATOMIC);
+	if (!cfg) {
+		dev_info(&vdev->dev, "skipping config change\n");
+		return;
+	}
+
+	cfg->vdev = vdev;
+	INIT_WORK(&cfg->work, virtblk_config_changed_work);
+	queue_work(virtblk_wq, &cfg->work);
+}
+
+
 static int __devinit virtblk_probe(struct virtio_device *vdev)
 {
 	struct virtio_blk *vblk;
@@ -508,27 +566,47 @@ static unsigned int features[] = {
  * Use __refdata to avoid this warning.
  */
 static struct virtio_driver __refdata virtio_blk = {
-	.feature_table = features,
-	.feature_table_size = ARRAY_SIZE(features),
-	.driver.name =	KBUILD_MODNAME,
-	.driver.owner =	THIS_MODULE,
-	.id_table =	id_table,
-	.probe =	virtblk_probe,
-	.remove =	__devexit_p(virtblk_remove),
+	.feature_table		= features,
+	.feature_table_size	= ARRAY_SIZE(features),
+	.driver.name		= KBUILD_MODNAME,
+	.driver.owner		= THIS_MODULE,
+	.id_table		= id_table,
+	.probe			= virtblk_probe,
+	.remove			= __devexit_p(virtblk_remove),
+	.config_changed		= virtblk_config_changed,
 };
 
 static int __init init(void)
 {
+	int error;
+
+	virtblk_wq = alloc_workqueue("md_misc", 0, 0);
+	if (!virtblk_wq)
+		return -ENOMEM;
+
 	major = register_blkdev(0, "virtblk");
-	if (major < 0)
-		return major;
-	return register_virtio_driver(&virtio_blk);
+	if (major < 0) {
+		error = major;
+		goto out_destroy_workqueue;
+	}
+
+	error = register_virtio_driver(&virtio_blk);
+	if (error)
+		goto out_unregister_blkdev;
+	return 0;
+
+out_unregister_blkdev:
+	unregister_blkdev(major, "virtblk");
+out_destroy_workqueue:
+	destroy_workqueue(virtblk_wq);
+	return error;
 }
 
 static void __exit fini(void)
 {
 	unregister_blkdev(major, "virtblk");
 	unregister_virtio_driver(&virtio_blk);
+	destroy_workqueue(virtblk_wq);
 }
 module_init(init);
 module_exit(fini);

Re: [PATCH] virtio_blk: allow re-reading config space at runtime

[Rusty Russell]

On Sat, 15 Jan 2011 02:31:37 am Christoph Hellwig wrote:
> Wire up the virtio_driver config_changed method to get notified about
> config changes raised by the host.  For now we just re-read the device
> size to support online resizing of devices, but once we add more
> attributes that might be changeable they could be added as well.
> 
> Note that the config_changed method is called from irq context, so
> we'll have to use the workqueue infrastructure to provide us a proper
> user context for our changes.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> Index: xfs/drivers/block/virtio_blk.c
> ===================================================================
> --- xfs.orig/drivers/block/virtio_blk.c	2011-01-13 18:17:23.730254665 +0100
> +++ xfs/drivers/block/virtio_blk.c	2011-01-14 16:57:50.572032906 +0100
> @@ -6,10 +6,12 @@
>  #include <linux/virtio.h>
>  #include <linux/virtio_blk.h>
>  #include <linux/scatterlist.h>
> +#include <linux/string_helpers.h>
>  
>  #define PART_BITS 4
>  
>  static int major, index;
> +struct workqueue_struct *virtblk_wq;
>  
>  struct virtio_blk
>  {
> @@ -42,6 +44,11 @@ struct virtblk_req
>  	u8 status;
>  };
>  
> +struct virtblk_config_change {
> +	struct virtio_device *vdev;
> +	struct work_struct work;
> +};
> +
>  static void blk_done(struct virtqueue *vq)
>  {
>  	struct virtio_blk *vblk = vq->vdev->priv;
> @@ -291,6 +298,57 @@ static ssize_t virtblk_serial_show(struc
>  }
>  DEVICE_ATTR(serial, S_IRUGO, virtblk_serial_show, NULL);
>  
> +static void virtblk_config_changed_work(struct work_struct *work)
> +{
> +	struct virtblk_config_change *cfg =
> +		container_of(work, struct virtblk_config_change, work);
> +	struct virtio_device *vdev = cfg->vdev;
> +	struct virtio_blk *vblk = vdev->priv;
> +	struct request_queue *q = vblk->disk->queue;
> +	char cap_str_2[10], cap_str_10[10];
> +	u64 capacity, size;
> +
> +	/* Host must always specify the capacity. */
> +	vdev->config->get(vdev, offsetof(struct virtio_blk_config, capacity),
> +			  &capacity, sizeof(capacity));
> +
> +	/* If capacity is too big, truncate with warning. */
> +	if ((sector_t)capacity != capacity) {
> +		dev_warn(&vdev->dev, "Capacity %llu too large: truncating\n",
> +			 (unsigned long long)capacity);
> +		capacity = (sector_t)-1;
> +	}
> +
> +	size = capacity * queue_logical_block_size(q);
> +	string_get_size(size, STRING_UNITS_2, cap_str_2, sizeof(cap_str_2));
> +	string_get_size(size, STRING_UNITS_10, cap_str_10, sizeof(cap_str_10));
> +
> +	dev_notice(&vdev->dev,
> +		  "new size: %llu %d-byte logical blocks (%s/%s)\n",
> +		  (unsigned long long)capacity,
> +		  queue_logical_block_size(q),
> +		  cap_str_10, cap_str_2);
> +
> +	set_capacity(vblk->disk, capacity);
> +
> +}
> +
> +static void virtblk_config_changed(struct virtio_device *vdev)
> +{
> +	struct virtblk_config_change *cfg;
> +
> +	cfg = kmalloc(sizeof(*cfg), GFP_ATOMIC);
> +	if (!cfg) {
> +		dev_info(&vdev->dev, "skipping config change\n");
> +		return;
> +	}

I think we need to do better than this.  What would it take?

Rusty.

Re: [PATCH] virtio_blk: allow re-reading config space at runtime

[Christoph Hellwig]

On Tue, Jan 18, 2011 at 11:38:50AM +1030, Rusty Russell wrote:
> I think we need to do better than this.  What would it take?

I don't think we have much choice.  We're getting called from irq
context, so we'll need to allocate memory to offload things.  That's
unless we keep a thread around that we just need to wake up, which
would be a huge waste of ressources.

Re: [PATCH] virtio_blk: allow re-reading config space at runtime

[Rusty Russell]

On Tue, 18 Jan 2011 10:57:51 pm Christoph Hellwig wrote:
> On Tue, Jan 18, 2011 at 11:38:50AM +1030, Rusty Russell wrote:
> > I think we need to do better than this.  What would it take?
> 
> I don't think we have much choice.  We're getting called from irq
> context, so we'll need to allocate memory to offload things.  That's
> unless we keep a thread around that we just need to wake up, which
> would be a huge waste of ressources.

Yes, but can we keep a workqueue struct around?

Rusty.

Re: [PATCH] virtio_blk: allow re-reading config space at runtime

[Christoph Hellwig]

On Wed, Jan 19, 2011 at 09:56:09AM +1030, Rusty Russell wrote:
> > I don't think we have much choice.  We're getting called from irq
> > context, so we'll need to allocate memory to offload things.  That's
> > unless we keep a thread around that we just need to wake up, which
> > would be a huge waste of ressources.
> 
> Yes, but can we keep a workqueue struct around?

We can keep one around, but we can't prevent the virtio core code from
calling us again while it's still in use.

Re: [PATCH] virtio_blk: allow re-reading config space at runtime

[Michael S. Tsirkin]

On Wed, Jan 19, 2011 at 12:54:01AM +0100, Christoph Hellwig wrote:
> On Wed, Jan 19, 2011 at 09:56:09AM +1030, Rusty Russell wrote:
> > > I don't think we have much choice.  We're getting called from irq
> > > context, so we'll need to allocate memory to offload things.  That's
> > > unless we keep a thread around that we just need to wake up, which
> > > would be a huge waste of ressources.
> > 
> > Yes, but can we keep a workqueue struct around?
> 
> We can keep one around, but we can't prevent the virtio core code from
> calling us again while it's still in use.

Yes but in that specific situation (double schedule before
it runs once) discarding one change notification
would not hurt at all.

> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

Re: [PATCH] virtio_blk: allow re-reading config space at runtime

[Christoph Hellwig]

On Thu, Jan 27, 2011 at 05:29:26PM +0200, Michael S. Tsirkin wrote:
> Yes but in that specific situation (double schedule before
> it runs once) discarding one change notification
> would not hurt at all.

Except that you can't easily do it.  We can't simply reuse a work
structure embedded in say struct virtio_blk without first doing a
flush_work_sync/cancel_work_sync to remove it from the internal
workqueue.c queues.  And those calls can sleep and thus can't be called
from interrupt context either.

Re: [PATCH] virtio_blk: allow re-reading config space at runtime

[Michael S. Tsirkin]

On Tue, Feb 01, 2011 at 06:32:34PM +0100, Christoph Hellwig wrote:
> On Thu, Jan 27, 2011 at 05:29:26PM +0200, Michael S. Tsirkin wrote:
> > Yes but in that specific situation (double schedule before
> > it runs once) discarding one change notification
> > would not hurt at all.
> 
> Except that you can't easily do it.  We can't simply reuse a work
> structure embedded in say struct virtio_blk without first doing a
> flush_work_sync/cancel_work_sync to remove it from the internal
> workqueue.c queues.
>  And those calls can sleep and thus can't be called
> from interrupt context either.

Should not be hard to solve.
If it's running, it is ok to requeue.  I don't remember offhand if it's
ok to requeue if it's queued but not running, if not we could have a
flag that signals that's the case.

Re: [PATCH] virtio_blk: allow re-reading config space at runtime

[Christoph Hellwig]

On Tue, Feb 01, 2011 at 08:38:27PM +0200, Michael S. Tsirkin wrote:
> Should not be hard to solve.
> If it's running, it is ok to requeue.  I don't remember offhand if it's
> ok to requeue if it's queued but not running, if not we could have a
> flag that signals that's the case.

Looks like the workqueue infrastructure does indeed handle requing
an pending work_struct, and we're free to reuse it from the point
just before the callback is called.

Does the version below look okay?

---
From: Christoph Hellwig <hch@lst.de>
Subject: [PATCH v2] virtio_blk: allow re-reading config space at runtime

Wire up the virtio_driver config_changed method to get notified about
config changes raised by the host.  For now we just re-read the device
size to support online resizing of devices, but once we add more
attributes that might be changeable they could be added as well.

Note that the config_changed method is called from irq context, so
we'll have to use the workqueue infrastructure to provide us a proper
user context for our changes.

Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: linux-2.6/drivers/block/virtio_blk.c
===================================================================
--- linux-2.6.orig/drivers/block/virtio_blk.c	2010-12-01 09:30:59.647253809 -0700
+++ linux-2.6/drivers/block/virtio_blk.c	2011-02-01 12:10:02.803164641 -0700
@@ -6,10 +6,12 @@
 #include <linux/virtio.h>
 #include <linux/virtio_blk.h>
 #include <linux/scatterlist.h>
+#include <linux/string_helpers.h>
 
 #define PART_BITS 4
 
 static int major, index;
+struct workqueue_struct *virtblk_wq;
 
 struct virtio_blk
 {
@@ -26,6 +28,9 @@ struct virtio_blk
 
 	mempool_t *pool;
 
+	/* Process context for config space updates */
+	struct work_struct config_work;
+
 	/* What host tells us, plus 2 for header & tailer. */
 	unsigned int sg_elems;
 
@@ -291,6 +296,47 @@ static ssize_t virtblk_serial_show(struc
 }
 DEVICE_ATTR(serial, S_IRUGO, virtblk_serial_show, NULL);
 
+static void virtblk_config_changed_work(struct work_struct *work)
+{
+	struct virtio_blk *vblk =
+		container_of(work, struct virtio_blk, config_work);
+	struct virtio_device *vdev = vblk->vdev;
+	struct request_queue *q = vblk->disk->queue;
+	char cap_str_2[10], cap_str_10[10];
+	u64 capacity, size;
+
+	/* Host must always specify the capacity. */
+	vdev->config->get(vdev, offsetof(struct virtio_blk_config, capacity),
+			  &capacity, sizeof(capacity));
+
+	/* If capacity is too big, truncate with warning. */
+	if ((sector_t)capacity != capacity) {
+		dev_warn(&vdev->dev, "Capacity %llu too large: truncating\n",
+			 (unsigned long long)capacity);
+		capacity = (sector_t)-1;
+	}
+
+	size = capacity * queue_logical_block_size(q);
+	string_get_size(size, STRING_UNITS_2, cap_str_2, sizeof(cap_str_2));
+	string_get_size(size, STRING_UNITS_10, cap_str_10, sizeof(cap_str_10));
+
+	dev_notice(&vdev->dev,
+		  "new size: %llu %d-byte logical blocks (%s/%s)\n",
+		  (unsigned long long)capacity,
+		  queue_logical_block_size(q),
+		  cap_str_10, cap_str_2);
+
+	set_capacity(vblk->disk, capacity);
+
+}
+
+static void virtblk_config_changed(struct virtio_device *vdev)
+{
+	struct virtio_blk *vblk = vdev->priv;
+
+	queue_work(virtblk_wq, &vblk->config_work);
+}
+
 static int __devinit virtblk_probe(struct virtio_device *vdev)
 {
 	struct virtio_blk *vblk;
@@ -327,6 +373,7 @@ static int __devinit virtblk_probe(struc
 	vblk->vdev = vdev;
 	vblk->sg_elems = sg_elems;
 	sg_init_table(vblk->sg, vblk->sg_elems);
+	INIT_WORK(&vblk->config_work, virtblk_config_changed_work);
 
 	/* We expect one virtqueue, for output. */
 	vblk->vq = virtio_find_single_vq(vdev, blk_done, "requests");
@@ -477,6 +524,8 @@ static void __devexit virtblk_remove(str
 {
 	struct virtio_blk *vblk = vdev->priv;
 
+	flush_work(&vblk->config_work);
+
 	/* Nothing should be pending. */
 	BUG_ON(!list_empty(&vblk->reqs));
 
@@ -508,27 +557,47 @@ static unsigned int features[] = {
  * Use __refdata to avoid this warning.
  */
 static struct virtio_driver __refdata virtio_blk = {
-	.feature_table = features,
-	.feature_table_size = ARRAY_SIZE(features),
-	.driver.name =	KBUILD_MODNAME,
-	.driver.owner =	THIS_MODULE,
-	.id_table =	id_table,
-	.probe =	virtblk_probe,
-	.remove =	__devexit_p(virtblk_remove),
+	.feature_table		= features,
+	.feature_table_size	= ARRAY_SIZE(features),
+	.driver.name		= KBUILD_MODNAME,
+	.driver.owner		= THIS_MODULE,
+	.id_table		= id_table,
+	.probe			= virtblk_probe,
+	.remove			= __devexit_p(virtblk_remove),
+	.config_changed		= virtblk_config_changed,
 };
 
 static int __init init(void)
 {
+	int error;
+
+	virtblk_wq = alloc_workqueue("md_misc", 0, 0);
+	if (!virtblk_wq)
+		return -ENOMEM;
+
 	major = register_blkdev(0, "virtblk");
-	if (major < 0)
-		return major;
-	return register_virtio_driver(&virtio_blk);
+	if (major < 0) {
+		error = major;
+		goto out_destroy_workqueue;
+	}
+
+	error = register_virtio_driver(&virtio_blk);
+	if (error)
+		goto out_unregister_blkdev;
+	return 0;
+
+out_unregister_blkdev:
+	unregister_blkdev(major, "virtblk");
+out_destroy_workqueue:
+	destroy_workqueue(virtblk_wq);
+	return error;
 }
 
 static void __exit fini(void)
 {
 	unregister_blkdev(major, "virtblk");
 	unregister_virtio_driver(&virtio_blk);
+	destroy_workqueue(virtblk_wq);
 }
 module_init(init);
 module_exit(fini);

Re: [PATCH] virtio_blk: allow re-reading config space at runtime

[Michael S. Tsirkin]

On Tue, Feb 01, 2011 at 08:21:56PM +0100, Christoph Hellwig wrote:
> On Tue, Feb 01, 2011 at 08:38:27PM +0200, Michael S. Tsirkin wrote:
> > Should not be hard to solve.
> > If it's running, it is ok to requeue.  I don't remember offhand if it's
> > ok to requeue if it's queued but not running, if not we could have a
> > flag that signals that's the case.
> 
> Looks like the workqueue infrastructure does indeed handle requing
> an pending work_struct, and we're free to reuse it from the point
> just before the callback is called.
> 
> Does the version below look okay?

Yes, looks good to me. A couple of minor comments below
but they don't really matter. FWIW

Acked-by: Michael S. Tsirkin <mst@redhat.com>


> ---
> From: Christoph Hellwig <hch@lst.de>
> Subject: [PATCH v2] virtio_blk: allow re-reading config space at runtime
> 
> Wire up the virtio_driver config_changed method to get notified about
> config changes raised by the host.  For now we just re-read the device
> size to support online resizing of devices, but once we add more
> attributes that might be changeable they could be added as well.
> 
> Note that the config_changed method is called from irq context, so
> we'll have to use the workqueue infrastructure to provide us a proper
> user context for our changes.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> Index: linux-2.6/drivers/block/virtio_blk.c
> ===================================================================
> --- linux-2.6.orig/drivers/block/virtio_blk.c	2010-12-01 09:30:59.647253809 -0700
> +++ linux-2.6/drivers/block/virtio_blk.c	2011-02-01 12:10:02.803164641 -0700
> @@ -6,10 +6,12 @@
>  #include <linux/virtio.h>
>  #include <linux/virtio_blk.h>
>  #include <linux/scatterlist.h>
> +#include <linux/string_helpers.h>
>  
>  #define PART_BITS 4
>  
>  static int major, index;
> +struct workqueue_struct *virtblk_wq;
>  
>  struct virtio_blk
>  {
> @@ -26,6 +28,9 @@ struct virtio_blk
>  
>  	mempool_t *pool;
>  
> +	/* Process context for config space updates */
> +	struct work_struct config_work;
> +
>  	/* What host tells us, plus 2 for header & tailer. */
>  	unsigned int sg_elems;
>  
> @@ -291,6 +296,47 @@ static ssize_t virtblk_serial_show(struc
>  }
>  DEVICE_ATTR(serial, S_IRUGO, virtblk_serial_show, NULL);
>  
> +static void virtblk_config_changed_work(struct work_struct *work)
> +{
> +	struct virtio_blk *vblk =
> +		container_of(work, struct virtio_blk, config_work);
> +	struct virtio_device *vdev = vblk->vdev;
> +	struct request_queue *q = vblk->disk->queue;
> +	char cap_str_2[10], cap_str_10[10];
> +	u64 capacity, size;
> +
> +	/* Host must always specify the capacity. */
> +	vdev->config->get(vdev, offsetof(struct virtio_blk_config, capacity),
> +			  &capacity, sizeof(capacity));
> +
> +	/* If capacity is too big, truncate with warning. */
> +	if ((sector_t)capacity != capacity) {
> +		dev_warn(&vdev->dev, "Capacity %llu too large: truncating\n",
> +			 (unsigned long long)capacity);
> +		capacity = (sector_t)-1;

One thing to note here is that we will get this warning
on each config change even if capacity is not updated.
Not sure whether it matters.

> +	}
> +
> +	size = capacity * queue_logical_block_size(q);
> +	string_get_size(size, STRING_UNITS_2, cap_str_2, sizeof(cap_str_2));
> +	string_get_size(size, STRING_UNITS_10, cap_str_10, sizeof(cap_str_10));
> +
> +	dev_notice(&vdev->dev,
> +		  "new size: %llu %d-byte logical blocks (%s/%s)\n",
> +		  (unsigned long long)capacity,
> +		  queue_logical_block_size(q),
> +		  cap_str_10, cap_str_2);
> +
> +	set_capacity(vblk->disk, capacity);
> +

empty line :)

> +}
> +
> +static void virtblk_config_changed(struct virtio_device *vdev)
> +{
> +	struct virtio_blk *vblk = vdev->priv;
> +
> +	queue_work(virtblk_wq, &vblk->config_work);
> +}
> +
>  static int __devinit virtblk_probe(struct virtio_device *vdev)
>  {
>  	struct virtio_blk *vblk;
> @@ -327,6 +373,7 @@ static int __devinit virtblk_probe(struc
>  	vblk->vdev = vdev;
>  	vblk->sg_elems = sg_elems;
>  	sg_init_table(vblk->sg, vblk->sg_elems);
> +	INIT_WORK(&vblk->config_work, virtblk_config_changed_work);
>  
>  	/* We expect one virtqueue, for output. */
>  	vblk->vq = virtio_find_single_vq(vdev, blk_done, "requests");
> @@ -477,6 +524,8 @@ static void __devexit virtblk_remove(str
>  {
>  	struct virtio_blk *vblk = vdev->priv;
>  
> +	flush_work(&vblk->config_work);
> +
>  	/* Nothing should be pending. */
>  	BUG_ON(!list_empty(&vblk->reqs));
>  
> @@ -508,27 +557,47 @@ static unsigned int features[] = {
>   * Use __refdata to avoid this warning.
>   */
>  static struct virtio_driver __refdata virtio_blk = {
> -	.feature_table = features,
> -	.feature_table_size = ARRAY_SIZE(features),
> -	.driver.name =	KBUILD_MODNAME,
> -	.driver.owner =	THIS_MODULE,
> -	.id_table =	id_table,
> -	.probe =	virtblk_probe,
> -	.remove =	__devexit_p(virtblk_remove),
> +	.feature_table		= features,
> +	.feature_table_size	= ARRAY_SIZE(features),
> +	.driver.name		= KBUILD_MODNAME,
> +	.driver.owner		= THIS_MODULE,
> +	.id_table		= id_table,
> +	.probe			= virtblk_probe,
> +	.remove			= __devexit_p(virtblk_remove),
> +	.config_changed		= virtblk_config_changed,
>  };
>  
>  static int __init init(void)
>  {
> +	int error;
> +
> +	virtblk_wq = alloc_workqueue("md_misc", 0, 0);

What does md_misc stand for? Would virtblk_wq be a better name?

> +	if (!virtblk_wq)
> +		return -ENOMEM;
> +
>  	major = register_blkdev(0, "virtblk");
> -	if (major < 0)
> -		return major;
> -	return register_virtio_driver(&virtio_blk);
> +	if (major < 0) {
> +		error = major;
> +		goto out_destroy_workqueue;
> +	}
> +
> +	error = register_virtio_driver(&virtio_blk);
> +	if (error)
> +		goto out_unregister_blkdev;
> +	return 0;
> +
> +out_unregister_blkdev:
> +	unregister_blkdev(major, "virtblk");
> +out_destroy_workqueue:
> +	destroy_workqueue(virtblk_wq);
> +	return error;
>  }
>  
>  static void __exit fini(void)
>  {
>  	unregister_blkdev(major, "virtblk");
>  	unregister_virtio_driver(&virtio_blk);
> +	destroy_workqueue(virtblk_wq);
>  }
>  module_init(init);
>  module_exit(fini);

[PATCH v3] virtio_blk: allow re-reading config space at runtime

[Christoph Hellwig]

Wire up the virtio_driver config_changed method to get notified about
config changes raised by the host.  For now we just re-read the device
size to support online resizing of devices, but once we add more
attributes that might be changeable they could be added as well.

Note that the config_changed method is called from irq context, so
we'll have to use the workqueue infrastructure to provide us a proper
user context for our changes.

Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: linux-2.6/drivers/block/virtio_blk.c
===================================================================
--- linux-2.6.orig/drivers/block/virtio_blk.c	2010-12-01 09:30:59.647253809 -0700
+++ linux-2.6/drivers/block/virtio_blk.c	2011-02-01 13:37:29.355728726 -0700
@@ -6,10 +6,12 @@
 #include <linux/virtio.h>
 #include <linux/virtio_blk.h>
 #include <linux/scatterlist.h>
+#include <linux/string_helpers.h>
 
 #define PART_BITS 4
 
 static int major, index;
+struct workqueue_struct *virtblk_wq;
 
 struct virtio_blk
 {
@@ -26,6 +28,9 @@ struct virtio_blk
 
 	mempool_t *pool;
 
+	/* Process context for config space updates */
+	struct work_struct config_work;
+
 	/* What host tells us, plus 2 for header & tailer. */
 	unsigned int sg_elems;
 
@@ -291,6 +296,46 @@ static ssize_t virtblk_serial_show(struc
 }
 DEVICE_ATTR(serial, S_IRUGO, virtblk_serial_show, NULL);
 
+static void virtblk_config_changed_work(struct work_struct *work)
+{
+	struct virtio_blk *vblk =
+		container_of(work, struct virtio_blk, config_work);
+	struct virtio_device *vdev = vblk->vdev;
+	struct request_queue *q = vblk->disk->queue;
+	char cap_str_2[10], cap_str_10[10];
+	u64 capacity, size;
+
+	/* Host must always specify the capacity. */
+	vdev->config->get(vdev, offsetof(struct virtio_blk_config, capacity),
+			  &capacity, sizeof(capacity));
+
+	/* If capacity is too big, truncate with warning. */
+	if ((sector_t)capacity != capacity) {
+		dev_warn(&vdev->dev, "Capacity %llu too large: truncating\n",
+			 (unsigned long long)capacity);
+		capacity = (sector_t)-1;
+	}
+
+	size = capacity * queue_logical_block_size(q);
+	string_get_size(size, STRING_UNITS_2, cap_str_2, sizeof(cap_str_2));
+	string_get_size(size, STRING_UNITS_10, cap_str_10, sizeof(cap_str_10));
+
+	dev_notice(&vdev->dev,
+		  "new size: %llu %d-byte logical blocks (%s/%s)\n",
+		  (unsigned long long)capacity,
+		  queue_logical_block_size(q),
+		  cap_str_10, cap_str_2);
+
+	set_capacity(vblk->disk, capacity);
+}
+
+static void virtblk_config_changed(struct virtio_device *vdev)
+{
+	struct virtio_blk *vblk = vdev->priv;
+
+	queue_work(virtblk_wq, &vblk->config_work);
+}
+
 static int __devinit virtblk_probe(struct virtio_device *vdev)
 {
 	struct virtio_blk *vblk;
@@ -327,6 +372,7 @@ static int __devinit virtblk_probe(struc
 	vblk->vdev = vdev;
 	vblk->sg_elems = sg_elems;
 	sg_init_table(vblk->sg, vblk->sg_elems);
+	INIT_WORK(&vblk->config_work, virtblk_config_changed_work);
 
 	/* We expect one virtqueue, for output. */
 	vblk->vq = virtio_find_single_vq(vdev, blk_done, "requests");
@@ -477,6 +523,8 @@ static void __devexit virtblk_remove(str
 {
 	struct virtio_blk *vblk = vdev->priv;
 
+	flush_work(&vblk->config_work);
+
 	/* Nothing should be pending. */
 	BUG_ON(!list_empty(&vblk->reqs));
 
@@ -508,27 +556,47 @@ static unsigned int features[] = {
  * Use __refdata to avoid this warning.
  */
 static struct virtio_driver __refdata virtio_blk = {
-	.feature_table = features,
-	.feature_table_size = ARRAY_SIZE(features),
-	.driver.name =	KBUILD_MODNAME,
-	.driver.owner =	THIS_MODULE,
-	.id_table =	id_table,
-	.probe =	virtblk_probe,
-	.remove =	__devexit_p(virtblk_remove),
+	.feature_table		= features,
+	.feature_table_size	= ARRAY_SIZE(features),
+	.driver.name		= KBUILD_MODNAME,
+	.driver.owner		= THIS_MODULE,
+	.id_table		= id_table,
+	.probe			= virtblk_probe,
+	.remove			= __devexit_p(virtblk_remove),
+	.config_changed		= virtblk_config_changed,
 };
 
 static int __init init(void)
 {
+	int error;
+
+	virtblk_wq = alloc_workqueue("virtio-blk", 0, 0);
+	if (!virtblk_wq)
+		return -ENOMEM;
+
 	major = register_blkdev(0, "virtblk");
-	if (major < 0)
-		return major;
-	return register_virtio_driver(&virtio_blk);
+	if (major < 0) {
+		error = major;
+		goto out_destroy_workqueue;
+	}
+
+	error = register_virtio_driver(&virtio_blk);
+	if (error)
+		goto out_unregister_blkdev;
+	return 0;
+
+out_unregister_blkdev:
+	unregister_blkdev(major, "virtblk");
+out_destroy_workqueue:
+	destroy_workqueue(virtblk_wq);
+	return error;
 }
 
 static void __exit fini(void)
 {
 	unregister_blkdev(major, "virtblk");
 	unregister_virtio_driver(&virtio_blk);
+	destroy_workqueue(virtblk_wq);
 }
 module_init(init);
 module_exit(fini);

Re: [PATCH v3] virtio_blk: allow re-reading config space at runtime

[Rusty Russell]

On Wed, 2 Feb 2011 07:13:48 am Christoph Hellwig wrote:
> Wire up the virtio_driver config_changed method to get notified about
> config changes raised by the host.  For now we just re-read the device
> size to support online resizing of devices, but once we add more
> attributes that might be changeable they could be added as well.
> 
> Note that the config_changed method is called from irq context, so
> we'll have to use the workqueue infrastructure to provide us a proper
> user context for our changes.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Thanks, applied!

Cheers,
Rusty.

Re: [PATCH] virtio_blk: allow re-reading config space at runtime

[Michael S. Tsirkin]

On Fri, Jan 14, 2011 at 05:01:37PM +0100, Christoph Hellwig wrote:
> Wire up the virtio_driver config_changed method to get notified about
> config changes raised by the host.  For now we just re-read the device
> size to support online resizing of devices, but once we add more
> attributes that might be changeable they could be added as well.
> 
> Note that the config_changed method is called from irq context, so
> we'll have to use the workqueue infrastructure to provide us a proper
> user context for our changes.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> Index: xfs/drivers/block/virtio_blk.c
> ===================================================================
> --- xfs.orig/drivers/block/virtio_blk.c	2011-01-13 18:17:23.730254665 +0100
> +++ xfs/drivers/block/virtio_blk.c	2011-01-14 16:57:50.572032906 +0100
> @@ -6,10 +6,12 @@
>  #include <linux/virtio.h>
>  #include <linux/virtio_blk.h>
>  #include <linux/scatterlist.h>
> +#include <linux/string_helpers.h>
>  
>  #define PART_BITS 4
>  
>  static int major, index;
> +struct workqueue_struct *virtblk_wq;
>  
>  struct virtio_blk
>  {
> @@ -42,6 +44,11 @@ struct virtblk_req
>  	u8 status;
>  };
>  
> +struct virtblk_config_change {
> +	struct virtio_device *vdev;
> +	struct work_struct work;
> +};
> +
>  static void blk_done(struct virtqueue *vq)
>  {
>  	struct virtio_blk *vblk = vq->vdev->priv;
> @@ -291,6 +298,57 @@ static ssize_t virtblk_serial_show(struc
>  }
>  DEVICE_ATTR(serial, S_IRUGO, virtblk_serial_show, NULL);
>  
> +static void virtblk_config_changed_work(struct work_struct *work)
> +{
> +	struct virtblk_config_change *cfg =
> +		container_of(work, struct virtblk_config_change, work);
> +	struct virtio_device *vdev = cfg->vdev;
> +	struct virtio_blk *vblk = vdev->priv;
> +	struct request_queue *q = vblk->disk->queue;
> +	char cap_str_2[10], cap_str_10[10];
> +	u64 capacity, size;
> +
> +	/* Host must always specify the capacity. */
> +	vdev->config->get(vdev, offsetof(struct virtio_blk_config, capacity),
> +			  &capacity, sizeof(capacity));
> +
> +	/* If capacity is too big, truncate with warning. */
> +	if ((sector_t)capacity != capacity) {
> +		dev_warn(&vdev->dev, "Capacity %llu too large: truncating\n",
> +			 (unsigned long long)capacity);
> +		capacity = (sector_t)-1;
> +	}
> +
> +	size = capacity * queue_logical_block_size(q);
> +	string_get_size(size, STRING_UNITS_2, cap_str_2, sizeof(cap_str_2));
> +	string_get_size(size, STRING_UNITS_10, cap_str_10, sizeof(cap_str_10));
> +
> +	dev_notice(&vdev->dev,
> +		  "new size: %llu %d-byte logical blocks (%s/%s)\n",
> +		  (unsigned long long)capacity,
> +		  queue_logical_block_size(q),
> +		  cap_str_10, cap_str_2);
> +
> +	set_capacity(vblk->disk, capacity);
> +
> +}
> +
> +static void virtblk_config_changed(struct virtio_device *vdev)
> +{
> +	struct virtblk_config_change *cfg;
> +
> +	cfg = kmalloc(sizeof(*cfg), GFP_ATOMIC);
> +	if (!cfg) {
> +		dev_info(&vdev->dev, "skipping config change\n");
> +		return;
> +	}
> +
> +	cfg->vdev = vdev;
> +	INIT_WORK(&cfg->work, virtblk_config_changed_work);
> +	queue_work(virtblk_wq, &cfg->work);

This needs to be flushed on device removal, I think,
otherwise the vdev pointer will go stale.

> +}
> +
> +

Two empty lines :)

>  static int __devinit virtblk_probe(struct virtio_device *vdev)
>  {
>  	struct virtio_blk *vblk;
> @@ -508,27 +566,47 @@ static unsigned int features[] = {
>   * Use __refdata to avoid this warning.
>   */
>  static struct virtio_driver __refdata virtio_blk = {
> -	.feature_table = features,
> -	.feature_table_size = ARRAY_SIZE(features),
> -	.driver.name =	KBUILD_MODNAME,
> -	.driver.owner =	THIS_MODULE,
> -	.id_table =	id_table,
> -	.probe =	virtblk_probe,
> -	.remove =	__devexit_p(virtblk_remove),
> +	.feature_table		= features,
> +	.feature_table_size	= ARRAY_SIZE(features),
> +	.driver.name		= KBUILD_MODNAME,
> +	.driver.owner		= THIS_MODULE,
> +	.id_table		= id_table,
> +	.probe			= virtblk_probe,
> +	.remove			= __devexit_p(virtblk_remove),
> +	.config_changed		= virtblk_config_changed,
>  };
>  
>  static int __init init(void)
>  {
> +	int error;
> +
> +	virtblk_wq = alloc_workqueue("md_misc", 0, 0);
> +	if (!virtblk_wq)
> +		return -ENOMEM;
> +
>  	major = register_blkdev(0, "virtblk");
> -	if (major < 0)
> -		return major;
> -	return register_virtio_driver(&virtio_blk);
> +	if (major < 0) {
> +		error = major;
> +		goto out_destroy_workqueue;
> +	}
> +
> +	error = register_virtio_driver(&virtio_blk);
> +	if (error)
> +		goto out_unregister_blkdev;
> +	return 0;
> +
> +out_unregister_blkdev:
> +	unregister_blkdev(major, "virtblk");
> +out_destroy_workqueue:
> +	destroy_workqueue(virtblk_wq);
> +	return error;
>  }
>  
>  static void __exit fini(void)
>  {
>  	unregister_blkdev(major, "virtblk");
>  	unregister_virtio_driver(&virtio_blk);
> +	destroy_workqueue(virtblk_wq);
>  }
>  module_init(init);
>  module_exit(fini);
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

Re: [PATCH] virtio_blk: allow re-reading config space at runtime

[Christoph Hellwig]

On Thu, Jan 27, 2011 at 05:32:21PM +0200, Michael S. Tsirkin wrote:
> This needs to be flushed on device removal, I think,
> otherwise the vdev pointer will go stale.

Indeed.

> 
> > +}
> > +
> > +
> 
> Two empty lines :)


I'll fix it up.