Re: WARNING: CPU: 2 PID: 207 at drivers/nvme/host/core.c:527 nvme_setup_cmd+0x3d3
From: jianchao.wang <hidden>
Date: 2018-02-01 03:33:12
Also in:
linux-nvme
Subsystem:
block layer, the rest · Maintainers:
Jens Axboe, Linus Torvalds
Sorry, Jens, I think I didn't get the point. Do I miss anything ? On 02/01/2018 11:07 AM, Jens Axboe wrote:
quoted hunk ↗ jump to hunk
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.
quoted hunk ↗ jump to hunk
/* * First check if the either of the requests are re-queued@@ -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.
quoted hunk ↗ jump to hunk
+ 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 -- Each discard bio merged into a request is counted as one segment -- Maybe patch below should be followed with yours.
diff --git a/block/blk-core.c b/block/blk-core.c
index a2005a4..b444fb7 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c@@ -1763,7 +1763,6 @@ bool bio_attempt_discard_merge(struct request_queue *q, struct request *req, req->biotail = bio; req->__data_len += bio->bi_iter.bi_size; req->ioprio = ioprio_best(req->ioprio, bio_prio(bio)); - req->nr_phys_segments = segments + 1; blk_account_io_start(req, false); return true;
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 4f3df80..1af2138 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h@@ -1312,7 +1312,13 @@ static inline unsigned short blk_rq_nr_phys_segments(struct request *rq) */ static inline unsigned short blk_rq_nr_discard_segments(struct request *rq) { - return max_t(unsigned short, rq->nr_phys_segments, 1); + struct bio *bio; + unsigned short count = 0; + + __rq_for_each_bio(bio, req) + count ++; + + return count; }
Thanks Jianchao