[PATCH v2 00/14] Change the integrity configuration method in block

[PATCH v2 00/14] Change the integrity configuration method in block

[Jinyoung CHOI]

In the case of NVMe, it has an integrity payload consisting of one segment.
So, rather than configuring SG_LIST, it was changed by direct DMA mapping.

The page-merge is not performed for the struct bio_vec when creating 
a integrity payload in block.
As a result, when creating an integrity paylaod beyond one page, each 
struct bio_vec is generated, and its bv_len does not exceed the PAGESIZE.

To solve it, bio_integrity_add_page() should just add to the existing 
bvec, similar to bio_add_page() and friends. 
As the bip configuration changed, the code related to sg_list was
also modified.

(ref: https://lore.kernel.org/linux-nvme/yq18rewbmay.fsf@ca-mkp.ca.oracle.com/T/#t)


Tested like this:

- Format (support pi)
$ sudo nvme format /dev/nvme2n1 --force -n 1 -i 1 -p 0 -m 0 -l 1 -r

- Run FIO
[global]
ioengine=libaio
group_reporting

[job]
bs=512k
iodepth=256
rw=write
numjobs=8
direct=1
runtime=10s
filename=/dev/nvme2n1

- Result
...
[   93.496218] nvme2n1: I/O Cmd(0x1) @ LBA 62464, 1024 blocks, I/O Error (sct 0x2 / sc 0x82) MORE
[   93.496227] protection error, dev nvme2n1, sector 62464 op 0x1:(WRITE) flags 0x18800 phys_seg 3 prio class 2
[   93.538788] nvme2n1: I/O Cmd(0x1) @ LBA 6144, 1024 blocks, I/O Error (sct 0x2 / sc 0x82) MORE
[   93.538798] protection error, dev nvme2n1, sector 6144 op 0x1:(WRITE) flags 0x18800 phys_seg 3 prio class 2
[   93.566231] nvme2n1: I/O Cmd(0x1) @ LBA 124928, 1024 blocks, I/O Error (sct 0x0 / sc 0x4)
[   93.566241] I/O error, dev nvme2n1, sector 124928 op 0x1:(WRITE) flags 0x18800 phys_seg 3 prio class 2
[   93.694147] nvme2n1: I/O Cmd(0x1) @ LBA 64512, 1024 blocks, I/O Error (sct 0x2 / sc 0x82) MORE
[   93.694155] protection error, dev nvme2n1, sector 64512 op 0x1:(WRITE) flags 0x18800 phys_seg 3 prio class 2
[   93.694299] nvme2n1: I/O Cmd(0x1) @ LBA 5120, 1024 blocks, I/O Error (sct 0x2 / sc 0x82) MORE
[   93.694305] protection error, dev nvme2n1, sector 5120 op 0x1:(WRITE) flags 0x18800 phys_seg 3 prio class 2
...


Changes to v1:
- Add tag to fix patches 
- Unification of the page merge process of data and integrity
- Solve the build failure when CONFIG_BLK_DEV_INTEGRITY is disabled


Jinyoung Choi (14):
  block: bio: separation to reuse a part of the function
  block: bio-integrity: modify bio_integrity_add_page()
  block: bio-integiry: cleanup bio_integrity_prep
  block: fix not to apply bip information in blk_rq_bio_prep()
  block: blk-merge: fix to add the number of integrity segments to the
    request twice
  block: blk-merge: fix merging two requests in ll_merge_requests_fn
  block: add helper function to get the number of integrity segments
  scsi: add scsi_alloc_integrity_sgtables() for integrity process
  scsi: change to use blk_rq_nr_integrity_segments() instead of
    blk_rq_count_integrity_sg()
  block: blk-integrity: change how to find the number of integrity of
    bio
  nvme: rdma: change how to find the number of integrity of request
  block: add helper function for iteration of bip's bvec
  block: blk-integrity: change sg-table configuration method for
    integrity
  block: blk-integrity: remove blk_rq_count_integrity_sg()

 block/bio-integrity.c         |  73 +++++++++++++------
 block/bio.c                   |  87 ++++++++++++++--------
 block/blk-integrity.c         | 132 +++++++++-------------------------
 block/blk-merge.c             |  68 ++++++++++++++++--
 block/blk.h                   |  20 ++++++
 drivers/nvme/host/rdma.c      |   2 +-
 drivers/scsi/scsi_lib.c       |  67 ++++++++---------
 include/linux/bio.h           |   4 ++
 include/linux/blk-integrity.h |  10 ++-
 include/linux/blk-mq.h        |   5 ++
 include/linux/bvec.h          |   6 ++
 11 files changed, 284 insertions(+), 190 deletions(-)

-- 
2.34.1

[PATCH v2 01/14] block: bio: separation to reuse a part of the function

[Jinyoung CHOI]

__bio_try_merge_page(), which is called by the general page-add functions
and the function considering the constraints of hw, was separated.

Condition tests for general page-add functions were performed in
bio_try_merge_page(). And when the parameters of __bio_try_merge_page()
were changed, there were fewer functions affected by this.

Cc: Christoph Hellwig <hch@lst.de>
Cc: Martin K. Petersen <martin.petersen@oracle.com>

Signed-off-by: Jinyoung Choi <j-young.choi@samsung.com>
---
 block/bio.c | 49 ++++++++++++++++++++++++++++++-------------------
 1 file changed, 30 insertions(+), 19 deletions(-)

diff --git a/block/bio.c b/block/bio.c
index fd11614bba4d..1be17dea603a 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -926,8 +926,28 @@ static inline bool page_is_mergeable(const struct bio_vec *bv,
 	return (bv->bv_page + bv_end / PAGE_SIZE) == (page + off / PAGE_SIZE);
 }
 
+static bool __bio_try_merge_page(struct bio *bio, struct page *page,
+				 unsigned int len, unsigned int off,
+				 bool *same_page)
+{
+	struct bio_vec *bv = &bio->bi_io_vec[bio->bi_vcnt - 1];
+
+	if (!page_is_mergeable(bv, page, len, off, same_page))
+		return false;
+
+	if (bio->bi_iter.bi_size > UINT_MAX - len) {
+		*same_page = false;
+		return false;
+	}
+
+	bv->bv_len += len;
+	bio->bi_iter.bi_size += len;
+
+	return true;
+}
+
 /**
- * __bio_try_merge_page - try appending data to an existing bvec.
+ * bio_try_merge_page - try appending data to an existing bvec.
  * @bio: destination bio
  * @page: start page to add
  * @len: length of the data to add
@@ -942,26 +962,17 @@ static inline bool page_is_mergeable(const struct bio_vec *bv,
  *
  * Return %true on success or %false on failure.
  */
-static bool __bio_try_merge_page(struct bio *bio, struct page *page,
-		unsigned int len, unsigned int off, bool *same_page)
+static bool bio_try_merge_page(struct bio *bio, struct page *page,
+			       unsigned int len, unsigned int off,
+			       bool *same_page)
 {
 	if (WARN_ON_ONCE(bio_flagged(bio, BIO_CLONED)))
 		return false;
 
-	if (bio->bi_vcnt > 0) {
-		struct bio_vec *bv = &bio->bi_io_vec[bio->bi_vcnt - 1];
-
-		if (page_is_mergeable(bv, page, len, off, same_page)) {
-			if (bio->bi_iter.bi_size > UINT_MAX - len) {
-				*same_page = false;
-				return false;
-			}
-			bv->bv_len += len;
-			bio->bi_iter.bi_size += len;
-			return true;
-		}
-	}
-	return false;
+	if (!bio->bi_vcnt)
+		return false;
+
+	return __bio_try_merge_page(bio, page, len, off, same_page);
 }
 
 /*
@@ -1129,7 +1140,7 @@ int bio_add_page(struct bio *bio, struct page *page,
 {
 	bool same_page = false;
 
-	if (!__bio_try_merge_page(bio, page, len, offset, &same_page)) {
+	if (!bio_try_merge_page(bio, page, len, offset, &same_page)) {
 		if (bio_full(bio, len))
 			return 0;
 		__bio_add_page(bio, page, len, offset);
@@ -1199,7 +1210,7 @@ static int bio_iov_add_page(struct bio *bio, struct page *page,
 {
 	bool same_page = false;
 
-	if (!__bio_try_merge_page(bio, page, len, offset, &same_page)) {
+	if (!bio_try_merge_page(bio, page, len, offset, &same_page)) {
 		__bio_add_page(bio, page, len, offset);
 		return 0;
 	}
-- 
2.34.1

[PATCH v2 13/14] block: blk-integrity: change sg-table configuration method for integrity

[Jinyoung CHOI]

Previously, a bio_vec of bip was made of one page in the block layer, and
sg_list was generated using hw information in lld.

This is done in the block layer and the bio_vec has been changed to
multi-page, so it is changed to configure the sg-table using the existing
api, such as the sg-table setting for the bio. (e.g. multi-page map)

Cc: Christoph Hellwig <hch@lst.de>
Cc: Martin K. Petersen <martin.petersen@oracle.com>

Signed-off-by: Jinyoung Choi <j-young.choi@samsung.com>
---
 block/blk-integrity.c | 52 -----------------------------------------
 block/blk-merge.c     | 54 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 54 insertions(+), 52 deletions(-)

diff --git a/block/blk-integrity.c b/block/blk-integrity.c
index 64407b412947..c50954652177 100644
--- a/block/blk-integrity.c
+++ b/block/blk-integrity.c
@@ -55,58 +55,6 @@ int blk_rq_count_integrity_sg(struct request_queue *q, struct bio *bio)
 }
 EXPORT_SYMBOL(blk_rq_count_integrity_sg);
 
-/**
- * blk_rq_map_integrity_sg - Map integrity metadata into a scatterlist
- * @q:		request queue
- * @bio:	bio with integrity metadata attached
- * @sglist:	target scatterlist
- *
- * 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().
- */
-int blk_rq_map_integrity_sg(struct request_queue *q, 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;
-
-	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++;
-		}
-
-		prev = 1;
-		ivprv = iv;
-	}
-
-	if (sg)
-		sg_mark_end(sg);
-
-	return segments;
-}
-EXPORT_SYMBOL(blk_rq_map_integrity_sg);
-
 /**
  * blk_integrity_compare - Compare integrity profile of two disks
  * @gd1:	Disk to compare
diff --git a/block/blk-merge.c b/block/blk-merge.c
index c6a0958e8df1..f17e19ff3a11 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -524,6 +524,60 @@ __blk_segment_map_sg_merge(struct request_queue *q, struct bio_vec *bvec,
 	return true;
 }
 
+#if defined(CONFIG_BLK_DEV_INTEGRITY)
+/**
+ * blk_rq_map_integrity_sg - Map integrity metadata into a scatterlist
+ * @q:		request queue
+ * @bio:	bio with integrity metadata attached
+ * @sglist:	target scatterlist
+ *
+ * Description: Map the integrity vectors in request into a scatterlist.
+ * The scatterlist must be big enough to hold all elements.
+ */
+int blk_rq_map_integrity_sg(struct request_queue *q, struct bio *bio,
+			    struct scatterlist *sglist)
+{
+	struct bio_vec iv, ivprv = { NULL };
+	struct scatterlist *sg = NULL;
+	unsigned int nsegs = 0;
+	struct bvec_iter iter;
+	bool new_bio = false;
+
+	for_each_bio(bio) {
+		struct bio_integrity_payload *bip = bio->bi_integrity;
+
+		bip_for_each_mp_bvec(iv, bip, iter) {
+			/*
+			 * Only try to merge bvecs from two bios given we
+			 * have done bio internal merge when adding pages
+			 * to bio
+			 */
+			if (new_bio &&
+			    __blk_segment_map_sg_merge(q, &iv, &ivprv, &sg))
+				goto next_iv;
+
+			if (iv.bv_offset + iv.bv_len <= PAGE_SIZE)
+				nsegs += __blk_bvec_map_sg(iv, sglist, &sg);
+			else
+				nsegs += blk_bvec_map_sg(q, &iv, sglist, &sg);
+ next_iv:
+			new_bio = false;
+		}
+
+		if (likely(bip->bip_iter.bi_size)) {
+			ivprv = iv;
+			new_bio = true;
+		}
+	}
+
+	if (sg)
+		sg_mark_end(sg);
+
+	return nsegs;
+}
+EXPORT_SYMBOL(blk_rq_map_integrity_sg);
+#endif /* CONFIG_BLK_DEV_INTEGRITY */
+
 static int __blk_bios_map_sg(struct request_queue *q, struct bio *bio,
 			     struct scatterlist *sglist,
 			     struct scatterlist **sg)
