On Thu, Mar 3, 2022 at 3:22 PM Michael S. Tsirkin <mst@xxxxxxxxxx> wrote: > > 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; Got it. Thanks, Yongji
