On Wed, May 21, 2025 at 06:37:41AM +0000, Parav Pandit wrote: > When the PCI device is surprise removed, requests may not complete > the device as the VQ is marked as broken. Due to this, the disk > deletion hangs. > > Fix it by aborting the requests 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 iterations with pending requests in > virtio used ring and some pending with the device. > > Fixes: 43bb40c5b926 ("virtio_pci: Support surprise removal of virtio pci > device") > Cc: sta...@vger.kernel.org > Reported-by: lirongq...@baidu.com > Closes: > https://lore.kernel.org/virtualization/c45dd68698cd47238c55fb73ca9b4...@baidu.com/ > Reviewed-by: Max Gurtovoy <mgurto...@nvidia.com> > Reviewed-by: Israel Rukshin <isra...@nvidia.com> > Signed-off-by: Parav Pandit <pa...@nvidia.com> > --- > changelog: > v0->v1: > - Fixed comments from Stefan to rename a cleanup function > - Improved logic for handling any outstanding requests > in bio layer > - improved cancel callback to sync with ongoing done()
thanks for the patch! questions: > --- > drivers/block/virtio_blk.c | 95 ++++++++++++++++++++++++++++++++++++++ > 1 file changed, 95 insertions(+) > > diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c > index 7cffea01d868..5212afdbd3c7 100644 > --- a/drivers/block/virtio_blk.c > +++ b/drivers/block/virtio_blk.c > @@ -435,6 +435,13 @@ static blk_status_t virtio_queue_rq(struct blk_mq_hw_ctx > *hctx, > blk_status_t status; > int err; > > + /* Immediately fail all incoming requests if the vq is broken. > + * Once the queue is unquiesced, upper block layer flushes any pending > + * queued requests; fail them right away. > + */ > + if (unlikely(virtqueue_is_broken(vblk->vqs[qid].vq))) > + return BLK_STS_IOERR; > + > status = virtblk_prep_rq(hctx, vblk, req, vbr); > if (unlikely(status)) > return status; just below this: spin_lock_irqsave(&vblk->vqs[qid].lock, flags); err = virtblk_add_req(vblk->vqs[qid].vq, vbr); if (err) { and virtblk_add_req calls virtqueue_add_sgs, so it will fail on a broken vq. Why do we need to check it one extra time here? > @@ -508,6 +515,11 @@ static void virtio_queue_rqs(struct rq_list *rqlist) > while ((req = rq_list_pop(rqlist))) { > struct virtio_blk_vq *this_vq = get_virtio_blk_vq(req->mq_hctx); > > + if (unlikely(virtqueue_is_broken(this_vq->vq))) { > + rq_list_add_tail(&requeue_list, req); > + continue; > + } > + > if (vq && vq != this_vq) > virtblk_add_req_batch(vq, &submit_list); > vq = this_vq; similarly > @@ -1554,6 +1566,87 @@ static int virtblk_probe(struct virtio_device *vdev) > return err; > } > > +static bool virtblk_request_cancel(struct request *rq, void *data) > +{ > + struct virtblk_req *vbr = blk_mq_rq_to_pdu(rq); > + struct virtio_blk *vblk = data; > + struct virtio_blk_vq *vq; > + unsigned long flags; > + > + vq = &vblk->vqs[rq->mq_hctx->queue_num]; > + > + spin_lock_irqsave(&vq->lock, flags); > + > + vbr->in_hdr.status = VIRTIO_BLK_S_IOERR; > + if (blk_mq_request_started(rq) && !blk_mq_request_completed(rq)) > + blk_mq_complete_request(rq); > + > + spin_unlock_irqrestore(&vq->lock, flags); > + return true; > +} > + > +static void virtblk_broken_device_cleanup(struct virtio_blk *vblk) > +{ > + struct request_queue *q = vblk->disk->queue; > + > + if (!virtqueue_is_broken(vblk->vqs[0].vq)) > + return; > + > + /* Start freezing the queue, so that new requests keeps waitng at the > + * door of bio_queue_enter(). We cannot fully freeze the queue because > + * freezed queue is an empty queue and there are pending requests, so > + * only start freezing it. > + */ > + blk_freeze_queue_start(q); > + > + /* When quiescing completes, all ongoing dispatches have completed > + * and no new dispatch will happen towards the driver. > + * This ensures that later when cancel is attempted, then are not > + * getting processed by the queue_rq() or queue_rqs() handlers. > + */ > + blk_mq_quiesce_queue(q); > + > + /* > + * Synchronize with any ongoing VQ callbacks, effectively quiescing > + * the device and preventing it from completing further requests > + * to the block layer. Any outstanding, incomplete requests will be > + * completed by virtblk_request_cancel(). > + */ > + virtio_synchronize_cbs(vblk->vdev); > + > + /* At this point, no new requests can enter the queue_rq() and > + * completion routine will not complete any new requests either for the > + * broken vq. Hence, it is safe to cancel all requests which are > + * started. > + */ > + blk_mq_tagset_busy_iter(&vblk->tag_set, virtblk_request_cancel, vblk); > + blk_mq_tagset_wait_completed_request(&vblk->tag_set); > + > + /* All pending requests are cleaned up. Time to resume so that disk > + * deletion can be smooth. Start the HW queues so that when queue is > + * unquiesced requests can again enter the driver. > + */ > + blk_mq_start_stopped_hw_queues(q, true); > + > + /* Unquiescing will trigger dispatching any pending requests to the > + * driver which has crossed bio_queue_enter() to the driver. > + */ > + blk_mq_unquiesce_queue(q); > + > + /* Wait for all pending dispatches to terminate which may have been > + * initiated after unquiescing. > + */ > + blk_mq_freeze_queue_wait(q); > + > + /* Mark the disk dead so that once queue unfreeze, the requests > + * waiting at the door of bio_queue_enter() can be aborted right away. > + */ > + blk_mark_disk_dead(vblk->disk); > + > + /* Unfreeze the queue so that any waiting requests will be aborted. */ > + blk_mq_unfreeze_queue_nomemrestore(q); > +} > + > static void virtblk_remove(struct virtio_device *vdev) > { > struct virtio_blk *vblk = vdev->priv; > @@ -1561,6 +1654,8 @@ static void virtblk_remove(struct virtio_device *vdev) > /* Make sure no work handler is accessing the device. */ > flush_work(&vblk->config_work); > > + virtblk_broken_device_cleanup(vblk); > + > del_gendisk(vblk->disk); > blk_mq_free_tag_set(&vblk->tag_set); > > -- > 2.34.1