-- 
2.34.1

RE:(2) [PATCH v2 12/14] block: add helper function for iteration of bip's bvec

[Jinyoung CHOI]

>On Wed, May 10, 2023 at 05:59:41PM +0900, Jinyoung CHOI wrote:
>> bip_for_each_vec() performs the iteration in a page unit.
>> 
>> Since a bio_vec of bip is composed of multi-page in the block, a macro
>> that can be carried out in multi-page units has been added.
>
>The naming here is a bit confused.  The rest of the block layers uses
>_segment for the per-page iterators and _vec for the one that doesn't
>break up.  I'd suggest we follow this naming scheme here.  And also
>skip the extra for_each_mp_bvec level, just like we don't have that
>intermediate level for bio_for_each_vec

Thank you for letting me know. I was not aware of the rule.
And as you said, I will make one without making the middle level.

Best Regards,
Jinyoung.

[PATCH v2 02/14] block: bio-integrity: modify bio_integrity_add_page()

[Jinyoung CHOI]

Considering the constraints of hardware, the physically continuous pages
were to be composed of one bio_vec.
Previously, bio_vec was created separately for each page entering the
parameter.

The page merge process for data and integrity is almost the same. Thus,
the bio was not used as a parameter, but the values referred to in
the merge process were passed to the parameter. At this time, the
parameter could be more than seven, so the page_merge_ctx structure
was added.

Cc: Christoph Hellwig <hch@lst.de>
Cc: Martin K. Petersen <martin.petersen@oracle.com>

Fixes: 783b94bd9250 ("nvme-pci: do not build a scatterlist to map metadata")
Signed-off-by: Jinyoung Choi <j-young.choi@samsung.com>
---
 block/bio-integrity.c | 66 ++++++++++++++++++++++++++++++++++---------
 block/bio.c           | 56 +++++++++++++++++++++++-------------
 block/blk.h           | 13 +++++++++
 3 files changed, 102 insertions(+), 33 deletions(-)

diff --git a/block/bio-integrity.c b/block/bio-integrity.c
index 4533eb491661..20444ec447cb 100644
--- a/block/bio-integrity.c
+++ b/block/bio-integrity.c
@@ -111,6 +111,23 @@ void bio_integrity_free(struct bio *bio)
 	bio->bi_opf &= ~REQ_INTEGRITY;
 }
 
