Re: [PATCH] virtio: Remove virtio device during shutdown

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

On Wed, Mar 11, 2015 at 06:11:35PM +0800, Fam Zheng wrote:
> On Wed, 03/11 10:06, Michael S. Tsirkin wrote:
> > On Wed, Mar 11, 2015 at 04:09:17PM +0800, Fam Zheng wrote:
> > > Currently shutdown is nop for virtio devices, but the core code could
> > > remove things behind us such as MSI-X handler etc. For example in the
> > > case of virtio-scsi-pci, the device may still try to send interupts,
> > > which will be on IRQ lines seeing MSI-X disabled. Those interrupts will
> > > be unhandled, and may cause flood.
> 
> Here is the problem I want to solve - file system driver hang:
> 
> If a fs code happen to hit __wait_on_buffer right after pci pci_device_shutdown
> disabled msix, it will never make progress because the requests it waits for
> will never be completed. So the system hangs.

Paolo says that pci reset of virtio scsi device guarantees
that all outstanding requests complete.

If true and implemented correctly, I don't see what else
needs to be done.

You will need to debug this some more.


> In other words we will want to reset virtio device before pci_device_shutdown
> AND wake up all waiters.
> 
> Unfortunately, neither your patch nor mine does that, because virtio bus can be
> shutdown after pci bus (thanks to Jason for pointing out this). In that case,
> any completion after disabling msix is lost.
> 
> Maybe we need both the pci shutdown handler to reset the device and the virtio
> shutdown handler to remove the device?
> 
> Fam
> 
> > 
> > This sounds very tentative. Do you, in fact, observe some problems
> > with virtio scsi? How to reproduce them? this needs to go
> > into the commit messages.
> 
> OK, my bad.
> 
> > 
> > > Remove the device in "shutdown" callback to allow device drivers clean
> > > up things.
> > > 
> > > Signed-off-by: Fam Zheng <famz@redhat.com>
> > 
> > I'm concerned this will cause more hangs on shutdown: one
> > of the reasons for reboot is device mal-functioning.
> > How about we just reset devices instead? Something like
> > the below (untested).
> > 
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > 
> > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> > index 5ce2aa4..0769941 100644
> > --- a/drivers/virtio/virtio.c
> > +++ b/drivers/virtio/virtio.c
> > @@ -269,6 +269,17 @@ static int virtio_dev_remove(struct device *_d)
> >  	return 0;
> >  }
> >  
> > +static void virtio_dev_shutdown(struct device *_d)
> > +{
> > +	struct virtio_device *dev = dev_to_virtio(_d);
> > +	/*
> > +	 * Reset the device to make it stop sending interrupts, DMA, etc.
> > +	 * We are shutting down, no need for full cleanup.
> > +	 */
> > +	dev->config->reset(dev);
> > +
> > +}
> > +
> >  static struct bus_type virtio_bus = {
> >  	.name  = "virtio",
> >  	.match = virtio_dev_match,
> > @@ -276,6 +288,7 @@ static struct bus_type virtio_bus = {
> >  	.uevent = virtio_uevent,
> >  	.probe = virtio_dev_probe,
> >  	.remove = virtio_dev_remove,
> > +	.shutdown = virtio_dev_shutdown,
> >  };
> >  
> >  bool virtio_device_is_legacy_only(struct virtio_device_id id)