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 8:33 PM, jianchao.wang wrote:
> Sorry, Jens, I think I didn't get the point.
> Do I miss anything ?
> 
> On 02/01/2018 11:07 AM, Jens Axboe wrote:
>> Yeah I agree, and my last patch missed that we do care about segments for
>> discards. Below should be better...
>>
>> diff --git a/block/blk-merge.c b/block/blk-merge.c
>> index 8452fc7164cc..055057bd727f 100644
>> --- a/block/blk-merge.c
>> +++ b/block/blk-merge.c
>> @@ -553,9 +553,8 @@ 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 = req->nr_phys_segments +
>> +					next->nr_phys_segments;
> 
> For DISCARD reqs, the total_phys_segments is still zero here.

This seems broken, it should count the ranges in the discard request.

>> @@ -574,8 +573,15 @@ 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 the requests aren't carrying any data payloads, we don't need
>> +	 * to look at the segment count
>> +	 */
>> +	if (bio_has_data(next->bio) &&
>> +	    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;
> 
> Yes, total_phys_segments will not be decreased.
> 
>> +
>>  		if (req->nr_phys_segments == 1)
>>  			req->bio->bi_seg_front_size = seg_size;
>>  		if (next->nr_phys_segments == 1)
>> @@ -584,7 +590,7 @@ static int ll_merge_requests_fn(struct request_queue *q, struct request *req,
>>  	}
>>  
>>  	if (total_phys_segments > queue_max_segments(q))
>> -		return 0;
>> +			return 0;
>>  
>>  	if (blk_integrity_merge_rq(q, req, next) == false)
>>  		return 0;
> 
> But finally, the merged DISCARD req's nr_phys_segment is still zero.
> 
> blk_rq_nr_discard_segments will return 1 but this req has two bios.
> blk_rq_nr_discard_segments 's comment says

They should have the range count. The discard merge stuff is a bit of
a hack, it would be nice to get that improved.

-- 
Jens Axboe