Re: [PATCH 6/6] block: don't check if adjacent bvecs in one bio can be mergeable

[Christoph Hellwig <hch@lst.de>]

> +	bool new_bio;
>  
>  	if (!bio)
>  		return 0;
> @@ -377,9 +377,10 @@ static unsigned int __blk_recalc_rq_segments(struct request_queue *q,
>  	fbio = bio;
>  	seg_size = 0;
>  	nr_phys_segs = 0;
> +	new_bio = false;

I'd just initialize it to false in the declaration line.

> +/* only try to merge bvecs into one sg if they are from two bios */
> +static inline bool
> +__blk_segment_map_sg_merge(struct request_queue *q, struct bio_vec *bvec,
> +			   struct bio_vec *bvprv, struct scatterlist **sg)
>  {
>  	int nbytes = bvec->bv_len;
>  
> +	if (!*sg)
> +		return false;
>  
> +	if ((*sg)->length + nbytes > queue_max_segment_size(q))
> +		return false;
> +
> +	if (!biovec_phys_mergeable(q, bvprv, bvec))
> +		return false;
> +
> +	(*sg)->length += nbytes;
> +
> +	return true;
> +}
> +
> +static inline void
> +__blk_segment_map_sg(struct request_queue *q, struct bio_vec *bvec,
> +		     struct scatterlist *sglist, struct scatterlist **sg,
> +		     int *nsegs)
> +{
> +	if (bvec->bv_offset + bvec->bv_len <= PAGE_SIZE) {
> +		*sg = blk_next_sg(sg, sglist);
> +		sg_set_page(*sg, bvec->bv_page, bvec->bv_len, bvec->bv_offset);
> +		(*nsegs) += 1;

This branch is basically the same as __blk_bvec_map_sg, I wonder if
we can reuse it.

> +	} else
> +		(*nsegs) += blk_bvec_map_sg(q, bvec, sglist, sg);
>  }

And then maybe just kill off __blk_segment_map_sg entirely by moving
the if else into the caller.

> @@ -535,11 +543,24 @@ static int __blk_bios_map_sg(struct request_queue *q, struct bio *bio,
>  	struct bio_vec bvec, bvprv = { NULL };
>  	struct bvec_iter iter;
>  	int nsegs = 0;
> +	bool new_bio = false;
>  
> +	for_each_bio(bio) {
> +		bio_for_each_bvec(bvec, bio, iter) {
> +			/*
> +			 * Only try to merge bvecs from two bios given we
> +			 * have done bio internal merge when adding pages
> +			 * to bio
> +			 */
> +			if (!new_bio || !__blk_segment_map_sg_merge(q,
> +						&bvec, &bvprv, sg))

Somewhat hard to read.  Why not:

			if (new_bio ||
			    !__blk_segment_map_sg_merge(q, &bvec, &bvprv, sg))

That being said I'd really like to see some stats on workloads that
actually trigger cross-bio segment merges.  If we get a lot of those
we are doing something wrong.