Fixes for gapped scatters

For drivers that don't support gaps in the SG lists handed to them we must not create bios from multiple iovecs as they can be (and usually are) discontiguous. This doesn't matter for any current user, but will help to allow iSER which can't handle gaps to use the QUEUE_FLAG_SG_GAPS flag instead of using driver-local bounce buffering.

Quoted 6 lines · Sagi Grimberg

For drivers that don't support gaps in the SG lists handed to them we must
not create bios from multiple iovecs as they can be (and usually are) discontiguous. This doesn't matter for any current user, but will help to allow iSER which can't handle gaps to use the QUEUE_FLAG_SG_GAPS flag instead of using driver-local bounce buffering.

Maybe SG_GAPS is a bit of a misnomer. There are cases we can directly map a user iov with multiple discontiguous vectors. At least for NVMe it'd work if each iov's base and length are on page boundaries, we don't need to do the indirect copy.

Like with this user example:
---

     int fd = open("/dev/nvme0n1", O_RDONLY | O_DIRECT);
     struct iovec iov[2];
      unsigned char *buffer;

      posix_memalign((void **)&buffer, 0x4000, 0x1000);

      iov[0].iov_base = &buffer[0];
      iov[1].iov_base = &buffer[0x2000];
      iov[0].iov_len = 0x1000;
      iov[1].iov_len = 0x1000;

     readv(fd, iov, 2);

SG_GAPS is a bit of a misnomer, but it's not easy to explain exactly what that logic is in a few short words! SG gaps was the closest I could come up with.

Quoted 3 lines · Jens Axboe

SG_GAPS is a bit of a misnomer, but it's not easy to explain exactly what that logic is in a few short words! SG gaps was the closest I could come up with.

The sense is wrong -- surely it should be 'NOGAPS'?

SG_GAPS is a bit of a misnomer, but it's not easy to explain exactly what that logic is in a few short words! SG gaps was the closest I could come up with.

Quoted 5 lines · Sagi Grimberg

For drivers that don't support gaps in the SG lists handed to them we must not create bios from multiple iovecs as they can be (and usually are) discontiguous. This doesn't matter for any current user, but will help to allow iSER which can't handle gaps to use the QUEUE_FLAG_SG_GAPS flag instead of using driver-local bounce buffering.

Maybe SG_GAPS is a bit of a misnomer. There are cases we can directly map a user iov with multiple discontiguous vectors. At least for NVMe it'd work if each iov's base and length are on page boundaries, we don't need to do the indirect copy.

This is only theoretical at the moment given that the only subsystems that generate integrity payloads are the block layer itself and the scsi target. But when we will expose integrity meta-data to user-space, we'll need to refuse appending a page with a gap when QUEUE_FLAG_SG_GAPS flag is on.

Signed-off-by Sagi Grimberg <sagig at mellanox.com>

---
  block/bio-integrity.c | 9 +++++++++
  1 file changed, 9 insertions(+)

block/bio-integrity.c +0 -0

index 5cbd5d9..a18edb8 100644
@@ -140,6 +140,15 @@ int bio_integrity_add_page(struct bio *bio, struct page *page,
 

This chunk of code largely ends up being duplicated in several places with your patches, might be a good idea to pull that into a helper function. Then you can add the comment in one place as well :-)

Quoted 3 lines · Jens Axboe

This chunk of code largely ends up being duplicated in several places with your patches, might be a good idea to pull that into a helper function. Then you can add the comment in one place as well :-)

Basically all the bio_vec-related code in block-integrity is copy & paste of the I/O path. In the long run we'll need to refactor it by adding bio_vec iteration helpers that can pick different anchors from the bio. But for now I think I'd rather have this quick fix.

Quoted 7 lines · Christoph Hellwig

Quoted 5 lines · Jens Axboe

This chunk of code largely ends up being duplicated in several places with
your patches, might be a good idea to pull that into a helper function. Then
you can add the comment in one place as well :-)

Quoted 4 lines

Basically all the bio_vec-related code in block-integrity is copy & paste of the I/O path. In the long run we'll need to refactor it by adding bio_vec iteration helpers that can pick different anchors from the bio. But for now I think I'd rather have this quick fix.