+/**
+ * bip_full - check if the bip is full
+ * @bip:	bip to check
+ * @len:	length of one segment to be added
+ *
+ * Return true if @bip is full and one segment with @len bytes can't be
+ * added to the bip, otherwise return false
+ */
+static inline bool bip_full(struct bio_integrity_payload *bip, unsigned int len)
+{
+	if (bip->bip_vcnt >= bip->bip_max_vcnt)
+		return true;
+	if (bip->bip_iter.bi_size > UINT_MAX - len)
+		return true;
+	return false;
+}
+
 /**
  * bio_integrity_add_page - Attach integrity metadata
  * @bio:	bio to update
@@ -118,25 +135,53 @@ void bio_integrity_free(struct bio *bio)
  * @len:	number of bytes of integrity metadata in page
  * @offset:	start offset within page
  *
- * Description: Attach a page containing integrity metadata to bio.
+ * Add a page containing integrity metadata to a bio while respecting
+ * the hardware max_sectors, max_segment and gap limitations.
  */
 int bio_integrity_add_page(struct bio *bio, struct page *page,
 			   unsigned int len, unsigned int offset)
 {
+	struct request_queue *q = bdev_get_queue(bio->bi_bdev);
 	struct bio_integrity_payload *bip = bio_integrity(bio);
 
-	if (bip->bip_vcnt >= bip->bip_max_vcnt) {
+	if (((bip->bip_iter.bi_size + len) >> 9) > queue_max_hw_sectors(q))
+		return 0;
+
+	if (bip->bip_vcnt > 0) {
+		struct bio_vec *bv = &bip->bip_vec[bip->bip_vcnt - 1];
+		bool same_page = false;
+		struct page_merge_ctx pmc = {
+			.bv = bv,
+			.bi_vcnt = bip->bip_vcnt,
+			.bi_iter = &bip->bip_iter,
+			.page = page,
+			.len = len,
+			.offset = offset,
+			.same_page = &same_page,
+		};
+
+		if (bio_try_merge_hw_seg(q, &pmc))
+			return len;
+
+		/*
+		 * If the queue doesn't support SG gaps and adding this segment
+		 * would create a gap, disallow it.
+		 */
+		if (bvec_gap_to_prev(&q->limits, bv, offset))
+			return 0;
+	}
+
+	if (bip_full(bip, len)) {
 		printk(KERN_ERR "%s: bip_vec full\n", __func__);
 		return 0;
 	}
 
-	if (bip->bip_vcnt &&
-	    bvec_gap_to_prev(&bdev_get_queue(bio->bi_bdev)->limits,
-			     &bip->bip_vec[bip->bip_vcnt - 1], offset))
+	if (bip->bip_vcnt >= queue_max_integrity_segments(q))
 		return 0;
 
 	bvec_set_page(&bip->bip_vec[bip->bip_vcnt], page, len, offset);
 	bip->bip_vcnt++;
+	bip->bip_iter.bi_size += len;
 
 	return len;
 }
@@ -249,7 +294,6 @@ bool bio_integrity_prep(struct bio *bio)
 	}
 
 	bip->bip_flags |= BIP_BLOCK_INTEGRITY;
-	bip->bip_iter.bi_size = len;
 	bip_set_seed(bip, bio->bi_iter.bi_sector);
 
 	if (bi->flags & BLK_INTEGRITY_IP_CHECKSUM)
