Re: [PATCH 01/15] block: factor out a bio_integrity_action helper

["Darrick J. Wong" <djwong@kernel.org>]

On Wed, Jan 21, 2026 at 07:43:09AM +0100, Christoph Hellwig wrote:
> Split the logic to see if a bio needs integrity metadata from
> bio_integrity_prep into a reusable helper than can be called from
> file system code.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  block/bio-integrity-auto.c    | 64 +++++------------------------------
>  block/bio-integrity.c         | 48 ++++++++++++++++++++++++++
>  block/blk-mq.c                |  6 ++--
>  drivers/nvdimm/btt.c          |  6 ++--
>  include/linux/bio-integrity.h |  5 ++-
>  include/linux/blk-integrity.h | 16 +++++++++
>  6 files changed, 82 insertions(+), 63 deletions(-)
> 
> diff --git a/block/bio-integrity-auto.c b/block/bio-integrity-auto.c
> index 44dcdf7520c5..3a4141a9de0c 100644
> --- a/block/bio-integrity-auto.c
> +++ b/block/bio-integrity-auto.c
> @@ -50,11 +50,6 @@ static bool bip_should_check(struct bio_integrity_payload *bip)
>  	return bip->bip_flags & BIP_CHECK_FLAGS;
>  }
>  
> -static bool bi_offload_capable(struct blk_integrity *bi)
> -{
> -	return bi->metadata_size == bi->pi_tuple_size;
> -}
> -
>  /**
>   * __bio_integrity_endio - Integrity I/O completion function
>   * @bio:	Protected bio
> @@ -84,69 +79,27 @@ bool __bio_integrity_endio(struct bio *bio)
>  /**
>   * bio_integrity_prep - Prepare bio for integrity I/O
>   * @bio:	bio to prepare
> + * @action:	preparation action needed

What is @action?  Is it a bitset of BI_ACT_* values?  If yes, then can
the comment please say that explicitly?

> + *
> + * Allocate the integrity payload.  For writes, generate the integrity metadata
> + * and for reads, setup the completion handler to verify the metadata.
>   *
> - * Checks if the bio already has an integrity payload attached.  If it does, the
> - * payload has been generated by another kernel subsystem, and we just pass it
> - * through.
> - * Otherwise allocates integrity payload and for writes the integrity metadata
> - * will be generated.  For reads, the completion handler will verify the
> - * metadata.
> + * This is used for bios that do not have user integrity payloads attached.
>   */
> -bool bio_integrity_prep(struct bio *bio)
> +void bio_integrity_prep(struct bio *bio, unsigned int action)
>  {
>  	struct blk_integrity *bi = blk_get_integrity(bio->bi_bdev->bd_disk);
>  	struct bio_integrity_data *bid;
> -	bool set_flags = true;
> -	gfp_t gfp = GFP_NOIO;
> -
> -	if (!bi)
> -		return true;
> -
> -	if (!bio_sectors(bio))
> -		return true;
> -
> -	/* Already protected? */
> -	if (bio_integrity(bio))
> -		return true;
> -
> -	switch (bio_op(bio)) {
> -	case REQ_OP_READ:
> -		if (bi->flags & BLK_INTEGRITY_NOVERIFY) {
> -			if (bi_offload_capable(bi))
> -				return true;
> -			set_flags = false;
> -		}
> -		break;
> -	case REQ_OP_WRITE:
> -		/*
> -		 * Zero the memory allocated to not leak uninitialized kernel
> -		 * memory to disk for non-integrity metadata where nothing else
> -		 * initializes the memory.
> -		 */
> -		if (bi->flags & BLK_INTEGRITY_NOGENERATE) {
> -			if (bi_offload_capable(bi))
> -				return true;
> -			set_flags = false;
> -			gfp |= __GFP_ZERO;
> -		} else if (bi->metadata_size > bi->pi_tuple_size)
> -			gfp |= __GFP_ZERO;
> -		break;
> -	default:
> -		return true;
> -	}
> -
> -	if (WARN_ON_ONCE(bio_has_crypt_ctx(bio)))
> -		return true;
>  
>  	bid = mempool_alloc(&bid_pool, GFP_NOIO);
>  	bio_integrity_init(bio, &bid->bip, &bid->bvec, 1);
>  	bid->bio = bio;
>  	bid->bip.bip_flags |= BIP_BLOCK_INTEGRITY;
> -	bio_integrity_alloc_buf(bio, gfp & __GFP_ZERO);
> +	bio_integrity_alloc_buf(bio, action & BI_ACT_ZERO);
>  
>  	bip_set_seed(&bid->bip, bio->bi_iter.bi_sector);
>  
> -	if (set_flags) {
> +	if (action & BI_ACT_CHECK) {

Yes, I suppose it is a bitset.

--D

>  		if (bi->csum_type == BLK_INTEGRITY_CSUM_IP)
>  			bid->bip.bip_flags |= BIP_IP_CHECKSUM;
>  		if (bi->csum_type)
> @@ -160,7 +113,6 @@ bool bio_integrity_prep(struct bio *bio)
>  		blk_integrity_generate(bio);
>  	else
>  		bid->saved_bio_iter = bio->bi_iter;
> -	return true;
>  }
>  EXPORT_SYMBOL(bio_integrity_prep);
>  
> diff --git a/block/bio-integrity.c b/block/bio-integrity.c
> index 09eeaf6e74b8..6bdbb4ed2d1a 100644
> --- a/block/bio-integrity.c
> +++ b/block/bio-integrity.c
> @@ -7,6 +7,7 @@
>   */
>  
>  #include <linux/blk-integrity.h>
> +#include <linux/t10-pi.h>
>  #include "blk.h"
>  
>  struct bio_integrity_alloc {
> @@ -16,6 +17,53 @@ struct bio_integrity_alloc {
>  
>  static mempool_t integrity_buf_pool;
>  
> +static bool bi_offload_capable(struct blk_integrity *bi)
> +{
> +	return bi->metadata_size == bi->pi_tuple_size;
> +}

Just out of curiosity, what happens if metadata_size > pi_tuple_size?

Can it be the case that metadata_size < pi_tuple_size?

> +unsigned int __bio_integrity_action(struct bio *bio)

Hrm, this function returns a bitset of BI_ACT_* flags, doesn't it?

Would be kinda nice if a comment could say that.

> +{
> +	struct blk_integrity *bi = blk_get_integrity(bio->bi_bdev->bd_disk);
> +
> +	if (WARN_ON_ONCE(bio_has_crypt_ctx(bio)))
> +		return 0;
> +
> +	switch (bio_op(bio)) {
> +	case REQ_OP_READ:
> +		if (bi->flags & BLK_INTEGRITY_NOVERIFY) {
> +			if (bi_offload_capable(bi))
> +				return 0;
> +			return BI_ACT_BUFFER;
> +		}
> +		return BI_ACT_BUFFER | BI_ACT_CHECK;
> +	case REQ_OP_WRITE:
> +		/*
> +		 * Flush masquerading as write?
> +		 */
> +		if (!bio_sectors(bio))
> +			return 0;
> +
> +		/*
> +		 * Zero the memory allocated to not leak uninitialized kernel
> +		 * memory to disk for non-integrity metadata where nothing else
> +		 * initializes the memory.

Er... does someone initialize it eventually?  Such as the filesystem?
Or maybe an io_uring caller?

> +		 */
> +		if (bi->flags & BLK_INTEGRITY_NOGENERATE) {
> +			if (bi_offload_capable(bi))
> +				return 0;
> +			return BI_ACT_BUFFER | BI_ACT_ZERO;
> +		}
> +
> +		if (bi->metadata_size > bi->pi_tuple_size)
> +			return BI_ACT_BUFFER | BI_ACT_CHECK | BI_ACT_ZERO;
> +		return BI_ACT_BUFFER | BI_ACT_CHECK;

"check" feels like a weird name for a write, where we're generating the
PI information.  It really means "block layer takes care of PI
generation and validation", right?  As opposed to whichever upper layer
is using the block device?

BI_ACT_YOUDOIT <snerk>

How about BI_ACT_BDEV /* block layer checks/validates PI */

Everything else in here looks reasonable.

--D

> +	default:
> +		return 0;
> +	}
> +}
> +EXPORT_SYMBOL_GPL(__bio_integrity_action);
> +
>  void bio_integrity_alloc_buf(struct bio *bio, bool zero_buffer)
>  {
>  	struct blk_integrity *bi = blk_get_integrity(bio->bi_bdev->bd_disk);
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index a29d8ac9d3e3..3e58f6d50a1a 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -3133,6 +3133,7 @@ void blk_mq_submit_bio(struct bio *bio)
>  	struct request_queue *q = bdev_get_queue(bio->bi_bdev);
>  	struct blk_plug *plug = current->plug;
>  	const int is_sync = op_is_sync(bio->bi_opf);
> +	unsigned int integrity_action;
>  	struct blk_mq_hw_ctx *hctx;
>  	unsigned int nr_segs;
>  	struct request *rq;
> @@ -3185,8 +3186,9 @@ void blk_mq_submit_bio(struct bio *bio)
>  	if (!bio)
>  		goto queue_exit;
>  
> -	if (!bio_integrity_prep(bio))
> -		goto queue_exit;
> +	integrity_action = bio_integrity_action(bio);
> +	if (integrity_action)
> +		bio_integrity_prep(bio, integrity_action);
>  
>  	blk_mq_bio_issue_init(q, bio);
>  	if (blk_mq_attempt_bio_merge(q, bio, nr_segs))
> diff --git a/drivers/nvdimm/btt.c b/drivers/nvdimm/btt.c
> index a933db961ed7..9cc4b659de1a 100644
> --- a/drivers/nvdimm/btt.c
> +++ b/drivers/nvdimm/btt.c
> @@ -1437,14 +1437,16 @@ static void btt_submit_bio(struct bio *bio)
>  {
>  	struct bio_integrity_payload *bip = bio_integrity(bio);
>  	struct btt *btt = bio->bi_bdev->bd_disk->private_data;
> +	unsigned int integrity_action;
>  	struct bvec_iter iter;
>  	unsigned long start;
>  	struct bio_vec bvec;
>  	int err = 0;
>  	bool do_acct;
>  
> -	if (!bio_integrity_prep(bio))
> -		return;
> +	integrity_action = bio_integrity_action(bio);
> +	if (integrity_action)
> +		bio_integrity_prep(bio, integrity_action);
>  
>  	do_acct = blk_queue_io_stat(bio->bi_bdev->bd_disk->queue);
>  	if (do_acct)
> diff --git a/include/linux/bio-integrity.h b/include/linux/bio-integrity.h
> index 21e4652dcfd2..276cbbdd2c9d 100644
> --- a/include/linux/bio-integrity.h
> +++ b/include/linux/bio-integrity.h
> @@ -78,7 +78,7 @@ int bio_integrity_add_page(struct bio *bio, struct page *page, unsigned int len,
>  int bio_integrity_map_user(struct bio *bio, struct iov_iter *iter);
>  int bio_integrity_map_iter(struct bio *bio, struct uio_meta *meta);
>  void bio_integrity_unmap_user(struct bio *bio);
> -bool bio_integrity_prep(struct bio *bio);
> +void bio_integrity_prep(struct bio *bio, unsigned int action);
>  void bio_integrity_advance(struct bio *bio, unsigned int bytes_done);
>  void bio_integrity_trim(struct bio *bio);
>  int bio_integrity_clone(struct bio *bio, struct bio *bio_src, gfp_t gfp_mask);
> @@ -104,9 +104,8 @@ static inline void bio_integrity_unmap_user(struct bio *bio)
>  {
>  }
>  
> -static inline bool bio_integrity_prep(struct bio *bio)
> +static inline void bio_integrity_prep(struct bio *bio, unsigned int action)
>  {
> -	return true;
>  }
>  
>  static inline int bio_integrity_clone(struct bio *bio, struct bio *bio_src,
> diff --git a/include/linux/blk-integrity.h b/include/linux/blk-integrity.h
> index c15b1ac62765..91d12610d252 100644
> --- a/include/linux/blk-integrity.h
> +++ b/include/linux/blk-integrity.h
> @@ -180,4 +180,20 @@ static inline struct bio_vec rq_integrity_vec(struct request *rq)
>  }
>  #endif /* CONFIG_BLK_DEV_INTEGRITY */
>  
> +enum bio_integrity_action {
> +	BI_ACT_BUFFER		= (1u << 0),	/* allocate buffer */
> +	BI_ACT_CHECK		= (1u << 1),	/* generate / verify PI */
> +	BI_ACT_ZERO		= (1u << 2),	/* zero buffer */
> +};
> +
> +unsigned int __bio_integrity_action(struct bio *bio);
> +static inline unsigned int bio_integrity_action(struct bio *bio)
> +{
> +	if (!blk_get_integrity(bio->bi_bdev->bd_disk))
> +		return 0;
> +	if (bio_integrity(bio))
> +		return 0;
> +	return __bio_integrity_action(bio);
> +}
> +
>  #endif /* _LINUX_BLK_INTEGRITY_H */
> -- 
> 2.47.3
> 
>