Re: [PATCH RFC 0/3] virtio: add new notify() callback to virtio_driver

["Michael S. Tsirkin" <mst@redhat.com>]

On Thu, Nov 21, 2013 at 03:43:51PM +0100, Heinz Graalfs wrote:
> 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.)

I see. All this makes sense.

So it's really important that the event is sent
*before* device is removed.

Maybe it's a good idea to rename event GONE->GOING_AWAY ?

> 
> >
> >
> >>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
> >