@@ -258,7 +302,6 @@ bool bio_integrity_prep(struct bio *bio)
 	/* Map it */
 	offset = offset_in_page(buf);
 	for (i = 0 ; i < nr_pages ; i++) {
-		int ret;
 		bytes = PAGE_SIZE - offset;
 
 		if (len <= 0)
@@ -267,18 +310,13 @@ bool bio_integrity_prep(struct bio *bio)
 		if (bytes > len)
 			bytes = len;
 
-		ret = bio_integrity_add_page(bio, virt_to_page(buf),
-					     bytes, offset);
-
-		if (ret == 0) {
+		if (bio_integrity_add_page(bio, virt_to_page(buf),
+					   bytes, offset) < bytes) {
 			printk(KERN_ERR "could not attach integrity payload\n");
 			status = BLK_STS_RESOURCE;
 			goto err_end_io;
 		}
 
-		if (ret < bytes)
-			break;
-
 		buf += bytes;
 		len -= bytes;
 		offset = 0;
diff --git a/block/bio.c b/block/bio.c
index 1be17dea603a..45af9e39acff 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -926,22 +926,23 @@ static inline bool page_is_mergeable(const struct bio_vec *bv,
 	return (bv->bv_page + bv_end / PAGE_SIZE) == (page + off / PAGE_SIZE);
 }
 
-static bool __bio_try_merge_page(struct bio *bio, struct page *page,
-				 unsigned int len, unsigned int off,
-				 bool *same_page)
+static bool __bio_try_merge_page(struct page_merge_ctx *pmc)
 {
-	struct bio_vec *bv = &bio->bi_io_vec[bio->bi_vcnt - 1];
+	struct bio_vec *bv = pmc->bv;
+	struct bvec_iter *bi_iter = pmc->bi_iter;
+	unsigned int len = pmc->len;
+	bool *same_page = pmc->same_page;
 
-	if (!page_is_mergeable(bv, page, len, off, same_page))
+	if (!page_is_mergeable(bv, pmc->page, len, pmc->offset, same_page))
 		return false;
 
-	if (bio->bi_iter.bi_size > UINT_MAX - len) {
+	if (bi_iter->bi_size > UINT_MAX - len) {
 		*same_page = false;
 		return false;
 	}
 
 	bv->bv_len += len;
-	bio->bi_iter.bi_size += len;
+	bi_iter->bi_size += len;
 
 	return true;
 }
@@ -966,13 +967,23 @@ static bool bio_try_merge_page(struct bio *bio, struct page *page,
 			       unsigned int len, unsigned int off,
 			       bool *same_page)
 {
+	struct page_merge_ctx pmc;
+
 	if (WARN_ON_ONCE(bio_flagged(bio, BIO_CLONED)))
 		return false;
 
 	if (!bio->bi_vcnt)
 		return false;
 
-	return __bio_try_merge_page(bio, page, len, off, same_page);
+	pmc.bv = &bio->bi_io_vec[bio->bi_vcnt - 1];
+	pmc.bi_vcnt = bio->bi_vcnt;
+	pmc.bi_iter = &bio->bi_iter;
+	pmc.page = page;
+	pmc.len = len;
+	pmc.offset = off;
+	pmc.same_page = same_page;
+
+	return __bio_try_merge_page(&pmc);
 }
 
 /*
@@ -980,20 +991,19 @@ static bool bio_try_merge_page(struct bio *bio, struct page *page,
  * size limit.  This is not for normal read/write bios, but for passthrough
  * or Zone Append operations that we can't split.
  */
-static bool bio_try_merge_hw_seg(struct request_queue *q, struct bio *bio,
-				 struct page *page, unsigned len,
-				 unsigned offset, bool *same_page)
+bool bio_try_merge_hw_seg(struct request_queue *q, struct page_merge_ctx *pmc)
 {
-	struct bio_vec *bv = &bio->bi_io_vec[bio->bi_vcnt - 1];
 	unsigned long mask = queue_segment_boundary(q);
+	struct bio_vec *bv = pmc->bv;
+	unsigned int len = pmc->len;
 	phys_addr_t addr1 = page_to_phys(bv->bv_page) + bv->bv_offset;
-	phys_addr_t addr2 = page_to_phys(page) + offset + len - 1;
+	phys_addr_t addr2 = page_to_phys(pmc->page) + pmc->offset + len - 1;
 
 	if ((addr1 | mask) != (addr2 | mask))
 		return false;
 	if (bv->bv_len + len > queue_max_segment_size(q))
 		return false;
-	return __bio_try_merge_page(bio, page, len, offset, same_page);
+	return __bio_try_merge_page(pmc);
 }
 
 /**
@@ -1013,8 +1023,6 @@ int bio_add_hw_page(struct request_queue *q, struct bio *bio,
 		struct page *page, unsigned int len, unsigned int offset,
 		unsigned int max_sectors, bool *same_page)
 {
-	struct bio_vec *bvec;
-
 	if (WARN_ON_ONCE(bio_flagged(bio, BIO_CLONED)))
 		return 0;
 
@@ -1022,15 +1030,25 @@ int bio_add_hw_page(struct request_queue *q, struct bio *bio,
 		return 0;
 
 	if (bio->bi_vcnt > 0) {
-		if (bio_try_merge_hw_seg(q, bio, page, len, offset, same_page))
+		struct bio_vec *bv = &bio->bi_io_vec[bio->bi_vcnt - 1];
+		struct page_merge_ctx pmc = {
+			.bv = bv,
+			.bi_vcnt = bio->bi_vcnt,
+			.bi_iter = &bio->bi_iter,
+			.page = page,
+			.len = len,
+			.offset = offset,
+			.same_page = same_page,
+		};
+
+		if (bio_try_merge_hw_seg(q, &pmc))
 			return len;
 
 		/*
 		 * If the queue doesn't support SG gaps and adding this segment
 		 * would create a gap, disallow it.
 		 */
-		bvec = &bio->bi_io_vec[bio->bi_vcnt - 1];
-		if (bvec_gap_to_prev(&q->limits, bvec, offset))
+		if (bvec_gap_to_prev(&q->limits, bv, offset))
 			return 0;
 	}
 
diff --git a/block/blk.h b/block/blk.h
index 45547bcf1119..dd7cbb57ce43 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -118,6 +118,19 @@ static inline bool bvec_gap_to_prev(const struct queue_limits *lim,
 	return __bvec_gap_to_prev(lim, bprv, offset);
 }
 
+/* page merge context */
+struct page_merge_ctx {
+	struct bio_vec *bv;		/* bvec where @page will be merged */
+	unsigned short bi_vcnt;		/* how many bio_vec's */
+	struct bvec_iter *bi_iter;	/* actual i/o information on device */
+	struct page *page;		/* start page to add */
+	unsigned int len;		/* length of the data to add */
+	unsigned int offset;		/* offset of the data relative to @page */
+	bool *same_page;		/* return if the segment has been merged inside the same page*/
+};
+
+bool bio_try_merge_hw_seg(struct request_queue *q, struct page_merge_ctx *pmc);
+
 static inline bool rq_mergeable(struct request *rq)
 {
 	if (blk_rq_is_passthrough(rq))
-- 
2.34.1

Re: [PATCH v2 02/14] block: bio-integrity: modify bio_integrity_add_page()

[hch]

Hi Jinyoung,

can you work a bit on the commit log and especially the subject line?

I'd word this as something like:

"Subject: bio-integrity: create multi-page bvecs in bio_integrity_add_page()

Allow bio_integrity_add_page to create multi-page bvecs, just like
the bio payloads.  This simplifies adding larger payloads, and fixes
support for non-tiny workloads with nvme, which stopped using scatterlist
for metadata a while ago"

It should probably also mentioned somewhere that you did an audit to
ensure all drivers and the core code is fine with these multi-page
segments.  If it's not, this patch should only be added after that
has been made the case.

I think the extra arguments struct is a bit overcompliated, and mostly
due to me making the existing code to weird things in the low-level
helpers.  With the "rationalize the flow in bio_add_page and friends"
series I just sent out, I think we can drop the previous patch and
simplify this one down to:

diff --git a/block/bio-integrity.c b/block/bio-integrity.c
index 4533eb49166109..85d70dc723f0ed 100644
--- a/block/bio-integrity.c
+++ b/block/bio-integrity.c
@@ -118,26 +118,44 @@ void bio_integrity_free(struct bio *bio)
  * @len:	number of bytes of integrity metadata in page
  * @offset:	start offset within page
  *
- * Description: Attach a page containing integrity metadata to bio.
+ * Add a page containing integrity metadata to a bio while respecting
+ * the hardware max_sectors, max_segment and gap limitations.
  */
 int bio_integrity_add_page(struct bio *bio, struct page *page,
 			   unsigned int len, unsigned int offset)
 {
+	struct request_queue *q = bdev_get_queue(bio->bi_bdev);
 	struct bio_integrity_payload *bip = bio_integrity(bio);
 
-	if (bip->bip_vcnt >= bip->bip_max_vcnt) {
-		printk(KERN_ERR "%s: bip_vec full\n", __func__);
+	if (((bip->bip_iter.bi_size + len) >> SECTOR_SHIFT) >
+	    queue_max_hw_sectors(q))
 		return 0;
-	}
 
-	if (bip->bip_vcnt &&
-	    bvec_gap_to_prev(&bdev_get_queue(bio->bi_bdev)->limits,
-			     &bip->bip_vec[bip->bip_vcnt - 1], offset))
-		return 0;
+	if (bip->bip_vcnt > 0) {
+		struct bio_vec *bv = &bip->bip_vec[bip->bip_vcnt - 1];
+		bool same_page = false;
+
+		if (bvec_try_merge_hw_page(q, bv, page, len, offset,
+				&same_page)) {
+			bip->bip_iter.bi_size += len;
+			return len;
+		}
+
+		if (bip->bip_vcnt >=
+		    min(bip->bip_max_vcnt, queue_max_integrity_segments(q)))
+			return 0;
+
+		/*
+		 * If the queue doesn't support SG gaps and adding this segment
+		 * would create a gap, disallow it.
+		 */
+		if (bvec_gap_to_prev(&q->limits, bv, offset))
+			return 0;
+	}
 
 	bvec_set_page(&bip->bip_vec[bip->bip_vcnt], page, len, offset);
 	bip->bip_vcnt++;
-
+	bip->bip_iter.bi_size += len;
 	return len;
 }
 EXPORT_SYMBOL(bio_integrity_add_page);
@@ -249,7 +267,6 @@ bool bio_integrity_prep(struct bio *bio)
 	}
 
 	bip->bip_flags |= BIP_BLOCK_INTEGRITY;
-	bip->bip_iter.bi_size = len;
 	bip_set_seed(bip, bio->bi_iter.bi_sector);
 
 	if (bi->flags & BLK_INTEGRITY_IP_CHECKSUM)
diff --git a/block/bio.c b/block/bio.c
index 79e8aa600ddbe2..050b57e09ac362 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -934,7 +934,7 @@ static bool bvec_try_merge_page(struct bio_vec *bv, struct page *page,
  * size limit.  This is not for normal read/write bios, but for passthrough
  * or Zone Append operations that we can't split.
  */
-static bool bvec_try_merge_hw_page(struct request_queue *q, struct bio_vec *bv,
+bool bvec_try_merge_hw_page(struct request_queue *q, struct bio_vec *bv,
 		struct page *page, unsigned len, unsigned offset,
 		bool *same_page)
 {
diff --git a/block/blk.h b/block/blk.h
index 45547bcf111938..1e67f738b52191 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -486,4 +486,8 @@ static inline int req_ref_read(struct request *req)
 	return atomic_read(&req->ref);
 }
 
+bool bvec_try_merge_hw_page(struct request_queue *q, struct bio_vec *bv,
+		struct page *page, unsigned len, unsigned offset,
+		bool *same_page);
+
 #endif /* BLK_INTERNAL_H */
-- 
2.39.2

[PATCH v2 04/14] block: fix not to apply bip information in blk_rq_bio_prep()

[Jinyoung CHOI]

When a request is initialized through the bio, bio's integrity
information is not reflected in the request. It seems to be missing
when the codes associated with 'nr_integrity_segment' were added.

the lld such as scsi does not refer to this variable. It uses
integrity's bevc to calculate the number of segments for sg-list. So
there seems to be no problem related to this.

Cc: Christoph Hellwig <hch@lst.de>
Cc: Martin K. Petersen <martin.petersen@oracle.com>

Fixes: 13f05c8d8e98 ("block/scsi: Provide a limit on the number of integrity segments")
Signed-off-by: Jinyoung Choi <j-young.choi@samsung.com>
---
 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 1dacb2c81fdd..9310c94577c7 100644
--- a/include/linux/blk-mq.h
+++ b/include/linux/blk-mq.h
@@ -961,6 +961,11 @@ static inline void blk_rq_bio_prep(struct request *rq, struct bio *bio,
 	rq->__data_len = bio->bi_iter.bi_size;
 	rq->bio = rq->biotail = bio;
 	rq->ioprio = bio_prio(bio);
+
+#if defined(CONFIG_BLK_DEV_INTEGRITY)
+	if (bio_integrity(bio))
+		rq->nr_integrity_segments = bio_integrity(bio)->bip_vcnt;
+#endif
 }
 
 void blk_mq_hctx_set_fq_lock_class(struct blk_mq_hw_ctx *hctx,
-- 
2.34.1

Re: [PATCH v2 04/14] block: fix not to apply bip information in blk_rq_bio_prep()

[hch]

> +#if defined(CONFIG_BLK_DEV_INTEGRITY)

The normal style is to use #ifdef.

> +	if (bio_integrity(bio))
> +		rq->nr_integrity_segments = bio_integrity(bio)->bip_vcnt;
> +#endif
> >

Don't we need to walk the iter here, as it might already have been
advanced?  Although it seems nothing in the integrity code follows
that model right now, so I'm not quite sure how it's even working with
clones at the moment.

[PATCH v2 14/14] block: blk-integrity: remove blk_rq_count_integrity_sg()

[Jinyoung CHOI]

blk_rq_nr_nr_integrity_segments() allows you to obtain the number of
integrity. Therefore, blk_rq_count_integrity_sg() is no longer necessary.

Cc: Christoph Hellwig <hch@lst.de>
Cc: Martin K. Petersen <martin.petersen@oracle.com>

Signed-off-by: Jinyoung Choi <j-young.choi@samsung.com>
---
 block/blk-integrity.c         | 39 -----------------------------------
 include/linux/blk-integrity.h |  1 -
 2 files changed, 40 deletions(-)

diff --git a/block/blk-integrity.c b/block/blk-integrity.c
index c50954652177..9bac2836c3ff 100644
--- a/block/blk-integrity.c
+++ b/block/blk-integrity.c
@@ -16,45 +16,6 @@
 
 #include "blk.h"
 
-/**
- * blk_rq_count_integrity_sg - Count number of integrity scatterlist elements
- * @q:		request queue
- * @bio:	bio with integrity metadata attached
- *
- * Description: Returns the number of elements required in a
- * scatterlist corresponding to the integrity metadata in a bio.
- */
-int blk_rq_count_integrity_sg(struct request_queue *q, struct bio *bio)
-{
-	struct bio_vec iv, ivprv = { NULL };
-	unsigned int segments = 0;
-	unsigned int seg_size = 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;
-			if (seg_size + iv.bv_len > queue_max_segment_size(q))
-				goto new_segment;
-
-			seg_size += iv.bv_len;
-		} else {
-new_segment:
-			segments++;
-			seg_size = iv.bv_len;
-		}
-
-		prev = 1;
-		ivprv = iv;
-	}
-
-	return segments;
-}
-EXPORT_SYMBOL(blk_rq_count_integrity_sg);
-
 /**
  * blk_integrity_compare - Compare integrity profile of two disks
  * @gd1:	Disk to compare
diff --git a/include/linux/blk-integrity.h b/include/linux/blk-integrity.h
index 45b9fde1fee1..a2a9d72e8fab 100644
--- a/include/linux/blk-integrity.h
+++ b/include/linux/blk-integrity.h
@@ -41,7 +41,6 @@ void blk_integrity_unregister(struct gendisk *);
 int blk_integrity_compare(struct gendisk *, struct gendisk *);
 int blk_rq_map_integrity_sg(struct request_queue *, struct bio *,
 				   struct scatterlist *);
-int blk_rq_count_integrity_sg(struct request_queue *, struct bio *);
 
 static inline unsigned short blk_rq_nr_integrity_segments(struct request *rq)
 {
-- 
2.34.1

[PATCH v2 06/14] block: blk-merge: fix merging two requests in ll_merge_requests_fn

[Jinyoung CHOI]

blk_integrity_merge_rq() merges integrity segment information of two
requests. However, it is only a condition check and does not perform the
actual integrity information update. So this was modified.

After it is called, the merge process of the requests may fail
due to other conditions. At this time, there is an error in the integrity
segment information of request. So the call location was also changed.

Cc: Christoph Hellwig <hch@lst.de>
Cc: Martin K. Petersen <martin.petersen@oracle.com>

Fixes: 13f05c8d8e98 ("block/scsi: Provide a limit on the number of integrity segments")
Signed-off-by: Jinyoung Choi <j-young.choi@samsung.com>
---
 block/blk-integrity.c | 2 ++
 block/blk-merge.c     | 5 +++--
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/block/blk-integrity.c b/block/blk-integrity.c
index 03a85e1f6d2e..f97b7e8a6d4d 100644
--- a/block/blk-integrity.c
+++ b/block/blk-integrity.c
@@ -181,6 +181,8 @@ bool blk_integrity_merge_rq(struct request_queue *q, struct request *req,
 	if (integrity_req_gap_back_merge(req, next->bio))
 		return false;
 
+	req->nr_integrity_segments += next->nr_integrity_segments;
+
 	return true;
 }
 
diff --git a/block/blk-merge.c b/block/blk-merge.c
index 8509f468d6d4..c6a0958e8df1 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -711,10 +711,11 @@ static int ll_merge_requests_fn(struct request_queue *q, struct request *req,
 	if (!blk_cgroup_mergeable(req, next->bio))
 		return 0;
 
-	if (blk_integrity_merge_rq(q, req, next) == false)
+	if (!bio_crypt_ctx_merge_rq(req, next))
 		return 0;
 
-	if (!bio_crypt_ctx_merge_rq(req, next))
+	/* this will merge integrity segments */
+	if (!blk_integrity_merge_rq(q, req, next))
 		return 0;
 
 	/* Merge is OK... */
-- 
2.34.1

RE:(2) [PATCH v2 05/14] block: blk-merge: fix to add the number of integrity segments to the request twice

[Jinyoung CHOI]

>The subject looks a bit odd, I think you're trying to say:
>
>"do not add the number of integrity segments to the request twice"
>
>based on the actual patch, is this correct?
>

Yes. I will fix it.

>> +static inline bool blk_integrity_bypass_check(struct request *req,
>> +                                              struct bio *bio)
>> +{
>> +        return blk_integrity_rq(req) == 0 && bio_integrity(bio) == NULL;
>> +}
>
>No need for the explicit comparisms, this could just be:
>
>        return !blk_integrity_rq(req) && !bio_integrity(bio);
>
>and given that it just has two callers I'm not sure the helper is
>all that useful to start with.

There are many conditional sentences like that, so I left them for unity,
If it's okay to change, I'll do so.

>> +static bool __blk_integrity_mergeable(struct request_queue *q,
>> +                                      struct request *req, struct bio *bio)
>> +{
>> +        if (blk_integrity_rq(req) == 0 || bio_integrity(bio) == NULL)
>> +                return false;
>> +
>> +        if (bio_integrity(req->bio)->bip_flags != bio_integrity(bio)->bip_flags)
>> +                return false;
>> +
>> +        return true;
>> +}
>> +
>> +bool blk_integrity_mergeable(struct request_queue *q, struct request *req,
>> +                             struct bio *bio)
>> +{
>> +        if (blk_integrity_bypass_check(req, bio))
>> +                return true;
>> +
>> +        return __blk_integrity_mergeable(q, req, bio);
>> +}
>
>Similarly here, I'm not even sure we need all these helpers.  I supect
>the code would become more readable by dropping these helpers and just
>making the checks explicitlẏ

OK. I will drop this.

Best Regards,
Jinyoung.

[PATCH v2 08/14] scsi: add scsi_alloc_integrity_sgtables() for integrity process

[Jinyoung CHOI]

Separate the integrity mapping process of scsi_alloc_sgtables() into a
new function for readability.

Cc: Christoph Hellwig <hch@lst.de>
Cc: Martin K. Petersen <martin.petersen@oracle.com>

Signed-off-by: Jinyoung Choi <j-young.choi@samsung.com>
---
 drivers/scsi/scsi_lib.c | 71 ++++++++++++++++++++++-------------------
 1 file changed, 38 insertions(+), 33 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index b7c569a42aa4..89cf21345e1a 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1003,6 +1003,40 @@ static inline bool scsi_cmd_needs_dma_drain(struct scsi_device *sdev,
 	       sdev->host->hostt->dma_need_drain(rq);
 }
 
+static blk_status_t scsi_alloc_integrity_sgtables(struct scsi_cmnd *cmd)
+{
+	struct request *rq = scsi_cmd_to_rq(cmd);
+	struct scsi_data_buffer *prot_sdb = cmd->prot_sdb;
+	int count, ivecs;
+
+	if (WARN_ON_ONCE(!prot_sdb)) {
+		/*
+		 * This can happen if someone (e.g. multipath)
+		 * queues a command to a device on an adapter
+		 * that does not support DIX.
+		 */
+		return BLK_STS_IOERR;
+	}
+
+	ivecs = blk_rq_count_integrity_sg(rq->q, rq->bio);
+
+	if (sg_alloc_table_chained(&prot_sdb->table, ivecs,
+				   prot_sdb->table.sgl,
+				   SCSI_INLINE_PROT_SG_CNT)) {
+		return BLK_STS_RESOURCE;
+	}
+
+	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));
+
+	cmd->prot_sdb = prot_sdb;
+	cmd->prot_sdb->table.nents = count;
+
+	return BLK_STS_OK;
+}
+
 /**
  * scsi_alloc_sgtables - Allocate and initialize data and integrity scatterlists
  * @cmd: SCSI command data structure to initialize.
@@ -1021,7 +1055,7 @@ blk_status_t scsi_alloc_sgtables(struct scsi_cmnd *cmd)
 	struct request *rq = scsi_cmd_to_rq(cmd);
 	unsigned short nr_segs = blk_rq_nr_phys_segments(rq);
 	struct scatterlist *last_sg = NULL;
-	blk_status_t ret;
+	blk_status_t ret = BLK_STS_OK;
 	bool need_drain = scsi_cmd_needs_dma_drain(sdev, rq);
 	int count;
 
@@ -1071,40 +1105,11 @@ blk_status_t scsi_alloc_sgtables(struct scsi_cmnd *cmd)
 	cmd->sdb.length = blk_rq_payload_bytes(rq);
 
 	if (blk_integrity_rq(rq)) {
-		struct scsi_data_buffer *prot_sdb = cmd->prot_sdb;
-		int ivecs;
-
-		if (WARN_ON_ONCE(!prot_sdb)) {
-			/*
-			 * This can happen if someone (e.g. multipath)
-			 * queues a command to a device on an adapter
-			 * that does not support DIX.
-			 */
-			ret = BLK_STS_IOERR;
-			goto out_free_sgtables;
-		}
-
-		ivecs = blk_rq_count_integrity_sg(rq->q, rq->bio);
-
-		if (sg_alloc_table_chained(&prot_sdb->table, ivecs,
-				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));
-
-		cmd->prot_sdb = prot_sdb;
-		cmd->prot_sdb->table.nents = count;
+		ret = scsi_alloc_integrity_sgtables(cmd);
+		if (ret != BLK_STS_OK)
+			scsi_free_sgtables(cmd);
 	}
 
