mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
* [PATCH] virtio-blk: clamp max_segments when indirect descriptors are disabled
@ 2026-08-14 10:59 Sergii Ushakov
  2026-08-17  7:47 ` Christoph Hellwig
  2026-08-17 13:42 ` [PATCH v2] " Sergii Ushakov
  0 siblings, 2 replies; 12+ messages in thread
From: Sergii Ushakov @ 2026-08-14 10:59 UTC (permalink / raw)
  To: virtualization, linux-block
  Cc: linux-kernel, Michael S . Tsirkin, Jason Wang, Jens Axboe,
	Xuan Zhuo, Eugenio Pérez, Paolo Bonzini, Stefan Hajnoczi,
	Sergii Ushakov

When VIRTIO_RING_F_INDIRECT_DESC is not negotiated by the host, every
scatter-gather segment in a request must consume a physical slot in
the virtqueue ring.

If the host does not advertise VIRTIO_BLK_F_SEG_MAX and provides a small
virtqueue (e.g. 128 descriptors on QNX Hypervisor), the block layer
defaults max_segments to BLK_MAX_SEGMENTS (1024). When a multi-page
compound bio arrives from the page cache, virtqueue_add_split() rejects
the request with -ENOSPC and triggers:

  WARNING: at drivers/virtio/virtio_ring.c:1493 virtqueue_add+...
  WARN_ON_ONCE(total_sg > vq->split.vring.num && !vq->indirect);

This permanently wedges the blk-mq queue and blocks all subsequent disk
I/O in uninterruptible sleep (D state).

Add a virtio_blk.max_segments module parameter to allow runtime cmdline
overrides, and automatically clamp sg_elems to
(virtqueue_get_vring_size - 2) when indirect descriptors are disabled.

Signed-off-by: Sergii Ushakov <sergiiushakov@google.com>
---
 drivers/block/virtio_blk.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 32bf3ba07a9d..082acd90a02d 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -41,6 +41,10 @@ static unsigned int poll_queues;
 module_param(poll_queues, uint, 0644);
 MODULE_PARM_DESC(poll_queues, "The number of dedicated virtqueues for polling I/O");
 
+static unsigned int max_segments;
+module_param(max_segments, uint, 0644);
+MODULE_PARM_DESC(max_segments, "Override maximum number of segments per request");
+
 static int major;
 static DEFINE_IDA(vd_index_ida);
 
@@ -1267,6 +1271,12 @@ static int virtblk_read_limits(struct virtio_blk *vblk,
 	/* Prevent integer overflows and honor max vq size */
 	sg_elems = min_t(u32, sg_elems, VIRTIO_BLK_MAX_SG_ELEMS - 2);
 
+	if (max_segments)
+		sg_elems = min_t(u32, sg_elems, max_segments);
+	else if (!virtio_has_feature(vdev, VIRTIO_RING_F_INDIRECT_DESC))
+		sg_elems = min_t(u32, sg_elems,
+				 virtqueue_get_vring_size(vblk->vqs[0].vq) - 2);
+
 	/* We can handle whatever the host told us to handle. */
 	lim->max_segments = sg_elems;
 
-- 
2.55.0.691.gc56d675ccc-goog


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] virtio-blk: clamp max_segments when indirect descriptors are disabled
  2026-08-14 10:59 [PATCH] virtio-blk: clamp max_segments when indirect descriptors are disabled Sergii Ushakov
@ 2026-08-17  7:47 ` Christoph Hellwig
  2026-08-17  8:08   ` Sergii Ushakov
  2026-08-17 13:42 ` [PATCH v2] " Sergii Ushakov
  1 sibling, 1 reply; 12+ messages in thread
From: Christoph Hellwig @ 2026-08-17  7:47 UTC (permalink / raw)
  To: Sergii Ushakov
  Cc: virtualization, linux-block, linux-kernel, Michael S . Tsirkin,
	Jason Wang, Jens Axboe, Xuan Zhuo, Eugenio Pérez,
	Paolo Bonzini, Stefan Hajnoczi

On Fri, Aug 14, 2026 at 12:59:54PM +0200, Sergii Ushakov wrote:
> When VIRTIO_RING_F_INDIRECT_DESC is not negotiated by the host, every
> scatter-gather segment in a request must consume a physical slot in
> the virtqueue ring.
> 
> If the host does not advertise VIRTIO_BLK_F_SEG_MAX and provides a small
> virtqueue (e.g. 128 descriptors on QNX Hypervisor), the block layer
> defaults max_segments to BLK_MAX_SEGMENTS (1024). When a multi-page
> compound bio arrives from the page cache, virtqueue_add_split() rejects
> the request with -ENOSPC and triggers:
> 
>   WARNING: at drivers/virtio/virtio_ring.c:1493 virtqueue_add+...
>   WARN_ON_ONCE(total_sg > vq->split.vring.num && !vq->indirect);
> 
> This permanently wedges the blk-mq queue and blocks all subsequent disk
> I/O in uninterruptible sleep (D state).
> 
> Add a virtio_blk.max_segments module parameter to allow runtime cmdline
> overrides, and automatically clamp sg_elems to
> (virtqueue_get_vring_size - 2) when indirect descriptors are disabled.

What is the reason for the override?


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] virtio-blk: clamp max_segments when indirect descriptors are disabled
  2026-08-17  7:47 ` Christoph Hellwig
@ 2026-08-17  8:08   ` Sergii Ushakov
  2026-08-17 12:41     ` Michael S. Tsirkin
  0 siblings, 1 reply; 12+ messages in thread
From: Sergii Ushakov @ 2026-08-17  8:08 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: virtualization, linux-block, linux-kernel, Michael S . Tsirkin,
	Jason Wang, Jens Axboe, Xuan Zhuo, Eugenio Pérez,
	Paolo Bonzini, Stefan Hajnoczi

