Re: [PATCH v2] virtio-blk: clamp max_segments when indirect descriptors are disabled

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

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