RE: [PATCH v5] virtio_blk: Fix disk deletion hang on device surprise removal

From: Parav Pandit <hidden>
Date: 2025-06-26 06:29:14
Also in: stable, virtualization

Possibly related (same subject, not in this thread)

2025-06-18 · RE: [PATCH v5] virtio_blk: Fix disk deletion hang on device surprise removal · Parav Pandit <hidden>
2025-06-02 · Re: [PATCH v5] virtio_blk: Fix disk deletion hang on device surprise removal · ALOK TIWARI <hidden>

From: Michael S. Tsirkin <mst@redhat.com>
Sent: 26 June 2025 11:34 AM

On Thu, Jun 26, 2025 at 03:26:12AM +0000, Parav Pandit wrote:

quoted

From: Michael S. Tsirkin <mst@redhat.com>
Sent: 26 June 2025 12:52 AM

On Wed, Jun 25, 2025 at 07:08:54PM +0000, Parav Pandit wrote:

quoted

From: Michael S. Tsirkin <mst@redhat.com>
Sent: 25 June 2025 05:15 PM

On Wed, Jun 25, 2025 at 11:08:42AM +0000, Parav Pandit wrote:

quoted

From: Michael S. Tsirkin <mst@redhat.com>
Sent: 25 June 2025 04:34 PM

On Wed, Jun 25, 2025 at 02:55:27AM +0000, Parav Pandit wrote:

quoted

From: Michael S. Tsirkin <mst@redhat.com>
Sent: 25 June 2025 01:24 AM

On Tue, Jun 24, 2025 at 07:11:29PM +0000, Parav Pandit wrote:

quoted

From: Michael S. Tsirkin <mst@redhat.com>
Sent: 25 June 2025 12:37 AM

On Tue, Jun 24, 2025 at 07:01:44PM +0000, Parav Pandit

wrote:

quoted

quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
From: Stefan Hajnoczi <stefanha@redhat.com>
Sent: 25 June 2025 12:26 AM

On Mon, Jun 02, 2025 at 02:44:33AM +0000, Parav
Pandit
wrote:
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
When the PCI device is surprise removed,
requests may not complete the device as the VQ
is marked as

broken.

quoted

Due to this, the disk deletion hangs.

There are loops in the core virtio driver code
that expect device register reads to eventually return 0:
drivers/virtio/virtio_pci_modern.c:vp_reset()
drivers/virtio/virtio_pci_modern_dev.c:vp_modern
_set
_que
ue_r
eset
()

Is there a hang if these loops are hit when a
device has been surprise removed? I'm trying to
understand whether surprise removal is fully
supported or whether this patch is one step in
that

direction.

quoted

In one of the previous replies I answered to
Michael, but don't have the link

handy.

quoted

It is not fully supported by this patch. It will hang.

This patch restores driver back to the same state
what it was before the fixes

tag patch.

quoted

The virtio stack level work is needed to support
surprise removal, including

the reset flow you rightly pointed.

Have plans to do that?

Didn't give enough thoughts on it yet.

This one is kind of pointless then? It just fixes the
specific race window that your test harness happens to hit?