On Mon, 17 Aug 2026 at 09:47, Christoph Hellwig <hch@infradead.org> wrote:
>
> On Fri, Aug 14, 2026 at 12:59:54PM +0200, Sergii Ushakov wrote:
> > When VIRTIO_RING_F_INDIRECT_DESC is not negotiated by the host, every
> > scatter-gather segment in a request must consume a physical slot in
> > the virtqueue ring.
> >
> > If the host does not advertise VIRTIO_BLK_F_SEG_MAX and provides a small
> > virtqueue (e.g. 128 descriptors on QNX Hypervisor), the block layer
> > defaults max_segments to BLK_MAX_SEGMENTS (1024). When a multi-page
> > compound bio arrives from the page cache, virtqueue_add_split() rejects
> > the request with -ENOSPC and triggers:
> >
> >   WARNING: at drivers/virtio/virtio_ring.c:1493 virtqueue_add+...
> >   WARN_ON_ONCE(total_sg > vq->split.vring.num && !vq->indirect);
> >
> > This permanently wedges the blk-mq queue and blocks all subsequent disk
> > I/O in uninterruptible sleep (D state).
> >
> > Add a virtio_blk.max_segments module parameter to allow runtime cmdline
> > overrides, and automatically clamp sg_elems to
> > (virtqueue_get_vring_size - 2) when indirect descriptors are disabled.
>
> What is the reason for the override?

The module parameter was intended for two main reasons:
1. A safety fallback for non-compliant/buggy hypervisors that may have
   internal segment limits lower than the advertised ring size without
   advertising VIRTIO_BLK_F_SEG_MAX.
2. Debugging and performance benchmarking of smaller scatter-gather lists
   without needing kernel rebuilds.
That said, the automatic clamping to (vring_size - 2) resolves the
hang and panic out-of-the-box. If the preference is to avoid adding a new
module parameter, we may drop it and keep only the automatic
clamping.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] virtio-blk: clamp max_segments when indirect descriptors are disabled
  2026-08-17  8:08   ` Sergii Ushakov
@ 2026-08-17 12:41     ` Michael S. Tsirkin
  0 siblings, 0 replies; 12+ messages in thread
From: Michael S. Tsirkin @ 2026-08-17 12:41 UTC (permalink / raw)
  To: Sergii Ushakov
  Cc: Christoph Hellwig, virtualization, linux-block, linux-kernel,
	Jason Wang, Jens Axboe, Xuan Zhuo, Eugenio Pérez,
	Paolo Bonzini, Stefan Hajnoczi

On Mon, Aug 17, 2026 at 10:08:21AM +0200, Sergii Ushakov wrote:
> On Mon, 17 Aug 2026 at 09:47, Christoph Hellwig <hch@infradead.org> wrote:
> >
> > On Fri, Aug 14, 2026 at 12:59:54PM +0200, Sergii Ushakov wrote:
> > > When VIRTIO_RING_F_INDIRECT_DESC is not negotiated by the host, every
> > > scatter-gather segment in a request must consume a physical slot in
> > > the virtqueue ring.
> > >
> > > If the host does not advertise VIRTIO_BLK_F_SEG_MAX and provides a small
> > > virtqueue (e.g. 128 descriptors on QNX Hypervisor), the block layer
> > > defaults max_segments to BLK_MAX_SEGMENTS (1024). When a multi-page
> > > compound bio arrives from the page cache, virtqueue_add_split() rejects
> > > the request with -ENOSPC and triggers:
> > >
> > >   WARNING: at drivers/virtio/virtio_ring.c:1493 virtqueue_add+...
> > >   WARN_ON_ONCE(total_sg > vq->split.vring.num && !vq->indirect);
> > >
> > > This permanently wedges the blk-mq queue and blocks all subsequent disk
> > > I/O in uninterruptible sleep (D state).
> > >
> > > Add a virtio_blk.max_segments module parameter to allow runtime cmdline
> > > overrides, and automatically clamp sg_elems to
> > > (virtqueue_get_vring_size - 2) when indirect descriptors are disabled.
> >
> > What is the reason for the override?
> 
> The module parameter was intended for two main reasons:
> 1. A safety fallback for non-compliant/buggy hypervisors that may have
>    internal segment limits lower than the advertised ring size without
>    advertising VIRTIO_BLK_F_SEG_MAX.
> 2. Debugging and performance benchmarking of smaller scatter-gather lists
>    without needing kernel rebuilds.
> That said, the automatic clamping to (vring_size - 2) resolves the
> hang and panic out-of-the-box. If the preference is to avoid adding a new
> module parameter, we may drop it and keep only the automatic
> clamping.

sounds better to me.


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v2] virtio-blk: clamp max_segments when indirect descriptors are disabled
  2026-08-14 10:59 [PATCH] virtio-blk: clamp max_segments when indirect descriptors are disabled Sergii Ushakov
  2026-08-17  7:47 ` Christoph Hellwig
@ 2026-08-17 13:42 ` Sergii Ushakov
  2026-08-17 17:28   ` Stefan Hajnoczi
                     ` (3 more replies)
  1 sibling, 4 replies; 12+ messages in thread
From: Sergii Ushakov @ 2026-08-17 13:42 UTC (permalink / raw)
  To: virtualization, linux-block
  Cc: linux-kernel, Christoph Hellwig, Michael S . Tsirkin, Jason Wang,
	Jens Axboe, Xuan Zhuo, Eugenio Pérez, Paolo Bonzini,
	Stefan Hajnoczi, Sergii Ushakov

When VIRTIO_RING_F_INDIRECT_DESC is not negotiated by the host, every
scatter-gather segment in a request must consume a physical slot in
the virtqueue ring.

If the host does not advertise VIRTIO_BLK_F_SEG_MAX and provides a small
virtqueue (e.g. 128 descriptors on QNX Hypervisor), the block layer
defaults max_segments to BLK_MAX_SEGMENTS (1024). When a multi-page
compound bio arrives from the page cache, virtqueue_add_split() rejects
the request with -ENOSPC and triggers:

  WARNING: at drivers/virtio/virtio_ring.c:1493 virtqueue_add+...
  WARN_ON_ONCE(total_sg > vq->split.vring.num && !vq->indirect);

