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 <mst@xxxxxxxxxx> wrote: > > > > On Wed, Mar 02, 2022 at 03:24:51PM +0200, Max Gurtovoy wrote: > > > > > > On 3/2/2022 3:17 PM, Michael S. Tsirkin wrote: > > > > On Wed, Mar 02, 2022 at 11:51:27AM +0200, Max Gurtovoy wrote: > > > > > On 3/1/2022 5:43 PM, Michael S. Tsirkin wrote: > > > > > > On Mon, Feb 28, 2022 at 02:57:20PM +0800, Xie Yongji wrote: > > > > > > > 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
