Linux Virtualization - Nov 2013 - [PATCH RFC 0/3] virtio: add new notify() callback to virtio_driver

Hi,

when an active virtio block device is hot-unplugged from a KVM guest, running
affected guest user applications are not aware of any errors that occur due
to the lost device. This patch-set adds code to avoid further request queueing
when a lost block device is detected, resulting in appropriate error info.

On System z there exists no handshake mechanism between host and guest
when a device is hot-unplugged. The device is removed and no further I/O
is possible.

When an online channel device disappears on System z the kernel's CIO layer
informs the driver (virtio_ccw) about the lost device.

It's just the block device drivers that care to provide a notify
callback.

Here are some more error details:

For a particular block device virtio's request function virtblk_request()
is called by the block layer to queue requests to be handled by the host.
In case of a lost device requests can still be queued, but an appropriate
subsequent host kick usually fails. This leads to situations where no error
feedback is shown.

In order to prevent request queueing for lost devices appropriate settings
in the block layer should be made. Exploiting System z's CIO notify handler
callback, and adding a corresponding new virtio_driver notify() handler to
'inform' the block layer, solve this task.

Patch 1 adds an optional notify() callback to virtio_driver.

Patch 2 adds a new notify() callback for the virtio_blk driver. When called
for a lost device settings are made to prevent future request queueing.

Patch 3 modifies the CIO notify handler in virtio_ccw's transport layer to
pass
on the lost device info to virtio's backend driver virtio_blk.

Heinz Graalfs (3):
  virtio: add notify() callback to virtio_driver
  virtio_blk: add virtblk_notify() as virtio_driver's notify() callback
  virtio_ccw: invoke virtio_driver's notify() on CIO_GONE notification

 drivers/block/virtio_blk.c    | 14 ++++++++++++++
 drivers/s390/kvm/virtio_ccw.c | 14 ++++++++++++--
 drivers/virtio/virtio.c       |  8 ++++++++
 include/linux/virtio.h        | 10 ++++++++++
 4 files changed, 44 insertions(+), 2 deletions(-)

-- 
1.8.3.1

Add an optional notify() callback to virtio_driver. A backend
driver can provide this callback to perform actions for a lost
device.

notify() event values are inherited from virtio_ccw's notify()
callback. We might want to support even more of them lateron.

notify() return values are defined in include/linux/notifier.h.

Signed-off-by: Heinz Graalfs <graalfs at linux.vnet.ibm.com>
---
 drivers/virtio/virtio.c |  8 ++++++++
 include/linux/virtio.h  | 10 ++++++++++
 2 files changed, 18 insertions(+)

diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index ee59b74..a09abb4 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -186,6 +186,14 @@ void unregister_virtio_driver(struct virtio_driver *driver)
 }
 EXPORT_SYMBOL_GPL(unregister_virtio_driver);
 
