On Thu, Mar 03, 2022 at 11:31:35AM +0800, Yongji Xie wrote: > On Wed, Mar 2, 2022 at 11:05 PM Max Gurtovoy <mgurtovoy@xxxxxxxxxx> wrote: > > > > > > On 3/2/2022 3:15 PM, Michael S. Tsirkin wrote: > > > On Wed, Mar 02, 2022 at 06:46:03PM +0800, Yongji Xie wrote: > > >> On Tue, Mar 1, 2022 at 11:43 PM Michael S. Tsirkin <mst@xxxxxxxxxx> 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? > > >>> > > >> One example: the device doesn't specify the value of max_discard_seg > > >> in the config space, then the virtio-blk driver will use > > >> MAX_DISCARD_SEGMENTS (256) by default. Then we're able to trigger the > > >> BUG_ON() if the seg_max is less than 256. > > >> > > >> While the spec didn't say what should happen if max_discard_seg > > >> exceeds seg_max, it also doesn't explicitly prohibit this > > >> configuration. So I think we should at least not panic the kernel in > > >> this case. > > >> > > >> Thanks, > > >> Yongji > > > Oh that last one sounds like a bug, I think it should be > > > min(MAX_DISCARD_SEGMENTS, seg_max) > > > > > > When max_discard_seg and seg_max both exist, that's a different question. We can > > > - do min(max_discard_seg, seg_max) > > > - fail probe > > > - clear the relevant feature flag > > > > > > I feel we need a better plan than submitting an invalid request to device. > > > > We should cover only for a buggy devices. > > > > The situation that max_discard_seg > seg_max should be fine. > > > > Thus the bellow can be added to this patch: > > > > diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c > > index c443cd64fc9b..3e372b97fe10 100644 > > --- a/drivers/block/virtio_blk.c > > +++ b/drivers/block/virtio_blk.c > > @@ -926,8 +926,8 @@ static int virtblk_probe(struct virtio_device *vdev) > > virtio_cread(vdev, struct virtio_blk_config, > > max_discard_seg, > > &v); > > blk_queue_max_discard_segments(q, > > - min_not_zero(v, > > - MAX_DISCARD_SEGMENTS)); > > + min_t(u32, (v ? v : > > sg_elems), > > + MAX_DISCARD_SEGMENTS)); > > > > blk_queue_flag_set(QUEUE_FLAG_DISCARD, q); > > } > > > > > > LGTM, I can add this in v3. > > Thanks, > Yongji Except the logic is convoluted then. I would instead add /* max_seg == 0 is out of spec but we always handled it */ if (!v) v = sg_elems; -- MST
