[PATCHv4 00/10] block integrity merging and counting

[PATCHv4 00/10] block integrity merging and counting

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

Some fixes and cleanups to counting integrity segments when metadata is
used. This addresses merging issues when integrity data is present.

Changes from v3:

  Unconditionally define nr_integrity_segments. (Christoph)

  Dropped the "simplify" patches and the queue limits checks; they are
  not correct under some conditions. (Christoph)

  Fix a commit message typo. (Anuj)

  Reordered the driver patches to appear after the integrity segment
  count is accurate.

  Added a patch (5/9) that accounts for user mapped integrity segments.

  Added reviews.

Keith Busch (10):
  blk-mq: unconditional nr_integrity_segments
  blk-mq: set the nr_integrity_segments from bio
  blk-integrity: properly account for segments
  blk-integrity: consider entire bio list for merging
  block: provide a request helper for user integrity segments
  block: provide helper for nr_integrity_segments
  scsi: use request helper to get integrity segments
  nvme-rdma: use request helper to get integrity segments
  block: unexport blk_rq_count_integrity_sg
  blk-integrity: improved sg segment mapping

 block/bio-integrity.c         |  1 -
 block/blk-integrity.c         | 36 ++++++++++++++++++++++++-----------
 block/blk-merge.c             |  4 ++++
 block/blk-mq.c                |  5 +++--
 drivers/nvme/host/ioctl.c     |  6 ++----
 drivers/nvme/host/rdma.c      |  6 +++---
 drivers/scsi/scsi_lib.c       | 12 +++---------
 include/linux/blk-integrity.h | 13 +++++++++----
 include/linux/blk-mq.h        |  8 +++++---
 9 files changed, 54 insertions(+), 37 deletions(-)

-- 
2.43.5

[PATCHv4 01/10] blk-mq: unconditional nr_integrity_segments

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

Always defining the field will make using it easier and less error prone
in future patches.

There shouldn't be any downside to this: the field fits in what would
otherwise be a 2-byte hole, so we're not saving space by conditionally
leaving it out.

