On 3/2/2022 3:33 PM, Michael S. Tsirkin wrote:

On Wed, Mar 02, 2022 at 03:24:51PM +0200, Max Gurtovoy wrote:

quoted

On 3/2/2022 3:17 PM, Michael S. Tsirkin wrote:

quoted

On Wed, Mar 02, 2022 at 11:51:27AM +0200, Max Gurtovoy wrote:

quoted

On 3/1/2022 5:43 PM, Michael S. Tsirkin wrote:

quoted

On Mon, Feb 28, 2022 at 02:57:20PM +0800, Xie Yongji wrote:

quoted

Currently we have a BUG_ON() to make sure the number of sg
list does not exceed queue_max_segments() in virtio_queue_rq().
However, the block layer uses queue_max_discard_segments()
instead of queue_max_segments() to limit the sg list for
discard requests. So the BUG_ON() might be triggered if
virtio-blk device reports a larger value for max discard
segment than queue_max_segments().

Hmm the spec does not say what should happen if max_discard_seg
exceeds seg_max. Is this the config you have in mind? how do you
create it?

I don't think it's hard to create it. Just change some registers in the
device.

But with the dynamic sgl allocation that I added recently, there is no
problem with this scenario.

Well the problem is device says it can't handle such large descriptors,
I guess it works anyway, but it seems scary.

I don't follow.

The only problem this patch solves is when a virtio blk device reports
larger value for max_discard_segments than max_segments.

No, the peroblem reported is when virtio blk device reports
max_segments < 256 but not max_discard_segments.

You mean the code will work in case device report max_discard_segments  
 > max_segments ?

I don't think so.

This is exactly what Xie Yongji mention in the commit message and what I 
was seeing.

But the code will work if VIRTIO_BLK_F_DISCARD is not supported by the 
device (even if max_segments < 256) , since blk layer set 
queue_max_discard_segments = 1 in the initialization.

And the virtio-blk driver won't change it unless VIRTIO_BLK_F_DISCARD is 
supported.

I would expect discard to follow max_segments restrictions then.

quoted

Probably no such devices, but we need to be prepared.

Right, question is how to handle this.

quoted

quoted

quoted

This commit looks good to me, thanks Xie Yongji.

Reviewed-by: Max Gurtovoy <mgurtovoy@nvidia.com>

quoted

quoted

To fix it, let's simply
remove the BUG_ON() which has become unnecessary after commit
02746e26c39e("virtio-blk: avoid preallocating big SGL for data").
And the unused vblk->sg_elems can also be removed together.

Fixes: 1f23816b8eb8 ("virtio_blk: add discard and write zeroes support")
Suggested-by: Christoph Hellwig <hch@infradead.org>
Signed-off-by: Xie Yongji <redacted>
---
    drivers/block/virtio_blk.c | 10 +---------
    1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index c443cd64fc9b..a43eb1813cec 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c

@@ -76,9 +76,6 @@ struct virtio_blk {
    	 */
    	refcount_t refs;
-	/* What host tells us, plus 2 for header & tailer. */
-	unsigned int sg_elems;
-
    	/* Ida index - used to track minor number allocations. */
    	int index;

@@ -322,8 +319,6 @@ static blk_status_t virtio_queue_rq(struct blk_mq_hw_ctx *hctx,
    	blk_status_t status;
    	int err;
-	BUG_ON(req->nr_phys_segments + 2 > vblk->sg_elems);
-
    	status = virtblk_setup_cmd(vblk->vdev, req, vbr);
    	if (unlikely(status))
    		return status;

@@ -783,8 +778,6 @@ static int virtblk_probe(struct virtio_device *vdev)
    	/* Prevent integer overflows and honor max vq size */
    	sg_elems = min_t(u32, sg_elems, VIRTIO_BLK_MAX_SG_ELEMS - 2);
-	/* We need extra sg elements at head and tail. */
-	sg_elems += 2;
    	vdev->priv = vblk = kmalloc(sizeof(*vblk), GFP_KERNEL);
    	if (!vblk) {
    		err = -ENOMEM;

@@ -796,7 +789,6 @@ static int virtblk_probe(struct virtio_device *vdev)
    	mutex_init(&vblk->vdev_mutex);
    	vblk->vdev = vdev;
-	vblk->sg_elems = sg_elems;
    	INIT_WORK(&vblk->config_work, virtblk_config_changed_work);

@@ -853,7 +845,7 @@ static int virtblk_probe(struct virtio_device *vdev)
    		set_disk_ro(vblk->disk, 1);
    	/* We can handle whatever the host told us to handle. */
-	blk_queue_max_segments(q, vblk->sg_elems-2);
+	blk_queue_max_segments(q, sg_elems);
    	/* No real sector limit. */
    	blk_queue_max_hw_sectors(q, -1U);

-- 
2.20.1