This permanently wedges the blk-mq queue and blocks all subsequent disk
I/O in uninterruptible sleep (D state).

Automatically clamp sg_elems to (ring_size - 2) when indirect
descriptors are disabled.

Signed-off-by: Sergii Ushakov <sergiiushakov@google.com>
---
v1 -> v2:
- Drop max_segments module parameter and rely solely on automatic clamping
  when indirect descriptors are disabled (suggested by Christoph Hellwig).
- Guard (ring_size - 2) calculation with ring_size > 2 to prevent underflow.
- Update commit description accordingly.

 drivers/block/virtio_blk.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 32bf3ba07a9d..8f5a2d5323a6 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -1267,6 +1267,13 @@ static int virtblk_read_limits(struct virtio_blk *vblk,
 	/* Prevent integer overflows and honor max vq size */
 	sg_elems = min_t(u32, sg_elems, VIRTIO_BLK_MAX_SG_ELEMS - 2);
 
+	if (!virtio_has_feature(vdev, VIRTIO_RING_F_INDIRECT_DESC)) {
+		u32 ring_size = virtqueue_get_vring_size(vblk->vqs[0].vq);
+
+		if (ring_size > 2)
+			sg_elems = min(sg_elems, ring_size - 2);
+	}
+
 	/* We can handle whatever the host told us to handle. */
 	lim->max_segments = sg_elems;
 
-- 
2.55.0.691.gc56d675ccc-goog


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] virtio-blk: clamp max_segments when indirect descriptors are disabled
  2026-08-17 13:42 ` [PATCH v2] " Sergii Ushakov
@ 2026-08-17 17:28   ` Stefan Hajnoczi
  2026-08-19  5:52   ` Christoph Hellwig
                     ` (2 subsequent siblings)
  3 siblings, 0 replies; 12+ messages in thread
From: Stefan Hajnoczi @ 2026-08-17 17:28 UTC (permalink / raw)
  To: Sergii Ushakov
  Cc: virtualization, linux-block, linux-kernel, Christoph Hellwig,
	Michael S . Tsirkin, Jason Wang, Jens Axboe, Xuan Zhuo,
	Eugenio Pérez, Paolo Bonzini

[-- Attachment #1: Type: text/plain, Size: 1433 bytes --]

On Mon, Aug 17, 2026 at 03:42:02PM +0200, Sergii Ushakov wrote:
> When VIRTIO_RING_F_INDIRECT_DESC is not negotiated by the host, every
> scatter-gather segment in a request must consume a physical slot in
> the virtqueue ring.
> 
> If the host does not advertise VIRTIO_BLK_F_SEG_MAX and provides a small
> virtqueue (e.g. 128 descriptors on QNX Hypervisor), the block layer
> defaults max_segments to BLK_MAX_SEGMENTS (1024). When a multi-page
> compound bio arrives from the page cache, virtqueue_add_split() rejects
> the request with -ENOSPC and triggers:
> 
>   WARNING: at drivers/virtio/virtio_ring.c:1493 virtqueue_add+...
>   WARN_ON_ONCE(total_sg > vq->split.vring.num && !vq->indirect);
> 
> This permanently wedges the blk-mq queue and blocks all subsequent disk
> I/O in uninterruptible sleep (D state).
> 
> Automatically clamp sg_elems to (ring_size - 2) when indirect
> descriptors are disabled.
> 
> Signed-off-by: Sergii Ushakov <sergiiushakov@google.com>
> ---
> v1 -> v2:
> - Drop max_segments module parameter and rely solely on automatic clamping
>   when indirect descriptors are disabled (suggested by Christoph Hellwig).
> - Guard (ring_size - 2) calculation with ring_size > 2 to prevent underflow.
> - Update commit description accordingly.
> 
>  drivers/block/virtio_blk.c | 7 +++++++
>  1 file changed, 7 insertions(+)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] virtio-blk: clamp max_segments when indirect descriptors are disabled
  2026-08-17 13:42 ` [PATCH v2] " Sergii Ushakov
  2026-08-17 17:28   ` Stefan Hajnoczi
@ 2026-08-19  5:52   ` Christoph Hellwig
  2026-09-16 14:07   ` Sergii Ushakov
  2026-09-16 15:00   ` Michael S. Tsirkin
  3 siblings, 0 replies; 12+ messages in thread
From: Christoph Hellwig @ 2026-08-19  5:52 UTC (permalink / raw)
  To: Sergii Ushakov
  Cc: virtualization, linux-block, linux-kernel, Christoph Hellwig,
	Michael S . Tsirkin, Jason Wang, Jens Axboe, Xuan Zhuo,
	Eugenio Pérez, Paolo Bonzini, Stefan Hajnoczi

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] virtio-blk: clamp max_segments when indirect descriptors are disabled
  2026-08-17 13:42 ` [PATCH v2] " Sergii Ushakov
  2026-08-17 17:28   ` Stefan Hajnoczi
  2026-08-19  5:52   ` Christoph Hellwig
@ 2026-09-16 14:07   ` Sergii Ushakov
  2026-09-16 15:00   ` Michael S. Tsirkin
  3 siblings, 0 replies; 12+ messages in thread
From: Sergii Ushakov @ 2026-09-16 14:07 UTC (permalink / raw)
  To: mst, axboe
  Cc: virtualization, linux-block, linux-kernel, stefanha, hch,
	jasowangio, xuanzhuo, eperezma, pbonzini, Sergii Ushakov,
	Christoph Hellwig

Hi Michael, Jens,

Gentle ping on this v2 patch. It has Reviewed-by tags from Stefan and
Christoph:

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>

Could this be picked up for the next cycle?

