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

[Christoph Hellwig <hch@lst.de>]

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.

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

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

> +	u8			bi_bvec_gap_bit;

Aren't we normally calling something like this _mask, i.e., something
like:

		bi_bvec_page_gap_mask;