Re: WARNING: CPU: 2 PID: 207 at drivers/nvme/host/core.c:527 nvme_setup_cmd+0x3d3

[Jens Axboe <axboe@kernel.dk>]

On 1/31/18 4:33 PM, Keith Busch wrote:
> On Wed, Jan 31, 2018 at 08:29:37AM -0700, Jens Axboe wrote:
>>
>> 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