Thanks,
Sergii

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] virtio-blk: clamp max_segments when indirect descriptors are disabled
  2026-08-17 13:42 ` [PATCH v2] " Sergii Ushakov
                     ` (2 preceding siblings ...)
  2026-09-16 14:07   ` Sergii Ushakov
@ 2026-09-16 15:00   ` Michael S. Tsirkin
  2026-09-18 11:34     ` [PATCH v3] virtio-blk: clamp max_segments to virtqueue ring size Sergii Ushakov
  3 siblings, 1 reply; 12+ messages in thread
From: Michael S. Tsirkin @ 2026-09-16 15:00 UTC (permalink / raw)
  To: Sergii Ushakov
  Cc: virtualization, linux-block, linux-kernel, Christoph Hellwig,
	Jason Wang, Jens Axboe, Xuan Zhuo, Eugenio Pérez,
	Paolo Bonzini, Stefan Hajnoczi

On Mon, Aug 17, 2026 at 03:42:02PM +0200, Sergii Ushakov wrote:
> When VIRTIO_RING_F_INDIRECT_DESC is not negotiated by the host, every
> scatter-gather segment in a request must consume a physical slot in
> the virtqueue ring.
> 
> If the host does not advertise VIRTIO_BLK_F_SEG_MAX and provides a small
> virtqueue (e.g. 128 descriptors on QNX Hypervisor), the block layer
> defaults max_segments to BLK_MAX_SEGMENTS (1024). When a multi-page
> compound bio arrives from the page cache, virtqueue_add_split() rejects
> the request with -ENOSPC and triggers:
> 
>   WARNING: at drivers/virtio/virtio_ring.c:1493 virtqueue_add+...
>   WARN_ON_ONCE(total_sg > vq->split.vring.num && !vq->indirect);
> 
> This permanently wedges the blk-mq queue and blocks all subsequent disk
> I/O in uninterruptible sleep (D state).
> 
> Automatically clamp sg_elems to (ring_size - 2) when indirect
> descriptors are disabled.
> 
> Signed-off-by: Sergii Ushakov <sergiiushakov@google.com>
> ---
> v1 -> v2:
> - Drop max_segments module parameter and rely solely on automatic clamping
>   when indirect descriptors are disabled (suggested by Christoph Hellwig).
> - Guard (ring_size - 2) calculation with ring_size > 2 to prevent underflow.
> - Update commit description accordingly.
> 
>  drivers/block/virtio_blk.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> index 32bf3ba07a9d..8f5a2d5323a6 100644
> --- a/drivers/block/virtio_blk.c
> +++ b/drivers/block/virtio_blk.c
> @@ -1267,6 +1267,13 @@ static int virtblk_read_limits(struct virtio_blk *vblk,
>  	/* Prevent integer overflows and honor max vq size */
>  	sg_elems = min_t(u32, sg_elems, VIRTIO_BLK_MAX_SG_ELEMS - 2);
>  
> +	if (!virtio_has_feature(vdev, VIRTIO_RING_F_INDIRECT_DESC)) {
> +		u32 ring_size = virtqueue_get_vring_size(vblk->vqs[0].vq);
> +
> +		if (ring_size > 2)
> +			sg_elems = min(sg_elems, ring_size - 2);
> +	}
> +
>  	/* We can handle whatever the host told us to handle. */
>  	lim->max_segments = sg_elems;


I do not get why it does not clamp with VIRTIO_RING_F_INDIRECT_DESC.
Spec says:

 A driver MUST NOT create a descriptor chain longer than the Queue Size of the device.

Also, pls add a comment explaining where does this 2 come from.

What about ring size 2? I guess



> When VIRTIO_RING_F_INDIRECT_DESC is not negotiated by the host, every
> scatter-gather segment in a request must consume a physical slot in
> the virtqueue ring.
> 
> If the host does not advertise VIRTIO_BLK_F_SEG_MAX and provides a small
> virtqueue (e.g. 128 descriptors on QNX Hypervisor), the block layer
> defaults max_segments to BLK_MAX_SEGMENTS

Does it?

        err = virtio_cread_feature(vdev, VIRTIO_BLK_F_SEG_MAX,
                                   struct virtio_blk_config, seg_max,
                                   &sg_elems);
                 
        /* We need at least one SG element, whatever they say. */
        if (err || !sg_elems)
                sg_elems = 1;

so set to 1 without VIRTIO_BLK_F_SEG_MAX


        /* Prevent integer overflows and honor max vq size */
        sg_elems = min_t(u32, sg_elems, VIRTIO_BLK_MAX_SG_ELEMS - 2);

unchanged here

        /* We can handle whatever the host told us to handle. */
        lim->max_segments = sg_elems;

assigned to max_segments here


> BLK_MAX_SEGMENTS (1024).

In which tree does BLK_MAX_SEGMENTS equal 1024?

git show next-20260915:include/linux/blkdev.h | grep -n 'BLK_MAX_SEGMENTS'
1234:      BLK_MAX_SEGMENTS        = 128,




> When a multi-page
> compound bio arrives from the page cache, virtqueue_add_split() rejects
> the request with -ENOSPC and triggers:
> 
>   WARNING: at drivers/virtio/virtio_ring.c:1493 virtqueue_add+...
>   WARN_ON_ONCE(total_sg > vq->split.vring.num && !vq->indirect);
> 
> This permanently wedges the blk-mq queue and blocks all subsequent disk
> I/O in uninterruptible sleep (D state).

Please clarify the reproducer, including the negotiated features,
max_segments, actual ring sizes, and total_sg at the failure. As
described, it should not trigger and I do not see how
the patch is supposed to change the failing configuration.

> 
> Automatically clamp sg_elems to (ring_size - 2) when indirect
> descriptors are disabled.
> 
> Signed-off-by: Sergii Ushakov <sergiiushakov@google.com>
> ---
> v1 -> v2:
> - Drop max_segments module parameter and rely solely on automatic clamping
>   when indirect descriptors are disabled (suggested by Christoph Hellwig).
> - Guard (ring_size - 2) calculation with ring_size > 2 to prevent underflow.
> - Update commit description accordingly.
> 
>  drivers/block/virtio_blk.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> index 32bf3ba07a9d..8f5a2d5323a6 100644
> --- a/drivers/block/virtio_blk.c
> +++ b/drivers/block/virtio_blk.c
> @@ -1267,6 +1267,13 @@ static int virtblk_read_limits(struct virtio_blk *vblk,

Well we only set the limit at probe. virtblk_restore_priv() recreates
the queues on resume and reset recovery, then resumes dispatch without
checking their sizes against the existing limit. ring allocation
can reduce the size under memory pressure, including during recovery.

Since you are now (correctly) tying request size to vq size,
this needs to be resolved.



>  	/* Prevent integer overflows and honor max vq size */
>  	sg_elems = min_t(u32, sg_elems, VIRTIO_BLK_MAX_SG_ELEMS - 2);
>  
> +	if (!virtio_has_feature(vdev, VIRTIO_RING_F_INDIRECT_DESC)) {

Why exempt indirect descriptors? The virtio spec says:

	A driver MUST NOT create a descriptor chain longer than the Queue Size
	of the device.

Even on devices ignoring that -
there is a failure path even when indirect descriptors are
negotiated: virtqueue_add_split() falls back to direct descriptors if
the indirect-table allocation fails. So if the request exceeds the ring
size, it returns -ENOSPC even on an empty ring. virtio_queue_rq() then
stops the hardware queue, with no outstanding completion to restart it.


> +		u32 ring_size = virtqueue_get_vring_size(vblk->vqs[0].vq);

Why 0? VQ 0 is not necessarily the smallest queue. The transport specifies
sizes per queue, and Linux can reduce individual split-ring sizes
during allocation.

> +
> +		if (ring_size > 2)
> +			sg_elems = min(sg_elems, ring_size - 2);

The subtraction is correct: virtblk_add_req() uses separate outgoing
and incoming header descriptors, including for zone append.
However, we really should have a comment explaining that, here.

And, ring_size <= 2 must be rejected rather than bypassing the clamp:
A two-entry direct ring can pass probe, yet even one data segment
needs three descriptors and hits the same permanent -ENOSPC condition.

> +	}
> +
>  	/* We can handle whatever the host told us to handle. */
>  	lim->max_segments = sg_elems;
>  
> -- 
> 2.55.0.691.gc56d675ccc-goog
> 

>  
> -- 
> 2.55.0.691.gc56d675ccc-goog


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v3] virtio-blk: clamp max_segments to virtqueue ring size
  2026-09-16 15:00   ` Michael S. Tsirkin
