> From: Michael S. Tsirkin <[email protected]> > Sent: Monday, February 19, 2024 4:17 PM > > On Mon, Feb 19, 2024 at 10:39:36AM +0000, Parav Pandit wrote: > > > From: Michael S. Tsirkin <[email protected]> > > > Sent: Monday, February 19, 2024 1:45 PM > > > > > > On Mon, Feb 19, 2024 at 03:14:54AM +0000, Parav Pandit wrote: > > > > Hi Ming, > > > > > > > > > From: Ming Lei <[email protected]> > > > > > Sent: Sunday, February 18, 2024 6:57 PM > > > > > > > > > > On Sat, Feb 17, 2024 at 08:08:48PM +0200, Parav Pandit wrote: > > > > > > When the PCI device is surprise removed, requests won't > > > > > > complete from the device. These IOs are never completed and > > > > > > disk deletion hangs indefinitely. > > > > > > > > > > > > Fix it by aborting the IOs which the device will never > > > > > > complete when the VQ is broken. > > > > > > > > > > > > With this fix now fio completes swiftly. > > > > > > An alternative of IO timeout has been considered, however when > > > > > > the driver knows about unresponsive block device, swiftly > > > > > > clearing them enables users and upper layers to react quickly. > > > > > > > > > > > > Verified with multiple device unplug cycles with pending IOs > > > > > > in virtio used ring and some pending with device. > > > > > > > > > > > > In future instead of VQ broken, a more elegant method can be used. > > > > > > At the moment the patch is kept to its minimal changes given > > > > > > its urgency to fix broken kernels. > > > > > > > > > > > > Fixes: 43bb40c5b926 ("virtio_pci: Support surprise removal of > > > > > > virtio pci device") > > > > > > Cc: [email protected] > > > > > > Reported-by: [email protected] > > > > > > Closes: > > > > > > https://lore.kernel.org/virtualization/c45dd68698cd47238c55fb7 > > > > > > 3ca9 > > > > > > b474 > > > > > > [email protected]/ > > > > > > Co-developed-by: Chaitanya Kulkarni <[email protected]> > > > > > > Signed-off-by: Chaitanya Kulkarni <[email protected]> > > > > > > Signed-off-by: Parav Pandit <[email protected]> > > > > > > --- > > > > > > drivers/block/virtio_blk.c | 54 > > > > > > ++++++++++++++++++++++++++++++++++++++ > > > > > > 1 file changed, 54 insertions(+) > > > > > > > > > > > > diff --git a/drivers/block/virtio_blk.c > > > > > > b/drivers/block/virtio_blk.c index 2bf14a0e2815..59b49899b229 > > > > > > 100644 > > > > > > --- a/drivers/block/virtio_blk.c > > > > > > +++ b/drivers/block/virtio_blk.c > > > > > > @@ -1562,10 +1562,64 @@ static int virtblk_probe(struct > > > > > > virtio_device > > > > > *vdev) > > > > > > return err; > > > > > > } > > > > > > > > > > > > +static bool virtblk_cancel_request(struct request *rq, void *data) > > > > > > { > > > > > > + struct virtblk_req *vbr = blk_mq_rq_to_pdu(rq); > > > > > > + > > > > > > + vbr->in_hdr.status = VIRTIO_BLK_S_IOERR; > > > > > > + if (blk_mq_request_started(rq) && > !blk_mq_request_completed(rq)) > > > > > > + blk_mq_complete_request(rq); > > > > > > + > > > > > > + return true; > > > > > > +} > > > > > > + > > > > > > +static void virtblk_cleanup_reqs(struct virtio_blk *vblk) { > > > > > > + struct virtio_blk_vq *blk_vq; > > > > > > + struct request_queue *q; > > > > > > + struct virtqueue *vq; > > > > > > + unsigned long flags; > > > > > > + int i; > > > > > > + > > > > > > + vq = vblk->vqs[0].vq; > > > > > > + if (!virtqueue_is_broken(vq)) > > > > > > + return; > > > > > > + > > > > > > > > > > What if the surprise happens after the above check? > > > > > > > > > > > > > > In that small timing window, the race still exists. > > > > > > > > I think, blk_mq_quiesce_queue(q); should move up before > > > > cleanup_reqs() > > > regardless of surprise case along with other below changes. > > > > > > > > Additionally, for non-surprise case, better to have a graceful > > > > timeout to > > > complete already queued requests. > > > > In absence of timeout scheme for this regression, shall we only > > > > complete the > > > requests which the device has already completed (instead of waiting > > > for the grace time)? > > > > There was past work from Chaitanaya, for the graceful timeout. > > > > > > > > The sequence for the fix I have in mind is: > > > > 1. quiesce the queue > > > > 2. complete all requests which has completed, with its status 3. > > > > stop the transport (queues) 4. complete remaining pending requests > > > > with error status > > > > > > > > This should work regardless of surprise case. > > > > An additional/optional graceful timeout on non-surprise case can > > > > be helpful > > > for #2. > > > > > > > > WDYT? > > > > > > All this is unnecessarily hard for drivers... I am thinking maybe > > > after we set broken we should go ahead and invoke all callbacks. > > > > Yes, #2 is about invoking the callbacks. > > > > The issue is not with setting the flag broken. As Ming pointed, the issue > > is : > we may miss setting the broken. > > > So if we did get callbacks, we'd be able to test broken flag in the callback. > Yes, getting callbacks is fine. But when the device is surprise removed, we wont get the callbacks and completions are missed.
> > Without graceful time out it is straight forward code, just rearrangement of > APIs in this patch with existing code. > > > > The question is : it is really if we really care for that grace period when > > the > device or driver is already on its exit path and VQ is not broken. > > If we don't wait for the request in progress, is it ok? > > > > If we are talking about physical hardware, it seems quite possible that > removal > triggers then user gets impatient and yanks the card out. > Yes, regardless of surprise or not, completing the remaining IOs is just good enough. Device is anyway on its exit path, so completing 10 commands vs 12, does not make a lot of difference with extra complexity of timeout. So better to not complicate the driver, at least not when adding Fixes tag patch. > > > > interrupt handling core is not making it easy for us - we must > > > disable real interrupts if we do, and in the past we failed to do it. > > > See e.g. > > > > > > > > > commit eb4cecb453a19b34d5454b49532e09e9cb0c1529 > > > Author: Jason Wang <[email protected]> > > > Date: Wed Mar 23 11:15:24 2022 +0800 > > > > > > Revert "virtio_pci: harden MSI-X interrupts" > > > > > > This reverts commit 9e35276a5344f74d4a3600fc4100b3dd251d5c56. > > > Issue > > > were reported for the drivers that are using affinity managed IRQ > > > where manually toggling IRQ status is not expected. And we forget to > > > enable the interrupts in the restore path as well. > > > > > > In the future, we will rework on the interrupt hardening. > > > > > > Fixes: 9e35276a5344 ("virtio_pci: harden MSI-X interrupts") > > > > > > > > > > > > If someone can figure out a way to make toggling interrupt state > > > play nice with affinity managed interrupts, that would solve a host of > issues I feel. > > > > > > > > > > > > > > Thanks, > > > > > Ming
