On Wed, Mar 02, 2022 at 09:53:17PM +0800, Yongji Xie wrote:

On Wed, Mar 2, 2022 at 9:33 PM Michael S. Tsirkin [off-list ref] wrote:

quoted

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.
I would expect discard to follow max_segments restrictions then.

I think one point is whether we want to allow the corner case that the
device reports a larger value for max_discard_segments than
max_segments. For example, queue size is 256, max_segments is 128 - 2,
max_discard_segments is 256 - 2.

Thanks,
Yongji

So if device specifies that, then I guess it's fine and from that POV
the patch is correct.  But I think the issue is when device specifies 0
which we interpret as 256 with no basis in hardware.

-- 
MST