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