It was reported by Li from Baidu, whose tests failed.
I missed to tag "reported-by" in v5. I had it until v4. :(

quoted

Maybe it's better to wait until someone does a comprehensive

fix..

quoted

Oh, I was under impression is that you wanted to step
forward in discussion

of v4.

quoted

If you prefer a comprehensive support across layers of
virtio, I suggest you

should revert the cited patch in fixes tag.

quoted

Otherwise, it is in degraded state as virtio never
supported surprise

removal.

quoted

By reverting the cited patch (or with this fix), the
requests and disk deletion

will not hang.

But they will hung in virtio core on reset, will they not?
The tests just do not happen to trigger this?

It will hang if it a true surprise removal which no device did
so far because it

never worked.

quoted

(or did, but always hung that no one reported yet)

I am familiar with 2 or more PCI devices who reports surprise
removal,

which do not complete the requests but yet allows device reset flow.

quoted

This is because device is still there on the PCI bus. Only via
side band signals

device removal was reported.

So why do we care about it so much? I think it's great this
patch exists, for example it makes it easier to test surprise
removal and find more bugs. But is it better to just have it
hang unconditionally? Are we now making a commitment that it's
working -

one we don't seem to intend to implement?

quoted

The patch improves the situation from its current state.
But as you posted, more changes in pci layer are needed.
I didn't audit where else it may be needed.

vp_reset() may need to return the status back of successful/failure reset.
Otherwise during probe(), vp_reset() aborts the reset and attempts
to load

the driver for removed device.

yes however this is not at all different that hotunplug right after reset.

For hotunplug after reset, we likely need a timeout handler.
Because block driver running inside the remove() callback waiting for the IO,

may not get notified from driver core to synchronize ongoing remove().


Notified of what?

Notification that surprise-removal occurred.

So is the scenario that graceful remove starts, and
meanwhile a surprise removal happens?

Right.

quoted

I guess suspend() callback also infinitely waits during freezing
the queue

also needs adaptation.

Which callback is that I don't understand.

virtblk_freeze() at [1].

[1]
https://elixir.bootlin.com/linux/v6.15.3/source/drivers/block/virtio_b
lk.c#L1622

quoted

But I agree that for full support, virtio all layer changes
would be needed as

new functionality (without fixes tag  :) ).


Or with a fixes tag - lots of people just use it as a signal to
mean "where can this be reasonably backported to".

Yes, I think the fix for the older kernels is needed, hence I cced stable too.

quoted

Please let me know if I should re-send to revert the patch
listed in fixes

tag.

quoted

quoted
quoted
quoted
quoted
quoted
Apart from that, I'm happy with the virtio_blk.c
aspects of the

patch:

quoted

Reviewed-by: Stefan Hajnoczi
[off-list ref]

Thanks.

quoted

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.

quoted

Verified with multiple device unplug
iterations with pending requests in virtio
used ring and some pending with the

device.

quoted

Fixes: 43bb40c5b926 ("virtio_pci: Support
surprise removal of virtio pci device")
Cc: stable@vger.kernel.org
Reported-by: Li RongQing
[off-list ref]
Closes:
https://lore.kernel.org/virtualization/c45dd68
698c
d472
38c5
5fb7
3ca9
b474
1@baidu.com/
Reviewed-by: Max Gurtovoy
[off-list ref]
Reviewed-by: Israel Rukshin
[off-list ref]
Signed-off-by: Parav Pandit <redacted>

---
v4->v5:
- fixed comment style where comment to start
with one empty line at start
- Addressed comments from Alok
- fixed typo in broken vq check
v3->v4:
- Addressed comments from Michael
- renamed virtblk_request_cancel() to
  virtblk_complete_request_with_ioerr()
- Added comments for
virtblk_complete_request_with_ioerr()
- Renamed virtblk_broken_device_cleanup() to
  virtblk_cleanup_broken_device()
- Added comments for
virtblk_cleanup_broken_device()
- Moved the broken vq check in
virtblk_remove()
- Fixed comment style to have first empty line
- replaced freezed to frozen
- Fixed comments rephrased

v2->v3:
- Addressed comments from Michael
- updated comment for synchronizing with
callbacks

v1->v2:
- Addressed comments from Stephan
- fixed spelling to 'waiting'
- Addressed comments from Michael
- Dropped checking broken vq from queue_rq()
and

queue_rqs()

quoted

  because it is checked in lower layer
routines in virtio core

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()
---
 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..c5e383c0ac48
100644

--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c

@@ -1554,6 +1554,98 @@ static int

virtblk_probe(struct virtio_device

*vdev)

quoted

 	return err;
 }

+/*
+ * If the vq is broken, device will not complete requests.
+ * So we do it for the device.
+ */
+static bool
+virtblk_complete_request_with_ioerr(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))