+int notify_virtio_device(struct virtio_device *vdev, int event)
+{
+	struct virtio_driver *drv = drv_to_virtio(vdev->dev.driver);
+
+	return drv->notify ? drv->notify(vdev, event) : NOTIFY_DONE;
+}
+EXPORT_SYMBOL_GPL(notify_virtio_device);
+
 int register_virtio_device(struct virtio_device *dev)
 {
 	int err;
diff --git a/include/linux/virtio.h b/include/linux/virtio.h
index f15f6e7..da18e9a 100644
--- a/include/linux/virtio.h
+++ b/include/linux/virtio.h
@@ -110,6 +110,15 @@ int register_virtio_device(struct virtio_device *dev);
 void unregister_virtio_device(struct virtio_device *dev);
 
 /**
+ * notify event values
+ * @VDEV_GONE: device gone
+ */
+enum {
+	VDEV_GONE		= 1,
+};
+int notify_virtio_device(struct virtio_device *dev, int event);
+
+/**
  * virtio_driver - operations for a virtio I/O driver
  * @driver: underlying device driver (populate name and owner).
  * @id_table: the ids serviced by this driver.
@@ -129,6 +138,7 @@ struct virtio_driver {
 	void (*scan)(struct virtio_device *dev);
 	void (*remove)(struct virtio_device *dev);
 	void (*config_changed)(struct virtio_device *dev);
+	int (*notify)(struct virtio_device *dev, int event);
 #ifdef CONFIG_PM
 	int (*freeze)(struct virtio_device *dev);
 	int (*restore)(struct virtio_device *dev);
-- 
1.8.3.1

Add virtblk_notify() as virtio_driver's notify() callback.

When a transport driver is notified that a device disappeared it
should invoke this callback to prevent further request queueing.

Subsequent block layer calls of virtio_blk's request function will
fail, resulting in appropriate I/O errors.

Signed-off-by: Heinz Graalfs <graalfs at linux.vnet.ibm.com>
---
 drivers/block/virtio_blk.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 2d43be4..7fc1d62 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -901,6 +901,19 @@ static void virtblk_remove(struct virtio_device *vdev)
 		ida_simple_remove(&vd_index_ida, index);
 }
 
+static int virtblk_notify(struct virtio_device *vdev, int event)
+{
+	struct virtio_blk *vblk = vdev->priv;
+
+	if (event == VDEV_GONE) {
+		queue_flag_set_unlocked(QUEUE_FLAG_DYING, vblk->disk->queue);
+		queue_flag_set_unlocked(QUEUE_FLAG_NOMERGES, vblk->disk->queue);
+		queue_flag_set_unlocked(QUEUE_FLAG_NOXMERGES,
+					vblk->disk->queue);
+	}
+	return NOTIFY_DONE;
+}
+
 #ifdef CONFIG_PM
 static int virtblk_freeze(struct virtio_device *vdev)
 {
@@ -961,6 +974,7 @@ static struct virtio_driver virtio_blk = {
 	.probe			= virtblk_probe,
 	.remove			= virtblk_remove,
 	.config_changed		= virtblk_config_changed,
+	.notify			= virtblk_notify,
 #ifdef CONFIG_PM
 	.freeze			= virtblk_freeze,
 	.restore		= virtblk_restore,
-- 
1.8.3.1

virtio_ccw's notify() callback for the common IO layer invokes
virtio_driver's notify() callback to pass-on information to a
backend driver if an online device disappeared.

Signed-off-by: Heinz Graalfs <graalfs at linux.vnet.ibm.com>
---
 drivers/s390/kvm/virtio_ccw.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/s390/kvm/virtio_ccw.c b/drivers/s390/kvm/virtio_ccw.c
index 35b9aaa..420010d 100644
--- a/drivers/s390/kvm/virtio_ccw.c
+++ b/drivers/s390/kvm/virtio_ccw.c
@@ -1064,8 +1064,18 @@ out_free:
 
 static int virtio_ccw_cio_notify(struct ccw_device *cdev, int event)
 {
-	/* TODO: Check whether we need special handling here. */
-	return 0;
+	int rc;
+	struct virtio_ccw_device *vcdev = dev_get_drvdata(&cdev->dev);
+
+	switch (event) {
+	case CIO_GONE:
+		rc = notify_virtio_device(&vcdev->vdev, VDEV_GONE);
+		break;
+	default:
+		rc = NOTIFY_DONE;
+		break;
+	}
+	return rc;
 }
 
 static struct ccw_device_id virtio_ids[] = {
-- 
1.8.3.1

Heinz Graalfs <graalfs at linux.vnet.ibm.com>
writes:
> Add an optional notify() callback to virtio_driver. A backend
> driver can provide this callback to perform actions for a lost
> device.
>
> notify() event values are inherited from virtio_ccw's notify()
> callback. We might want to support even more of them lateron.
>
> notify() return values are defined in include/linux/notifier.h.
>
> Signed-off-by: Heinz Graalfs <graalfs at linux.vnet.ibm.com>
These patches seem sensible.  I've applied them in my pending queue,
but it'd be nice to have some feedback on the virtio_blk.c patch.

Cheers,
Rusty.

On Wed, Nov 20, 2013 at 04:22:02PM +0100, Heinz Graalfs
wrote:
> Add virtblk_notify() as virtio_driver's notify() callback.
> 
> When a transport driver is notified that a device disappeared it
> should invoke this callback to prevent further request queueing.
> 
> Subsequent block layer calls of virtio_blk's request function will
> fail, resulting in appropriate I/O errors.
> 
> Signed-off-by: Heinz Graalfs <graalfs at linux.vnet.ibm.com>
> ---
>  drivers/block/virtio_blk.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> index 2d43be4..7fc1d62 100644
> --- a/drivers/block/virtio_blk.c
> +++ b/drivers/block/virtio_blk.c
> @@ -901,6 +901,19 @@ static void virtblk_remove(struct virtio_device *vdev)
>  		ida_simple_remove(&vd_index_ida, index);
>  }
>  
> +static int virtblk_notify(struct virtio_device *vdev, int event)
> +{
> +	struct virtio_blk *vblk = vdev->priv;
> +
> +	if (event == VDEV_GONE) {
> +		queue_flag_set_unlocked(QUEUE_FLAG_DYING, vblk->disk->queue);
> +		queue_flag_set_unlocked(QUEUE_FLAG_NOMERGES, vblk->disk->queue);
> +		queue_flag_set_unlocked(QUEUE_FLAG_NOXMERGES,
> +					vblk->disk->queue);
> +	}
> +	return NOTIFY_DONE;
> +}
> +
But what serializes with the block layer?
And is unlocked really safe here?
Don't we need to take the queue lock?

