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

[Christoph Hellwig <hch@lst.de>]

On Thu, Jan 22, 2026 at 04:01:13PM -0800, Darrick J. Wong wrote:
> >  /**
> >   * bio_integrity_prep - Prepare bio for integrity I/O
> >   * @bio:	bio to prepare
> > + * @action:	preparation action needed
> 
> What is @action?

Yes.

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

Is this good enough?

 * @action:     preparation action needed (BI_ACT_*)

> > +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?

Then we still have to provide a buffer as the automatic insert/strip
doesn't work. (I find the offload name rather confusing for this)

> Can it be the case that metadata_size < pi_tuple_size?

No.  See blk_validate_integrity_limits:

	if (bi->pi_offset + bi->pi_tuple_size > bi->metadata_size) {
		pr_warn("pi_offset (%u) + pi_tuple_size (%u) exceeds metadata_size (%u)\n",
			bi->pi_offset, bi->pi_tuple_size,
			bi->metadata_size);
		return -EINVAL;
	}


> 
> > +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.

Is this ok?

/**
 * bio_integrity_action - return the integrity action needed for a bio
 * @bio:        bio to operate on
 *
 * Returns the mask of integrity actions (BI_ACT_*) that need to be performed
 * for @bio.
 */


> > +		/*
> > +		 * 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?

For integrity metadata?  The code called later fills it out.  But it
doesn't fill non-integrity metadata, so we need to zero it.

> > +		 */
> > +		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 */

I think BI_ACT_BDEV is not very useful.  Check is supposed to
include generate and verify, but I'm not sure how we could word this
in a nice way.