quoted

+		blk_mq_complete_request(rq);
+
+	spin_unlock_irqrestore(&vq->lock, flags);
+	return true; }
+
+/*
+ * If the device is broken, it will not use
+any buffers and waiting
+ * for that to happen is pointless. We'll do
+the cleanup in the driver,
+ * completing all requests for the device.
+ */
+static void
+virtblk_cleanup_broken_device(struct
+virtio_blk *vblk)

quoted

+	struct request_queue *q = vblk->disk->queue;
+
+	/*
+	 * Start freezing the queue, so that new
+requests keeps

waiting at the

quoted

+	 * door of bio_queue_enter(). We cannot
+fully freeze the queue

because

quoted

+	 * frozen queue is an empty queue and there
+are pending

requests, so

quoted

+	 * only start freezing it.
+	 */
+	blk_freeze_queue_start(q);
+
+	/*
+	 * When quiescing completes, all ongoing
+dispatches have

completed

quoted

quoted
quoted
quoted
quoted
+	 * and no new dispatch will happen towards
+the

driver.

quoted

+	 */
+	blk_mq_quiesce_queue(q);
+
+	/*
+	 * Synchronize with any ongoing VQ callbacks
+that may have

started

quoted

+	 * before the VQs were marked as broken. Any
+outstanding

requests

quoted

quoted
quoted
quoted
quoted
+	 * will be completed by

virtblk_complete_request_with_ioerr().

quoted

+	 */
+	virtio_synchronize_cbs(vblk->vdev);
+
+	/*
+	 * At this point, no new requests can enter
+the
+queue_rq()

and

quoted

+	 * completion routine will not complete any
+new requests either for

the

quoted

+	 * broken vq. Hence, it is safe to cancel
+all requests

which are

quoted

quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
+	 * started.
+	 */
+	blk_mq_tagset_busy_iter(&vblk->tag_set,
+

	virtblk_complete_request_with_ioerr,

quoted

vblk);

quoted

+
+blk_mq_tagset_wait_completed_request(&vblk->t
+ag_s
+et);
+
+	/*
+	 * All pending requests are cleaned up. Time
+to resume so

that disk

quoted

+	 * deletion can be smooth. Start the HW
+queues so that when queue

is

quoted

+	 * unquiesced requests can again enter the driver.
+	 */
+	blk_mq_start_stopped_hw_queues(q, true);
+
+	/*
+	 * Unquiescing will trigger dispatching any
+pending requests

to the

quoted

quoted
quoted
quoted
quoted
+	 * driver which has crossed
+bio_queue_enter() to the

driver.

quoted

+	 */
+	blk_mq_unquiesce_queue(q);
+
+	/*
+	 * Wait for all pending dispatches to
+terminate which may

have been

quoted

+	 * initiated after unquiescing.
+	 */
+	blk_mq_freeze_queue_wait(q);
+
+	/*
+	 * Mark the disk dead so that once we
+unfreeze the queue,

requests

quoted

+	 * waiting at the door of bio_queue_enter()
+can be aborted right

away.

quoted

+	 */
+	blk_mark_disk_dead(vblk->disk);
+
+	/* Unfreeze the queue so that any waiting
+requests will be

aborted. */

quoted

+	blk_mq_unfreeze_queue_nomemrestore(q);
+}
+
 static void virtblk_remove(struct virtio_device *vdev)  {
 	struct virtio_blk *vblk = vdev->priv; @@
-1561,6
+1653,9 @@ static void virtblk_remove(struct
+virtio_device

*vdev)

quoted

quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
 	/* Make sure no work handler is accessing the device.

*/

quoted

 	flush_work(&vblk->config_work);

+	if (virtqueue_is_broken(vblk->vqs[0].vq))
+		virtblk_cleanup_broken_device(vblk);
+
 	del_gendisk(vblk->disk);
 	blk_mq_free_tag_set(&vblk->tag_set);

--
2.34.1

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help