Re: [PATCH] Revert "virtio_pci: Support surprise removal of virtio pci device"

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

On Wed, Apr 09, 2025 at 01:50:18PM +0000, Parav Pandit wrote:
> Hi Michael,
> 
> > From: Michael S. Tsirkin <mst@redhat.com>
> > Sent: Wednesday, April 9, 2025 1:45 AM
> > 
> > On Tue, Apr 08, 2025 at 05:59:08PM +0300, Parav Pandit wrote:
> > > This reverts commit 43bb40c5b926 ("virtio_pci: Support surprise removal of
> > virtio pci device").
> > >
> > > The cited commit introduced a fix that marks the device as broken
> > > during surprise removal. However, this approach causes uncompleted I/O
> > > requests on virtio-blk device. The presence of uncompleted I/O
> > > requests prevents the successful removal of virtio-blk devices.
> > >
> > > This fix allows devices that simulate a surprise removal but actually
> > > remove gracefully to continue working as before.
> > >
> > > For surprise removals, a better solution will be preferred in the future.
> > 
> > Sorry I'm not breaking one thing to fix another.
> > Device is gone so no new requests will be completed. Why not complete all
> > unfinished requests, for example?
> > 
> > Come up with a proper fix pls.
> > 
> I would also like to have a proper fix that can be backportable.
> However, an attempt [1] had race.
> To overcome the race, a different approach also tried, however the block layer was stuck even if all requests in virtio-blk driver layer was completed like you suggested.
> 
> It appeared that supporting uncompleted requests won't be so straightforward to backport.
> 
> Hence, the request is to revert and restore the previous behavior.
> This at least improves the case where the OS thinks that surprise removal occurred, but the device eventually completes the IO.
> And hence, virtio block driver successfully unloads.
> And virtio-net also does not experience the mentioned crash.
> 
> [1] https://lore.kernel.org/all/20240217180848.241068-1-parav@nvidia.com/

Parav this is a commit from 2021. I am not reverting it "because it
seems to help". We'll never make progress like this.
You will have to debug and fix it properly. Sorry.

Once we have a fix, we will worry about backports and stuff, this
is how we do kernel development here.


> > >
> > > Fixes: 43bb40c5b926 ("virtio_pci: Support surprise removal of virtio
> > > pci device")
> > > Cc: stable@vger.kernel.org
> > > Reported-by: lirongqing@baidu.com
> > > Closes:
> > > https://lore.kernel.org/virtualization/c45dd68698cd47238c55fb73ca9b474
> > > 1@baidu.com/
> > > Reviewed-by: Max Gurtovoy<mgurtovoy@nvidia.com>
> > > Signed-off-by: Parav Pandit <parav@nvidia.com>
> > 
> > 
> > > ---
> > >  drivers/virtio/virtio_pci_common.c | 7 -------
> > >  1 file changed, 7 deletions(-)
> > >
> > > diff --git a/drivers/virtio/virtio_pci_common.c
> > > b/drivers/virtio/virtio_pci_common.c
> > > index d6d79af44569..dba5eb2eaff9 100644
> > > --- a/drivers/virtio/virtio_pci_common.c
> > > +++ b/drivers/virtio/virtio_pci_common.c
> > > @@ -747,13 +747,6 @@ static void virtio_pci_remove(struct pci_dev
> > *pci_dev)
> > >  	struct virtio_pci_device *vp_dev = pci_get_drvdata(pci_dev);
> > >  	struct device *dev = get_device(&vp_dev->vdev.dev);
> > >
> > > -	/*
> > > -	 * Device is marked broken on surprise removal so that virtio upper
> > > -	 * layers can abort any ongoing operation.
> > > -	 */
> > > -	if (!pci_device_is_present(pci_dev))
> > > -		virtio_break_device(&vp_dev->vdev);
> > > -
> > >  	pci_disable_sriov(pci_dev);
> > >
> > >  	unregister_virtio_device(&vp_dev->vdev);
> > > --
> > > 2.26.2