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

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

On Fri, Jan 23, 2026 at 07:03:24AM +0100, Christoph Hellwig wrote:
> 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_*)

Yes.

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

Excellent!

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

Ahhh, right, the app tag or whatever?

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

Me neither.  If nobody else here comes up with a better suggestion then
I guess BI_ACT_CHECK + its comment wins.

--D