Re: [PATCHv2] block: always allocate integrity buffer

[Christoph Hellwig <hch@lst.de>]

On Wed, May 07, 2025 at 12:14:24PM -0700, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> The integrity buffer, whether or not you want it generated or verified, is
> mandatory for nvme formats that have metadata. The block integrity attributes
> read_verify and write_generate had been stopping the metadata buffer from being

This commit log exceeds the 73 characters allocated to it, please reformat
it.

> allocated and attached to the bio entirely. We only want to suppress the
> protection checks on the device and host, but we still need the buffer.
> 
> Otherwise, reads and writes will just get IO errors and this nvme warning:

But to a point - the metadata buffer is only required for non-PI
metadata.  I think from looking at the code that is exactly what
this patch does, but the commit log sounds different.

Also this should probably have a fixes tag.

> -	if (bio_op(bio) == REQ_OP_READ && !bio->bi_status && bi->csum_type) {
> +	if (bio_op(bio) == REQ_OP_READ && !bio->bi_status &&
> +	    bip->bip_flags & BIP_CHECK_GUARD) {

While Martin correctly points out we currently always do both guard
and reftag checking, we really should check for either, especially as
some code below is written in a way to allow for formats that only
have one of them.

> +static inline void bio_set_bip_flags(struct blk_integrity *bi, u16 *bip_flags)
> +{
> +	if (bi->csum_type == BLK_INTEGRITY_CSUM_IP)
> +		*bip_flags |= BIP_IP_CHECKSUM;
> +	if (bi->csum_type)
> +		*bip_flags |= BIP_CHECK_GUARD;
> +	if (bi->flags & BLK_INTEGRITY_REF_TAG)
> +		*bip_flags |= BIP_CHECK_REFTAG;
> +

Just return the flags here instead of the somewhat odd output by
pointer return?

> +			break;
> +		bio_set_bip_flags(bi, &bip_flags);
>  		break;
>  	case REQ_OP_WRITE:

...

> +		bio_set_bip_flags(bi, &bip_flags);
>  		break;
>  	default:
>  		return true;
> @@ -134,22 +148,15 @@ bool bio_integrity_prep(struct bio *bio)

Just move this after the switch to have a single callsite.  And maybe
don't even bother with the helper then?