@ 2026-09-18 11:34     ` Sergii Ushakov
  2026-09-18 12:39       ` Michael S. Tsirkin
  0 siblings, 1 reply; 12+ messages in thread
From: Sergii Ushakov @ 2026-09-18 11:34 UTC (permalink / raw)
  To: mst, axboe
  Cc: virtualization, linux-block, linux-kernel, stefanha, hch,
	jasowangio, xuanzhuo, eperezma, pbonzini, Sergii Ushakov

In virtblk_add_req(), each request consumes 2 extra descriptors (out_hdr
and in_hdr) in addition to the data scatter-gather segments.

When a hypervisor (e.g. QNX Hypervisor) advertises VIRTIO_BLK_F_SEG_MAX
with seg_max = 1024 alongside a 1024-entry split ring (vring.num = 1024)
and VIRTIO_RING_F_INDIRECT_DESC disabled, lim->max_segments is set to
1024. When the block layer submits requests with 1023 or 1024 data
segments, total_sg reaches 1025 or 1026. This exceeds vring.num (1024),
causing virtqueue_add_split() to return -ENOSPC and permanently wedge
the blk-mq queue.

Furthermore, the Virtio specification (2.7.5.3.1) requires that a
descriptor chain never exceed the Queue Size, and virtqueue_add_split()
falls back to direct descriptors if indirect table allocation fails.

Fix this by:
1. Rejecting queues with ring_size < 3 at probe, or
   ring_size < (queue_max_segments + 2) during resume/reset recovery in
   init_vq().
2. Unconditionally clamping sg_elems to (ring_size - 2) across all
   virtqueues in virtblk_read_limits().

Signed-off-by: Sergii Ushakov <sergiiushakov@google.com>
---
Hi Michael,

Thank you for the detailed review!

I logged the values in virtblk_read_limits() on the QNX Hypervisor guest:
  virtio_blk virtio0: 1/0/0 default/read/poll queues
  virtio_blk virtio0: F_SEG_MAX=1 seg_max=1024 F_INDIRECT=0 vring_size=1024 max_segments=1024

  dev_info(&vdev->dev,
           "virtio_blk debug: F_SEG_MAX=%d seg_max=%u F_INDIRECT=%d vring_size=%u max_segments=%u\n",
           virtio_has_feature(vdev, VIRTIO_BLK_F_SEG_MAX),
           sg_elems,
           virtio_has_feature(vdev, VIRTIO_RING_F_INDIRECT_DESC),
           virtqueue_get_vring_size(vblk->vqs[0].vq),
           lim->max_segments);

So what actually happens:
1. The host *does* advertise VIRTIO_BLK_F_SEG_MAX=1 with seg_max=1024 (setting
   seg_max equal to the virtqueue size vring_size=1024), and does *not*
   offer VIRTIO_RING_F_INDIRECT_DESC (F_INDIRECT=0).
2. Because seg_max (1024) <= VIRTIO_BLK_MAX_SG_ELEMS - 2 (32766),
   virtblk_read_limits() sets lim->max_segments = 1024.
3. The block layer submits requests with 1023 or 1024 data segments.
   virtblk_add_req() adds 2 extra descriptors (out_hdr and in_hdr),
   resulting in total_sg > 1024.
4. Since total_sg > vq->split.vring.num and !vq->indirect,
   virtqueue_add_split() triggers:
     WARN_ON_ONCE(total_sg > vq->split.vring.num && !vq->indirect);

