On Wed, Mar 02, 2022 at 04:27:15PM +0200, Max Gurtovoy wrote:
>
> On 3/2/2022 4:15 PM, Michael S. Tsirkin wrote:
> > On Wed, Mar 02, 2022 at 03:45:10PM +0200, Max Gurtovoy wrote:
> > > On 3/2/2022 3:33 PM, Michael S. Tsirkin wrote:
> > > > On Wed, Mar 02, 2022 at 03:24:51PM +0200, Max Gurtovoy wrote:
> > > > > On 3/2/2022 3:17 PM, Michael S. Tsirkin wrote:
> > > > > > On Wed, Mar 02, 2022 at 11:51:27AM +0200, Max Gurtovoy wrote:
> > > > > > > On 3/1/2022 5:43 PM, Michael S. Tsirkin 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?
> > > > > > > I don't think it's hard to create it. Just change some registers
> > > > > > > in the
> > > > > > > device.
> > > > > > >
> > > > > > > But with the dynamic sgl allocation that I added recently, there
> > > > > > > is no
> > > > > > > problem with this scenario.
> > > > > > Well the problem is device says it can't handle such large
> > > > > > descriptors,
> > > > > > I guess it works anyway, but it seems scary.
> > > > > I don't follow.
> > > > >
> > > > > The only problem this patch solves is when a virtio blk device reports
> > > > > larger value for max_discard_segments than max_segments.
> > > > >
> > > > No, the peroblem reported is when virtio blk device reports
> > > > max_segments < 256 but not max_discard_segments.
> > > You mean the code will work in case device report max_discard_segments >
> > > max_segments ?
> > >
> > > I don't think so.
> > I think it's like this:
> >
> >
> > if (virtio_has_feature(vdev, VIRTIO_BLK_F_DISCARD)) {
> >
> > ....
> >
> > virtio_cread(vdev, struct virtio_blk_config,
> > max_discard_seg,
> > &v);
> > blk_queue_max_discard_segments(q,
> > min_not_zero(v,
> >
> > MAX_DISCARD_SEGMENTS));
> >
> > }
> >
> > so, IIUC the case is of a device that sets max_discard_seg to 0.
> >
> > Which is kind of broken, but we handled this since 2018 so I guess
> > we'll need to keep doing that.
>
> A device can't state VIRTIO_BLK_F_DISCARD and set max_discard_seg to 0.
>
> If so, it's a broken device and we can add a quirk for it.
Well we already have min_not_zero there, presumably for some reason.
> Do you have such device to test ?
Xie Yongji mentioned he does.
> >
> > > This is exactly what Xie Yongji mention in the commit message and what I
> > > was
> > > seeing.
> > >
> > > But the code will work if VIRTIO_BLK_F_DISCARD is not supported by the
> > > device (even if max_segments < 256) , since blk layer set
> > > queue_max_discard_segments = 1 in the initialization.
> > >
> > > And the virtio-blk driver won't change it unless VIRTIO_BLK_F_DISCARD is
> > > supported.
> > >
> > > > I would expect discard to follow max_segments restrictions then.
> > > >
> > > > > Probably no such devices, but we need to be prepared.
> > > > Right, question is how to handle this.
> > > >
> > > > > > > This commit looks good to me, thanks Xie Yongji.
> > > > > > >
> > > > > > > Reviewed-by: Max Gurtovoy <mgurto...@nvidia.com>
> > > > > > >
> > > > > > > > > To fix it, let's simply
> > > > > > > > > remove the BUG_ON() which has become unnecessary after commit
> > > > > > > > > 02746e26c39e("virtio-blk: avoid preallocating big SGL for
> > > > > > > > > data").
> > > > > > > > > And the unused vblk->sg_elems can also be removed together.
> > > > > > > > >
> > > > > > > > > Fixes: 1f23816b8eb8 ("virtio_blk: add discard and write
> > > > > > > > > zeroes support")
> > > > > > > > > Suggested-by: Christoph Hellwig <h...@infradead.org>
> > > > > > > > > Signed-off-by: Xie Yongji <xieyon...@bytedance.com>
> > > > > > > > > ---
> > > > > > > > > drivers/block/virtio_blk.c | 10 +---------
> > > > > > > > > 1 file changed, 1 insertion(+), 9 deletions(-)
> > > > > > > > >
> > > > > > > > > diff --git a/drivers/block/virtio_blk.c
> > > > > > > > > b/drivers/block/virtio_blk.c
> > > > > > > > > index c443cd64fc9b..a43eb1813cec 100644
> > > > > > > > > --- a/drivers/block/virtio_blk.c
> > > > > > > > > +++ b/drivers/block/virtio_blk.c
> > > > > > > > > @@ -76,9 +76,6 @@ struct virtio_blk {
> > > > > > > > > */
> > > > > > > > > refcount_t refs;
> > > > > > > > > - /* What host tells us, plus 2 for header & tailer. */
> > > > > > > > > - unsigned int sg_elems;
> > > > > > > > > -
> > > > > > > > > /* Ida index - used to track minor number allocations.
> > > > > > > > > */
> > > > > > > > > int index;
> > > > > > > > > @@ -322,8 +319,6 @@ static blk_status_t
> > > > > > > > > virtio_queue_rq(struct blk_mq_hw_ctx *hctx,
> > > > > > > > > blk_status_t status;
> > > > > > > > > int err;
> > > > > > > > > - BUG_ON(req->nr_phys_segments + 2 > vblk->sg_elems);
> > > > > > > > > -
> > > > > > > > > status = virtblk_setup_cmd(vblk->vdev, req, vbr);
> > > > > > > > > if (unlikely(status))
> > > > > > > > > return status;
> > > > > > > > > @@ -783,8 +778,6 @@ static int virtblk_probe(struct
> > > > > > > > > virtio_device *vdev)
> > > > > > > > > /* Prevent integer overflows and honor max vq size */
> > > > > > > > > sg_elems = min_t(u32, sg_elems, VIRTIO_BLK_MAX_SG_ELEMS
> > > > > > > > > - 2);
> > > > > > > > > - /* We need extra sg elements at head and tail. */
> > > > > > > > > - sg_elems += 2;
> > > > > > > > > vdev->priv = vblk = kmalloc(sizeof(*vblk), GFP_KERNEL);
> > > > > > > > > if (!vblk) {
> > > > > > > > > err = -ENOMEM;
> > > > > > > > > @@ -796,7 +789,6 @@ static int virtblk_probe(struct
> > > > > > > > > virtio_device *vdev)
> > > > > > > > > mutex_init(&vblk->vdev_mutex);
> > > > > > > > > vblk->vdev = vdev;
> > > > > > > > > - vblk->sg_elems = sg_elems;
> > > > > > > > > INIT_WORK(&vblk->config_work,
> > > > > > > > > virtblk_config_changed_work);
> > > > > > > > > @@ -853,7 +845,7 @@ static int virtblk_probe(struct
> > > > > > > > > virtio_device *vdev)
> > > > > > > > > set_disk_ro(vblk->disk, 1);
> > > > > > > > > /* We can handle whatever the host told us to handle. */
> > > > > > > > > - blk_queue_max_segments(q, vblk->sg_elems-2);
> > > > > > > > > + blk_queue_max_segments(q, sg_elems);
> > > > > > > > > /* No real sector limit. */
> > > > > > > > > blk_queue_max_hw_sectors(q, -1U);
> > > > > > > > > --
> > > > > > > > > 2.20.1
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