I wasn't asking for a refactor of the bio_vec integrity code (though that would be nice...), just that we don't add 3 more instances of 'does this gap with previous'.

I'll centralize the checks. No problem.

Quoted 1 line · Sagi Grimberg

I'll centralize the checks. No problem.

Did you get a chance to respin these patches? It would be good to get them in this merge window.

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?

Since I was busy with the iser-target iWARP support with Steve so I didn't get a chance to get back to it yet...

Quoted 3 lines · Sagi Grimberg

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.

Quoted 7 lines · Christoph Hellwig

Quoted 3 lines · Sagi Grimberg

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?

Quoted 27 lines · Sagi Grimberg

Quoted 7 lines · Christoph Hellwig

Quoted 3 lines · Sagi Grimberg

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.

Quoted 7 lines

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?

block/blk-merge.c +33 -0

index 30a0d9f89017..d226bc5e3b8d 100644
@@ -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;

block/blk-merge.c +0 -8

@@ -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)
{

block/blk-merge.c +1 -1

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

     /*

block/blk-merge.c +0 -11

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

Quoted 40 lines · Jens Axboe

Quoted 27 lines · Sagi Grimberg

Quoted 7 lines · Christoph Hellwig

Quoted 3 lines · Sagi Grimberg

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.

Quoted 7 lines

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.

Quoted 4 lines

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...

Quoted 1 line

Does the below work for you?

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

Quoted 1 line

Does the below work for you?

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

Quoted 4 lines · Sagi Grimberg

Quoted 1 line

Does the below work for you?

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

Updated for that.

block/blk-merge.c +18 -0

index b2625271a572..5f35a05337b1 100644
@@ -436,9 +436,27 @@ no_merge:
  	return 0;
  }
 
+static bool req_gap_to_prev(struct request *req, struct bio *next)
+{
+	struct bio *prev = req->biotail;
+
+	return bvec_gap_to_prev(req->q, &prev->bi_io_vec[prev->bi_vcnt - 1],
+				next->bi_io_vec[0].bv_offset);
+}
+
+static bool req_gap_to_next(struct request *req, struct bio *bio)
+{
+	struct bio *next = req->bio;
+
+	return bvec_gap_to_prev(req->q, &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 (bio_has_data(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;

Quoted 8 lines · Jens Axboe

Quoted 4 lines · Sagi Grimberg

Quoted 1 line

Does the below work for you?

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

Updated for that.

Thanks Jens,

I'll test it.

Quoted 36 lines

block/blk-merge.c +18 -0

index b2625271a572..5f35a05337b1 100644
@@ -436,9 +436,27 @@ no_merge:
      return 0;
  }
 
+static bool req_gap_to_prev(struct request *req, struct bio *next)
+{
+    struct bio *prev = req->biotail;
+
+    return bvec_gap_to_prev(req->q, &prev->bi_io_vec[prev->bi_vcnt - 1],
+                next->bi_io_vec[0].bv_offset);
+}
+
+static bool req_gap_to_next(struct request *req, struct bio *bio)
+{
+    struct bio *next = req->bio;
+
+    return bvec_gap_to_prev(req->q, &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 (bio_has_data(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;

@@ -457,6 +475,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,

block/blk-merge.c int ll_back_merge_fn(struct request_queue *q, struct request *req, · line 480

Quoted 5 lines

block/blk-merge.c +3 -0

                struct bio *bio)
  {
+
+    if (bio_has_data(bio) && req_gap_to_next(req, bio))
+        return 0;

I think it will be cleaner to git bio_has_data(bio) check into req_gap_to_[prev|next]

Quoted 6 lines · Jens Axboe

Quoted 4 lines · Sagi Grimberg

Quoted 1 line

Does the below work for you?

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

Quoted 2 lines

Updated for that.

Thanks Jens,

I'll test it.

Quoted 10 lines · Sagi Grimberg

Quoted 8 lines · Jens Axboe

Quoted 4 lines · Sagi Grimberg

Quoted 1 line

Does the below work for you?

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

Updated for that.

Quoted 3 lines

Thanks Jens,

I'll test it.

Cleaned up version, unified the gap checking and changed the names of the function to make it easier to understand. And then we only need to check bio_has_data() in bio_will_gap().

Quoted 15 lines · Jens Axboe

Quoted 13 lines · Sagi Grimberg

Quoted 8 lines · Jens Axboe

Quoted 4 lines · Sagi Grimberg

Quoted 1 line

Does the below work for you?

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

Updated for that.

Thanks Jens,

I'll test it.

Quoted 3 lines

Cleaned up version, unified the gap checking and changed the names of the function to make it easier to understand. And then we only need to check bio_has_data() in bio_will_gap().

:)

I was just going to say that maybe we should keep the check explicit..

I am going to fix this also for integrity payload [1], and there I need to check for blk_integrity_rq(req) explicitly because it doesn't really makes sense to call an integrity gap checker if you don't have integrity...

Also, lets move the gap helpers to be static inline in blkdev.h so I can put my integrity gap helpers there too.

Anyway, its just cosmetics...

Let me include your patch in a series I'm planning on sending soon enough as I don't see merges anymore so I guess the issue is gone now.

commit 48679ce5f6ffe6827d40287b521ea139cdf95ff7

Author: Sagi Grimberg <sagig at mellanox.com>
Date:   Wed Jul 15 15:06:04 2015 +0300

block: Refuse request/bio merges with gaps in the integrity payload

If a driver sets the block queue virtual boundary, it means that it cannot handle gaps so we must not allow those in the integrity payload as well.

Signed-off-by Sagi Grimberg <sagig at mellanox.com>

block/blk-integrity.c +3 -0

index f548b64..eee1d74 100644
@@ -204,6 +204,9 @@ bool blk_integrity_merge_rq(struct request_queue *q,
 struct request *req,
             q->limits.max_integrity_segments)
                 return false;
 
+       if (integrity_req_gap_to_prev(req, next->bio))
+               return false;
+
         return true;
  }

  EXPORT_SYMBOL(blk_integrity_merge_rq);

block/blk-merge.c +4 -0

index e6b426f..1a3f105 100644
@@ -315,6 +315,10 @@ int ll_back_merge_fn(struct request_queue *q,
 struct request *req,
         if (bio_has_data(bio) && req_gap_to_prev(req, bio))
                 return 0;
 
+       if (blk_integrity_rq(req) &&
+           integrity_req_gap_to_prev(req, bio))
+               return 0;
+
         if (blk_rq_sectors(req) + bio_sectors(bio) >
             blk_rq_get_max_sectors(req)) {

             blk_rq_get_max_sectors(req)) {
                 req->cmd_flags |= REQ_NOMERGE;

block/blk-merge.c +4 -0

@@ -336,6 +340,10 @@ int ll_front_merge_fn(struct request_queue *q,
 struct request *req,
         if (bio_has_data(bio) && req_gap_to_next(req, bio))
                 return 0;
 
+       if (blk_integrity_rq(req) &&
+           integrity_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;

include/linux/blkdev.h +19 -0

index b1bb60a..ad5a21a 100644
@@ -1389,6 +1389,26 @@ static inline bool req_gap_to_next(struct request
 *req, struct bio *bio)
                                 next->bi_io_vec[0].bv_offset);
  }
 
+static inline bool integrity_req_gap_to_prev(struct request *req,
+                                            struct bio *next)
+{
+       struct bio_integrity_payload *bip = bio_integrity(req->bio);
+       struct bio_integrity_payload *bip_next = bio_integrity(next);
+
+       return bvec_gap_to_prev(req->q, &bip->bip_vec[bip->bip_vcnt - 1],
+                               bip_next->bip_vec[0].bv_offset);
+}
+
+static inline bool integrity_req_gap_to_next(struct request *req,
+                                            struct bio *bio)
+{
+       struct bio_integrity_payload *bip = bio_integrity(bio);
+       struct bio_integrity_payload *bip_next = bio_integrity(req->bio);
+
+       return bvec_gap_to_prev(req->q, &bip->bip_vec[bip->bip_vcnt - 1],
+                               bip_next->bip_vec[0].bv_offset);
+}
  struct work_struct;
  int kblockd_schedule_work(struct work_struct *work);
  int kblockd_schedule_delayed_work(struct delayed_work *dwork, unsigned

long delay);

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

"Christoph" == Christoph Hellwig <hch at infradead.org> writes:

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

"Sagi" == Sagi Grimberg <sagig at mellanox.com> writes: