Re: WARNING: CPU: 2 PID: 207 at drivers/nvme/host/core.c:527 nvme_setup_cmd+0x3d3
From: Jens Axboe <axboe@kernel.dk>
Date: 2018-02-01 03:03:38
Also in:
linux-nvme
Subsystem:
block layer, the rest · Maintainers:
Jens Axboe, Linus Torvalds
On 1/31/18 4:33 PM, Keith Busch wrote:
On Wed, Jan 31, 2018 at 08:29:37AM -0700, Jens Axboe wrote:quoted
How about something like the below?diff --git a/block/blk-merge.c b/block/blk-merge.c index 8452fc7164cc..cee102fb060e 100644 --- a/block/blk-merge.c +++ b/block/blk-merge.c@@ -574,8 +574,13 @@ static int ll_merge_requests_fn(struct request_queue *q, struct request *req, blk_rq_get_max_sectors(req, blk_rq_pos(req))) return 0; + /* + * For DISCARDs, the segment count isn't interesting since + * the requests have no data attached. + */ total_phys_segments = req->nr_phys_segments + next->nr_phys_segments; - if (blk_phys_contig_segment(q, req->biotail, next->bio)) { + if (total_phys_segments && + blk_phys_contig_segment(q, req->biotail, next->bio)) { if (req->nr_phys_segments == 1) req->bio->bi_seg_front_size = seg_size; if (next->nr_phys_segments == 1)That'll keep it from going to 0xffff, but you'll still hit the warning and IO error. Even worse, this will corrupt memory: blk_rq_nr_discard_segments will return 1, and since you really had 2 segments, the nvme driver will overrun its array.
Yeah you are right, that patch was shit. How about the below? We only need to worry about segment size and number of segments if we are carrying data. req->biotail and next->bio must be the same type, so should be safe.
diff --git a/block/blk-merge.c b/block/blk-merge.c
index 8452fc7164cc..cf9adc4c64b5 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c@@ -553,9 +553,7 @@ static bool req_no_special_merge(struct request *req) static int ll_merge_requests_fn(struct request_queue *q, struct request *req, struct request *next) { - int total_phys_segments; - unsigned int seg_size = - req->biotail->bi_seg_back_size + next->bio->bi_seg_front_size; + int total_phys_segments = 0; /* * First check if the either of the requests are re-queued
@@ -574,17 +572,27 @@ static int ll_merge_requests_fn(struct request_queue *q, struct request *req, blk_rq_get_max_sectors(req, blk_rq_pos(req))) return 0; - total_phys_segments = req->nr_phys_segments + next->nr_phys_segments; - if (blk_phys_contig_segment(q, req->biotail, next->bio)) { - if (req->nr_phys_segments == 1) - req->bio->bi_seg_front_size = seg_size; - if (next->nr_phys_segments == 1) - next->biotail->bi_seg_back_size = seg_size; - total_phys_segments--; - } + /* + * If the requests aren't carrying any data payloads, we don't need + * to look at the segment count + */ + if (bio_has_data(next->bio)) { + total_phys_segments = req->nr_phys_segments + + next->nr_phys_segments; + if (blk_phys_contig_segment(q, req->biotail, next->bio)) { + unsigned int seg_size = req->biotail->bi_seg_back_size + + next->bio->bi_seg_front_size; + + if (req->nr_phys_segments == 1) + req->bio->bi_seg_front_size = seg_size; + if (next->nr_phys_segments == 1) + next->biotail->bi_seg_back_size = seg_size; + total_phys_segments--; + } - if (total_phys_segments > queue_max_segments(q)) - return 0; + if (total_phys_segments > queue_max_segments(q)) + return 0; + } if (blk_integrity_merge_rq(q, req, next) == false) return 0;
--
Jens Axboe