Suggested-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 block/blk-mq.c         | 2 --
 include/linux/blk-mq.h | 3 ---
 2 files changed, 5 deletions(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 3f1f7d0b3ff35..ef3a2ed499563 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -376,9 +376,7 @@ static struct request *blk_mq_rq_ctx_init(struct blk_mq_alloc_data *data,
 	rq->io_start_time_ns = 0;
 	rq->stats_sectors = 0;
 	rq->nr_phys_segments = 0;
-#if defined(CONFIG_BLK_DEV_INTEGRITY)
 	rq->nr_integrity_segments = 0;
-#endif
 	rq->end_io = NULL;
 	rq->end_io_data = NULL;
 
diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
index 8d304b1d16b15..4fecf46ef681b 100644
--- a/include/linux/blk-mq.h
+++ b/include/linux/blk-mq.h
@@ -149,10 +149,7 @@ struct request {
 	 * physical address coalescing is performed.
 	 */
 	unsigned short nr_phys_segments;
-
-#ifdef CONFIG_BLK_DEV_INTEGRITY
 	unsigned short nr_integrity_segments;
-#endif
 
 #ifdef CONFIG_BLK_INLINE_ENCRYPTION
 	struct bio_crypt_ctx *crypt_ctx;
-- 
2.43.5

[PATCHv4 02/10] blk-mq: set the nr_integrity_segments from bio

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

This value is used for merging considerations, so it needs to be
accurate.

Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Keith Busch <kbusch@kernel.org>
---
v3->v4:

  Removed the "#if defined" condition.

  Replaced the 'bi_opf & REQ_INTEGRITY' check with bio_integrity(). If
  CONFIG_BLK_DEV_INTEGRITY is not set, the stub function will return
  NULL inline, so the compiler will optimize the setting without
  adding runtime overhead.

 block/blk-mq.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index ef3a2ed499563..82219f0e9a256 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -2544,6 +2544,9 @@ static void blk_mq_bio_to_request(struct request *rq, struct bio *bio,
 	rq->__sector = bio->bi_iter.bi_sector;
 	rq->write_hint = bio->bi_write_hint;
 	blk_rq_bio_prep(rq, bio, nr_segs);
+	if (bio_integrity(bio))
+		rq->nr_integrity_segments = blk_rq_count_integrity_sg(rq->q,
+								      bio);
 
 	/* This can't fail, since GFP_NOIO includes __GFP_DIRECT_RECLAIM. */
 	err = blk_crypto_rq_bio_prep(rq, bio, GFP_NOIO);
-- 
2.43.5

[PATCHv4 03/10] blk-integrity: properly account for segments

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

Both types of merging when integrity data is used are miscounting the
segments:

Merging two requests wasn't accounting for the new segment count, so add
the "next" segment count to the first on a successful merge to ensure
this value is accurate.

Merging a bio into an existing request was double counting the bio's
segments, even if the merge failed later on. Move the segment accounting
to the end when the merge is successful.

Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 block/blk-integrity.c | 2 --
 block/blk-merge.c     | 4 ++++
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/block/blk-integrity.c b/block/blk-integrity.c
index 010decc892eaa..afd101555d3cb 100644
--- a/block/blk-integrity.c
+++ b/block/blk-integrity.c
@@ -153,8 +153,6 @@ bool blk_integrity_merge_bio(struct request_queue *q, struct request *req,
 	    q->limits.max_integrity_segments)
 		return false;
 
-	req->nr_integrity_segments += nr_integrity_segs;
-
 	return true;
 }
 
diff --git a/block/blk-merge.c b/block/blk-merge.c
index 56769c4bcd799..ad763ec313b6a 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -639,6 +639,9 @@ static inline int ll_new_hw_segment(struct request *req, struct bio *bio,
 	 * counters.
 	 */
 	req->nr_phys_segments += nr_phys_segs;
+	if (bio_integrity(bio))
+		req->nr_integrity_segments += blk_rq_count_integrity_sg(req->q,
+									bio);
 	return 1;
 
 no_merge:
@@ -731,6 +734,7 @@ static int ll_merge_requests_fn(struct request_queue *q, struct request *req,
 
 	/* Merge is OK... */
 	req->nr_phys_segments = total_phys_segments;
+	req->nr_integrity_segments += next->nr_integrity_segments;
 	return 1;
 }
 
-- 
2.43.5

[PATCHv4 04/10] blk-integrity: consider entire bio list for merging

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

If a bio is merged to a request, the entire bio list is merged, so don't
temporarily detach it from its list when counting segments. In most
cases, bi_next will already be NULL, so detaching is usually a no-op.
But if the bio does have a list, the current code is miscounting the
segments for the resulting merge.

Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 block/blk-integrity.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/block/blk-integrity.c b/block/blk-integrity.c
index afd101555d3cb..84065691aaed0 100644
--- a/block/blk-integrity.c
+++ b/block/blk-integrity.c
@@ -134,7 +134,6 @@ bool blk_integrity_merge_bio(struct request_queue *q, struct request *req,
 			     struct bio *bio)
 {
 	int nr_integrity_segs;
-	struct bio *next = bio->bi_next;
 
 	if (blk_integrity_rq(req) == 0 && bio_integrity(bio) == NULL)
 		return true;
@@ -145,10 +144,7 @@ bool blk_integrity_merge_bio(struct request_queue *q, struct request *req,
 	if (bio_integrity(req->bio)->bip_flags != bio_integrity(bio)->bip_flags)
 		return false;
 
-	bio->bi_next = NULL;
 	nr_integrity_segs = blk_rq_count_integrity_sg(q, bio);
-	bio->bi_next = next;
-
 	if (req->nr_integrity_segments + nr_integrity_segs >
 	    q->limits.max_integrity_segments)
 		return false;
-- 
2.43.5

[PATCHv4 05/10] block: provide a request helper for user integrity segments

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

Provide a helper keep the request flags and nr_integrity_segments in
sync with the bio's integrity payload. This is an integrity equivalent
to the normal data helper function, 'blk_rq_map_user()'.

Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 block/bio-integrity.c         |  1 -
 block/blk-integrity.c         | 14 ++++++++++++++
 drivers/nvme/host/ioctl.c     |  6 ++----
 include/linux/blk-integrity.h |  7 +++++++
 4 files changed, 23 insertions(+), 5 deletions(-)

diff --git a/block/bio-integrity.c b/block/bio-integrity.c
index 8d1fb38f745f9..357a022eed411 100644
--- a/block/bio-integrity.c
+++ b/block/bio-integrity.c
@@ -371,7 +371,6 @@ int bio_integrity_map_user(struct bio *bio, void __user *ubuf, ssize_t bytes,
 		kfree(bvec);
 	return ret;
 }
-EXPORT_SYMBOL_GPL(bio_integrity_map_user);
 
 /**
  * bio_integrity_prep - Prepare bio for integrity I/O
diff --git a/block/blk-integrity.c b/block/blk-integrity.c
index 84065691aaed0..ddc742d58330b 100644
--- a/block/blk-integrity.c
+++ b/block/blk-integrity.c
@@ -107,6 +107,20 @@ int blk_rq_map_integrity_sg(struct request_queue *q, struct bio *bio,
 }
 EXPORT_SYMBOL(blk_rq_map_integrity_sg);
 
+int blk_rq_integrity_map_user(struct request *rq, void __user *ubuf,
+			      ssize_t bytes, u32 seed)
+{
+	int ret = bio_integrity_map_user(rq->bio, ubuf, bytes, seed);
+
+	if (ret)
+		return ret;
+
+	rq->nr_integrity_segments = blk_rq_count_integrity_sg(rq->q, rq->bio);
+	rq->cmd_flags |= REQ_INTEGRITY;
+	return 0;
+}
+EXPORT_SYMBOL_GPL(blk_rq_integrity_map_user);
+
 bool blk_integrity_merge_rq(struct request_queue *q, struct request *req,
 			    struct request *next)
 {
diff --git a/drivers/nvme/host/ioctl.c b/drivers/nvme/host/ioctl.c
index 1d769c842fbf5..b9b79ccfabf8a 100644
--- a/drivers/nvme/host/ioctl.c
+++ b/drivers/nvme/host/ioctl.c
@@ -3,7 +3,6 @@
  * Copyright (c) 2011-2014, Intel Corporation.
  * Copyright (c) 2017-2021 Christoph Hellwig.
  */
-#include <linux/bio-integrity.h>
 #include <linux/blk-integrity.h>
 #include <linux/ptrace.h>	/* for force_successful_syscall_return */
 #include <linux/nvme_ioctl.h>
@@ -153,11 +152,10 @@ static int nvme_map_user_request(struct request *req, u64 ubuffer,
 		bio_set_dev(bio, bdev);
 
 	if (has_metadata) {
-		ret = bio_integrity_map_user(bio, meta_buffer, meta_len,
-					     meta_seed);
+		ret = blk_rq_integrity_map_user(req, meta_buffer, meta_len,
+						meta_seed);
 		if (ret)
 			goto out_unmap;
-		req->cmd_flags |= REQ_INTEGRITY;
 	}
 
 	return ret;
diff --git a/include/linux/blk-integrity.h b/include/linux/blk-integrity.h
index de98049b7ded9..9c7029aa9c22a 100644
--- a/include/linux/blk-integrity.h
+++ b/include/linux/blk-integrity.h
@@ -28,6 +28,8 @@ static inline bool queue_limits_stack_integrity_bdev(struct queue_limits *t,
 int blk_rq_map_integrity_sg(struct request_queue *, struct bio *,
 				   struct scatterlist *);
 int blk_rq_count_integrity_sg(struct request_queue *, struct bio *);
+int blk_rq_integrity_map_user(struct request *rq, void __user *ubuf,
+			      ssize_t bytes, u32 seed);
 
 static inline bool
 blk_integrity_queue_supports_integrity(struct request_queue *q)
@@ -102,6 +104,11 @@ static inline int blk_rq_map_integrity_sg(struct request_queue *q,
 {
 	return 0;
 }
+static inline int blk_rq_integrity_map_user(struct request *rq, void __user *ubuf,
+			      ssize_t bytes, u32 seed)
+{
+	return -EINVAL;
+}
 static inline struct blk_integrity *bdev_get_integrity(struct block_device *b)
 {
 	return NULL;
-- 
2.43.5

[PATCHv4 06/10] block: provide helper for nr_integrity_segments

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

Provide an integrity equivalent to blk_rq_nr_phys_segments().

Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 include/linux/blk-mq.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
index 4fecf46ef681b..33557af495100 100644
--- a/include/linux/blk-mq.h
+++ b/include/linux/blk-mq.h
@@ -1156,6 +1156,11 @@ static inline unsigned short blk_rq_nr_phys_segments(struct request *rq)
 	return rq->nr_phys_segments;
 }
 
+static inline unsigned short blk_rq_nr_integrity_segments(struct request *rq)
+{
+	return rq->nr_integrity_segments;
+}
+
 /*
  * Number of discard segments (or ranges) the driver needs to fill in.
  * Each discard bio merged into a request is counted as one segment.
-- 
2.43.5

[PATCHv4 07/10] scsi: use request helper to get integrity segments

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

The request tracks the integrity segments already, so no need to recount
the segements again.

Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 drivers/scsi/scsi_lib.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 3958a6d14bf45..fa59b54a8f4c6 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1175,8 +1175,7 @@ blk_status_t scsi_alloc_sgtables(struct scsi_cmnd *cmd)
 			goto out_free_sgtables;
 		}
 
-		ivecs = blk_rq_count_integrity_sg(rq->q, rq->bio);
-
+		ivecs = blk_rq_nr_integrity_segments(rq);
 		if (sg_alloc_table_chained(&prot_sdb->table, ivecs,
 				prot_sdb->table.sgl,
 				SCSI_INLINE_PROT_SG_CNT)) {
-- 
2.43.5

[PATCHv4 08/10] nvme-rdma: use request helper to get integrity segments

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

The request tracks the integrity segments already, so no need to
recount the segements again.

Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 drivers/nvme/host/rdma.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index 15b5e06039a5f..537844ee906b3 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -1496,7 +1496,7 @@ static int nvme_rdma_dma_map_req(struct ib_device *ibdev, struct request *rq,
 		req->metadata_sgl->sg_table.sgl =
 			(struct scatterlist *)(req->metadata_sgl + 1);
 		ret = sg_alloc_table_chained(&req->metadata_sgl->sg_table,
-				blk_rq_count_integrity_sg(rq->q, rq->bio),
+				blk_rq_nr_integrity_segments(rq),
 				req->metadata_sgl->sg_table.sgl,
 				NVME_INLINE_METADATA_SG_CNT);
 		if (unlikely(ret)) {
-- 
2.43.5

[PATCHv4 09/10] block: unexport blk_rq_count_integrity_sg

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

There are no external users of this.

Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 block/blk-integrity.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/block/blk-integrity.c b/block/blk-integrity.c
index ddc742d58330b..1d82b18e06f8e 100644
--- a/block/blk-integrity.c
+++ b/block/blk-integrity.c
@@ -53,7 +53,6 @@ int blk_rq_count_integrity_sg(struct request_queue *q, struct bio *bio)
 
 	return segments;
 }
-EXPORT_SYMBOL(blk_rq_count_integrity_sg);
 
 /**
  * blk_rq_map_integrity_sg - Map integrity metadata into a scatterlist
-- 
2.43.5

[PATCHv4 10/10] blk-integrity: improved sg segment mapping

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

Make the integrity mapping more like data mapping, blk_rq_map_sg. Use
the request to validate the segment count, and update the callers so
they don't have to.

Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 block/blk-integrity.c         | 15 +++++++++++----
 drivers/nvme/host/rdma.c      |  4 ++--
 drivers/scsi/scsi_lib.c       | 11 +++--------
 include/linux/blk-integrity.h |  6 ++----
 4 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/block/blk-integrity.c b/block/blk-integrity.c
index 1d82b18e06f8e..549480aa2a069 100644
--- a/block/blk-integrity.c
+++ b/block/blk-integrity.c
@@ -62,19 +62,20 @@ int blk_rq_count_integrity_sg(struct request_queue *q, struct bio *bio)
  *
  * Description: Map the integrity vectors in request into a
  * scatterlist.  The scatterlist must be big enough to hold all
- * elements.  I.e. sized using blk_rq_count_integrity_sg().
+ * elements.  I.e. sized using blk_rq_count_integrity_sg() or
+ * rq->nr_integrity_segments.
  */
-int blk_rq_map_integrity_sg(struct request_queue *q, struct bio *bio,
-			    struct scatterlist *sglist)
+int blk_rq_map_integrity_sg(struct request *rq, struct scatterlist *sglist)
 {
 	struct bio_vec iv, ivprv = { NULL };
+	struct request_queue *q = rq->q;
 	struct scatterlist *sg = NULL;
+	struct bio *bio = rq->bio;
 	unsigned int segments = 0;
 	struct bvec_iter iter;
 	int prev = 0;
 
 	bio_for_each_integrity_vec(iv, bio, iter) {
-
 		if (prev) {
 			if (!biovec_phys_mergeable(q, &ivprv, &iv))
 				goto new_segment;
@@ -102,6 +103,12 @@ int blk_rq_map_integrity_sg(struct request_queue *q, struct bio *bio,
 	if (sg)
 		sg_mark_end(sg);
 
+	/*
+	 * Something must have been wrong if the figured number of segment
+	 * is bigger than number of req's physical integrity segments
+	 */
+	BUG_ON(segments > blk_rq_nr_phys_segments(rq));
+	BUG_ON(segments > queue_max_integrity_segments(q));
 	return segments;
 }
 EXPORT_SYMBOL(blk_rq_map_integrity_sg);
diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index 537844ee906b3..0d6d8431208a5 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,
+				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 fa59b54a8f4c6..16e97925606b6 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1163,7 +1163,6 @@ blk_status_t scsi_alloc_sgtables(struct scsi_cmnd *cmd)
 
 	if (blk_integrity_rq(rq)) {
 		struct scsi_data_buffer *prot_sdb = cmd->prot_sdb;
-		int ivecs;
 
 		if (WARN_ON_ONCE(!prot_sdb)) {
 			/*
@@ -1175,19 +1174,15 @@ blk_status_t scsi_alloc_sgtables(struct scsi_cmnd *cmd)
 			goto out_free_sgtables;
 		}
 
-		ivecs = blk_rq_nr_integrity_segments(rq);
-		if (sg_alloc_table_chained(&prot_sdb->table, ivecs,
+		if (sg_alloc_table_chained(&prot_sdb->table,
+				blk_rq_nr_integrity_segments(rq),
 				prot_sdb->table.sgl,
 				SCSI_INLINE_PROT_SG_CNT)) {
 			ret = BLK_STS_RESOURCE;
 			goto out_free_sgtables;
 		}
 
-		count = blk_rq_map_integrity_sg(rq->q, rq->bio,
-						prot_sdb->table.sgl);
-		BUG_ON(count > ivecs);
-		BUG_ON(count > queue_max_integrity_segments(rq->q));
-
+		count = blk_rq_map_integrity_sg(rq, prot_sdb->table.sgl);
 		cmd->prot_sdb = prot_sdb;
 		cmd->prot_sdb->table.nents = count;
 	}
diff --git a/include/linux/blk-integrity.h b/include/linux/blk-integrity.h
index 9c7029aa9c22a..6a62885a6beab 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 request *, struct scatterlist *);
 int blk_rq_count_integrity_sg(struct request_queue *, struct bio *);
 int blk_rq_integrity_map_user(struct request *rq, void __user *ubuf,
 			      ssize_t bytes, u32 seed);
@@ -98,8 +97,7 @@ static inline int blk_rq_count_integrity_sg(struct request_queue *q,
 {
 	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 request *q,
 					  struct scatterlist *s)
 {
 	return 0;
-- 
2.43.5

Re: [PATCHv4 10/10] blk-integrity: improved sg segment mapping

[Keith Busch]

On Wed, Sep 11, 2024 at 01:12:40PM -0700, Keith Busch wrote:
> @@ -102,6 +103,12 @@ int blk_rq_map_integrity_sg(struct request_queue *q, struct bio *bio,
> +	 */
> +	BUG_ON(segments > blk_rq_nr_phys_segments(rq));

Doh, this was mixed up with the copy from blk_rq_map_sg. It should say:

	BUG_ON(segments > blk_rq_nr_integrity_segments(rq));

Question though, blk_rq_map_sg uses WARN and scsi used BUG for this
check. But if the condition is true, a buffer overrun occured. So BUG,
right?

Re: [PATCHv4 01/10] blk-mq: unconditional nr_integrity_segments

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCHv4 04/10] blk-integrity: consider entire bio list for merging

[Christoph Hellwig]

On Wed, Sep 11, 2024 at 01:12:34PM -0700, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> If a bio is merged to a request, the entire bio list is merged, so don't
> temporarily detach it from its list when counting segments. In most
> cases, bi_next will already be NULL, so detaching is usually a no-op.
> But if the bio does have a list, the current code is miscounting the
> segments for the resulting merge.

As far as I can tell we can never get here with bi_next set.  Rationale:

blk_integrity_merge_bio has two callers: ll_new_hw_segment and
blk_rq_merge_ok.

ll_new_hw_segment is called from ll_back_merge_fn and ll_front_merge_fn.

ll_back_merge_fn is called from blk_rq_append_bio and
bio_attempt_back_merge.

blk_rq_append_bio is always used for a single bio and in fact used
to build bio chains.

bio_attempt_back_merge is called from blk_attempt_bio_merge,
blk_mq_sched_try_merge and blk_zone_write_plug_init_request.

blk_attempt_bio_merge is called from blk_attempt_plug_merge and
blk_bio_list_merge.

blk_attempt_plug_merge is called from blk_mq_attempt_bio_merge,
which always operates on a single bio.

blk_bio_list_merge is called from blk_mq_sched_bio_merge,
kyber_bio_merge where the latter is just an indirect call from
the former, and blk_mq_sched_bio_merge is called from
blk_mq_attempt_bio_merge which was considered above.

blk_mq_sched_try_merge is called from bfq_bio_merge and dd_bio_merge,
both of which are implementation of the ->bio_merge elevator_mq_ops
method called from blk_mq_sched_bio_merge, which was considered above.

blk_zone_write_plug_init_request is called from blk_mq_submit_bio
and always operates on a single bio.

ll_front_merge_fn is called from bio_attempt_front_merge.

bio_attempt_front_merge is called from blk_attempt_bio_merge and
blk_mq_sched_try_merge, both of which were considered above.

blk_rq_merge_ok is called from blk_attempt_bio_merge,
blk_zone_write_plug_init_request and elv_bio_merge_ok.

blk_attempt_bio_merge and blk_zone_write_plug_init_request were
already considered above.

elv_bio_merge_ok is called from bfq_request_merge and dd_request_merge
and elv_merge.  The first two are implementations of the ->request_merge
elevator_mq_ops method called from elv_merge, which is called from
blk_mq_sched_try_merge, which was considered above.

Also it feels like the call in blk_rq_merge_ok is superflous from this.

Re: [PATCHv4 05/10] block: provide a request helper for user integrity segments

[Christoph Hellwig]

On Wed, Sep 11, 2024 at 01:12:35PM -0700, Keith Busch wrote:
> +static inline int blk_rq_integrity_map_user(struct request *rq, void __user *ubuf,
> +			      ssize_t bytes, u32 seed)

Please avoid the overly long line here and maybe use normal two tab
indents for the second line.

Otherwise looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCHv4 06/10] block: provide helper for nr_integrity_segments

[Christoph Hellwig]

On Wed, Sep 11, 2024 at 01:12:36PM -0700, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> Provide an integrity equivalent to blk_rq_nr_phys_segments().

Now that the field is unconditional the helper seems a bit pointless.
blk_rq_nr_phys_segments is mostly need for the special payload magic
for discard.

Re: [PATCHv4 10/10] blk-integrity: improved sg segment mapping

[Christoph Hellwig]

On Wed, Sep 11, 2024 at 05:23:26PM -0600, Keith Busch wrote:
> On Wed, Sep 11, 2024 at 01:12:40PM -0700, Keith Busch wrote:
> > @@ -102,6 +103,12 @@ int blk_rq_map_integrity_sg(struct request_queue *q, struct bio *bio,
> > +	 */
> > +	BUG_ON(segments > blk_rq_nr_phys_segments(rq));
> 
> Doh, this was mixed up with the copy from blk_rq_map_sg. It should say:
> 
> 	BUG_ON(segments > blk_rq_nr_integrity_segments(rq));
> 
> Question though, blk_rq_map_sg uses WARN and scsi used BUG for this
> check. But if the condition is true, a buffer overrun occured. So BUG,
> right?

That would be my preference, unless we manage to add a error return
condition.  Note that Linus seems to be on his weird anti-BUG crusade
again, though.

Re: [PATCHv4 10/10] blk-integrity: improved sg segment mapping

[Christoph Hellwig]

Looks good modulo the BUG thing:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCHv4 01/10] blk-mq: unconditional nr_integrity_segments

[Martin K. Petersen]

Keith,

> Always defining the field will make using it easier and less error prone
> in future patches.

Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCHv4 02/10] blk-mq: set the nr_integrity_segments from bio

[Martin K. Petersen]

Keith,

> This value is used for merging considerations, so it needs to be
> accurate.

Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCHv4 03/10] blk-integrity: properly account for segments

[Martin K. Petersen]

Keith,

> Both types of merging when integrity data is used are miscounting the
> segments:
>
> Merging two requests wasn't accounting for the new segment count, so
> add the "next" segment count to the first on a successful merge to
> ensure this value is accurate.
>
> Merging a bio into an existing request was double counting the bio's
> segments, even if the merge failed later on. Move the segment
> accounting to the end when the merge is successful.

Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCHv4 05/10] block: provide a request helper for user integrity segments

[Martin K. Petersen]

Keith,

> Provide a helper keep the request flags and nr_integrity_segments in
> sync with the bio's integrity payload. This is an integrity equivalent
> to the normal data helper function, 'blk_rq_map_user()'.

Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCHv4 06/10] block: provide helper for nr_integrity_segments

[Martin K. Petersen]

Christoph,

> Now that the field is unconditional the helper seems a bit pointless.

Yeah, maybe. I'm OK either way.

Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCHv4 07/10] scsi: use request helper to get integrity segments

[Martin K. Petersen]

Keith,

> The request tracks the integrity segments already, so no need to recount
> the segements again.

s/segements/segments/

I'm fine with accessing nr_integrity_segments directly or through the
wrapper.

Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCHv4 08/10] nvme-rdma: use request helper to get integrity segments

[Martin K. Petersen]

Keith,

> The request tracks the integrity segments already, so no need to
> recount the segements again.

s/segements/segments/

Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCHv4 09/10] block: unexport blk_rq_count_integrity_sg

[Martin K. Petersen]

Keith,

> There are no external users of this.

Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCHv4 10/10] blk-integrity: improved sg segment mapping

[Martin K. Petersen]

Keith,

> Make the integrity mapping more like data mapping, blk_rq_map_sg. Use
> the request to validate the segment count, and update the callers so
> they don't have to.

Looks OK except for the phys vs. integrity snafu.

It has been a constant source of problems that we haven't been able to
have a common mapping function that works for both data and metadata.
blk_rq_map_sg() and blk_rq_map_integrity_sg() always seem to get out of
sync in peculiar ways.

Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCHv4 10/10] blk-integrity: improved sg segment mapping

[kernel test robot]

Hi Keith,

kernel test robot noticed the following build warnings:

[auto build test WARNING on axboe-block/for-next]
[also build test WARNING on next-20240912]
[cannot apply to mkp-scsi/for-next jejb-scsi/for-next linus/master v6.11-rc7]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Keith-Busch/blk-mq-unconditional-nr_integrity_segments/20240912-041504
base:   https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git for-next
patch link:    https://lore.kernel.org/r/20240911201240.3982856-11-kbusch%40meta.com
patch subject: [PATCHv4 10/10] blk-integrity: improved sg segment mapping
config: openrisc-randconfig-r072-20240913 (https://download.01.org/0day-ci/archive/20240913/202409131138.fuzBKPCG-lkp@intel.com/config)
compiler: or1k-linux-gcc (GCC) 14.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240913/202409131138.fuzBKPCG-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202409131138.fuzBKPCG-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> block/blk-integrity.c:69: warning: Function parameter or struct member 'rq' not described in 'blk_rq_map_integrity_sg'
>> block/blk-integrity.c:69: warning: Excess function parameter 'q' description in 'blk_rq_map_integrity_sg'
>> block/blk-integrity.c:69: warning: Excess function parameter 'bio' description in 'blk_rq_map_integrity_sg'


vim +69 block/blk-integrity.c

7ba1ba12eeef0a Martin K. Petersen 2008-06-30   56  
7ba1ba12eeef0a Martin K. Petersen 2008-06-30   57  /**
7ba1ba12eeef0a Martin K. Petersen 2008-06-30   58   * blk_rq_map_integrity_sg - Map integrity metadata into a scatterlist
13f05c8d8e98bb Martin K. Petersen 2010-09-10   59   * @q:		request queue
13f05c8d8e98bb Martin K. Petersen 2010-09-10   60   * @bio:	bio with integrity metadata attached
7ba1ba12eeef0a Martin K. Petersen 2008-06-30   61   * @sglist:	target scatterlist
7ba1ba12eeef0a Martin K. Petersen 2008-06-30   62   *
7ba1ba12eeef0a Martin K. Petersen 2008-06-30   63   * Description: Map the integrity vectors in request into a
7ba1ba12eeef0a Martin K. Petersen 2008-06-30   64   * scatterlist.  The scatterlist must be big enough to hold all
19f67fc3c069b6 Keith Busch        2024-09-11   65   * elements.  I.e. sized using blk_rq_count_integrity_sg() or
19f67fc3c069b6 Keith Busch        2024-09-11   66   * rq->nr_integrity_segments.
7ba1ba12eeef0a Martin K. Petersen 2008-06-30   67   */
19f67fc3c069b6 Keith Busch        2024-09-11   68  int blk_rq_map_integrity_sg(struct request *rq, struct scatterlist *sglist)
7ba1ba12eeef0a Martin K. Petersen 2008-06-30  @69  {
d57a5f7c6605f1 Kent Overstreet    2013-11-23   70  	struct bio_vec iv, ivprv = { NULL };
19f67fc3c069b6 Keith Busch        2024-09-11   71  	struct request_queue *q = rq->q;
13f05c8d8e98bb Martin K. Petersen 2010-09-10   72  	struct scatterlist *sg = NULL;
19f67fc3c069b6 Keith Busch        2024-09-11   73  	struct bio *bio = rq->bio;
13f05c8d8e98bb Martin K. Petersen 2010-09-10   74  	unsigned int segments = 0;
d57a5f7c6605f1 Kent Overstreet    2013-11-23   75  	struct bvec_iter iter;
d57a5f7c6605f1 Kent Overstreet    2013-11-23   76  	int prev = 0;
7ba1ba12eeef0a Martin K. Petersen 2008-06-30   77  
d57a5f7c6605f1 Kent Overstreet    2013-11-23   78  	bio_for_each_integrity_vec(iv, bio, iter) {
d57a5f7c6605f1 Kent Overstreet    2013-11-23   79  		if (prev) {
3dccdae54fe836 Christoph Hellwig  2018-09-24   80  			if (!biovec_phys_mergeable(q, &ivprv, &iv))
7ba1ba12eeef0a Martin K. Petersen 2008-06-30   81  				goto new_segment;
d57a5f7c6605f1 Kent Overstreet    2013-11-23   82  			if (sg->length + iv.bv_len > queue_max_segment_size(q))
13f05c8d8e98bb Martin K. Petersen 2010-09-10   83  				goto new_segment;
13f05c8d8e98bb Martin K. Petersen 2010-09-10   84  
d57a5f7c6605f1 Kent Overstreet    2013-11-23   85  			sg->length += iv.bv_len;
7ba1ba12eeef0a Martin K. Petersen 2008-06-30   86  		} else {
7ba1ba12eeef0a Martin K. Petersen 2008-06-30   87  new_segment:
7ba1ba12eeef0a Martin K. Petersen 2008-06-30   88  			if (!sg)
7ba1ba12eeef0a Martin K. Petersen 2008-06-30   89  				sg = sglist;
7ba1ba12eeef0a Martin K. Petersen 2008-06-30   90  			else {
c8164d8931fdee Paolo Bonzini      2013-03-20   91  				sg_unmark_end(sg);
7ba1ba12eeef0a Martin K. Petersen 2008-06-30   92  				sg = sg_next(sg);
7ba1ba12eeef0a Martin K. Petersen 2008-06-30   93  			}
7ba1ba12eeef0a Martin K. Petersen 2008-06-30   94  
d57a5f7c6605f1 Kent Overstreet    2013-11-23   95  			sg_set_page(sg, iv.bv_page, iv.bv_len, iv.bv_offset);
7ba1ba12eeef0a Martin K. Petersen 2008-06-30   96  			segments++;
7ba1ba12eeef0a Martin K. Petersen 2008-06-30   97  		}
7ba1ba12eeef0a Martin K. Petersen 2008-06-30   98  
d57a5f7c6605f1 Kent Overstreet    2013-11-23   99  		prev = 1;
7ba1ba12eeef0a Martin K. Petersen 2008-06-30  100  		ivprv = iv;
7ba1ba12eeef0a Martin K. Petersen 2008-06-30  101  	}
7ba1ba12eeef0a Martin K. Petersen 2008-06-30  102  
7ba1ba12eeef0a Martin K. Petersen 2008-06-30  103  	if (sg)
7ba1ba12eeef0a Martin K. Petersen 2008-06-30  104  		sg_mark_end(sg);
7ba1ba12eeef0a Martin K. Petersen 2008-06-30  105  
19f67fc3c069b6 Keith Busch        2024-09-11  106  	/*
19f67fc3c069b6 Keith Busch        2024-09-11  107  	 * Something must have been wrong if the figured number of segment
19f67fc3c069b6 Keith Busch        2024-09-11  108  	 * is bigger than number of req's physical integrity segments
19f67fc3c069b6 Keith Busch        2024-09-11  109  	 */
19f67fc3c069b6 Keith Busch        2024-09-11  110  	BUG_ON(segments > blk_rq_nr_phys_segments(rq));
19f67fc3c069b6 Keith Busch        2024-09-11  111  	BUG_ON(segments > queue_max_integrity_segments(q));
7ba1ba12eeef0a Martin K. Petersen 2008-06-30  112  	return segments;
7ba1ba12eeef0a Martin K. Petersen 2008-06-30  113  }
7ba1ba12eeef0a Martin K. Petersen 2008-06-30  114  EXPORT_SYMBOL(blk_rq_map_integrity_sg);
7ba1ba12eeef0a Martin K. Petersen 2008-06-30  115  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCHv4 05/10] block: provide a request helper for user integrity segments

[Kanchan Joshi]

Reviewed-by: Kanchan Joshi <joshi.k@samsung.com>

Re: [PATCHv4 06/10] block: provide helper for nr_integrity_segments

[Kanchan Joshi]

Reviewed-by: Kanchan Joshi <joshi.k@samsung.com>

Re: [PATCHv4 07/10] scsi: use request helper to get integrity segments

[Kanchan Joshi]

Reviewed-by: Kanchan Joshi <joshi.k@samsung.com>

Re: [PATCHv4 08/10] nvme-rdma: use request helper to get integrity segments

[Kanchan Joshi]

Reviewed-by: Kanchan Joshi <joshi.k@samsung.com>