In v3, I drop the !virtio_has_feature(vdev, VIRTIO_RING_F_INDIRECT_DESC)
condition and clamp sg_elems to (ring_size - 2) unconditionally across all
virtqueues.

Also, init_vq() checks all virtqueues:
- At initial probe (!vblk->disk), it enforces ring_size >= 3.
- On resume/recovery, it enforces
  ring_size >= queue_max_segments(vblk->disk->queue) + 2.

Logs after the v3 patch:
  virtio_blk virtio0: F_SEG_MAX=1 seg_max=1022 F_INDIRECT=0 vring_size=1024 max_segments=1022

 drivers/block/virtio_blk.c | 35 ++++++++++++++++++++++++++++++++++-
 1 file changed, 34 insertions(+), 1 deletion(-)

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 32bf3ba07a9d..c263d0e3db95 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -1021,6 +1021,21 @@ static int init_vq(struct virtio_blk *vblk)
 		goto out;
 
 	for (i = 0; i < num_vqs; i++) {
+		unsigned int ring_size = virtqueue_get_vring_size(vqs[i]);
+		unsigned int min_ring_size = 3;
+
+		if (vblk->disk)
+			min_ring_size = queue_max_segments(vblk->disk->queue) + 2;
+
+		if (ring_size < min_ring_size) {
+			dev_err(&vdev->dev,
+				"virtqueue %u ring size %u is smaller than minimum %u\n",
+				i, ring_size, min_ring_size);
+			vdev->config->del_vqs(vdev);
+			err = -EINVAL;
+			goto out;
+		}
+
 		spin_lock_init(&vblk->vqs[i].lock);
 		vblk->vqs[i].vq = vqs[i];
 	}
