[PATCHv3,07/10] blk-integrity: simplify mapping sg

Commit Message

Keith Busch Sept. 4, 2024, 3:26 p.m. UTC

From: Keith Busch <[e-mail]>

The segments are already packed to the queue limits when adding them to
the bio, so each vector is already its own segment. No need to attempt
compacting them even more.

Signed-off-by: Keith Busch <[e-mail]>
---
 block/blk-integrity.c         | 35 +++++++++--------------------------
 drivers/nvme/host/rdma.c      |  4 ++--
 drivers/scsi/scsi_lib.c       |  3 +--
 include/linux/blk-integrity.h |  6 ++----
 4 files changed, 14 insertions(+), 34 deletions(-)

Comments

Christoph Hellwig Sept. 10, 2024, 3:43 p.m. UTC | #1

On Wed, Sep 04, 2024 at 08:26:02AM -0700, Keith Busch wrote:
> From: Keith Busch <[e-mail]>
> 
> The segments are already packed to the queue limits when adding them to
> the bio, so each vector is already its own segment. No need to attempt
> compacting them even more.

Well.  For one the immutable bio API is explicitly designed so that
the callers don't need to know the limits, so this isn't really
true.

And the other thing the map_sg helpers for data and metadata do is
cross-bio merges, that is if the end of one bio is contiguous with
the start of the next, it will merge the segments.  I don't really
know how useful that is there days - maybe we can removed it with
a proper rational after a bit of testing.

Again the current code closely mirrors the code for mapping the data
bvecs, and I'd preferably keep them as close as possible.

Patch

diff --git a/block/blk-integrity.c b/block/blk-integrity.c
index c180141b7871c..cfb394eff35c8 100644
--- a/block/blk-integrity.c
+++ b/block/blk-integrity.c
@@ -35,7 +35,6 @@  int blk_rq_count_integrity_segs(struct bio *bio)
 
 /**
  * blk_rq_map_integrity_sg - Map integrity metadata into a scatterlist
- * @q:		request queue
  * @bio:	bio with integrity metadata attached
  * @sglist:	target scatterlist
  *
@@ -43,39 +42,23 @@  int blk_rq_count_integrity_segs(struct bio *bio)
  * scatterlist.  The scatterlist must be big enough to hold all
  * elements.  I.e. sized using blk_rq_count_integrity_segs().
  */
-int blk_rq_map_integrity_sg(struct request_queue *q, struct bio *bio,
-			    struct scatterlist *sglist)
+int blk_rq_map_integrity_sg(struct bio *bio, struct scatterlist *sglist)
 {
-	struct bio_vec iv, ivprv = { NULL };
 	struct scatterlist *sg = NULL;
 	unsigned int segments = 0;
 	struct bvec_iter iter;
-	int prev = 0;
+	struct bio_vec iv;
 
 	bio_for_each_integrity_vec(iv, bio, iter) {
-
-		if (prev) {
-			if (!biovec_phys_mergeable(q, &ivprv, &iv))
-				goto new_segment;
-			if (sg->length + iv.bv_len > queue_max_segment_size(q))
-				goto new_segment;
-
-			sg->length += iv.bv_len;
-		} else {
-new_segment:
-			if (!sg)
-				sg = sglist;
-			else {
-				sg_unmark_end(sg);
-				sg = sg_next(sg);
-			}
-
-			sg_set_page(sg, iv.bv_page, iv.bv_len, iv.bv_offset);
-			segments++;
+		if (!sg)
+			sg = sglist;
+		else {
+			sg_unmark_end(sg);
+			sg = sg_next(sg);
 		}
 
-		prev = 1;
-		ivprv = iv;
+		sg_set_page(sg, iv.bv_page, iv.bv_len, iv.bv_offset);
+		segments++;
 	}
 
 	if (sg)
diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index dc0987d42c6b2..fab205bb4f3ed 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -1504,8 +1504,8 @@  static int nvme_rdma_dma_map_req(struct ib_device *ibdev, struct request *rq,
 			goto out_unmap_sg;
 		}
 
-		req->metadata_sgl->nents = blk_rq_map_integrity_sg(rq->q,
-				rq->bio, req->metadata_sgl->sg_table.sgl);
+		req->metadata_sgl->nents = blk_rq_map_integrity_sg(rq->bio,
+					req->metadata_sgl->sg_table.sgl);
 		*pi_count = ib_dma_map_sg(ibdev,
 					  req->metadata_sgl->sg_table.sgl,
 					  req->metadata_sgl->nents,
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index dc1a1644cbc0c..33a7d07dcbe26 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1183,8 +1183,7 @@  blk_status_t scsi_alloc_sgtables(struct scsi_cmnd *cmd)
 			goto out_free_sgtables;
 		}
 
-		count = blk_rq_map_integrity_sg(rq->q, rq->bio,
-						prot_sdb->table.sgl);
+		count = blk_rq_map_integrity_sg(rq->bio, prot_sdb->table.sgl);
 		BUG_ON(count > ivecs);
 		BUG_ON(count > queue_max_integrity_segments(rq->q));
 
diff --git a/include/linux/blk-integrity.h b/include/linux/blk-integrity.h
index 0de05278ac824..38b43d6c797df 100644
--- a/include/linux/blk-integrity.h
+++ b/include/linux/blk-integrity.h
@@ -25,8 +25,7 @@  static inline bool queue_limits_stack_integrity_bdev(struct queue_limits *t,
 }
 
 #ifdef CONFIG_BLK_DEV_INTEGRITY
-int blk_rq_map_integrity_sg(struct request_queue *, struct bio *,
-				   struct scatterlist *);
+int blk_rq_map_integrity_sg(struct bio *, struct scatterlist *);
 int blk_rq_count_integrity_segs(struct bio *);
 
 static inline bool
@@ -95,8 +94,7 @@  static inline int blk_rq_count_integrity_segs(struct bio *b)
 {
 	return 0;
 }
-static inline int blk_rq_map_integrity_sg(struct request_queue *q,
-					  struct bio *b,
+static inline int blk_rq_map_integrity_sg(struct bio *b,
 					  struct scatterlist *s)
 {
 	return 0;