| Message ID | 1468387413-2754-1-git-send-email-damien.lemoal@hgst.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 07/13/2016 07:23 AM, Damien Le Moal wrote: > For a front merge, the maximum number of sectors of the > request must be checked against the front merge BIO sector, > not the current sector of the request. > > Signed-off-by: Damien Le Moal <damien.lemoal@hgst.com> > --- > block/blk-merge.c | 6 +++--- > include/linux/blkdev.h | 5 +++-- > 2 files changed, 6 insertions(+), 5 deletions(-) > Reviewed-by: Hannes Reinecke <hare@suse.com> Cheers, Hannes
On 07/12/2016 10:23 PM, Damien Le Moal wrote: > For a front merge, the maximum number of sectors of the > request must be checked against the front merge BIO sector, > not the current sector of the request. Why does this matter? The merging should only happen before we start the request, hence rq pos and first bio should be one and the same.
Jens, On 7/14/16 01:19, Jens Axboe wrote: > On 07/12/2016 10:23 PM, Damien Le Moal wrote: >> For a front merge, the maximum number of sectors of the >> request must be checked against the front merge BIO sector, >> not the current sector of the request. > > Why does this matter? The merging should only happen before we start the > request, hence rq pos and first bio should be one and the same. The block device of SMR drives is set up as chunked with the chunk size equal to the drive zone size. Since write requests directed to sequential zones cannot cross zone boundaries, both the front merge code and the back merge code must ensure that requests resulting from a merge do not cross chunk boundaries. The back merge code does this well, but the front merge code fails to prevent merging when the BIO is the last write in a zone and the request is the first write in the following (empty) zone. The check against the request LBA does not prevent the merge as the request+bio size fit in the empty zone. The check must be against the BIO LBA to detect the chunk boundary crossing. Best regards.
On 07/13/2016 07:23 PM, Damien Le Moal wrote: > > Jens, > > On 7/14/16 01:19, Jens Axboe wrote: >> On 07/12/2016 10:23 PM, Damien Le Moal wrote: >>> For a front merge, the maximum number of sectors of the >>> request must be checked against the front merge BIO sector, >>> not the current sector of the request. >> >> Why does this matter? The merging should only happen before we start the >> request, hence rq pos and first bio should be one and the same. > > The block device of SMR drives is set up as chunked with the chunk size > equal to the drive zone size. Since write requests directed to > sequential zones cannot cross zone boundaries, both the front merge code > and the back merge code must ensure that requests resulting from a merge > do not cross chunk boundaries. > > The back merge code does this well, but the front merge code fails to > prevent merging when the BIO is the last write in a zone and the request > is the first write in the following (empty) zone. The check against the > request LBA does not prevent the merge as the request+bio size fit in > the empty zone. The check must be against the BIO LBA to detect the > chunk boundary crossing. Ah that makes sense, it's on adding the new bio, not against the existing rq->bio. Applied for 4.8.
diff --git a/block/blk-merge.c b/block/blk-merge.c index 2613531..b736569 100644 --- a/block/blk-merge.c +++ b/block/blk-merge.c @@ -500,7 +500,7 @@ int ll_back_merge_fn(struct request_queue *q, struct request *req, integrity_req_gap_back_merge(req, bio)) return 0; if (blk_rq_sectors(req) + bio_sectors(bio) > - blk_rq_get_max_sectors(req)) { + blk_rq_get_max_sectors(req, blk_rq_pos(req))) { req->cmd_flags |= REQ_NOMERGE; if (req == q->last_merge) q->last_merge = NULL; @@ -524,7 +524,7 @@ int ll_front_merge_fn(struct request_queue *q, struct request *req, integrity_req_gap_front_merge(req, bio)) return 0; if (blk_rq_sectors(req) + bio_sectors(bio) > - blk_rq_get_max_sectors(req)) { + blk_rq_get_max_sectors(req, bio->bi_iter.bi_sector)) { req->cmd_flags |= REQ_NOMERGE; if (req == q->last_merge) q->last_merge = NULL; @@ -570,7 +570,7 @@ static int ll_merge_requests_fn(struct request_queue *q, struct request *req, * Will it become too large? */ if ((blk_rq_sectors(req) + blk_rq_sectors(next)) > - blk_rq_get_max_sectors(req)) + blk_rq_get_max_sectors(req, blk_rq_pos(req))) return 0; total_phys_segments = req->nr_phys_segments + next->nr_phys_segments; diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h index 3d9cf32..b99ef36 100644 --- a/include/linux/blkdev.h +++ b/include/linux/blkdev.h @@ -904,7 +904,8 @@ static inline unsigned int blk_max_size_offset(struct request_queue *q, (offset & (q->limits.chunk_sectors - 1)); } -static inline unsigned int blk_rq_get_max_sectors(struct request *rq) +static inline unsigned int blk_rq_get_max_sectors(struct request *rq, + sector_t offset) { struct request_queue *q = rq->q; @@ -914,7 +915,7 @@ static inline unsigned int blk_rq_get_max_sectors(struct request *rq) if (!q->limits.chunk_sectors || (rq->cmd_flags & REQ_DISCARD)) return blk_queue_get_max_sectors(q, rq->cmd_flags); - return min(blk_max_size_offset(q, blk_rq_pos(rq)), + return min(blk_max_size_offset(q, offset), blk_queue_get_max_sectors(q, rq->cmd_flags)); }
For a front merge, the maximum number of sectors of the request must be checked against the front merge BIO sector, not the current sector of the request. Signed-off-by: Damien Le Moal <damien.lemoal@hgst.com> --- block/blk-merge.c | 6 +++--- include/linux/blkdev.h | 5 +++-- 2 files changed, 6 insertions(+), 5 deletions(-)