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

[sagig@dev.mellanox.co.il (Sagi Grimberg)]

On 9/2/2015 5:37 PM, Jens Axboe wrote:
> 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.


Yea, we do get to checking back merges even for front merges in this
point...

> Does the below work for you?

It's not on top of Keith virt_boundary patch right?
First glance looks ok though.

>
>
> 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;
> }
>
>