blk-merge: BIO front merge size check for chunked devices

Commit Message

Damien Le Moal Feb. 2, 2016, 7:06 a.m. UTC

Hello,

In ll_front_merge_fn, the request size result of the eventual front
merge of a BIO with a request is checked against the maximum
size allowed for the request using blk_rq_get_max_sectors. This
function will also check that the merge result will not span block
chunks for chunked block devices using the function blk_max_size_offset.

However, blk_rq_get_max_sectors always calls blk_max_size_offset using
the request sector position, which in the case of a front merge is not
the position at which the merged request would be issued: the sector
position to use must be that of the BIO, and not that of the request.

This problem can trigger a “boundary violation error” for write
requests on ZAC/ZBC host-managed SMR disks as the last write BIO
of a zone (a chunk) can end up being front-merged with the first
request of the following zone (chunk).

The attached patch against linux-4.5-rc2 fixes this problem by adding
an offset argument to the function blk_rq_get_max_sectors.

Best regards.


--
Damien Le Moal,
System Software Group, WW Research,
HGST, a Western Digital company
Western Digital Corporation (and its subsidiaries) E-mail Confidentiality Notice & Disclaimer:

This e-mail and any files transmitted with it may contain confidential or legally privileged information of WDC and/or its affiliates, and are intended solely for the use of the individual or entity to which they are addressed. If you are not the intended recipient, any disclosure, copying, distribution or any action taken or omitted to be taken in reliance on it, is prohibited. If you have received this e-mail in error, please notify the sender immediately and delete the e-mail in its entirety from your system.

Comments

Hannes Reinecke Feb. 2, 2016, 7:46 a.m. UTC | #1

On 02/02/2016 08:06 AM, Damien Le Moal wrote:
> 
> Hello,
> 
> In ll_front_merge_fn, the request size result of the eventual front
> merge of a BIO with a request is checked against the maximum
> size allowed for the request using blk_rq_get_max_sectors. This
> function will also check that the merge result will not span block
> chunks for chunked block devices using the function blk_max_size_offset.
> 
> However, blk_rq_get_max_sectors always calls blk_max_size_offset using
> the request sector position, which in the case of a front merge is not
> the position at which the merged request would be issued: the sector
> position to use must be that of the BIO, and not that of the request.
> 
> This problem can trigger a “boundary violation error” for write
> requests on ZAC/ZBC host-managed SMR disks as the last write BIO
> of a zone (a chunk) can end up being front-merged with the first
> request of the following zone (chunk).
> 
> The attached patch against linux-4.5-rc2 fixes this problem by adding
> an offset argument to the function blk_rq_get_max_sectors.
> 

Reviewed-by: Hannes Reinecke <[e-mail]>

I can easily respin this as a proper patch if required.

Cheers,

Hannes

Patch

diff -Naur linux-4.5-rc2/block/blk-merge.c linux-4.5-rc2-patched/block/blk-merge.c
--- linux-4.5-rc2/block/blk-merge.c	2016-02-01 11:12:16.000000000 +0900
+++ linux-4.5-rc2-patched/block/blk-merge.c	2016-02-02 15:28:07.626107509 +0900
@@ -504,7 +504,7 @@ 
 	    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;
@@ -528,7 +528,7 @@ 
 	    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;
@@ -574,7 +574,7 @@ 
 	 * 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 -Naur linux-4.5-rc2/include/linux/blkdev.h linux-4.5-rc2-patched/include/linux/blkdev.h
--- linux-4.5-rc2/include/linux/blkdev.h	2016-02-01 11:12:16.000000000 +0900
+++ linux-4.5-rc2-patched/include/linux/blkdev.h	2016-02-02 15:26:39.313283902 +0900
@@ -888,7 +888,8 @@ 
 			(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;
 
@@ -898,7 +899,7 @@ 
 	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));
 }