-	return BLK_STS_OK;
-out_free_sgtables:
-	scsi_free_sgtables(cmd);
 	return ret;
 }
 EXPORT_SYMBOL(scsi_alloc_sgtables);
-- 
2.34.1

Re: [PATCH v2 08/14] scsi: add scsi_alloc_integrity_sgtables() for integrity process

[hch]

On Wed, May 10, 2023 at 05:56:07PM +0900, Jinyoung CHOI wrote:
> +	if (WARN_ON_ONCE(!prot_sdb)) {
> +		/*
> +		 * This can happen if someone (e.g. multipath)
> +		 * queues a command to a device on an adapter
> +		 * that does not support DIX.
> +		 */
> +		return BLK_STS_IOERR;

Nit: expand the comment to take up all 80 characters now that you've
unindented it.

[PATCH v2 09/14] scsi: change to use blk_rq_nr_integrity_segments() instead of blk_rq_count_integrity_sg()

[Jinyoung CHOI]

In the block layer, the number of segments is recorded in the request.
It is already divided into separate segments based on hw information.
Therefore, there is no need to count again.

Cc: Christoph Hellwig <hch@lst.de>
Cc: Martin K. Petersen <martin.petersen@oracle.com>

Signed-off-by: Jinyoung Choi <j-young.choi@samsung.com>
---
 drivers/scsi/scsi_lib.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 89cf21345e1a..5d67b6f6854e 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1006,8 +1006,9 @@ static inline bool scsi_cmd_needs_dma_drain(struct scsi_device *sdev,
 static blk_status_t scsi_alloc_integrity_sgtables(struct scsi_cmnd *cmd)
 {
 	struct request *rq = scsi_cmd_to_rq(cmd);
+	unsigned short nr_integrity_segs = blk_rq_nr_integrity_segments(rq);
 	struct scsi_data_buffer *prot_sdb = cmd->prot_sdb;
-	int count, ivecs;
+	int count;
 
 	if (WARN_ON_ONCE(!prot_sdb)) {
 		/*
@@ -1018,9 +1019,7 @@ static blk_status_t scsi_alloc_integrity_sgtables(struct scsi_cmnd *cmd)
 		return BLK_STS_IOERR;
 	}
 
-	ivecs = blk_rq_count_integrity_sg(rq->q, rq->bio);
-
-	if (sg_alloc_table_chained(&prot_sdb->table, ivecs,
+	if (sg_alloc_table_chained(&prot_sdb->table, nr_integrity_segs,
 				   prot_sdb->table.sgl,
 				   SCSI_INLINE_PROT_SG_CNT)) {
 		return BLK_STS_RESOURCE;
@@ -1028,10 +1027,7 @@ static blk_status_t scsi_alloc_integrity_sgtables(struct scsi_cmnd *cmd)
 
 	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));
-
-	cmd->prot_sdb = prot_sdb;
+	BUG_ON(count > prot_sdb->table.nents);
 	cmd->prot_sdb->table.nents = count;
 
 	return BLK_STS_OK;
-- 
2.34.1

[PATCH v2 10/14] block: blk-integrity: change how to find the number of integrity of bio

[Jinyoung CHOI]

The method of constructing a bip has been changed, the number of
segments can be obtained through bip_vcnt.

Cc: Christoph Hellwig <hch@lst.de>
Cc: Martin K. Petersen <martin.petersen@oracle.com>

Signed-off-by: Jinyoung Choi <j-young.choi@samsung.com>
---
 block/blk-integrity.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/block/blk-integrity.c b/block/blk-integrity.c
index f97b7e8a6d4d..64407b412947 100644
--- a/block/blk-integrity.c
+++ b/block/blk-integrity.c
@@ -217,7 +217,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_bypass_check(req, bio))
 		return true;
@@ -225,9 +224,7 @@ bool blk_integrity_merge_bio(struct request_queue *q, struct request *req,
 	if (!__blk_integrity_mergeable(q, req, bio))
 		return false;
 
-	bio->bi_next = NULL;
-	nr_integrity_segs = blk_rq_count_integrity_sg(q, bio);
-	bio->bi_next = next;
+	nr_integrity_segs = bio_integrity(bio)->bip_vcnt;
 
 	if (req->nr_integrity_segments + nr_integrity_segs >
 	    q->limits.max_integrity_segments)
-- 
2.34.1

[PATCH v2 12/14] block: add helper function for iteration of bip's bvec

[Jinyoung CHOI]

bip_for_each_vec() performs the iteration in a page unit.

Since a bio_vec of bip is composed of multi-page in the block, a macro
that can be carried out in multi-page units has been added.

Cc: Christoph Hellwig <hch@lst.de>
Cc: Martin K. Petersen <martin.petersen@oracle.com>

Signed-off-by: Jinyoung Choi <j-young.choi@samsung.com>
---
 include/linux/bio.h  | 4 ++++
 include/linux/bvec.h | 6 ++++++
 2 files changed, 10 insertions(+)

diff --git a/include/linux/bio.h b/include/linux/bio.h
index d766be7152e1..8b65463d4a55 100644
--- a/include/linux/bio.h
+++ b/include/linux/bio.h
@@ -692,6 +692,10 @@ static inline bool bioset_initialized(struct bio_set *bs)
 
 #if defined(CONFIG_BLK_DEV_INTEGRITY)
 
+/* iterate over multi-page bvec for integrity */
+#define bip_for_each_mp_bvec(bvl, bip, iter)				\
+	for_each_mp_bvec(bvl, (bip)->bip_vec, iter, (bip)->bip_iter)
+
 #define bip_for_each_vec(bvl, bip, iter)				\
 	for_each_bvec(bvl, (bip)->bip_vec, iter, (bip)->bip_iter)
 
diff --git a/include/linux/bvec.h b/include/linux/bvec.h
index 555aae5448ae..9364c258513e 100644
--- a/include/linux/bvec.h
+++ b/include/linux/bvec.h
@@ -184,6 +184,12 @@ static inline void bvec_iter_advance_single(const struct bio_vec *bv,
 		((bvl = bvec_iter_bvec((bio_vec), (iter))), 1);	\
 	     bvec_iter_advance_single((bio_vec), &(iter), (bvl).bv_len))
 
+#define for_each_mp_bvec(bvl, bio_vec, iter, start)			\
+	for (iter = (start);						\
+	     (iter).bi_size &&						\
+		((bvl = mp_bvec_iter_bvec((bio_vec), (iter))), 1);	\
+	     bvec_iter_advance_single((bio_vec), &(iter), (bvl).bv_len))
+
 /* for iterating one bio from start to end */
 #define BVEC_ITER_ALL_INIT (struct bvec_iter)				\
 {									\
-- 
2.34.1

Re: [PATCH v2 12/14] block: add helper function for iteration of bip's bvec

[hch]

On Wed, May 10, 2023 at 05:59:41PM +0900, Jinyoung CHOI wrote:
> bip_for_each_vec() performs the iteration in a page unit.
> 
> Since a bio_vec of bip is composed of multi-page in the block, a macro
> that can be carried out in multi-page units has been added.

The naming here is a bit confused.  The rest of the block layers uses
_segment for the per-page iterators and _vec for the one that doesn't
break up.  I'd suggest we follow this naming scheme here.  And also
skip the extra for_each_mp_bvec level, just like we don't have that
intermediate level for bio_for_each_vec

RE:(2) [PATCH v2 02/14] block: bio-integrity: modify bio_integrity_add_page()

[Jinyoung CHOI]

>Hi Jinyoung,
>
>can you work a bit on the commit log and especially the subject line?
>
>I'd word this as something like:
>
>"Subject: bio-integrity: create multi-page bvecs in bio_integrity_add_page()
>
>Allow bio_integrity_add_page to create multi-page bvecs, just like
>the bio payloads.  This simplifies adding larger payloads, and fixes
>support for non-tiny workloads with nvme, which stopped using scatterlist
>for metadata a while ago"
>

Hi. Christoph. I checked late.

OK. I will revise it like that.

>It should probably also mentioned somewhere that you did an audit to
>ensure all drivers and the core code is fine with these multi-page
>segments.  If it's not, this patch should only be added after that
>has been made the case.

Regardless of a single-page or a multi-page configuration,
block_rq_count_integrity_sg() and block_rq_map_integry_sg() check
the pages included in the bip to create a segment for sg.
So... there doesn't seem to be a problem yet.

First, the nvme device that supports fips was tested using the above
functions, and it was also checked with the dix of scsi_debug turned on.
I am also trying to test the sas device that supports sed. It seems that
a problem occurs while going through the laid controller (mpt3sas driver).
It works normally in DIF mode, but protection error occurs when DIX mode
is forcibly activated. Of course, the same is happening in the original code.
I will check additionally and be careful not to cause any problems.

>
>I think the extra arguments struct is a bit overcompliated, and mostly
>due to me making the existing code to weird things in the low-level
>helpers.  With the "rationalize the flow in bio_add_page and friends"
>series I just sent out, I think we can drop the previous patch and
>simplify this one down to:

I tried not to make a big change from the existing logic. So I added
struct unnecessarily. I thought it would be a problem if I added 
conditions to the callers and made them call the common code.
Thank you for solving this part.
If your code merges, I will modify it accordingly and proceed.

Best Regards,
Jinyoung.

[PATCH v2 03/14] block: bio-integrity: cleanup bio_integrity_prep

[Jinyoung CHOI]

If a problem occurs in the process of creating an integrity payload, the
status of bio is always BLK_STS_RESOURCE.

Cc: Christoph Hellwig <hch@lst.de>
Cc: Martin K. Petersen <martin.petersen@oracle.com>

Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Jinyoung Choi <j-young.choi@samsung.com>
---
 block/bio-integrity.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/block/bio-integrity.c b/block/bio-integrity.c
index 20444ec447cb..c49deed67bb2 100644
--- a/block/bio-integrity.c
+++ b/block/bio-integrity.c
@@ -245,7 +245,6 @@ bool bio_integrity_prep(struct bio *bio)
 	unsigned int len, nr_pages;
 	unsigned int bytes, offset, i;
 	unsigned int intervals;
-	blk_status_t status;
 
 	if (!bi)
 		return true;
@@ -274,7 +273,6 @@ bool bio_integrity_prep(struct bio *bio)
 	/* Allocate kernel buffer for protection data */
 	len = intervals * bi->tuple_size;
 	buf = kmalloc(len, GFP_NOIO);
-	status = BLK_STS_RESOURCE;
 	if (unlikely(buf == NULL)) {
 		printk(KERN_ERR "could not allocate integrity buffer\n");
 		goto err_end_io;
@@ -289,7 +287,6 @@ bool bio_integrity_prep(struct bio *bio)
 	if (IS_ERR(bip)) {
 		printk(KERN_ERR "could not allocate data integrity bioset\n");
 		kfree(buf);
-		status = BLK_STS_RESOURCE;
 		goto err_end_io;
 	}
 
@@ -313,7 +310,6 @@ bool bio_integrity_prep(struct bio *bio)
 		if (bio_integrity_add_page(bio, virt_to_page(buf),
 					   bytes, offset) < bytes) {
 			printk(KERN_ERR "could not attach integrity payload\n");
-			status = BLK_STS_RESOURCE;
 			goto err_end_io;
 		}
 
@@ -332,10 +328,9 @@ bool bio_integrity_prep(struct bio *bio)
 	return true;
 
 err_end_io:
-	bio->bi_status = status;
+	bio->bi_status = BLK_STS_RESOURCE;
 	bio_endio(bio);
 	return false;
-
 }
 EXPORT_SYMBOL(bio_integrity_prep);
 
-- 
2.34.1

Re: [PATCH v2 03/14] block: bio-integrity: cleanup bio_integrity_prep

[hch]

I find these "block: bio-integrity:" very unreadable.  It seems like
a lot of commits just use block, which looks neater to me.  The same
also applies to many of the other patches.

The patch itself looks good to me:

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

[PATCH v2 05/14] block: blk-merge: fix to add the number of integrity segments to the request twice

[Jinyoung CHOI]

blk_integrity_merge_bio() not only performs conditional tests, but also
updates the integrity segment information of request.
It can be called twice when merging the bio into an existing request.

bio_attempt_bio_merge() or blk_mq_sched_try_merge()
  blk_rq_merge_ok()
    blk_integrity_merge_bio()  -  1
  bio_attemp_{back|front}_merge()
    ll_{back|front}_merge_fn()
      ll_new_hw_segments()
        blk_integrity_merge_bio()  -  2

The part of checking the conditions and the code to update the
information of the actual request were separated. At this time, the
ll_back_merge_fn was called by passth-path, so the condition check was
called by all the separated functions.

And after success in blk_integrity_merge_bio(), the information of the
request may be wrong if it is impossible to merge due to other
conditional tests. Thus, it was changed to be called immediately before
merging the bio's segments.

Cc: Christoph Hellwig <hch@lst.de>
Cc: Martin K. Petersen <martin.petersen@oracle.com>

Fixes: 4eaf99beadce ("block: Don't merge requests if integrity flags differ")
Signed-off-by: Jinyoung Choi <j-young.choi@samsung.com>
---
 block/blk-integrity.c | 34 +++++++++++++++++++++++++++++-----
 block/blk-merge.c     |  9 +++++----
 block/blk.h           |  7 +++++++
 3 files changed, 41 insertions(+), 9 deletions(-)

diff --git a/block/blk-integrity.c b/block/blk-integrity.c
index d4e9b4556d14..03a85e1f6d2e 100644
--- a/block/blk-integrity.c
+++ b/block/blk-integrity.c
@@ -184,19 +184,43 @@ bool blk_integrity_merge_rq(struct request_queue *q, struct request *req,
 	return true;
 }
 
+static inline bool blk_integrity_bypass_check(struct request *req,
+					      struct bio *bio)
+{
+	return blk_integrity_rq(req) == 0 && bio_integrity(bio) == NULL;
+}
+
+static bool __blk_integrity_mergeable(struct request_queue *q,
+				      struct request *req, struct bio *bio)
+{
+	if (blk_integrity_rq(req) == 0 || bio_integrity(bio) == NULL)
+		return false;
+
+	if (bio_integrity(req->bio)->bip_flags != bio_integrity(bio)->bip_flags)
+		return false;
+
+	return true;
+}
+
+bool blk_integrity_mergeable(struct request_queue *q, struct request *req,
+			     struct bio *bio)
+{
+	if (blk_integrity_bypass_check(req, bio))
+		return true;
+
+	return __blk_integrity_mergeable(q, req, bio);
+}
+
 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)
+	if (blk_integrity_bypass_check(req, bio))
 		return true;
 
-	if (blk_integrity_rq(req) == 0 || bio_integrity(bio) == NULL)
-		return false;
-
-	if (bio_integrity(req->bio)->bip_flags != bio_integrity(bio)->bip_flags)
+	if (!__blk_integrity_mergeable(q, req, bio))
 		return false;
 
 	bio->bi_next = NULL;
diff --git a/block/blk-merge.c b/block/blk-merge.c
index 65e75efa9bd3..8509f468d6d4 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -611,9 +611,6 @@ static inline int ll_new_hw_segment(struct request *req, struct bio *bio,
 	if (!blk_cgroup_mergeable(req, bio))
 		goto no_merge;
 
-	if (blk_integrity_merge_bio(req->q, req, bio) == false)
-		goto no_merge;
-
 	/* discard request merge won't add new segment */
 	if (req_op(req) == REQ_OP_DISCARD)
 		return 1;
@@ -621,6 +618,10 @@ static inline int ll_new_hw_segment(struct request *req, struct bio *bio,
 	if (req->nr_phys_segments + nr_phys_segs > blk_rq_get_max_segments(req))
 		goto no_merge;
 
+	/* This will merge integrity segments */
+	if (!blk_integrity_merge_bio(req->q, req, bio))
+		goto no_merge;
+
 	/*
 	 * This will form the start of a new hw segment.  Bump both
 	 * counters.
@@ -934,7 +935,7 @@ bool blk_rq_merge_ok(struct request *rq, struct bio *bio)
 		return false;
 
 	/* only merge integrity protected bio into ditto rq */
-	if (blk_integrity_merge_bio(rq->q, rq, bio) == false)
+	if (!blk_integrity_mergeable(rq->q, rq, bio))
 		return false;
 
 	/* Only merge if the crypt contexts are compatible */
diff --git a/block/blk.h b/block/blk.h
index dd7cbb57ce43..b7677a5bdff1 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -202,6 +202,8 @@ static inline bool bio_integrity_endio(struct bio *bio)
 
 bool blk_integrity_merge_rq(struct request_queue *, struct request *,
 		struct request *);
+bool blk_integrity_mergeable(struct request_queue *rq, struct request *r,
+		struct bio *b);
 bool blk_integrity_merge_bio(struct request_queue *, struct request *,
 		struct bio *);
 
@@ -234,6 +236,11 @@ static inline bool blk_integrity_merge_rq(struct request_queue *rq,
 {
 	return true;
 }
+static inline bool blk_integrity_mergeable(struct request_queue *rq,
+		struct request *r, struct bio *b)
+{
+	return true;
+}
 static inline bool blk_integrity_merge_bio(struct request_queue *rq,
 		struct request *r, struct bio *b)
 {
-- 
2.34.1

Re: [PATCH v2 05/14] block: blk-merge: fix to add the number of integrity segments to the request twice

[hch]

The subject looks a bit odd, I think you're trying to say:

"do not add the number of integrity segments to the request twice"

based on the actual patch, is this correct?

> blk_integrity_merge_bio() not only performs conditional tests, but also
> updates the integrity segment information of request.
> It can be called twice when merging the bio into an existing request.
> 
> bio_attempt_bio_merge() or blk_mq_sched_try_merge()
>   blk_rq_merge_ok()
>     blk_integrity_merge_bio()  -  1
>   bio_attemp_{back|front}_merge()
>     ll_{back|front}_merge_fn()
>       ll_new_hw_segments()
>         blk_integrity_merge_bio()  -  2
> 
> The part of checking the conditions and the code to update the
> information of the actual request were separated. At this time, the
> ll_back_merge_fn was called by passth-path, so the condition check was
> called by all the separated functions.
> 
> And after success in blk_integrity_merge_bio(), the information of the
> request may be wrong if it is impossible to merge due to other
> conditional tests. Thus, it was changed to be called immediately before
> merging the bio's segments.


> +static inline bool blk_integrity_bypass_check(struct request *req,
> +					      struct bio *bio)
> +{
> +	return blk_integrity_rq(req) == 0 && bio_integrity(bio) == NULL;
> +}

No need for the explicit comparisms, this could just be:

	return !blk_integrity_rq(req) && !bio_integrity(bio);

and given that it just has two callers I'm not sure the helper is
all that useful to start with.

> +static bool __blk_integrity_mergeable(struct request_queue *q,
> +				      struct request *req, struct bio *bio)
> +{
> +	if (blk_integrity_rq(req) == 0 || bio_integrity(bio) == NULL)
> +		return false;
> +
> +	if (bio_integrity(req->bio)->bip_flags != bio_integrity(bio)->bip_flags)
> +		return false;
> +
> +	return true;
> +}
> +
> +bool blk_integrity_mergeable(struct request_queue *q, struct request *req,
> +			     struct bio *bio)
> +{
> +	if (blk_integrity_bypass_check(req, bio))
> +		return true;
> +
> +	return __blk_integrity_mergeable(q, req, bio);
> +}

Similarly here, I'm not even sure we need all these helpers.  I supect
the code would become more readable by dropping these helpers and just
making the checks explicitlẏ

[PATCH v2 07/14] block: add helper function to get the number of integrity segments

[Jinyoung CHOI]

Since request always has the number of integrity segments in the process
of generating and merging, a function for simply obtained this has been
added.

Cc: Christoph Hellwig <hch@lst.de>
Cc: Martin K. Petersen <martin.petersen@oracle.com>

Signed-off-by: Jinyoung Choi <j-young.choi@samsung.com>
---
 include/linux/blk-integrity.h | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/include/linux/blk-integrity.h b/include/linux/blk-integrity.h
index 378b2459efe2..45b9fde1fee1 100644
--- a/include/linux/blk-integrity.h
+++ b/include/linux/blk-integrity.h
@@ -43,6 +43,11 @@ int blk_rq_map_integrity_sg(struct request_queue *, struct bio *,
 				   struct scatterlist *);
 int blk_rq_count_integrity_sg(struct request_queue *, struct bio *);
 
+static inline unsigned short blk_rq_nr_integrity_segments(struct request *rq)
+{
+	return rq->nr_integrity_segments;
+}
+
 static inline struct blk_integrity *blk_get_integrity(struct gendisk *disk)
 {
 	struct blk_integrity *bi = &disk->queue->integrity;
@@ -120,6 +125,10 @@ static inline int blk_rq_count_integrity_sg(struct request_queue *q,
 {
 	return 0;
 }
+static inline unsigned short blk_rq_nr_integrity_segments(struct request *rq)
+{
+	return 0;
+}
 static inline int blk_rq_map_integrity_sg(struct request_queue *q,
 					  struct bio *b,
 					  struct scatterlist *s)
-- 
2.34.1

RE:(2) [PATCH v2 08/14] scsi: add scsi_alloc_integrity_sgtables() for integrity process

[Jinyoung CHOI]

>On Wed, May 10, 2023 at 05:56:07PM +0900, Jinyoung CHOI wrote:
>> +        if (WARN_ON_ONCE(!prot_sdb)) {
>> +                /*
>> +                 * This can happen if someone (e.g. multipath)
>> +                 * queues a command to a device on an adapter
>> +                 * that does not support DIX.
>> +                 */
>> +                return BLK_STS_IOERR;
>
>Nit: expand the comment to take up all 80 characters now that you've
>unindented it.

OK. I will fix it. Thank you.

Best Regards,
Jinyoung.

[PATCH v2 11/14] nvme: rdma: change how to find the number of integrity of request

[Jinyoung CHOI]

Since the request has the number of integrity segments, change to use
the relevant api.

Cc: Christoph Hellwig <hch@lst.de>
Cc: Martin K. Petersen <martin.petersen@oracle.com>

Signed-off-by: Jinyoung Choi <j-young.choi@samsung.com>
---
 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 0eb79696fb73..237d81ad54af 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -1507,7 +1507,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.34.1