>  #ifdef CONFIG_PM
>  static int virtblk_freeze(struct virtio_device *vdev)
>  {
> @@ -961,6 +974,7 @@ static struct virtio_driver virtio_blk = {
>  	.probe			= virtblk_probe,
>  	.remove			= virtblk_remove,
>  	.config_changed		= virtblk_config_changed,
> +	.notify			= virtblk_notify,
>  #ifdef CONFIG_PM
>  	.freeze			= virtblk_freeze,
>  	.restore		= virtblk_restore,
> -- 
> 1.8.3.1

On Wed, Nov 20, 2013 at 04:22:02PM +0100, Heinz Graalfs
wrote:
> Add virtblk_notify() as virtio_driver's notify() callback.
> 
> When a transport driver is notified that a device disappeared it
> should invoke this callback to prevent further request queueing.
> 
> Subsequent block layer calls of virtio_blk's request function will
> fail, resulting in appropriate I/O errors.
> 
> Signed-off-by: Heinz Graalfs <graalfs at linux.vnet.ibm.com>
> ---
>  drivers/block/virtio_blk.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> index 2d43be4..7fc1d62 100644
> --- a/drivers/block/virtio_blk.c
> +++ b/drivers/block/virtio_blk.c
> @@ -901,6 +901,19 @@ static void virtblk_remove(struct virtio_device *vdev)
>  		ida_simple_remove(&vd_index_ida, index);
>  }
>  
> +static int virtblk_notify(struct virtio_device *vdev, int event)
> +{
> +	struct virtio_blk *vblk = vdev->priv;
> +
> +	if (event == VDEV_GONE) {
> +		queue_flag_set_unlocked(QUEUE_FLAG_DYING, vblk->disk->queue);
> +		queue_flag_set_unlocked(QUEUE_FLAG_NOMERGES, vblk->disk->queue);
> +		queue_flag_set_unlocked(QUEUE_FLAG_NOXMERGES,
> +					vblk->disk->queue);
> +	}
> +	return NOTIFY_DONE;
Also pls include linux/notifier.h for this value.
> +}
> +
>  #ifdef CONFIG_PM
>  static int virtblk_freeze(struct virtio_device *vdev)
>  {
> @@ -961,6 +974,7 @@ static struct virtio_driver virtio_blk = {
>  	.probe			= virtblk_probe,
>  	.remove			= virtblk_remove,
>  	.config_changed		= virtblk_config_changed,
> +	.notify			= virtblk_notify,
>  #ifdef CONFIG_PM
>  	.freeze			= virtblk_freeze,
>  	.restore		= virtblk_restore,
> -- 
> 1.8.3.1

On Wed, Nov 20, 2013 at 04:22:01PM +0100, Heinz Graalfs
wrote:
> Add an optional notify() callback to virtio_driver. A backend
> driver can provide this callback to perform actions for a lost
> device.
> 
> notify() event values are inherited from virtio_ccw's notify()
> callback. We might want to support even more of them lateron.
> 
> notify() return values are defined in include/linux/notifier.h.
> 
> Signed-off-by: Heinz Graalfs <graalfs at linux.vnet.ibm.com>
> ---
>  drivers/virtio/virtio.c |  8 ++++++++
>  include/linux/virtio.h  | 10 ++++++++++
>  2 files changed, 18 insertions(+)
> 
> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> index ee59b74..a09abb4 100644
> --- a/drivers/virtio/virtio.c
> +++ b/drivers/virtio/virtio.c
> @@ -186,6 +186,14 @@ void unregister_virtio_driver(struct virtio_driver
*driver)
>  }
>  EXPORT_SYMBOL_GPL(unregister_virtio_driver);
>  
> +int notify_virtio_device(struct virtio_device *vdev, int event)
> +{
> +	struct virtio_driver *drv = drv_to_virtio(vdev->dev.driver);
> +
> +	return drv->notify ? drv->notify(vdev, event) : NOTIFY_DONE;
Also pls include linux/notifier.h for this value.
> +}
> +EXPORT_SYMBOL_GPL(notify_virtio_device);
> +
>  int register_virtio_device(struct virtio_device *dev)
>  {
>  	int err;
> diff --git a/include/linux/virtio.h b/include/linux/virtio.h
> index f15f6e7..da18e9a 100644
> --- a/include/linux/virtio.h
> +++ b/include/linux/virtio.h
> @@ -110,6 +110,15 @@ int register_virtio_device(struct virtio_device *dev);
>  void unregister_virtio_device(struct virtio_device *dev);
>  
>  /**
> + * notify event values
> + * @VDEV_GONE: device gone
> + */
> +enum {
> +	VDEV_GONE		= 1,
Seems a bit short, can lead to namespace pollution.
Let's rename to VIRTIO_DEVICE_GONE ?
> +};
> +int notify_virtio_device(struct virtio_device *dev, int event);
> +
> +/**
>   * virtio_driver - operations for a virtio I/O driver
>   * @driver: underlying device driver (populate name and owner).
>   * @id_table: the ids serviced by this driver.
> @@ -129,6 +138,7 @@ struct virtio_driver {
>  	void (*scan)(struct virtio_device *dev);
>  	void (*remove)(struct virtio_device *dev);
>  	void (*config_changed)(struct virtio_device *dev);
> +	int (*notify)(struct virtio_device *dev, int event);
>  #ifdef CONFIG_PM
>  	int (*freeze)(struct virtio_device *dev);
>  	int (*restore)(struct virtio_device *dev);
> -- 
> 1.8.3.1

On Wed, Nov 20, 2013 at 04:22:00PM +0100, Heinz Graalfs
wrote:
> Hi,
> 
> when an active virtio block device is hot-unplugged from a KVM guest,
running
> affected guest user applications are not aware of any errors that occur due
> to the lost device. This patch-set adds code to avoid further request
queueing
> when a lost block device is detected, resulting in appropriate error info.
> 
> On System z there exists no handshake mechanism between host and guest
> when a device is hot-unplugged. The device is removed and no further I/O
> is possible.
> 
> When an online channel device disappears on System z the kernel's CIO
layer
> informs the driver (virtio_ccw) about the lost device.
> 
> It's just the block device drivers that care to provide a notify
> callback.
> 
> Here are some more error details:
> 
> For a particular block device virtio's request function
virtblk_request()
> is called by the block layer to queue requests to be handled by the host.
> In case of a lost device requests can still be queued, but an appropriate
> subsequent host kick usually fails. This leads to situations where no error
> feedback is shown.
> 
> In order to prevent request queueing for lost devices appropriate settings
> in the block layer should be made. Exploiting System z's CIO notify
handler
> callback, and adding a corresponding new virtio_driver notify() handler to
> 'inform' the block layer, solve this task.
> 
> Patch 1 adds an optional notify() callback to virtio_driver.
> 
> Patch 2 adds a new notify() callback for the virtio_blk driver. When called
> for a lost device settings are made to prevent future request queueing.
> 
> Patch 3 modifies the CIO notify handler in virtio_ccw's transport layer
to pass
> on the lost device info to virtio's backend driver virtio_blk.
Question: I guess remove callback is invoked eventually?
Could you please clarify why isn't this sufficient?


> Heinz Graalfs (3):
>   virtio: add notify() callback to virtio_driver
>   virtio_blk: add virtblk_notify() as virtio_driver's notify() callback
>   virtio_ccw: invoke virtio_driver's notify() on CIO_GONE notification
> 
>  drivers/block/virtio_blk.c    | 14 ++++++++++++++
>  drivers/s390/kvm/virtio_ccw.c | 14 ++++++++++++--
>  drivers/virtio/virtio.c       |  8 ++++++++
>  include/linux/virtio.h        | 10 ++++++++++
>  4 files changed, 44 insertions(+), 2 deletions(-)
> 
> -- 
> 1.8.3.1

On 21/11/13 07:47, Michael S. Tsirkin wrote:
> On Wed, Nov 20, 2013 at 04:22:00PM +0100, Heinz Graalfs wrote:
>> Hi,
>>
>> when an active virtio block device is hot-unplugged from a KVM guest,
running
>> affected guest user applications are not aware of any errors that occur
due
>> to the lost device. This patch-set adds code to avoid further request
queueing
>> when a lost block device is detected, resulting in appropriate error
info.
>>
>> On System z there exists no handshake mechanism between host and guest
>> when a device is hot-unplugged. The device is removed and no further
I/O
>> is possible.
>>
>> When an online channel device disappears on System z the kernel's
CIO layer
>> informs the driver (virtio_ccw) about the lost device.
>>
>> It's just the block device drivers that care to provide a notify
>> callback.
>>
>> Here are some more error details:
>>
>> For a particular block device virtio's request function
virtblk_request()
>> is called by the block layer to queue requests to be handled by the
host.
>> In case of a lost device requests can still be queued, but an
appropriate
>> subsequent host kick usually fails. This leads to situations where no
error
>> feedback is shown.
>>
>> In order to prevent request queueing for lost devices appropriate
settings
>> in the block layer should be made. Exploiting System z's CIO notify
handler
>> callback, and adding a corresponding new virtio_driver notify() handler
to
>> 'inform' the block layer, solve this task.
>>
>> Patch 1 adds an optional notify() callback to virtio_driver.
>>
>> Patch 2 adds a new notify() callback for the virtio_blk driver. When
called
>> for a lost device settings are made to prevent future request queueing.
>>
>> Patch 3 modifies the CIO notify handler in virtio_ccw's transport
layer to pass
>> on the lost device info to virtio's backend driver virtio_blk.
>
> Question: I guess remove callback is invoked eventually?
> Could you please clarify why isn't this sufficient?
>
yes, the remove callback is invoked lateron, and it could be done there. 
However, it should be done conditionally, and prior to invoking 
del_gendisk() (which triggers final I/O). We would still have the need 
for such notification information. The remove callback is also invoked 
when a device is set offline, and in that case we don't want a queue to 
reject further requests. The way it is done right doesn't affect the 
remove callback. The weird situation, however, is solved by the new 
notify callback.

Doing it in blk_cleanup_queue() (also triggered from virtblk_remove()) 
is too late for this scenario of a lost device. One wouldn't see any 
errors, but experience a 'hang' due to inclomplete I/O. The invocation 
of virtblk_request() indirectly caused by del_gendisk() would accept 
requests, the subsequent host notification, however, would fail. (This 
is probably another 'window' that should be closed.)
>
>
>> Heinz Graalfs (3):
>>    virtio: add notify() callback to virtio_driver
>>    virtio_blk: add virtblk_notify() as virtio_driver's notify()
callback
>>    virtio_ccw: invoke virtio_driver's notify() on CIO_GONE
notification
>>
>>   drivers/block/virtio_blk.c    | 14 ++++++++++++++
>>   drivers/s390/kvm/virtio_ccw.c | 14 ++++++++++++--
>>   drivers/virtio/virtio.c       |  8 ++++++++
>>   include/linux/virtio.h        | 10 ++++++++++
>>   4 files changed, 44 insertions(+), 2 deletions(-)
>>
>> --
>> 1.8.3.1
>