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

From: Ming Lei <ming.lei@redhat.com>
To: Christoph Hellwig <hch@lst.de>
Cc: Jens Axboe <axboe@kernel.dk>,
	linux-block@vger.kernel.org, Omar Sandoval <osandov@fb.com>
Subject: Re: [PATCH 6/6] block: don't check if adjacent bvecs in one bio can be mergeable
Date: Tue, 12 Mar 2019 09:19:34 +0800	[thread overview]
Message-ID: <20190312011933.GB28841@ming.t460p> (raw)
In-Reply-To: <20190311144024.GF7850@lst.de>

On Mon, Mar 11, 2019 at 03:40:24PM +0100, Christoph Hellwig wrote:
> > +	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.

OK.

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

Good point!

> 
> > +	} 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))

OK.

> 
> 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.

Please see 729204ef49ec00b ("block: relax check on sg gap"):

    If the last bvec of the 1st bio and the 1st bvec of the next
    bio are physically contigious, and the latter can be merged
    to last segment of the 1st bio, we should think they don't
    violate sg gap(or virt boundary) limit.

    Both Vitaly and Dexuan reported lots of unmergeable small bios
    are observed when running mkfs on Hyper-V virtual storage, and
    performance becomes quite low. This patch fixes that performance
    issue.

    The same issue should exist on NVMe, since it sets virt boundary too.

However, if the related fs code can be fixed to not generate too many
small bios, we might kill the cross-bio segment merge. Also not sure if
mkfs is the only such case.

Thanks,
Ming