On Wed, May 07, 2025 at 12:14:24PM -0700, Keith Busch wrote:
> From: Keith Busch <kbusch@xxxxxxxxxx>
>
> 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?
