integrity cleanups

integrity cleanups

[Christoph Hellwig]

Hi Jens and Martin,

this series has a few cleanups to the block layer PI generation and
verification code.

Diffstat:
 block/bio-integrity.c         |  101 +++++++-----------------------------------
 block/blk.h                   |    7 --
 block/t10-pi.c                |   97 +++++++++++++++++++++++++++++++---------
 include/linux/blk-integrity.h |    9 ---
 4 files changed, 96 insertions(+), 118 deletions(-)

[PATCH 1/5] block: only zero non-PI metadata tuples in bio_integrity_prep

[Christoph Hellwig]

The PI generation helpers already zero the app tag, so relax the zeroing
to non-PI metadata.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/bio-integrity.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/block/bio-integrity.c b/block/bio-integrity.c
index 173ffd4d623788..8c5991a1c535af 100644
--- a/block/bio-integrity.c
+++ b/block/bio-integrity.c
@@ -456,11 +456,11 @@ bool bio_integrity_prep(struct bio *bio)
 
 		/*
 		 * Zero the memory allocated to not leak uninitialized kernel
-		 * memory to disk.  For PI this only affects the app tag, but
-		 * for non-integrity metadata it affects the entire metadata
-		 * buffer.
+		 * memory to disk for non-integrity metadata where nothing else
+		 * initializes the memory.
 		 */
-		gfp |= __GFP_ZERO;
+		if (bi->csum_type == BLK_INTEGRITY_CSUM_NONE)
+			gfp |= __GFP_ZERO;
 	}
 
 	/* Allocate kernel buffer for protection data */
-- 
2.43.0

[PATCH 2/5] block: simplify adding the payload in bio_integrity_prep

[Christoph Hellwig]

bio_integrity_add_page can add physically contiguous regions of any size,
so don't bother chunking up the kmalloced buffer.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/bio-integrity.c | 32 ++++++--------------------------
 1 file changed, 6 insertions(+), 26 deletions(-)

diff --git a/block/bio-integrity.c b/block/bio-integrity.c
index 8c5991a1c535af..2efab4b3957978 100644
--- a/block/bio-integrity.c
+++ b/block/bio-integrity.c
@@ -428,10 +428,8 @@ bool bio_integrity_prep(struct bio *bio)
 {
 	struct bio_integrity_payload *bip;
 	struct blk_integrity *bi = blk_get_integrity(bio->bi_bdev->bd_disk);
+	unsigned int len;
 	void *buf;
-	unsigned long start, end;
-	unsigned int len, nr_pages;
-	unsigned int bytes, offset, i;
 	gfp_t gfp = GFP_NOIO;
 
 	if (!bi)
@@ -471,12 +469,7 @@ bool bio_integrity_prep(struct bio *bio)
 		goto err_end_io;
 	}
 
