* [PATCH v2 RFC 0/3] virtio: add new notify() callback to virtio_driver
@ 2013-11-21 14:45 Heinz Graalfs
2013-11-21 14:45 ` [PATCH v2 RFC 1/3] virtio: add " Heinz Graalfs
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Heinz Graalfs @ 2013-11-21 14:45 UTC (permalink / raw)
To: rusty, virtualization; +Cc: borntraeger, mst
Hi, here is an updated patch-set with changes as suggested by Michael Tsirkin.
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.
v1->v2 changes:
- add include of linux/notifier.h (I also added it to the 3rd patch)
- get queue lock in order to be able to use safe queue_flag_set() functions
in virtblk_notify() handler
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 | 17 +++++++++++++++++
drivers/s390/kvm/virtio_ccw.c | 15 +++++++++++++--
drivers/virtio/virtio.c | 9 +++++++++
include/linux/virtio.h | 10 ++++++++++
4 files changed, 49 insertions(+), 2 deletions(-)
--
1.8.3.1
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 RFC 1/3] virtio: add notify() callback to virtio_driver
2013-11-21 14:45 [PATCH v2 RFC 0/3] virtio: add new notify() callback to virtio_driver Heinz Graalfs
@ 2013-11-21 14:45 ` Heinz Graalfs
2013-11-21 15:18 ` Michael S. Tsirkin
2013-11-21 14:45 ` [PATCH v2 RFC 2/3] virtio_blk: add virtblk_notify() as virtio_driver's notify() callback Heinz Graalfs
2013-11-21 14:45 ` [PATCH v2 RFC 3/3] virtio_ccw: invoke virtio_driver's notify() on CIO_GONE notification Heinz Graalfs
2 siblings, 1 reply; 10+ messages in thread
From: Heinz Graalfs @ 2013-11-21 14:45 UTC (permalink / raw)
To: rusty, virtualization; +Cc: borntraeger, mst
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@linux.vnet.ibm.com>
---
drivers/virtio/virtio.c | 9 +++++++++
include/linux/virtio.h | 10 ++++++++++
2 files changed, 19 insertions(+)
diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index ee59b74..a7f4302 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -3,6 +3,7 @@
#include <linux/virtio_config.h>
#include <linux/module.h>
#include <linux/idr.h>
+#include <linux/notifier.h>
/* Unique numbering for virtio devices. */
static DEFINE_IDA(virtio_index_ida);
@@ -186,6 +187,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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v2 RFC 2/3] virtio_blk: add virtblk_notify() as virtio_driver's notify() callback
2013-11-21 14:45 [PATCH v2 RFC 0/3] virtio: add new notify() callback to virtio_driver Heinz Graalfs
2013-11-21 14:45 ` [PATCH v2 RFC 1/3] virtio: add " Heinz Graalfs
@ 2013-11-21 14:45 ` Heinz Graalfs
2013-11-21 14:45 ` [PATCH v2 RFC 3/3] virtio_ccw: invoke virtio_driver's notify() on CIO_GONE notification Heinz Graalfs
2 siblings, 0 replies; 10+ messages in thread
From: Heinz Graalfs @ 2013-11-21 14:45 UTC (permalink / raw)
To: rusty, virtualization; +Cc: borntraeger, mst
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@linux.vnet.ibm.com>
---
drivers/block/virtio_blk.c | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 2d43be4..04fd635 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -11,6 +11,7 @@
#include <linux/string_helpers.h>
#include <scsi/scsi_cmnd.h>
#include <linux/idr.h>
+#include <linux/notifier.h>
#define PART_BITS 4
@@ -901,6 +902,21 @@ 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;
+ unsigned long flags;
+
+ if (event == VDEV_GONE) {
+ spin_lock_irqsave(vblk->disk->queue->queue_lock, flags);
+ queue_flag_set(QUEUE_FLAG_DYING, vblk->disk->queue);
+ queue_flag_set(QUEUE_FLAG_NOMERGES, vblk->disk->queue);
+ queue_flag_set(QUEUE_FLAG_NOXMERGES, vblk->disk->queue);
+ spin_unlock_irqrestore(vblk->disk->queue->queue_lock, flags);
+ }
+ return NOTIFY_DONE;
+}
+
#ifdef CONFIG_PM
static int virtblk_freeze(struct virtio_device *vdev)
{
@@ -961,6 +977,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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v2 RFC 3/3] virtio_ccw: invoke virtio_driver's notify() on CIO_GONE notification
2013-11-21 14:45 [PATCH v2 RFC 0/3] virtio: add new notify() callback to virtio_driver Heinz Graalfs
2013-11-21 14:45 ` [PATCH v2 RFC 1/3] virtio: add " Heinz Graalfs
2013-11-21 14:45 ` [PATCH v2 RFC 2/3] virtio_blk: add virtblk_notify() as virtio_driver's notify() callback Heinz Graalfs
@ 2013-11-21 14:45 ` Heinz Graalfs
2013-11-21 15:15 ` Michael S. Tsirkin
2 siblings, 1 reply; 10+ messages in thread
From: Heinz Graalfs @ 2013-11-21 14:45 UTC (permalink / raw)
To: rusty, virtualization; +Cc: borntraeger, mst
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@linux.vnet.ibm.com>
---
drivers/s390/kvm/virtio_ccw.c | 15 +++++++++++++--
1 file changed, 13 insertions(+), 2 deletions(-)
diff --git a/drivers/s390/kvm/virtio_ccw.c b/drivers/s390/kvm/virtio_ccw.c
index 35b9aaa..682f688 100644
--- a/drivers/s390/kvm/virtio_ccw.c
+++ b/drivers/s390/kvm/virtio_ccw.c
@@ -27,6 +27,7 @@
#include <linux/module.h>
#include <linux/io.h>
#include <linux/kvm_para.h>
+#include <linux/notifier.h>
#include <asm/setup.h>
#include <asm/irq.h>
#include <asm/cio.h>
@@ -1064,8 +1065,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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2 RFC 3/3] virtio_ccw: invoke virtio_driver's notify() on CIO_GONE notification
2013-11-21 14:45 ` [PATCH v2 RFC 3/3] virtio_ccw: invoke virtio_driver's notify() on CIO_GONE notification Heinz Graalfs
@ 2013-11-21 15:15 ` Michael S. Tsirkin
2013-11-21 17:12 ` Heinz Graalfs
0 siblings, 1 reply; 10+ messages in thread
From: Michael S. Tsirkin @ 2013-11-21 15:15 UTC (permalink / raw)
To: Heinz Graalfs; +Cc: borntraeger, virtualization
On Thu, Nov 21, 2013 at 03:45:33PM +0100, Heinz Graalfs wrote:
> 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@linux.vnet.ibm.com>
simple question: is this serialized with device removal?
If this races with removal we have a problem.
> ---
> drivers/s390/kvm/virtio_ccw.c | 15 +++++++++++++--
> 1 file changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/s390/kvm/virtio_ccw.c b/drivers/s390/kvm/virtio_ccw.c
> index 35b9aaa..682f688 100644
> --- a/drivers/s390/kvm/virtio_ccw.c
> +++ b/drivers/s390/kvm/virtio_ccw.c
> @@ -27,6 +27,7 @@
> #include <linux/module.h>
> #include <linux/io.h>
> #include <linux/kvm_para.h>
> +#include <linux/notifier.h>
> #include <asm/setup.h>
> #include <asm/irq.h>
> #include <asm/cio.h>
> @@ -1064,8 +1065,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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 RFC 1/3] virtio: add notify() callback to virtio_driver
2013-11-21 14:45 ` [PATCH v2 RFC 1/3] virtio: add " Heinz Graalfs
@ 2013-11-21 15:18 ` Michael S. Tsirkin
0 siblings, 0 replies; 10+ messages in thread
From: Michael S. Tsirkin @ 2013-11-21 15:18 UTC (permalink / raw)
To: Heinz Graalfs; +Cc: borntraeger, virtualization
On Thu, Nov 21, 2013 at 03:45:31PM +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@linux.vnet.ibm.com>
> ---
> drivers/virtio/virtio.c | 9 +++++++++
> include/linux/virtio.h | 10 ++++++++++
> 2 files changed, 19 insertions(+)
>
> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> index ee59b74..a7f4302 100644
> --- a/drivers/virtio/virtio.c
> +++ b/drivers/virtio/virtio.c
> @@ -3,6 +3,7 @@
> #include <linux/virtio_config.h>
> #include <linux/module.h>
> #include <linux/idr.h>
> +#include <linux/notifier.h>
>
> /* Unique numbering for virtio devices. */
> static DEFINE_IDA(virtio_index_ida);
> @@ -186,6 +187,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,
> +};
OK after understanding what this does, I think a
better name would be
VIRTIO_DEV_SURPRIZE_REMOVAL
this is what happened here.
> +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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 RFC 3/3] virtio_ccw: invoke virtio_driver's notify() on CIO_GONE notification
2013-11-21 15:15 ` Michael S. Tsirkin
@ 2013-11-21 17:12 ` Heinz Graalfs
2013-11-21 18:31 ` Michael S. Tsirkin
0 siblings, 1 reply; 10+ messages in thread
From: Heinz Graalfs @ 2013-11-21 17:12 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: borntraeger, virtualization
On 21/11/13 16:15, Michael S. Tsirkin wrote:
> On Thu, Nov 21, 2013 at 03:45:33PM +0100, Heinz Graalfs wrote:
>> 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@linux.vnet.ibm.com>
>
> simple question: is this serialized with device removal?
> If this races with removal we have a problem.
>
notify and remove callbacks are not serialized.
Additional processing in the notify handler is now locked by the queue lock.
If remove is already active (e.g. driver unregister) and performing its
cleanup (via virtblk_remove), we should definitely perform the (locked)
request queue processing in notify (little later), because if a notify
comes in, the device is GONE (not going away). Earlier triggered cleanup
processing is about to fail anyway.
>> ---
>> drivers/s390/kvm/virtio_ccw.c | 15 +++++++++++++--
>> 1 file changed, 13 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/s390/kvm/virtio_ccw.c b/drivers/s390/kvm/virtio_ccw.c
>> index 35b9aaa..682f688 100644
>> --- a/drivers/s390/kvm/virtio_ccw.c
>> +++ b/drivers/s390/kvm/virtio_ccw.c
>> @@ -27,6 +27,7 @@
>> #include <linux/module.h>
>> #include <linux/io.h>
>> #include <linux/kvm_para.h>
>> +#include <linux/notifier.h>
>> #include <asm/setup.h>
>> #include <asm/irq.h>
>> #include <asm/cio.h>
>> @@ -1064,8 +1065,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
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 RFC 3/3] virtio_ccw: invoke virtio_driver's notify() on CIO_GONE notification
2013-11-21 17:12 ` Heinz Graalfs
@ 2013-11-21 18:31 ` Michael S. Tsirkin
2013-11-27 10:32 ` Heinz Graalfs
0 siblings, 1 reply; 10+ messages in thread
From: Michael S. Tsirkin @ 2013-11-21 18:31 UTC (permalink / raw)
To: Heinz Graalfs; +Cc: borntraeger, virtualization
On Thu, Nov 21, 2013 at 06:12:21PM +0100, Heinz Graalfs wrote:
> On 21/11/13 16:15, Michael S. Tsirkin wrote:
> >On Thu, Nov 21, 2013 at 03:45:33PM +0100, Heinz Graalfs wrote:
> >>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@linux.vnet.ibm.com>
> >
> >simple question: is this serialized with device removal?
> >If this races with removal we have a problem.
> >
>
> notify and remove callbacks are not serialized.
>
> Additional processing in the notify handler is now locked by the queue lock.
>
> If remove is already active (e.g. driver unregister) and performing
> its cleanup (via virtblk_remove), we should definitely perform the
> (locked) request queue processing in notify (little later), because
> if a notify comes in, the device is GONE (not going away). Earlier
> triggered cleanup processing is about to fail anyway.
I don't get it. It's possible nothing is in queue
or everything timed out.
Then this notify will address freed memory.
I'm starting to think the right solution is to pass
a flag to remove: bool surprize_removal.
Then you will cleanly set flags before removing the device.
No new callbacks and no races.
> >>---
> >> drivers/s390/kvm/virtio_ccw.c | 15 +++++++++++++--
> >> 1 file changed, 13 insertions(+), 2 deletions(-)
> >>
> >>diff --git a/drivers/s390/kvm/virtio_ccw.c b/drivers/s390/kvm/virtio_ccw.c
> >>index 35b9aaa..682f688 100644
> >>--- a/drivers/s390/kvm/virtio_ccw.c
> >>+++ b/drivers/s390/kvm/virtio_ccw.c
> >>@@ -27,6 +27,7 @@
> >> #include <linux/module.h>
> >> #include <linux/io.h>
> >> #include <linux/kvm_para.h>
> >>+#include <linux/notifier.h>
> >> #include <asm/setup.h>
> >> #include <asm/irq.h>
> >> #include <asm/cio.h>
> >>@@ -1064,8 +1065,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
> >
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 RFC 3/3] virtio_ccw: invoke virtio_driver's notify() on CIO_GONE notification
2013-11-21 18:31 ` Michael S. Tsirkin
@ 2013-11-27 10:32 ` Heinz Graalfs
2013-11-27 10:42 ` Michael S. Tsirkin
0 siblings, 1 reply; 10+ messages in thread
From: Heinz Graalfs @ 2013-11-27 10:32 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: borntraeger, virtualization
On 21/11/13 19:31, Michael S. Tsirkin wrote:
> On Thu, Nov 21, 2013 at 06:12:21PM +0100, Heinz Graalfs wrote:
>> On 21/11/13 16:15, Michael S. Tsirkin wrote:
>>> On Thu, Nov 21, 2013 at 03:45:33PM +0100, Heinz Graalfs wrote:
>>>> 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@linux.vnet.ibm.com>
>>>
>>> simple question: is this serialized with device removal?
>>> If this races with removal we have a problem.
>>>
>>
>> notify and remove callbacks are not serialized.
>>
>> Additional processing in the notify handler is now locked by the queue lock.
>>
>> If remove is already active (e.g. driver unregister) and performing
>> its cleanup (via virtblk_remove), we should definitely perform the
>> (locked) request queue processing in notify (little later), because
>> if a notify comes in, the device is GONE (not going away). Earlier
>> triggered cleanup processing is about to fail anyway.
>
> I don't get it. It's possible nothing is in queue
> or everything timed out.
> Then this notify will address freed memory.
>
> I'm starting to think the right solution is to pass
> a flag to remove: bool surprize_removal.
>
> Then you will cleanly set flags before removing the device.
>
> No new callbacks and no races.
OK, I will send a v3 RFC.
However, I suppose we will always have some kind of 'race' wrt this
weird scenario, ending up in different kind of hang situations.
>
>>>> ---
>>>> drivers/s390/kvm/virtio_ccw.c | 15 +++++++++++++--
>>>> 1 file changed, 13 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/s390/kvm/virtio_ccw.c b/drivers/s390/kvm/virtio_ccw.c
>>>> index 35b9aaa..682f688 100644
>>>> --- a/drivers/s390/kvm/virtio_ccw.c
>>>> +++ b/drivers/s390/kvm/virtio_ccw.c
>>>> @@ -27,6 +27,7 @@
>>>> #include <linux/module.h>
>>>> #include <linux/io.h>
>>>> #include <linux/kvm_para.h>
>>>> +#include <linux/notifier.h>
>>>> #include <asm/setup.h>
>>>> #include <asm/irq.h>
>>>> #include <asm/cio.h>
>>>> @@ -1064,8 +1065,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
>>>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 RFC 3/3] virtio_ccw: invoke virtio_driver's notify() on CIO_GONE notification
2013-11-27 10:32 ` Heinz Graalfs
@ 2013-11-27 10:42 ` Michael S. Tsirkin
0 siblings, 0 replies; 10+ messages in thread
From: Michael S. Tsirkin @ 2013-11-27 10:42 UTC (permalink / raw)
To: Heinz Graalfs; +Cc: borntraeger, virtualization
On Wed, Nov 27, 2013 at 11:32:07AM +0100, Heinz Graalfs wrote:
> On 21/11/13 19:31, Michael S. Tsirkin wrote:
> >On Thu, Nov 21, 2013 at 06:12:21PM +0100, Heinz Graalfs wrote:
> >>On 21/11/13 16:15, Michael S. Tsirkin wrote:
> >>>On Thu, Nov 21, 2013 at 03:45:33PM +0100, Heinz Graalfs wrote:
> >>>>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@linux.vnet.ibm.com>
> >>>
> >>>simple question: is this serialized with device removal?
> >>>If this races with removal we have a problem.
> >>>
> >>
> >>notify and remove callbacks are not serialized.
> >>
> >>Additional processing in the notify handler is now locked by the queue lock.
> >>
> >>If remove is already active (e.g. driver unregister) and performing
> >>its cleanup (via virtblk_remove), we should definitely perform the
> >>(locked) request queue processing in notify (little later), because
> >>if a notify comes in, the device is GONE (not going away). Earlier
> >>triggered cleanup processing is about to fail anyway.
> >
> >I don't get it. It's possible nothing is in queue
> >or everything timed out.
> >Then this notify will address freed memory.
> >
> >I'm starting to think the right solution is to pass
> >a flag to remove: bool surprize_removal.
> >
> >Then you will cleanly set flags before removing the device.
> >
> >No new callbacks and no races.
>
> OK, I will send a v3 RFC.
>
> However, I suppose we will always have some kind of 'race' wrt this
> weird scenario, ending up in different kind of hang situations.
Handling surprise removal reliably is not easy.
WHQL has tests for this so it's doable if drivers are
programmed defencively.
> >
> >>>>---
> >>>> drivers/s390/kvm/virtio_ccw.c | 15 +++++++++++++--
> >>>> 1 file changed, 13 insertions(+), 2 deletions(-)
> >>>>
> >>>>diff --git a/drivers/s390/kvm/virtio_ccw.c b/drivers/s390/kvm/virtio_ccw.c
> >>>>index 35b9aaa..682f688 100644
> >>>>--- a/drivers/s390/kvm/virtio_ccw.c
> >>>>+++ b/drivers/s390/kvm/virtio_ccw.c
> >>>>@@ -27,6 +27,7 @@
> >>>> #include <linux/module.h>
> >>>> #include <linux/io.h>
> >>>> #include <linux/kvm_para.h>
> >>>>+#include <linux/notifier.h>
> >>>> #include <asm/setup.h>
> >>>> #include <asm/irq.h>
> >>>> #include <asm/cio.h>
> >>>>@@ -1064,8 +1065,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
> >>>
> >
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2013-11-27 10:42 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-11-21 14:45 [PATCH v2 RFC 0/3] virtio: add new notify() callback to virtio_driver Heinz Graalfs
2013-11-21 14:45 ` [PATCH v2 RFC 1/3] virtio: add " Heinz Graalfs
2013-11-21 15:18 ` Michael S. Tsirkin
2013-11-21 14:45 ` [PATCH v2 RFC 2/3] virtio_blk: add virtblk_notify() as virtio_driver's notify() callback Heinz Graalfs
2013-11-21 14:45 ` [PATCH v2 RFC 3/3] virtio_ccw: invoke virtio_driver's notify() on CIO_GONE notification Heinz Graalfs
2013-11-21 15:15 ` Michael S. Tsirkin
2013-11-21 17:12 ` Heinz Graalfs
2013-11-21 18:31 ` Michael S. Tsirkin
2013-11-27 10:32 ` Heinz Graalfs
2013-11-27 10:42 ` Michael S. Tsirkin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).