[PATCH] block: move sanity checking ahead of bi_front/back_seg_size updating

[PATCH] block: move sanity checking ahead of bi_front/back_seg_size updating

[Jianchao Wang]

If the bio_integrity_merge_rq() return false or nr_phys_segments exceeds
the max_segments, the merging fails, but the bi_front/back_seg_size may
have been modified. To avoid it, move the sanity checking ahead.

Signed-off-by: Jianchao Wang <jianchao.w.wang@oracle.com>
---
 block/blk-merge.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/block/blk-merge.c b/block/blk-merge.c
index 99038830..14b6e37 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -553,6 +553,7 @@ static bool req_no_special_merge(struct request *req)
 static int ll_merge_requests_fn(struct request_queue *q, struct request *req,
 				struct request *next)
 {
+	bool contig;
 	int total_phys_segments;
 	unsigned int seg_size =
 		req->biotail->bi_seg_back_size + next->bio->bi_seg_front_size;
@@ -575,13 +576,9 @@ static int ll_merge_requests_fn(struct request_queue *q, struct request *req,
 		return 0;
 
 	total_phys_segments = req->nr_phys_segments + next->nr_phys_segments;
-	if (blk_phys_contig_segment(q, req->biotail, next->bio)) {
-		if (req->nr_phys_segments == 1)
-			req->bio->bi_seg_front_size = seg_size;
-		if (next->nr_phys_segments == 1)
-			next->biotail->bi_seg_back_size = seg_size;
+	contig = blk_phys_contig_segment(q, req->biotail, next->bio);
+	if (contig)
 		total_phys_segments--;
-	}
 
 	if (total_phys_segments > queue_max_segments(q))
 		return 0;
@@ -589,6 +586,13 @@ static int ll_merge_requests_fn(struct request_queue *q, struct request *req,
 	if (blk_integrity_merge_rq(q, req, next) == false)
 		return 0;
 
+	if (contig) {
+		if (req->nr_phys_segments == 1)
+			req->bio->bi_seg_front_size = seg_size;
+		if (next->nr_phys_segments == 1)
+			next->biotail->bi_seg_back_size = seg_size;
+	}
+
 	/* Merge is OK... */
 	req->nr_phys_segments = total_phys_segments;
 	return 1;
-- 
2.7.4

Re: [PATCH] block: move sanity checking ahead of bi_front/back_seg_size updating

[Christoph Hellwig]

On Sat, Sep 16, 2017 at 07:10:30AM +0800, Jianchao Wang wrote:
> If the bio_integrity_merge_rq() return false or nr_phys_segments exceeds
> the max_segments, the merging fails, but the bi_front/back_seg_size may
> have been modified. To avoid it, move the sanity checking ahead.
> 
> Signed-off-by: Jianchao Wang <jianchao.w.wang@oracle.com>

This looks fine to me:

Reviewed-by: Christoph Hellwig <hch@lst.de>

But can you elaborate a little more on how this found and if there
is a way to easily reproduce it, say for a blktests test case?

Re: [PATCH] block: move sanity checking ahead of bi_front/back_seg_size updating

[jianchao.wang]

On 09/19/2017 07:51 AM, Christoph Hellwig wrote:
> On Sat, Sep 16, 2017 at 07:10:30AM +0800, Jianchao Wang wrote:
>> If the bio_integrity_merge_rq() return false or nr_phys_segments exceeds
>> the max_segments, the merging fails, but the bi_front/back_seg_size may
>> have been modified. To avoid it, move the sanity checking ahead.
>>
>> Signed-off-by: Jianchao Wang <jianchao.w.wang@oracle.com>
> 
> This looks fine to me:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> 
> But can you elaborate a little more on how this found and if there
> is a way to easily reproduce it, say for a blktests test case?
> 
It is found when I made the patch of 
'block: consider merge of segments when merge bio into rq' , not from an actual
issue or test case.

Thanks
Jianchao

Re: [PATCH] block: move sanity checking ahead of bi_front/back_seg_size updating

[Christoph Hellwig]

On Tue, Sep 19, 2017 at 08:55:59AM +0800, jianchao.wang wrote:
> > But can you elaborate a little more on how this found and if there
> > is a way to easily reproduce it, say for a blktests test case?
> > 
> It is found when I made the patch of 
> 'block: consider merge of segments when merge bio into rq' , not from an actual
> issue or test case.

Same question applies to that one, I just haven't finished understanding
all the changes in it yet.

Re: [PATCH] block: move sanity checking ahead of bi_front/back_seg_size updating

[jianchao.wang]

On 09/19/2017 10:36 PM, Christoph Hellwig wrote:
> On Tue, Sep 19, 2017 at 08:55:59AM +0800, jianchao.wang wrote:
>>> But can you elaborate a little more on how this found and if there
>>> is a way to easily reproduce it, say for a blktests test case?
>>>
>> It is found when I made the patch of 
>> 'block: consider merge of segments when merge bio into rq' , not from an actual
>> issue or test case.
> 
> Same question applies to that one, I just haven't finished understanding
> all the changes in it yet.
> 
Really sorry for that. I will elaborate the issue next in this mail thread and add
more comment that describes the issue and result after apply the patch in next version.

The issue is as following:
I executed mkfs.ext4 on my ThinkCentre M910s with a HDD TOSHIBA DT01ACA1.
The kernel version is 4.13.rc7.
The max_segments of the queue is 168, and max_sector_kb is 1280.

In the progress of sequential writing of mkfs, the max size of rq is only 168 sectors which 
is far away from max_sector_kb of 2560 sectors. 
       mkfs.ext4-28362 [007] ....   456.607657: block_unplug: [mkfs.ext4] 16
      mkfs.ext4-28362 [007] ....   456.607658: block_rq_insert: 8,0 WS 86016 () 1875915016 + 168 [mkfs.ext4]
       mkfs.ext4-28362 [007] ....   456.607659: block_rq_insert: 8,0 WS 86016 () 1875915184 + 168 [mkfs.ext4]

Looked into the code, found that ll_back/front_merge_fn() do not consider segment merge across the bios in one rq
, but only in one bio. However, in blk_rq_map_sg(), it will try to merge the segments across the bios in one rq.
I used to trace the return value of blk_rq_map_sg() which is count of mapped segments and got following result:

[ 3158.684851] <scsi_init_sgtable> post_handler: p->addr = 0xffffffff97641a38, ax = 2, nents = a8
[ 3158.684867] <scsi_init_sgtable> post_handler: p->addr = 0xffffffff97641a38, ax = 2, nents = a8
The ax is return value and the nents is the segments count of the sg list.

The 0xa8 segments were merged into 2 segments finally.

To fix this, consider segments merge in ll_back/front_merge_fn() whenever a new bio will to be merged into a rq, then 
could get a more accurate rq->nr_phys_segments and merge more fully.
After apply the patch, I get the result as following:
 mkfs.ext4-1359  [007] ....    78.125055: block_unplug: [mkfs.ext4] 1
       mkfs.ext4-1359  [007] ...1    78.125055: block_rq_insert: 8,0 WS 1310720 () 1875918848 + 2560 [mkfs.ext4]
       mkfs.ext4-1359  [007] ...1    78.125055: block_rq_issue: 8,0 WS 1310720 () 1875918848 + 2560 [mkfs.ext4]

Thanks
Jianchao