-	end = (((unsigned long) buf) + len + PAGE_SIZE - 1) >> PAGE_SHIFT;
-	start = ((unsigned long) buf) >> PAGE_SHIFT;
-	nr_pages = end - start;
-
-	/* Allocate bio integrity payload and integrity vectors */
-	bip = bio_integrity_alloc(bio, GFP_NOIO, nr_pages);
+	bip = bio_integrity_alloc(bio, GFP_NOIO, 1);
 	if (IS_ERR(bip)) {
 		printk(KERN_ERR "could not allocate data integrity bioset\n");
 		kfree(buf);
@@ -489,23 +482,10 @@ bool bio_integrity_prep(struct bio *bio)
 	if (bi->csum_type == BLK_INTEGRITY_CSUM_IP)
 		bip->bip_flags |= BIP_IP_CHECKSUM;
 
-	/* Map it */
-	offset = offset_in_page(buf);
-	for (i = 0; i < nr_pages && len > 0; i++) {
-		bytes = PAGE_SIZE - offset;
-
-		if (bytes > len)
-			bytes = len;
-
-		if (bio_integrity_add_page(bio, virt_to_page(buf),
-					   bytes, offset) < bytes) {
-			printk(KERN_ERR "could not attach integrity payload\n");
-			goto err_end_io;
-		}
-
-		buf += bytes;
-		len -= bytes;
-		offset = 0;
+	if (bio_integrity_add_page(bio, virt_to_page(buf), len,
+			offset_in_page(buf)) < len) {
+		printk(KERN_ERR "could not attach integrity payload\n");
+		goto err_end_io;
 	}
 
 	/* Auto-generate integrity metadata if this is a write */
-- 
2.43.0

[PATCH 3/5] block: remove allocation failure warnings in bio_integrity_prep

[Christoph Hellwig]

Allocation failures already leave a stack trace, so don't spew another
warning.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/bio-integrity.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/block/bio-integrity.c b/block/bio-integrity.c
index 2efab4b3957978..1017984552baf8 100644
--- a/block/bio-integrity.c
+++ b/block/bio-integrity.c
@@ -465,13 +465,11 @@ bool bio_integrity_prep(struct bio *bio)
 	len = bio_integrity_bytes(bi, bio_sectors(bio));
 	buf = kmalloc(len, gfp);
 	if (unlikely(buf == NULL)) {
-		printk(KERN_ERR "could not allocate integrity buffer\n");
 		goto err_end_io;
 	}
 
 	bip = bio_integrity_alloc(bio, GFP_NOIO, 1);
 	if (IS_ERR(bip)) {
-		printk(KERN_ERR "could not allocate data integrity bioset\n");
 		kfree(buf);
 		goto err_end_io;
 	}
-- 
2.43.0

[PATCH 4/5] block: switch on bio operation in bio_integrity_prep

[Christoph Hellwig]

Use a single switch to perform read and write specific checks and exit
early for other operations instead of having two checks using different
predicates.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/bio-integrity.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/block/bio-integrity.c b/block/bio-integrity.c
index 1017984552baf8..3cd867b0544cf0 100644
--- a/block/bio-integrity.c
+++ b/block/bio-integrity.c
@@ -435,9 +435,6 @@ bool bio_integrity_prep(struct bio *bio)
 	if (!bi)
 		return true;
 
-	if (bio_op(bio) != REQ_OP_READ && bio_op(bio) != REQ_OP_WRITE)
-		return true;
-
 	if (!bio_sectors(bio))
 		return true;
 
@@ -445,10 +442,12 @@ bool bio_integrity_prep(struct bio *bio)
 	if (bio_integrity(bio))
 		return true;
 
-	if (bio_data_dir(bio) == READ) {
+	switch (bio_op(bio)) {
+	case REQ_OP_READ:
 		if (bi->flags & BLK_INTEGRITY_NOVERIFY)
 			return true;
-	} else {
+		break;
+	case REQ_OP_WRITE:
 		if (bi->flags & BLK_INTEGRITY_NOGENERATE)
 			return true;
 
@@ -459,6 +458,9 @@ bool bio_integrity_prep(struct bio *bio)
 		 */
 		if (bi->csum_type == BLK_INTEGRITY_CSUM_NONE)
 			gfp |= __GFP_ZERO;
+		break;
+	default:
+		return true;
 	}
 
 	/* Allocate kernel buffer for protection data */
-- 
2.43.0

[PATCH 5/5] block: remove bio_integrity_process

[Christoph Hellwig]

Move the bvec interation into the generate/verify helpers to avoid a bit
of argument passing churn.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/bio-integrity.c         | 47 +----------------
 block/blk.h                   |  7 +--
 block/t10-pi.c                | 97 +++++++++++++++++++++++++++--------
 include/linux/blk-integrity.h |  9 ----
 4 files changed, 79 insertions(+), 81 deletions(-)

diff --git a/block/bio-integrity.c b/block/bio-integrity.c
index 3cd867b0544cf0..20bbfd5730dadd 100644
--- a/block/bio-integrity.c
+++ b/block/bio-integrity.c
@@ -374,44 +374,6 @@ int bio_integrity_map_user(struct bio *bio, void __user *ubuf, ssize_t bytes,
 }
 EXPORT_SYMBOL_GPL(bio_integrity_map_user);
 
-/**
- * bio_integrity_process - Process integrity metadata for a bio
- * @bio:	bio to generate/verify integrity metadata for
- * @proc_iter:  iterator to process
- */
-static blk_status_t bio_integrity_process(struct bio *bio,
-		struct bvec_iter *proc_iter)
-{
-	struct blk_integrity *bi = blk_get_integrity(bio->bi_bdev->bd_disk);
-	struct blk_integrity_iter iter;
-	struct bvec_iter bviter;
-	struct bio_vec bv;
-	struct bio_integrity_payload *bip = bio_integrity(bio);
-	blk_status_t ret = BLK_STS_OK;
-
-	iter.disk_name = bio->bi_bdev->bd_disk->disk_name;
-	iter.interval = 1 << bi->interval_exp;
-	iter.seed = proc_iter->bi_sector;
-	iter.prot_buf = bvec_virt(bip->bip_vec);
-
-	__bio_for_each_segment(bv, bio, bviter, *proc_iter) {
-		void *kaddr = bvec_kmap_local(&bv);
-
-		iter.data_buf = kaddr;
-		iter.data_size = bv.bv_len;
-		if (bio_data_dir(bio) == WRITE)
-			blk_integrity_generate(&iter, bi);
-		else
-			ret = blk_integrity_verify(&iter, bi);
-		kunmap_local(kaddr);
-
-		if (ret)
-			break;
-
-	}
-	return ret;
-}
-
 /**
  * bio_integrity_prep - Prepare bio for integrity I/O
  * @bio:	bio to prepare
@@ -490,7 +452,7 @@ bool bio_integrity_prep(struct bio *bio)
 
 	/* Auto-generate integrity metadata if this is a write */
 	if (bio_data_dir(bio) == WRITE)
-		bio_integrity_process(bio, &bio->bi_iter);
+		blk_integrity_generate(bio);
 	else
 		bip->bio_iter = bio->bi_iter;
 	return true;
@@ -516,12 +478,7 @@ static void bio_integrity_verify_fn(struct work_struct *work)
 		container_of(work, struct bio_integrity_payload, bip_work);
 	struct bio *bio = bip->bip_bio;
 
-	/*
-	 * At the moment verify is called bio's iterator was advanced
-	 * during split and completion, we need to rewind iterator to
-	 * it's original position.
-	 */
-	bio->bi_status = bio_integrity_process(bio, &bip->bio_iter);
+	blk_integrity_verify(bio);
 	bio_integrity_free(bio);
 	bio_endio(bio);
 }
diff --git a/block/blk.h b/block/blk.h
index d0a986d8ee507e..7917f86cca0ebd 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -9,7 +9,6 @@
 #include <xen/xen.h>
 #include "blk-crypto-internal.h"
 
-struct blk_integrity_iter;
 struct elevator_type;
 
 /* Max future timer expiry for timeouts */
@@ -679,10 +678,8 @@ int bdev_open(struct block_device *bdev, blk_mode_t mode, void *holder,
 	      const struct blk_holder_ops *hops, struct file *bdev_file);
 int bdev_permission(dev_t dev, blk_mode_t mode, void *holder);
 
-void blk_integrity_generate(struct blk_integrity_iter *iter,
-		struct blk_integrity *bi);
-blk_status_t blk_integrity_verify(struct blk_integrity_iter *iter,
-		struct blk_integrity *bi);
+void blk_integrity_generate(struct bio *bio);
+void blk_integrity_verify(struct bio *bio);
 void blk_integrity_prepare(struct request *rq);
 void blk_integrity_complete(struct request *rq, unsigned int nr_bytes);
 
diff --git a/block/t10-pi.c b/block/t10-pi.c
index cd7fa60d63ff21..425e2836f3e1d8 100644
--- a/block/t10-pi.c
+++ b/block/t10-pi.c
@@ -13,6 +13,15 @@
 #include <asm/unaligned.h>
 #include "blk.h"
 
+struct blk_integrity_iter {
+	void			*prot_buf;
+	void			*data_buf;
+	sector_t		seed;
+	unsigned int		data_size;
+	unsigned short		interval;
+	const char		*disk_name;
+};
+
 static __be16 t10_pi_csum(__be16 csum, void *data, unsigned int len,
 		unsigned char csum_type)
 {
@@ -364,33 +373,77 @@ static void ext_pi_type1_complete(struct request *rq, unsigned int nr_bytes)
 	}
 }
 
-void blk_integrity_generate(struct blk_integrity_iter *iter,
-		struct blk_integrity *bi)
+void blk_integrity_generate(struct bio *bio)
 {
-	switch (bi->csum_type) {
-	case BLK_INTEGRITY_CSUM_CRC64:
-		ext_pi_crc64_generate(iter, bi);
-		break;
-	case BLK_INTEGRITY_CSUM_CRC:
-	case BLK_INTEGRITY_CSUM_IP:
-		t10_pi_generate(iter, bi);
-		break;
-	default:
-		break;
+	struct blk_integrity *bi = blk_get_integrity(bio->bi_bdev->bd_disk);
+	struct bio_integrity_payload *bip = bio_integrity(bio);
+	struct blk_integrity_iter iter;
+	struct bvec_iter bviter;
+	struct bio_vec bv;
+
+	iter.disk_name = bio->bi_bdev->bd_disk->disk_name;
+	iter.interval = 1 << bi->interval_exp;
+	iter.seed = bio->bi_iter.bi_sector;
+	iter.prot_buf = bvec_virt(bip->bip_vec);
+	bio_for_each_segment(bv, bio, bviter) {
+		void *kaddr = bvec_kmap_local(&bv);
+
+		iter.data_buf = kaddr;
+		iter.data_size = bv.bv_len;
+		switch (bi->csum_type) {
+		case BLK_INTEGRITY_CSUM_CRC64:
+			ext_pi_crc64_generate(&iter, bi);
+			break;
+		case BLK_INTEGRITY_CSUM_CRC:
+		case BLK_INTEGRITY_CSUM_IP:
+			t10_pi_generate(&iter, bi);
+			break;
+		default:
+			break;
+		}
+		kunmap_local(kaddr);
 	}
 }
 
-blk_status_t blk_integrity_verify(struct blk_integrity_iter *iter,
-		struct blk_integrity *bi)
+void blk_integrity_verify(struct bio *bio)
 {
-	switch (bi->csum_type) {
-	case BLK_INTEGRITY_CSUM_CRC64:
-		return ext_pi_crc64_verify(iter, bi);
-	case BLK_INTEGRITY_CSUM_CRC:
-	case BLK_INTEGRITY_CSUM_IP:
-		return t10_pi_verify(iter, bi);
-	default:
-		return BLK_STS_OK;
+	struct blk_integrity *bi = blk_get_integrity(bio->bi_bdev->bd_disk);
+	struct bio_integrity_payload *bip = bio_integrity(bio);
+	struct blk_integrity_iter iter;
+	struct bvec_iter bviter;
+	struct bio_vec bv;
+
+	/*
+	 * At the moment verify is called bi_iter has been advanced during split
+	 * and completion, so use the copy created during submission here.
+	 */
+	iter.disk_name = bio->bi_bdev->bd_disk->disk_name;
+	iter.interval = 1 << bi->interval_exp;
+	iter.seed = bip->bio_iter.bi_sector;
+	iter.prot_buf = bvec_virt(bip->bip_vec);
+	__bio_for_each_segment(bv, bio, bviter, bip->bio_iter) {
+		void *kaddr = bvec_kmap_local(&bv);
+		blk_status_t ret = BLK_STS_OK;
+
+		iter.data_buf = kaddr;
+		iter.data_size = bv.bv_len;
+		switch (bi->csum_type) {
+		case BLK_INTEGRITY_CSUM_CRC64:
+			ret = ext_pi_crc64_verify(&iter, bi);
+			break;
+		case BLK_INTEGRITY_CSUM_CRC:
+		case BLK_INTEGRITY_CSUM_IP:
+			ret = t10_pi_verify(&iter, bi);
+			break;
+		default:
+			break;
+		}
+		kunmap_local(kaddr);
+
+		if (ret) {
+			bio->bi_status = ret;
+			return;
+		}
 	}
 }
 
diff --git a/include/linux/blk-integrity.h b/include/linux/blk-integrity.h
index d201140d77a336..2f015135f967d0 100644
--- a/include/linux/blk-integrity.h
+++ b/include/linux/blk-integrity.h
@@ -14,15 +14,6 @@ enum blk_integrity_flags {
 	BLK_INTEGRITY_STACKED		= 1 << 4,
 };
 
-struct blk_integrity_iter {
-	void			*prot_buf;
-	void			*data_buf;
-	sector_t		seed;
-	unsigned int		data_size;
-	unsigned short		interval;
-	const char		*disk_name;
-};
-
 const char *blk_integrity_profile_name(struct blk_integrity *bi);
 bool queue_limits_stack_integrity(struct queue_limits *t,
 		struct queue_limits *b);
-- 
2.43.0

Re: integrity cleanups

[Martin K. Petersen]

Christoph,

> this series has a few cleanups to the block layer PI generation and
> verification code.

Looks OK to me.

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

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH 5/5] block: remove bio_integrity_process

[Kanchan Joshi]

On 6/26/2024 10:29 AM, Christoph Hellwig wrote:
> +	struct blk_integrity *bi = blk_get_integrity(bio->bi_bdev->bd_disk);
> +	struct bio_integrity_payload *bip = bio_integrity(bio);
> +	struct blk_integrity_iter iter;
> +	struct bvec_iter bviter;
> +	struct bio_vec bv;
> +
> +	iter.disk_name = bio->bi_bdev->bd_disk->disk_name;
> +	iter.interval = 1 << bi->interval_exp;
> +	iter.seed = bio->bi_iter.bi_sector;
> +	iter.prot_buf = bvec_virt(bip->bip_vec);
> +	bio_for_each_segment(bv, bio, bviter) {
> +		void *kaddr = bvec_kmap_local(&bv);
> +
> +		iter.data_buf = kaddr;
> +		iter.data_size = bv.bv_len;
> +		switch (bi->csum_type) {
> +		case BLK_INTEGRITY_CSUM_CRC64:
> +			ext_pi_crc64_generate(&iter, bi);
> +			break;
> +		case BLK_INTEGRITY_CSUM_CRC:
> +		case BLK_INTEGRITY_CSUM_IP:
> +			t10_pi_generate(&iter, bi);
> +			break;
> +		default:
> +			break;
> +		}

The bi->csum_type is constant as far as this bio_for_each_segment loop 
is concerned.
Seems wasteful processing, and can rather be moved out where we set a 
function pointer to point to either ext_pi_crc64_generate or 
t10_pi_generate once.

Re: [PATCH 4/5] block: switch on bio operation in bio_integrity_prep

[Kanchan Joshi]

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

Re: [PATCH 3/5] block: remove allocation failure warnings in bio_integrity_prep

[Kanchan Joshi]

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

Re: [PATCH 2/5] block: simplify adding the payload in bio_integrity_prep

[Kanchan Joshi]

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

Re: [PATCH 1/5] block: only zero non-PI metadata tuples in bio_integrity_prep

[Kanchan Joshi]

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

Re: [PATCH 5/5] block: remove bio_integrity_process

[Christoph Hellwig]

On Thu, Jun 27, 2024 at 09:06:56PM +0530, Kanchan Joshi wrote:
> The bi->csum_type is constant as far as this bio_for_each_segment loop 
> is concerned.
> Seems wasteful processing, and can rather be moved out where we set a 
> function pointer to point to either ext_pi_crc64_generate or 
> t10_pi_generate once.

A function pointer is way more expensive than a few branches, especially
easily predictable ones.

Re: [PATCH 5/5] block: remove bio_integrity_process

[Kanchan Joshi]

On 6/27/2024 9:17 PM, Christoph Hellwig wrote:
> On Thu, Jun 27, 2024 at 09:06:56PM +0530, Kanchan Joshi wrote:
>> The bi->csum_type is constant as far as this bio_for_each_segment loop
>> is concerned.
>> Seems wasteful processing, and can rather be moved out where we set a
>> function pointer to point to either ext_pi_crc64_generate or
>> t10_pi_generate once.
> A function pointer is way more expensive than a few branches, especially
> easily predictable ones.
> 

In general yes. Maybe I can profile this particular case someday and get 
myself convinced. But regardless, I am unsure what the patch buys.

During write:
-               bio_integrity_process(bio, &bio->bi_iter);
+               blk_integrity_generate(bio);

During read:
-       bio->bi_status = bio_integrity_process(bio, &bip->bio_iter);
+       blk_integrity_verify(bio);

One less argument is passed, but common code of bio_integrity_process 
got mostly duplicated into blk_integrity_generate/verify now.

Re: [PATCH 5/5] block: remove bio_integrity_process

[Christoph Hellwig]

On Fri, Jun 28, 2024 at 12:03:09AM +0530, Kanchan Joshi wrote:
> In general yes. Maybe I can profile this particular case someday and get 
> myself convinced. But regardless, I am unsure what the patch buys.

It avoid a pointless indirection that make the code hard to follow.

Re: integrity cleanups

[Jens Axboe]

On Wed, 26 Jun 2024 06:59:33 +0200, Christoph Hellwig wrote:
> this series has a few cleanups to the block layer PI generation and
> verification code.
> 
> Diffstat:
>  block/bio-integrity.c         |  101 +++++++-----------------------------------
>  block/blk.h                   |    7 --
>  block/t10-pi.c                |   97 +++++++++++++++++++++++++++++++---------
>  include/linux/blk-integrity.h |    9 ---
>  4 files changed, 96 insertions(+), 118 deletions(-)
> 
> [...]

Applied, thanks!

[1/5] block: only zero non-PI metadata tuples in bio_integrity_prep
      commit: c546d6f438338017480d105ab597292da67f6f6a
[2/5] block: simplify adding the payload in bio_integrity_prep
      commit: c096df908393b0b3445f4335dd9bbd9d98252951
[3/5] block: remove allocation failure warnings in bio_integrity_prep
      commit: dac18fabba59149acec42621b9b603654e9459b2
[4/5] block: switch on bio operation in bio_integrity_prep
      commit: df3c485e0e60e8ad87f168092f1513a3d621fa4b
[5/5] block: remove bio_integrity_process
      commit: d19b46340b3c0ea66bef0f6c58876cc085813ba8

Best regards,
-- 
Jens Axboe