diff mbox series

[5/5] block: remove bio_integrity_process

Message ID 20240626045950.189758-6-hch@lst.de (mailing list archive)
State New, archived
Headers show
Series [1/5] block: only zero non-PI metadata tuples in bio_integrity_prep | expand

Commit Message

Christoph Hellwig June 26, 2024, 4:59 a.m. UTC 
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(-)

Comments

Kanchan Joshi June 27, 2024, 3:36 p.m. UTC | #1 
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.
Christoph Hellwig June 27, 2024, 3:47 p.m. UTC | #2 
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.
Kanchan Joshi June 27, 2024, 6:33 p.m. UTC | #3 
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.
Christoph Hellwig June 28, 2024, 6:02 a.m. UTC | #4 
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.
diff mbox series

Patch

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