Re: [PATCHv3 06/10] blk-integrity: simplify counting segments

[Keith Busch <kbusch@kernel.org>]

On Tue, Sep 10, 2024 at 05:41:05PM +0200, Christoph Hellwig wrote:
> On Wed, Sep 04, 2024 at 08:26:01AM -0700, Keith Busch wrote:
> > From: Keith Busch <kbusch@kernel.org>
> > 
> > The segments are already packed to the queue limits when adding them to
> > the bio,
> 
> I can't really parse this.  I guess this talks about
> bio_integrity_add_page trying to append the payload to the last
> vector when possible?

Exactly. bio_integrity_add_page will use the queue's limits to decide if
it can combine pages into one vector, so appending pages through that
interface will always result in the most compact bip vector.

This patch doesn't combine merged bio's but that's unlikely to have
mergable segments.
 
> > -int blk_rq_count_integrity_sg(struct request_queue *q, struct bio *bio)
> > +int blk_rq_count_integrity_segs(struct bio *bio)
> >  {
> > -	struct bio_vec iv, ivprv = { NULL };
> >  	unsigned int segments = 0;
> > -	unsigned int seg_size = 0;
> > -	struct bvec_iter iter;
> > -	int prev = 0;
> > -
> > -	bio_for_each_integrity_vec(iv, bio, iter) {
> >  
> > -		if (prev) {
> > -			if (!biovec_phys_mergeable(q, &ivprv, &iv))
> > -				goto new_segment;
> > -			if (seg_size + iv.bv_len > queue_max_segment_size(q))
> > -				goto new_segment;
> > -
> > -			seg_size += iv.bv_len;
> > -		} else {
> > -new_segment:
> > -			segments++;
> > -			seg_size = iv.bv_len;
> 
> Q: for the data path the caller submitted bio_vecs can be larger
> than the max segment size, and given that the metadata API tries
> to follow that in general, I'd assume we could also get metadata
> segments larger than the segment size in theory, in which case
> we'd need to split a bvec into multiple segments, similar to what
> bvec_split_segs does.  Do we need similar handling for metadata?
> Or are we going to say that metadata must e.g. always be smaller
> than PAGE_SIZE as max_segment_sizse must be >= PAGE_SIZE?

The common use cases don't add integrity data until after the bio is
already split in __bio_split_to_limits(), and it won't be split again
after integrity is added via bio_integrity_prep(). The common path
always adds integrity in a single segment, so it's always valid.

There are just a few other users that set their own bio integrity before
submitting (the nvme and scsi target drivers), and I think both can
break from possible bio splitting, but I haven't been able to test
those. 
 
> > +	for_each_bio(bio)
> > +		segments += bio->bi_integrity->bip_vcnt;
> 
> If a bio was cloned bip_vcnt isn't the correct value here,
> we'll need to use the iter to count the segments.

Darn. The common use case doesn't have integrity added until just before
it's dispatched, so the integrity cloning doesn't normally happen for
that case.

Let's just drop patches 6 and 7 from consideration for now. They are a
bit too optimistic, and doesn't really fix anything anyway.