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
