| Message ID | 20220223133627.102-1-xieyongji@bytedance.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | virtio-blk: Check the max discard segment for discard request | expand |
On Wed, Feb 23, 2022 at 09:36:27PM +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(). To fix it, this patch > checks the max discard segment for the discard request > in the BUG_ON() instead. This looks god, but jut removing the BUG_ON might be even better.
On Thu, Feb 24, 2022 at 9:34 PM Christoph Hellwig <hch@infradead.org> wrote: > > On Wed, Feb 23, 2022 at 09:36:27PM +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(). To fix it, this patch > > checks the max discard segment for the discard request > > in the BUG_ON() instead. > > This looks god, but jut removing the BUG_ON might be even better. LGTM. If no objection, I will do it in v2. Thanks, Yongji
diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c index c443cd64fc9b..a1f9045f848e 100644 --- a/drivers/block/virtio_blk.c +++ b/drivers/block/virtio_blk.c @@ -79,6 +79,9 @@ struct virtio_blk { /* What host tells us, plus 2 for header & tailer. */ unsigned int sg_elems; + /* The max discard segment. */ + unsigned int discard_sg_elems; + /* Ida index - used to track minor number allocations. */ int index; @@ -321,8 +324,10 @@ static blk_status_t virtio_queue_rq(struct blk_mq_hw_ctx *hctx, bool notify = false; blk_status_t status; int err; + u32 sg_elems = (req_op(req) == REQ_OP_DISCARD) ? + vblk->discard_sg_elems + 2 : vblk->sg_elems; - BUG_ON(req->nr_phys_segments + 2 > vblk->sg_elems); + BUG_ON(req->nr_phys_segments + 2 > sg_elems); status = virtblk_setup_cmd(vblk->vdev, req, vbr); if (unlikely(status)) @@ -925,9 +930,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)); + vblk->discard_sg_elems = min_not_zero(v, MAX_DISCARD_SEGMENTS); + blk_queue_max_discard_segments(q, vblk->discard_sg_elems); blk_queue_flag_set(QUEUE_FLAG_DISCARD, q); }
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, this patch checks the max discard segment for the discard request in the BUG_ON() instead. Fixes: 1f23816b8eb8 ("virtio_blk: add discard and write zeroes support") Signed-off-by: Xie Yongji <xieyongji@bytedance.com> --- drivers/block/virtio_blk.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)