| Message ID | ef22f6c5-84c4-1ebf-b4b5-d0a2d1d29927@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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; }