Re: [PATCH] block: consider merge of segments when merge bio into rq

[jianchao.wang]

On 09/21/2017 09:29 AM, Christoph Hellwig wrote:
> So the check change here looks good to me.
> 
> I don't like like the duplicate code, can you look into sharing
> the new segment checks between the two functions and the existing
> instance in ll_merge_requests_fn by passing say two struct bio *bio1
> and struct bio *bio2 pointer instead of using req->bio and req->biotail?
> 
> Also please include the information that you posted in the reply to
> the other patch in the description for this one.
> 
I have looked into the ll_back/front_merge_fn() and ll_merge_requests_fn().
It seems that segments check code fragment looks similar but not unified.
We could unify the part of calculation of variable of seg_size and contig,
but not the positions where use these two.
Especially the calculation of total_phys_segments and judgment of 
'nr_phys_segments == 1', rq variable cannot be avoided there. And the update
of bi_seg_front/back_size is also very tricky. If force to merge these code 
fragments together, the code may become hard to read.

So please refer to the V2 patch which contain more comment about the issue
and result.

Thanks
Jianchao

Re: [PATCH] block: consider merge of segments when merge bio into rq

[jianchao.wang]

On 09/21/2017 09:29 AM, Christoph Hellwig wrote:
> So the check change here looks good to me.
> 
> I don't like like the duplicate code, can you look into sharing
> the new segment checks between the two functions and the existing
> instance in ll_merge_requests_fn by passing say two struct bio *bio1
> and struct bio *bio2 pointer instead of using req->bio and req->biotail?
> 
> Also please include the information that you posted in the reply to
> the other patch in the description for this one.
> 

I have just sent a new version which contains more comment before I see
this mail. I will send out another new version that apply your advice later.
Thanks for your kind and fair comment.

Jianchao

Re: [PATCH] block: consider merge of segments when merge bio into rq

[Christoph Hellwig]

So the check change here looks good to me.

I don't like like the duplicate code, can you look into sharing
the new segment checks between the two functions and the existing
instance in ll_merge_requests_fn by passing say two struct bio *bio1
and struct bio *bio2 pointer instead of using req->bio and req->biotail?

Also please include the information that you posted in the reply to
the other patch in the description for this one.

[PATCH] block: consider merge of segments when merge bio into rq

[Jianchao Wang]

When account the nr_phys_segments during merging bios into rq,
only consider segments merging in individual bio but not all
the bios in a rq. This leads to the bigger nr_phys_segments of
rq than the real one when the segments of bios in rq are
contiguous and mergeable. The nr_phys_segments of rq will exceed
max_segmets of q while the sectors of rq maybe far away from
the max_sectors of q.

Consider the segments merge when account nr_phys_segments of rq
during merging bio into rq. Promote the merging of small and
contiguous IO. In addition, it could eliminate the wasting of
scatterlist structure.

Signed-off-by: Jianchao Wang 
---
 block/blk-merge.c | 98 ---
 1 file changed, 64 insertions(+), 34 deletions(-)

diff --git a/block/blk-merge.c b/block/blk-merge.c
index 14b6e37..b2f54fd 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -472,54 +472,60 @@ int blk_rq_map_sg(struct request_queue *q, struct request 
*rq,
 }
 EXPORT_SYMBOL(blk_rq_map_sg);
 
-static inline int ll_new_hw_segment(struct request_queue *q,
-   struct request *req,
-   struct bio *bio)
-{
-   int nr_phys_segs = bio_phys_segments(q, bio);
-
-   if (req->nr_phys_segments + nr_phys_segs > queue_max_segments(q))
-   goto no_merge;
-
-   if (blk_integrity_merge_bio(q, req, bio) == false)
-   goto no_merge;
-
-   /*
-* This will form the start of a new hw segment.  Bump both
-* counters.
-*/
-   req->nr_phys_segments += nr_phys_segs;
-   return 1;
-
-no_merge:
-   req_set_nomerge(q, req);
-   return 0;
-}
-
 int ll_back_merge_fn(struct request_queue *q, struct request *req,
 struct bio *bio)
 {
+   unsigned int seg_size;
+   int total_nr_phys_segs;
+   bool contig;
+
if (req_gap_back_merge(req, bio))
return 0;
if (blk_integrity_rq(req) &&
integrity_req_gap_back_merge(req, bio))
return 0;
if (blk_rq_sectors(req) + bio_sectors(bio) >
-   blk_rq_get_max_sectors(req, blk_rq_pos(req))) {
-   req_set_nomerge(q, req);
-   return 0;
-   }
+   blk_rq_get_max_sectors(req, blk_rq_pos(req)))
+   goto no_merge;
+
if (!bio_flagged(req->biotail, BIO_SEG_VALID))
blk_recount_segments(q, req->biotail);
if (!bio_flagged(bio, BIO_SEG_VALID))
blk_recount_segments(q, bio);
 
-   return ll_new_hw_segment(q, req, bio);
+   if (blk_integrity_merge_bio(q, req, bio) == false)
+   goto no_merge;
+
+   seg_size = req->biotail->bi_seg_back_size + bio->bi_seg_front_size;
+   total_nr_phys_segs = req->nr_phys_segments + bio_phys_segments(q, bio);
+
+   contig = blk_phys_contig_segment(q, req->biotail, bio);
+   if (contig)
+   total_nr_phys_segs--;
+
+   if (unlikely(total_nr_phys_segs > queue_max_segments(q)))
+   goto no_merge;
+
+   if (contig) {
+   if (req->nr_phys_segments == 1)
+   req->bio->bi_seg_front_size = seg_size;
+   if (bio->bi_phys_segments == 1)
+   bio->bi_seg_back_size = seg_size;
+   }
+   req->nr_phys_segments = total_nr_phys_segs;
+   return 1;
+
+no_merge:
+   req_set_nomerge(q, req);
+   return 0;
 }
 
 int ll_front_merge_fn(struct request_queue *q, struct request *req,
  struct bio *bio)
 {
+   unsigned int seg_size;
+   int total_nr_phys_segs;
+   bool contig;
 
if (req_gap_front_merge(req, bio))
return 0;
@@ -527,16 +533,40 @@ int ll_front_merge_fn(struct request_queue *q, struct 
request *req,
integrity_req_gap_front_merge(req, bio))
return 0;
if (blk_rq_sectors(req) + bio_sectors(bio) >
-   blk_rq_get_max_sectors(req, bio->bi_iter.bi_sector)) {
-   req_set_nomerge(q, req);
-   return 0;
-   }
+   blk_rq_get_max_sectors(req, bio->bi_iter.bi_sector))
+   goto no_merge;
+
if (!bio_flagged(bio, BIO_SEG_VALID))
blk_recount_segments(q, bio);
if (!bio_flagged(req->bio, BIO_SEG_VALID))
blk_recount_segments(q, req->bio);
 
-   return ll_new_hw_segment(q, req, bio);
+   if (blk_integrity_merge_bio(q, req, bio) == false)
+   goto no_merge;
+
+   seg_size = req->bio->bi_seg_front_size + bio->bi_seg_back_size;
+   total_nr_phys_segs = req->nr_phys_segments + bio_phys_segments(q, bio);
+
+   contig = blk_phys_contig_segment(q, bio, req->bio);
+   if (contig)
+   total_nr_phys_segs--;
+
+   if (unlikely(total_nr_phys_segs > queue_max_segments(q)))
+   goto no_merge;
+
+   if (contig) {
+   if (req->nr_phys_segments == 1)
+