* [PATCH v2] virtio-blk: Remove BUG_ON() in virtio_queue_rq() @ 2022-02-28 6:57 Xie Yongji 2022-03-01 12:59 ` Christoph Hellwig 2022-03-01 15:43 ` Michael S. Tsirkin 0 siblings, 2 replies; 28+ messages in thread From: Xie Yongji @ 2022-02-28 6:57 UTC (permalink / raw) To: mst, jasowang, axboe, hch; +Cc: virtualization, linux-block 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(). 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 <hch@infradead.org> Signed-off-by: Xie Yongji <xieyongji@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 ^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH v2] virtio-blk: Remove BUG_ON() in virtio_queue_rq() 2022-02-28 6:57 [PATCH v2] virtio-blk: Remove BUG_ON() in virtio_queue_rq() Xie Yongji @ 2022-03-01 12:59 ` Christoph Hellwig 2022-03-01 15:43 ` Michael S. Tsirkin 1 sibling, 0 replies; 28+ messages in thread From: Christoph Hellwig @ 2022-03-01 12:59 UTC (permalink / raw) To: Xie Yongji; +Cc: mst, jasowang, axboe, hch, virtualization, linux-block Looks good, Reviewed-by: Christoph Hellwig <hch@lst.de> ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2] virtio-blk: Remove BUG_ON() in virtio_queue_rq() @ 2022-03-01 12:59 ` Christoph Hellwig 0 siblings, 0 replies; 28+ messages in thread From: Christoph Hellwig @ 2022-03-01 12:59 UTC (permalink / raw) To: Xie Yongji; +Cc: axboe, linux-block, mst, virtualization, hch Looks good, Reviewed-by: Christoph Hellwig <hch@lst.de> _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2] virtio-blk: Remove BUG_ON() in virtio_queue_rq() 2022-02-28 6:57 [PATCH v2] virtio-blk: Remove BUG_ON() in virtio_queue_rq() Xie Yongji @ 2022-03-01 15:43 ` Michael S. Tsirkin 2022-03-01 15:43 ` Michael S. Tsirkin 1 sibling, 0 replies; 28+ messages in thread From: Michael S. Tsirkin @ 2022-03-01 15:43 UTC (permalink / raw) To: Xie Yongji; +Cc: jasowang, axboe, hch, virtualization, linux-block 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? > 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 <hch@infradead.org> > Signed-off-by: Xie Yongji <xieyongji@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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2] virtio-blk: Remove BUG_ON() in virtio_queue_rq() @ 2022-03-01 15:43 ` Michael S. Tsirkin 0 siblings, 0 replies; 28+ messages in thread From: Michael S. Tsirkin @ 2022-03-01 15:43 UTC (permalink / raw) To: Xie Yongji; +Cc: axboe, hch, linux-block, virtualization 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? > 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 <hch@infradead.org> > Signed-off-by: Xie Yongji <xieyongji@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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2] virtio-blk: Remove BUG_ON() in virtio_queue_rq() 2022-03-01 15:43 ` Michael S. Tsirkin (?) @ 2022-03-02 9:51 ` Max Gurtovoy 2022-03-02 13:17 ` Michael S. Tsirkin -1 siblings, 1 reply; 28+ messages in thread From: Max Gurtovoy @ 2022-03-02 9:51 UTC (permalink / raw) To: Michael S. Tsirkin, Xie Yongji Cc: jasowang, axboe, hch, virtualization, linux-block 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. This commit looks good to me, thanks Xie Yongji. Reviewed-by: Max Gurtovoy <mgurtovoy@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 <hch@infradead.org> >> Signed-off-by: Xie Yongji <xieyongji@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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2] virtio-blk: Remove BUG_ON() in virtio_queue_rq() 2022-03-02 9:51 ` Max Gurtovoy @ 2022-03-02 13:17 ` Michael S. Tsirkin 0 siblings, 0 replies; 28+ messages in thread From: Michael S. Tsirkin @ 2022-03-02 13:17 UTC (permalink / raw) To: Max Gurtovoy Cc: Xie Yongji, jasowang, axboe, hch, virtualization, linux-block 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. > This commit looks good to me, thanks Xie Yongji. > > Reviewed-by: Max Gurtovoy <mgurtovoy@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 <hch@infradead.org> > > > Signed-off-by: Xie Yongji <xieyongji@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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2] virtio-blk: Remove BUG_ON() in virtio_queue_rq() @ 2022-03-02 13:17 ` Michael S. Tsirkin 0 siblings, 0 replies; 28+ messages in thread From: Michael S. Tsirkin @ 2022-03-02 13:17 UTC (permalink / raw) To: Max Gurtovoy; +Cc: axboe, hch, virtualization, linux-block, Xie Yongji 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. > This commit looks good to me, thanks Xie Yongji. > > Reviewed-by: Max Gurtovoy <mgurtovoy@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 <hch@infradead.org> > > > Signed-off-by: Xie Yongji <xieyongji@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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2] virtio-blk: Remove BUG_ON() in virtio_queue_rq() 2022-03-02 13:17 ` Michael S. Tsirkin (?) @ 2022-03-02 13:24 ` Max Gurtovoy 2022-03-02 13:33 ` Michael S. Tsirkin -1 siblings, 1 reply; 28+ messages in thread From: Max Gurtovoy @ 2022-03-02 13:24 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Xie Yongji, jasowang, axboe, hch, virtualization, linux-block 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. Probably no such devices, but we need to be prepared. > >> This commit looks good to me, thanks Xie Yongji. >> >> Reviewed-by: Max Gurtovoy <mgurtovoy@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 <hch@infradead.org> >>>> Signed-off-by: Xie Yongji <xieyongji@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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2] virtio-blk: Remove BUG_ON() in virtio_queue_rq() 2022-03-02 13:24 ` Max Gurtovoy @ 2022-03-02 13:33 ` Michael S. Tsirkin 0 siblings, 0 replies; 28+ messages in thread From: Michael S. Tsirkin @ 2022-03-02 13:33 UTC (permalink / raw) To: Max Gurtovoy Cc: Xie Yongji, jasowang, axboe, hch, virtualization, linux-block 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. 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 <mgurtovoy@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 <hch@infradead.org> > > > > > Signed-off-by: Xie Yongji <xieyongji@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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2] virtio-blk: Remove BUG_ON() in virtio_queue_rq() @ 2022-03-02 13:33 ` Michael S. Tsirkin 0 siblings, 0 replies; 28+ messages in thread From: Michael S. Tsirkin @ 2022-03-02 13:33 UTC (permalink / raw) To: Max Gurtovoy; +Cc: axboe, hch, virtualization, linux-block, Xie Yongji 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. 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 <mgurtovoy@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 <hch@infradead.org> > > > > > Signed-off-by: Xie Yongji <xieyongji@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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2] virtio-blk: Remove BUG_ON() in virtio_queue_rq() 2022-03-02 13:33 ` Michael S. Tsirkin (?) @ 2022-03-02 13:45 ` Max Gurtovoy 2022-03-02 14:15 ` Michael S. Tsirkin -1 siblings, 1 reply; 28+ messages in thread From: Max Gurtovoy @ 2022-03-02 13:45 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Xie Yongji, jasowang, axboe, hch, virtualization, linux-block 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. 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 <mgurtovoy@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 <hch@infradead.org> >>>>>> Signed-off-by: Xie Yongji <xieyongji@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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2] virtio-blk: Remove BUG_ON() in virtio_queue_rq() 2022-03-02 13:45 ` Max Gurtovoy @ 2022-03-02 14:15 ` Michael S. Tsirkin 0 siblings, 0 replies; 28+ messages in thread From: Michael S. Tsirkin @ 2022-03-02 14:15 UTC (permalink / raw) To: Max Gurtovoy Cc: Xie Yongji, jasowang, axboe, hch, virtualization, linux-block 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. > 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 <mgurtovoy@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 <hch@infradead.org> > > > > > > > Signed-off-by: Xie Yongji <xieyongji@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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2] virtio-blk: Remove BUG_ON() in virtio_queue_rq() @ 2022-03-02 14:15 ` Michael S. Tsirkin 0 siblings, 0 replies; 28+ messages in thread From: Michael S. Tsirkin @ 2022-03-02 14:15 UTC (permalink / raw) To: Max Gurtovoy; +Cc: axboe, hch, virtualization, linux-block, Xie Yongji 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. > 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 <mgurtovoy@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 <hch@infradead.org> > > > > > > > Signed-off-by: Xie Yongji <xieyongji@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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2] virtio-blk: Remove BUG_ON() in virtio_queue_rq() 2022-03-02 14:15 ` Michael S. Tsirkin (?) @ 2022-03-02 14:27 ` Max Gurtovoy 2022-03-02 14:48 ` Michael S. Tsirkin -1 siblings, 1 reply; 28+ messages in thread From: Max Gurtovoy @ 2022-03-02 14:27 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Xie Yongji, jasowang, axboe, hch, virtualization, linux-block 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. Do you have such device to test ? > >> 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 <mgurtovoy@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 <hch@infradead.org> >>>>>>>> Signed-off-by: Xie Yongji <xieyongji@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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2] virtio-blk: Remove BUG_ON() in virtio_queue_rq() 2022-03-02 14:27 ` Max Gurtovoy @ 2022-03-02 14:48 ` Michael S. Tsirkin 0 siblings, 0 replies; 28+ messages in thread From: Michael S. Tsirkin @ 2022-03-02 14:48 UTC (permalink / raw) To: Max Gurtovoy Cc: Xie Yongji, jasowang, axboe, hch, virtualization, linux-block 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 <mgurtovoy@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 <hch@infradead.org> > > > > > > > > > Signed-off-by: Xie Yongji <xieyongji@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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2] virtio-blk: Remove BUG_ON() in virtio_queue_rq() @ 2022-03-02 14:48 ` Michael S. Tsirkin 0 siblings, 0 replies; 28+ messages in thread From: Michael S. Tsirkin @ 2022-03-02 14:48 UTC (permalink / raw) To: Max Gurtovoy; +Cc: axboe, hch, virtualization, linux-block, Xie Yongji 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 <mgurtovoy@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 <hch@infradead.org> > > > > > > > > > Signed-off-by: Xie Yongji <xieyongji@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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2] virtio-blk: Remove BUG_ON() in virtio_queue_rq() 2022-03-02 13:33 ` Michael S. Tsirkin (?) (?) @ 2022-03-02 13:53 ` Yongji Xie 2022-03-02 14:20 ` Michael S. Tsirkin -1 siblings, 1 reply; 28+ messages in thread From: Yongji Xie @ 2022-03-02 13:53 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Max Gurtovoy, Jason Wang, Jens Axboe, Christoph Hellwig, virtualization, linux-block On Wed, Mar 2, 2022 at 9:33 PM Michael S. Tsirkin <mst@redhat.com> 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. > I would expect discard to follow max_segments restrictions then. > I think one point is whether we want to allow the corner case that the device reports a larger value for max_discard_segments than max_segments. For example, queue size is 256, max_segments is 128 - 2, max_discard_segments is 256 - 2. Thanks, Yongji ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2] virtio-blk: Remove BUG_ON() in virtio_queue_rq() 2022-03-02 13:53 ` Yongji Xie @ 2022-03-02 14:20 ` Michael S. Tsirkin 0 siblings, 0 replies; 28+ messages in thread From: Michael S. Tsirkin @ 2022-03-02 14:20 UTC (permalink / raw) To: Yongji Xie Cc: Max Gurtovoy, Jason Wang, Jens Axboe, Christoph Hellwig, virtualization, linux-block On Wed, Mar 02, 2022 at 09:53:17PM +0800, Yongji Xie wrote: > On Wed, Mar 2, 2022 at 9:33 PM Michael S. Tsirkin <mst@redhat.com> 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. > > I would expect discard to follow max_segments restrictions then. > > > > I think one point is whether we want to allow the corner case that the > device reports a larger value for max_discard_segments than > max_segments. For example, queue size is 256, max_segments is 128 - 2, > max_discard_segments is 256 - 2. > > Thanks, > Yongji So if device specifies that, then I guess it's fine and from that POV the patch is correct. But I think the issue is when device specifies 0 which we interpret as 256 with no basis in hardware. -- MST ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2] virtio-blk: Remove BUG_ON() in virtio_queue_rq() @ 2022-03-02 14:20 ` Michael S. Tsirkin 0 siblings, 0 replies; 28+ messages in thread From: Michael S. Tsirkin @ 2022-03-02 14:20 UTC (permalink / raw) To: Yongji Xie Cc: Max Gurtovoy, Jens Axboe, Christoph Hellwig, virtualization, linux-block On Wed, Mar 02, 2022 at 09:53:17PM +0800, Yongji Xie wrote: > On Wed, Mar 2, 2022 at 9:33 PM Michael S. Tsirkin <mst@redhat.com> 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. > > I would expect discard to follow max_segments restrictions then. > > > > I think one point is whether we want to allow the corner case that the > device reports a larger value for max_discard_segments than > max_segments. For example, queue size is 256, max_segments is 128 - 2, > max_discard_segments is 256 - 2. > > Thanks, > Yongji So if device specifies that, then I guess it's fine and from that POV the patch is correct. But I think the issue is when device specifies 0 which we interpret as 256 with no basis in hardware. -- MST _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2] virtio-blk: Remove BUG_ON() in virtio_queue_rq() 2022-03-01 15:43 ` Michael S. Tsirkin (?) (?) @ 2022-03-02 10:46 ` Yongji Xie 2022-03-02 13:15 ` Michael S. Tsirkin -1 siblings, 1 reply; 28+ messages in thread From: Yongji Xie @ 2022-03-02 10:46 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Jason Wang, Jens Axboe, Christoph Hellwig, virtualization, linux-block 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2] virtio-blk: Remove BUG_ON() in virtio_queue_rq() 2022-03-02 10:46 ` Yongji Xie @ 2022-03-02 13:15 ` Michael S. Tsirkin 0 siblings, 0 replies; 28+ messages in thread From: Michael S. Tsirkin @ 2022-03-02 13:15 UTC (permalink / raw) To: Yongji Xie Cc: Jason Wang, Jens Axboe, Christoph Hellwig, virtualization, linux-block 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. -- MST ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2] virtio-blk: Remove BUG_ON() in virtio_queue_rq() @ 2022-03-02 13:15 ` Michael S. Tsirkin 0 siblings, 0 replies; 28+ messages in thread From: Michael S. Tsirkin @ 2022-03-02 13:15 UTC (permalink / raw) To: Yongji Xie; +Cc: Jens Axboe, Christoph Hellwig, linux-block, virtualization 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. -- MST _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2] virtio-blk: Remove BUG_ON() in virtio_queue_rq() 2022-03-02 13:15 ` Michael S. Tsirkin (?) @ 2022-03-02 15:05 ` Max Gurtovoy 2022-03-03 3:31 ` Yongji Xie -1 siblings, 1 reply; 28+ messages in thread From: Max Gurtovoy @ 2022-03-02 15:05 UTC (permalink / raw) To: Michael S. Tsirkin, Yongji Xie Cc: Jason Wang, Jens Axboe, Christoph Hellwig, virtualization, linux-block 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); } ^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH v2] virtio-blk: Remove BUG_ON() in virtio_queue_rq() 2022-03-02 15:05 ` Max Gurtovoy @ 2022-03-03 3:31 ` Yongji Xie 2022-03-03 7:22 ` Michael S. Tsirkin 0 siblings, 1 reply; 28+ messages in thread From: Yongji Xie @ 2022-03-03 3:31 UTC (permalink / raw) To: Max Gurtovoy Cc: Michael S. Tsirkin, Jason Wang, Jens Axboe, Christoph Hellwig, virtualization, linux-block 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2] virtio-blk: Remove BUG_ON() in virtio_queue_rq() 2022-03-03 3:31 ` Yongji Xie @ 2022-03-03 7:22 ` Michael S. Tsirkin 0 siblings, 0 replies; 28+ messages in thread From: Michael S. Tsirkin @ 2022-03-03 7:22 UTC (permalink / raw) To: Yongji Xie Cc: Max Gurtovoy, Jason Wang, Jens Axboe, Christoph Hellwig, virtualization, linux-block 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@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 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2] virtio-blk: Remove BUG_ON() in virtio_queue_rq() @ 2022-03-03 7:22 ` Michael S. Tsirkin 0 siblings, 0 replies; 28+ messages in thread From: Michael S. Tsirkin @ 2022-03-03 7:22 UTC (permalink / raw) To: Yongji Xie Cc: Max Gurtovoy, Jens Axboe, Christoph Hellwig, virtualization, linux-block 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@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 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 _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2] virtio-blk: Remove BUG_ON() in virtio_queue_rq() 2022-03-03 7:22 ` Michael S. Tsirkin (?) @ 2022-03-03 8:11 ` Yongji Xie -1 siblings, 0 replies; 28+ messages in thread From: Yongji Xie @ 2022-03-03 8:11 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Max Gurtovoy, Jason Wang, Jens Axboe, Christoph Hellwig, virtualization, linux-block On Thu, Mar 3, 2022 at 3:22 PM Michael S. Tsirkin <mst@redhat.com> 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@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 > > 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
end of thread, other threads:[~2022-03-03 8:11 UTC | newest] Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-02-28 6:57 [PATCH v2] virtio-blk: Remove BUG_ON() in virtio_queue_rq() Xie Yongji 2022-03-01 12:59 ` Christoph Hellwig 2022-03-01 12:59 ` Christoph Hellwig 2022-03-01 15:43 ` Michael S. Tsirkin 2022-03-01 15:43 ` Michael S. Tsirkin 2022-03-02 9:51 ` Max Gurtovoy 2022-03-02 13:17 ` Michael S. Tsirkin 2022-03-02 13:17 ` Michael S. Tsirkin 2022-03-02 13:24 ` Max Gurtovoy 2022-03-02 13:33 ` Michael S. Tsirkin 2022-03-02 13:33 ` Michael S. Tsirkin 2022-03-02 13:45 ` Max Gurtovoy 2022-03-02 14:15 ` Michael S. Tsirkin 2022-03-02 14:15 ` Michael S. Tsirkin 2022-03-02 14:27 ` Max Gurtovoy 2022-03-02 14:48 ` Michael S. Tsirkin 2022-03-02 14:48 ` Michael S. Tsirkin 2022-03-02 13:53 ` Yongji Xie 2022-03-02 14:20 ` Michael S. Tsirkin 2022-03-02 14:20 ` Michael S. Tsirkin 2022-03-02 10:46 ` Yongji Xie 2022-03-02 13:15 ` Michael S. Tsirkin 2022-03-02 13:15 ` Michael S. Tsirkin 2022-03-02 15:05 ` Max Gurtovoy 2022-03-03 3:31 ` Yongji Xie 2022-03-03 7:22 ` Michael S. Tsirkin 2022-03-03 7:22 ` Michael S. Tsirkin 2022-03-03 8:11 ` Yongji Xie
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.