@@ -1253,7 +1268,7 @@ static int virtblk_read_limits(struct virtio_blk *vblk,
 	u16 min_io_size;
 	u8 physical_block_exp, alignment_offset;
 	size_t max_dma_size;
-	int err;
+	int err, i;
 
 	/* We need to know how many segments before we allocate. */
 	err = virtio_cread_feature(vdev, VIRTIO_BLK_F_SEG_MAX,
@@ -1267,6 +1282,23 @@ static int virtblk_read_limits(struct virtio_blk *vblk,
 	/* Prevent integer overflows and honor max vq size */
 	sg_elems = min_t(u32, sg_elems, VIRTIO_BLK_MAX_SG_ELEMS - 2);
 
+	/*
+	 * virtblk_add_req() uses separate outgoing and incoming header
+	 * descriptors (out_hdr and in_hdr), consuming 2 extra descriptors
+	 * per request in addition to the data segments.
+	 *
+	 * Per virtio specification (2.7.5.3.1), a driver MUST NOT create a
+	 * descriptor chain longer than the Queue Size of the device.
+	 *
+	 * Clamp max_segments to (ring_size - 2) across all virtqueues so
+	 * that a request never exceeds the ring size of any queue.
+	 */
+	for (i = 0; i < vblk->num_vqs; i++) {
+		u32 ring_size = virtqueue_get_vring_size(vblk->vqs[i].vq);
+
+		sg_elems = min_t(u32, sg_elems, ring_size - 2);
+	}
+
 	/* We can handle whatever the host told us to handle. */
 	lim->max_segments = sg_elems;
 
@@ -1466,6 +1498,7 @@ static int virtblk_probe(struct virtio_device *vdev)
 	mutex_init(&vblk->vdev_mutex);
 
 	vblk->vdev = vdev;
+	vblk->disk = NULL;
 
 	INIT_WORK(&vblk->config_work, virtblk_config_changed_work);
 
-- 
2.55.0.1082.g2b9226bbc0-goog


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v3] virtio-blk: clamp max_segments to virtqueue ring size
  2026-09-18 11:34     ` [PATCH v3] virtio-blk: clamp max_segments to virtqueue ring size Sergii Ushakov
@ 2026-09-18 12:39       ` Michael S. Tsirkin
  2026-09-18 13:51         ` [PATCH v4] " Sergii Ushakov
  0 siblings, 1 reply; 12+ messages in thread
From: Michael S. Tsirkin @ 2026-09-18 12:39 UTC (permalink / raw)
  To: Sergii Ushakov
  Cc: axboe, virtualization, linux-block, linux-kernel, stefanha, hch,
	jasowangio, xuanzhuo, eperezma, pbonzini

Love the patch! yet something to improve:

On Fri, Sep 18, 2026 at 01:34:12PM +0200, Sergii Ushakov wrote:
> In virtblk_add_req(), each request consumes 2 extra descriptors (out_hdr
> and in_hdr) in addition to the data scatter-gather segments.
> 
> When a hypervisor (e.g. QNX Hypervisor) advertises VIRTIO_BLK_F_SEG_MAX
> with seg_max = 1024 alongside a 1024-entry split ring (vring.num = 1024)
> and VIRTIO_RING_F_INDIRECT_DESC disabled, lim->max_segments is set to
> 1024. When the block layer submits requests with 1023 or 1024 data
> segments, total_sg reaches 1025 or 1026. This exceeds vring.num (1024),
> causing virtqueue_add_split() to return -ENOSPC and permanently wedge
> the blk-mq queue.
> 
> Furthermore, the Virtio specification (2.7.5.3.1) requires that a
> descriptor chain never exceed the Queue Size, and virtqueue_add_split()
> falls back to direct descriptors if indirect table allocation fails.
> 
> Fix this by:
> 1. Rejecting queues with ring_size < 3 at probe, or
>    ring_size < (queue_max_segments + 2) during resume/reset recovery in
>    init_vq().
> 2. Unconditionally clamping sg_elems to (ring_size - 2) across all
>    virtqueues in virtblk_read_limits().
> 


Fixes tag please?

> Signed-off-by: Sergii Ushakov <sergiiushakov@google.com>
> ---
> Hi Michael,
> 
> Thank you for the detailed review!
> 
> I logged the values in virtblk_read_limits() on the QNX Hypervisor guest:
>   virtio_blk virtio0: 1/0/0 default/read/poll queues
>   virtio_blk virtio0: F_SEG_MAX=1 seg_max=1024 F_INDIRECT=0 vring_size=1024 max_segments=1024
> 
>   dev_info(&vdev->dev,
>            "virtio_blk debug: F_SEG_MAX=%d seg_max=%u F_INDIRECT=%d vring_size=%u max_segments=%u\n",
>            virtio_has_feature(vdev, VIRTIO_BLK_F_SEG_MAX),
>            sg_elems,
>            virtio_has_feature(vdev, VIRTIO_RING_F_INDIRECT_DESC),
>            virtqueue_get_vring_size(vblk->vqs[0].vq),
>            lim->max_segments);
> 
> So what actually happens:
> 1. The host *does* advertise VIRTIO_BLK_F_SEG_MAX=1 with seg_max=1024 (setting
>    seg_max equal to the virtqueue size vring_size=1024), and does *not*
>    offer VIRTIO_RING_F_INDIRECT_DESC (F_INDIRECT=0).
> 2. Because seg_max (1024) <= VIRTIO_BLK_MAX_SG_ELEMS - 2 (32766),
>    virtblk_read_limits() sets lim->max_segments = 1024.
> 3. The block layer submits requests with 1023 or 1024 data segments.
>    virtblk_add_req() adds 2 extra descriptors (out_hdr and in_hdr),
>    resulting in total_sg > 1024.
> 4. Since total_sg > vq->split.vring.num and !vq->indirect,
>    virtqueue_add_split() triggers:
>      WARN_ON_ONCE(total_sg > vq->split.vring.num && !vq->indirect);
> 
> In v3, I drop the !virtio_has_feature(vdev, VIRTIO_RING_F_INDIRECT_DESC)
> condition and clamp sg_elems to (ring_size - 2) unconditionally across all
> virtqueues.
> 
> Also, init_vq() checks all virtqueues:
> - At initial probe (!vblk->disk), it enforces ring_size >= 3.
> - On resume/recovery, it enforces
>   ring_size >= queue_max_segments(vblk->disk->queue) + 2.
> 
> Logs after the v3 patch:
>   virtio_blk virtio0: F_SEG_MAX=1 seg_max=1022 F_INDIRECT=0 vring_size=1024 max_segments=1022
> 
>  drivers/block/virtio_blk.c | 35 ++++++++++++++++++++++++++++++++++-
>  1 file changed, 34 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> index 32bf3ba07a9d..c263d0e3db95 100644
> --- a/drivers/block/virtio_blk.c
> +++ b/drivers/block/virtio_blk.c
> @@ -1021,6 +1021,21 @@ static int init_vq(struct virtio_blk *vblk)
>  		goto out;
>  
>  	for (i = 0; i < num_vqs; i++) {
> +		unsigned int ring_size = virtqueue_get_vring_size(vqs[i]);
> +		unsigned int min_ring_size = 3;
> +
> +		if (vblk->disk)
> +			min_ring_size = queue_max_segments(vblk->disk->queue) + 2;

could you pls add code comments documenting the constants here?


> +
> +		if (ring_size < min_ring_size) {
> +			dev_err(&vdev->dev,
> +				"virtqueue %u ring size %u is smaller than minimum %u\n",
> +				i, ring_size, min_ring_size);
> +			vdev->config->del_vqs(vdev);
> +			err = -EINVAL;
> +			goto out;
> +		}
> +
>  		spin_lock_init(&vblk->vqs[i].lock);
>  		vblk->vqs[i].vq = vqs[i];
>  	}
> @@ -1253,7 +1268,7 @@ static int virtblk_read_limits(struct virtio_blk *vblk,
>  	u16 min_io_size;
>  	u8 physical_block_exp, alignment_offset;
>  	size_t max_dma_size;
> -	int err;
> +	int err, i;
>  
>  	/* We need to know how many segments before we allocate. */
>  	err = virtio_cread_feature(vdev, VIRTIO_BLK_F_SEG_MAX,
> @@ -1267,6 +1282,23 @@ static int virtblk_read_limits(struct virtio_blk *vblk,
>  	/* Prevent integer overflows and honor max vq size */
>  	sg_elems = min_t(u32, sg_elems, VIRTIO_BLK_MAX_SG_ELEMS - 2);
>  
> +	/*
> +	 * virtblk_add_req() uses separate outgoing and incoming header
> +	 * descriptors (out_hdr and in_hdr), consuming 2 extra descriptors
> +	 * per request in addition to the data segments.
> +	 *
> +	 * Per virtio specification (2.7.5.3.1), a driver MUST NOT create a
> +	 * descriptor chain longer than the Queue Size of the device.
> +	 *
> +	 * Clamp max_segments to (ring_size - 2) across all virtqueues so
> +	 * that a request never exceeds the ring size of any queue.
> +	 */
> +	for (i = 0; i < vblk->num_vqs; i++) {
> +		u32 ring_size = virtqueue_get_vring_size(vblk->vqs[i].vq);
> +
> +		sg_elems = min_t(u32, sg_elems, ring_size - 2);
> +	}
> +
>  	/* We can handle whatever the host told us to handle. */
>  	lim->max_segments = sg_elems;
>  
> @@ -1466,6 +1498,7 @@ static int virtblk_probe(struct virtio_device *vdev)
>  	mutex_init(&vblk->vdev_mutex);
>  
>  	vblk->vdev = vdev;
> +	vblk->disk = NULL;
>  
>  	INIT_WORK(&vblk->config_work, virtblk_config_changed_work);
>  
> -- 
> 2.55.0.1082.g2b9226bbc0-goog


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v4] virtio-blk: clamp max_segments to virtqueue ring size
  2026-09-18 12:39       ` Michael S. Tsirkin
@ 2026-09-18 13:51         ` Sergii Ushakov
  0 siblings, 0 replies; 12+ messages in thread
From: Sergii Ushakov @ 2026-09-18 13:51 UTC (permalink / raw)
  To: mst, axboe, stefanha, hch
  Cc: virtualization, linux-block, linux-kernel, jasowang, xuanzhuo,
	eperezma, pbonzini, sergiiushakov

In virtblk_add_req(), each request consumes 2 extra descriptors (out_hdr
and in_hdr) in addition to the data scatter-gather segments.

When a hypervisor (e.g. QNX Hypervisor) advertises VIRTIO_BLK_F_SEG_MAX
with seg_max = 1024 alongside a 1024-entry split ring (vring.num = 1024)
and VIRTIO_RING_F_INDIRECT_DESC disabled, lim->max_segments is set to
1024. When the block layer submits requests with 1023 or 1024 data
segments, total_sg reaches 1025 or 1026. This exceeds vring.num (1024),
causing virtqueue_add_split() to return -ENOSPC and permanently wedge
the blk-mq queue.

Furthermore, the Virtio specification (2.7.5.3.1) requires that a
descriptor chain never exceed the Queue Size, and virtqueue_add_split()
falls back to direct descriptors if indirect table allocation fails.

Fix this by:
1. Rejecting queues with ring_size < 3 at probe, or
   ring_size < (queue_max_segments + 2) during resume/reset recovery in
   init_vq().
2. Unconditionally clamping sg_elems to (ring_size - 2) across all
   virtqueues in virtblk_read_limits().

Fixes: 0864b79a1533 ("virtio: block: dynamic maximum segments")
Signed-off-by: Sergii Ushakov <sergiiushakov@google.com>
---
Changes in v4:
- Add Fixes tag.
- Add code comments in init_vq() documenting the constants 3 and + 2.

 drivers/block/virtio_blk.c | 44 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 43 insertions(+), 1 deletion(-)

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 32bf3ba07a9d..cafca309b4d0 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -1021,6 +1021,30 @@ static int init_vq(struct virtio_blk *vblk)
 		goto out;
 
 	for (i = 0; i < num_vqs; i++) {
+		unsigned int ring_size = virtqueue_get_vring_size(vqs[i]);
+		/*
+		 * Each request consumes 2 extra descriptors (out_hdr and
+		 * in_hdr) in addition to the data segments.
+		 *
+		 * At initial probe (!vblk->disk), each virtqueue must fit at
+		 * least 1 data segment + 2 header descriptors (3).
+		 * On resume/recovery (vblk->disk), each virtqueue must fit the
+		 * existing queue_max_segments() + 2 header descriptors.
+		 */
+		unsigned int min_ring_size = 3;
+
+		if (vblk->disk)
+			min_ring_size = queue_max_segments(vblk->disk->queue) + 2;
+
+		if (ring_size < min_ring_size) {
+			dev_err(&vdev->dev,
+				"virtqueue %u ring size %u is smaller than minimum %u\n",
+				i, ring_size, min_ring_size);
+			vdev->config->del_vqs(vdev);
+			err = -EINVAL;
+			goto out;
+		}
+
 		spin_lock_init(&vblk->vqs[i].lock);
 		vblk->vqs[i].vq = vqs[i];
 	}
@@ -1253,7 +1277,7 @@ static int virtblk_read_limits(struct virtio_blk *vblk,
 	u16 min_io_size;
 	u8 physical_block_exp, alignment_offset;
 	size_t max_dma_size;
-	int err;
+	int err, i;
 
 	/* We need to know how many segments before we allocate. */
 	err = virtio_cread_feature(vdev, VIRTIO_BLK_F_SEG_MAX,
@@ -1267,6 +1291,23 @@ static int virtblk_read_limits(struct virtio_blk *vblk,
 	/* Prevent integer overflows and honor max vq size */
 	sg_elems = min_t(u32, sg_elems, VIRTIO_BLK_MAX_SG_ELEMS - 2);
 
+	/*
+	 * virtblk_add_req() uses separate outgoing and incoming header
+	 * descriptors (out_hdr and in_hdr), consuming 2 extra descriptors
+	 * per request in addition to the data segments.
+	 *
+	 * Per virtio specification (2.7.5.3.1), a driver MUST NOT create a
+	 * descriptor chain longer than the Queue Size of the device.
+	 *
+	 * Clamp max_segments to (ring_size - 2) across all virtqueues so
+	 * that a request never exceeds the ring size of any queue.
+	 */
+	for (i = 0; i < vblk->num_vqs; i++) {
+		u32 ring_size = virtqueue_get_vring_size(vblk->vqs[i].vq);
+
+		sg_elems = min_t(u32, sg_elems, ring_size - 2);
+	}
+
 	/* We can handle whatever the host told us to handle. */
 	lim->max_segments = sg_elems;
 
@@ -1466,6 +1507,7 @@ static int virtblk_probe(struct virtio_device *vdev)
 	mutex_init(&vblk->vdev_mutex);
 
 	vblk->vdev = vdev;
+	vblk->disk = NULL;
 
 	INIT_WORK(&vblk->config_work, virtblk_config_changed_work);
 
-- 
2.55.0.1082.g2b9226bbc0-goog


^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2026-09-18 13:51 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-08-14 10:59 [PATCH] virtio-blk: clamp max_segments when indirect descriptors are disabled Sergii Ushakov
2026-08-17  7:47 ` Christoph Hellwig
2026-08-17  8:08   ` Sergii Ushakov
2026-08-17 12:41     ` Michael S. Tsirkin
2026-08-17 13:42 ` [PATCH v2] " Sergii Ushakov
2026-08-17 17:28   ` Stefan Hajnoczi
2026-08-19  5:52   ` Christoph Hellwig
2026-09-16 14:07   ` Sergii Ushakov
2026-09-16 15:00   ` Michael S. Tsirkin
2026-09-18 11:34     ` [PATCH v3] virtio-blk: clamp max_segments to virtqueue ring size Sergii Ushakov
2026-09-18 12:39       ` Michael S. Tsirkin
2026-09-18 13:51         ` [PATCH v4] " Sergii Ushakov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®