From mboxrd@z Thu Jan 1 00:00:00 1970 From: axboe@kernel.dk (Jens Axboe) Date: Wed, 2 Sep 2015 08:37:56 -0600 Subject: [PATCH 3/3] block: Refuse adding appending a gapped integrity page to a bio In-Reply-To: <55E6ADA4.5040905@dev.mellanox.co.il> References: <1436966356-8979-1-git-send-email-sagig@mellanox.com> <1436966356-8979-4-git-send-email-sagig@mellanox.com> <55A67C17.4080706@kernel.dk> <20150716092646.GA17712@infradead.org> <55A7D4B6.9020101@kernel.dk> <55ABBFB1.7070201@dev.mellanox.co.il> <20150819094003.GB14734@infradead.org> <55D45AE0.50805@dev.mellanox.co.il> <20150819104246.GA4956@infradead.org> <55E6ADA4.5040905@dev.mellanox.co.il> Message-ID: <20150902143756.GA11062@kernel.dk> 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@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