Re: [PATCHv4 1/2] block: accumulate memory segment gaps per bio

[Keith Busch <kbusch@kernel.org>]

On Fri, Oct 10, 2025 at 07:34:22AM +0200, Christoph Hellwig wrote:
> On Tue, Oct 07, 2025 at 10:52:44AM -0700, Keith Busch wrote:
> > +static inline unsigned int bvec_seg_gap(struct bio_vec *bvprv,
> > +					struct bio_vec *bv)
> > +{
> > +	return __bvec_gap(bvprv, bv->bv_offset, U32_MAX);
> > +}
> 
> I find this helper (and the existing __bvec_gap* ones, but I'll send
> patches to clean that up in a bit..) very confusing.  Just open coding
> it in the callers like:
> 
> 	 gaps |= (bvprvp->bv_offset + bvprvp->bv_len);
> 	 gaps |= bv.bv_offset;
> 
> makes the intent clear, and also removes the pointless masking by 
> U32_MAX.

Sounds good, I'll rebase on your cleanup patch.

> > +	/*
> > +	 * A mask that contains bits set for virtual address gaps between
> > +	 * physical segments. This provides information necessary for dma
> > +	 * optimization opprotunities, like for testing if the segments can be
> > +	 * coalesced against the device's iommu granule.
> > +	 */
> > +	unsigned int phys_gap;
> 
> Any reason this is not a mask like in the bio?  Having the representation
> and naming match between the bio and request should make the code a bit
> easier to understand.

I thought it easier for the users to deal with the mask rather than a
set bit value. Not a big deal, I'll just introduce a helper to return a
mask from the value.
 
> > +
> > +	/*
> > +	 * The bvec gap bit indicates the lowest set bit in any address offset
> > +	 * between all bi_io_vecs. This field is initialized only after
> > +	 * splitting to the hardware limits. It may be used to consider DMA
> > +	 * optimization when performing that mapping. The value is compared to
> > +	 * a power of two mask where the result depends on any bit set within
> > +	 * the mask, so saving the lowest bit is sufficient to know if any
> > +	 * segment gap collides with the mask.
> > +	 */
> 
> This should grow a sentence explaining that the field is only set by
> bio_split_io_at, and not valid before as that's very different from the
> other bio fields.

I didn't mention the function by name, but the comment does say it's not
initialized until you split to limits. I'll add a pointer to
bio_split_io_at().

> > +	u8			bi_bvec_gap_bit;
> 
> Aren't we normally calling something like this _mask, i.e., something
> like:
> 
> 		bi_bvec_page_gap_mask;

A "mask" suffix in the name suggests you can AND it directly with
another value to get a useful result, but that's not how this is
encoded. You have to shift it to generate the intended mask.