[PATCH 3/3] block: Refuse adding appending a gapped integrity page to a bio

[Jens Axboe]

On 09/02/2015 02:04 AM, Sagi Grimberg wrote:
> On 8/19/2015 1:42 PM, Christoph Hellwig wrote:
>> On Wed, Aug 19, 2015 at 01:30:56PM +0300, Sagi Grimberg wrote:
>>> Actually I didn't. I started to, but then I noticed that
>>> I was still seeing gaps when using cfq (e.g. non-mq code
>>> path), I assume that this was never tested?
>>
>> It probably wasn't.  The only user so far is the NVMe driver which
>> is blk-mq only.
>
> So I got back to have a look on this. I originally thought that
> this issue was specific to io schedulers, but I don't think it is
> anymore, its just easier to trigger with io schedulers.
>
> It seems we are only protecting against gapped back merges (i.e.
> appending a gapped bio to a request biotail) but we are not protecting
> against front merges (i.e. adding a gapped bio to request as the bio
> head).
>
> Imagine we have two bio_vec elements and the queue boundary is 0xfff:
> req_bvec: offset=0xe00 length=0x200
> bio_bvec: offset=0x0 length=0x200
>
> bvec_gap_to_prev() will allow back merging {req_bvec, bio_bvec} as
> bio_vec->offset=0x0 and req_bvec->offset + req_bvec->length is aligned
> to the queue boundary, but the problem is we might do a front merge
> {bio_bvec, req_bvec} which gives us a gap.
>
> I'm able to reproduce this with iser with 512B sequential reads
> (encourage gapped merges) but I wonder how this issue was missed in
> nvme (the code seem to allow front merges)?
>
> Anyway, the patch below seems to solved the issue for me:
>
> Comments?

Weird, I guess front merging was overlooked when the initial patch was
added. Looks correct to me, and as far as I can see, we have now got all
bases covered.

But there's room for some cleanup now, where we check is a bit of a
mess. If we kill the check in blk_rq_merge_ok() and only do them in the
front/back merge points (and the req-to-req case), then I think that
would be an improvement. Does the below work for you?


diff --git a/block/blk-merge.c b/block/blk-merge.c
index 30a0d9f89017..d226bc5e3b8d 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -309,9 +309,39 @@ no_merge:
 	return 0;
 }
 
+static bool queue_gap_check(struct request_queue *q, struct bio *bio)
+{
+	if (q->queue_flags & (1 << QUEUE_FLAG_SG_GAPS) && bio_has_data(bio))
+		return true;
+
+	return false;
+}
+
+static bool bio_gap_to_prev(struct bio *prev, struct bio *next)
+{
+	return bvec_gap_to_prev(&prev->bi_io_vec[prev->bi_vcnt - 1],
+					next->bi_io_vec[0].bv_offset);
+}
+
+static bool req_gap_to_prev(struct request *req, struct bio *bio)
+{
+	return bio_gap_to_prev(req->biotail, bio);
+}
+
+static bool req_gap_to_next(struct request *req, struct bio *bio)
+{
+	struct bio *next = req->bio;
+
+	return bvec_gap_to_prev(&bio->bi_io_vec[bio->bi_vcnt - 1],
+					next->bi_io_vec[0].bv_offset);
+}
+
 int ll_back_merge_fn(struct request_queue *q, struct request *req,
 		     struct bio *bio)
 {
+	if (queue_gap_check(q, bio) && req_gap_to_prev(req, bio))
+		return 0;
+
 	if (blk_rq_sectors(req) + bio_sectors(bio) >
 	    blk_rq_get_max_sectors(req)) {
 		req->cmd_flags |= REQ_NOMERGE;
@@ -330,6 +360,9 @@ int ll_back_merge_fn(struct request_queue *q, struct request *req,
 int ll_front_merge_fn(struct request_queue *q, struct request *req,
 		      struct bio *bio)
 {
+	if (queue_gap_check(q, bio) && req_gap_to_next(req, bio))
+		return 0;
+
 	if (blk_rq_sectors(req) + bio_sectors(bio) >
 	    blk_rq_get_max_sectors(req)) {
 		req->cmd_flags |= REQ_NOMERGE;
@@ -356,14 +389,6 @@ static bool req_no_special_merge(struct request *req)
 	return !q->mq_ops && req->special;
 }
 
-static int req_gap_to_prev(struct request *req, struct request *next)
-{
-	struct bio *prev = req->biotail;
-
-	return bvec_gap_to_prev(&prev->bi_io_vec[prev->bi_vcnt - 1],
-				next->bio->bi_io_vec[0].bv_offset);
-}
-
 static int ll_merge_requests_fn(struct request_queue *q, struct request *req,
 				struct request *next)
 {
@@ -379,7 +404,7 @@ static int ll_merge_requests_fn(struct request_queue *q, struct request *req,
 		return 0;
 
 	if (test_bit(QUEUE_FLAG_SG_GAPS, &q->queue_flags) &&
-	    req_gap_to_prev(req, next))
+	    req_gap_to_prev(req, next->bio))
 		return 0;
 
 	/*
@@ -564,8 +589,6 @@ int blk_attempt_req_merge(struct request_queue *q, struct request *rq,
 
 bool blk_rq_merge_ok(struct request *rq, struct bio *bio)
 {
-	struct request_queue *q = rq->q;
-
 	if (!rq_mergeable(rq) || !bio_mergeable(bio))
 		return false;
 
@@ -589,15 +612,6 @@ bool blk_rq_merge_ok(struct request *rq, struct bio *bio)
 	    !blk_write_same_mergeable(rq->bio, bio))
 		return false;
 
-	/* Only check gaps if the bio carries data */
-	if (q->queue_flags & (1 << QUEUE_FLAG_SG_GAPS) && bio_has_data(bio)) {
-		struct bio_vec *bprev;
-
-		bprev = &rq->biotail->bi_io_vec[rq->biotail->bi_vcnt - 1];
-		if (bvec_gap_to_prev(bprev, bio->bi_io_vec[0].bv_offset))
-			return false;
-	}
-
 	return true;
 }
 

-- 
Jens Axboe