Re: [PATCH v2] virtio-blk: Remove BUG_ON() in virtio_queue_rq()

[Yongji Xie <xieyongji@bytedance.com>]

On Wed, Mar 2, 2022 at 11:05 PM Max Gurtovoy <mgurtovoy@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@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