Virtualization - Mar 2022 - [PATCH v2] virtio-blk: Remove BUG_ON() in virtio_queue_rq()

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 at
nvidia.com> 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 at
redhat.com> 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