Skip to contentSkip to conversation

Some clean-ups for bio merge

PATCH RESEND Superseded Baolin Wang 3 participants 21 messages Aug 17, 2020 – Aug 18, 2020 Block layer

v1 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Baolin Wang

author
#0 lore raw

cover Some clean-ups for bio merge

Hi,

There are some duplicated code when trying to merge bio from pluged list and software queue, thus this patch set did some clean-ups when merging a bio. Any comments are welcome. Thanks.

Baolin Wang (5):
block: Move bio merge related functions into blk-merge.c
block: Remove redundant blk_mq_sched_allow_merge() validation
block: Add a new helper to attempt to merge a bio
block: Remove blk_mq_attempt_merge() function
block: Remove __blk_mq_sched_bio_merge() helper

 block/blk-core.c     | 156 -----------------------------------------------
 block/blk-merge.c    | 168 +++++++++++++++++++++++++++++++++++++++++++++++++++
 block/blk-mq-sched.c |  64 ++++++--------------
 block/blk-mq-sched.h |  13 +---
 block/blk.h          |   9 +++
 5 files changed, 197 insertions(+), 213 deletions(-)
Signature · 1 line
1.8.3.1
author
#1 lore raw

PATCH RESEND 1/5 block: Move bio merge related functions into blk-merge.c

It's better to move bio merge related functions into blk-merge.c, which contains all merge related functions.

Signed-off-by Baolin Wang <[email protected]>
---
 block/blk-core.c  | 156 -----------------------------------------------------
 block/blk-merge.c | 157 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 157 insertions(+), 156 deletions(-)
block/blk-core.c +0 -156
index d9d6326..ed79109 100644
@@ -642,162 +642,6 @@ void blk_put_request(struct request *req)
 }
 EXPORT_SYMBOL(blk_put_request);
 
-static void blk_account_io_merge_bio(struct request *req)
-{
-	if (!blk_do_io_stat(req))
-		return;
-
-	part_stat_lock();
-	part_stat_inc(req->part, merges[op_stat_group(req_op(req))]);
-	part_stat_unlock();
-}
-
-bool bio_attempt_back_merge(struct request *req, struct bio *bio,
-		unsigned int nr_segs)
-{
-	const int ff = bio->bi_opf & REQ_FAILFAST_MASK;
-
-	if (!ll_back_merge_fn(req, bio, nr_segs))
-		return false;
-
-	trace_block_bio_backmerge(req->q, req, bio);
-	rq_qos_merge(req->q, req, bio);
-
-	if ((req->cmd_flags & REQ_FAILFAST_MASK) != ff)
-		blk_rq_set_mixed_merge(req);
-
-	req->biotail->bi_next = bio;
-	req->biotail = bio;
-	req->__data_len += bio->bi_iter.bi_size;
-
-	bio_crypt_free_ctx(bio);
-
-	blk_account_io_merge_bio(req);
-	return true;
-}
-
-bool bio_attempt_front_merge(struct request *req, struct bio *bio,
-		unsigned int nr_segs)
-{
-	const int ff = bio->bi_opf & REQ_FAILFAST_MASK;
-
-	if (!ll_front_merge_fn(req, bio, nr_segs))
-		return false;
-
-	trace_block_bio_frontmerge(req->q, req, bio);
-	rq_qos_merge(req->q, req, bio);
-
-	if ((req->cmd_flags & REQ_FAILFAST_MASK) != ff)
-		blk_rq_set_mixed_merge(req);
-
-	bio->bi_next = req->bio;
-	req->bio = bio;
-
-	req->__sector = bio->bi_iter.bi_sector;
-	req->__data_len += bio->bi_iter.bi_size;
-
-	bio_crypt_do_front_merge(req, bio);
-
-	blk_account_io_merge_bio(req);
-	return true;
-}
-
-bool bio_attempt_discard_merge(struct request_queue *q, struct request *req,
-		struct bio *bio)
-{
-	unsigned short segments = blk_rq_nr_discard_segments(req);
-
-	if (segments >= queue_max_discard_segments(q))
-		goto no_merge;
-	if (blk_rq_sectors(req) + bio_sectors(bio) >
-	    blk_rq_get_max_sectors(req, blk_rq_pos(req)))
-		goto no_merge;
-
-	rq_qos_merge(q, req, bio);
-
-	req->biotail->bi_next = bio;
-	req->biotail = bio;
-	req->__data_len += bio->bi_iter.bi_size;
-	req->nr_phys_segments = segments + 1;
-
-	blk_account_io_merge_bio(req);
-	return true;
-no_merge:
-	req_set_nomerge(q, req);
-	return false;
-}
-
-/**
- * blk_attempt_plug_merge - try to merge with %current's plugged list
- * @q: request_queue new bio is being queued at
- * @bio: new bio being queued
- * @nr_segs: number of segments in @bio
- * @same_queue_rq: pointer to &struct request that gets filled in when
- * another request associated with @q is found on the plug list
- * (optional, may be %NULL)
- *
- * Determine whether @bio being queued on @q can be merged with a request
- * on %current's plugged list.  Returns %true if merge was successful,
- * otherwise %false.
- *
- * Plugging coalesces IOs from the same issuer for the same purpose without
- * going through @q->queue_lock.  As such it's more of an issuing mechanism
- * than scheduling, and the request, while may have elvpriv data, is not
- * added on the elevator at this point.  In addition, we don't have
- * reliable access to the elevator outside queue lock.  Only check basic
- * merging parameters without querying the elevator.
- *
- * Caller must ensure !blk_queue_nomerges(q) beforehand.
- */
-bool blk_attempt_plug_merge(struct request_queue *q, struct bio *bio,
-		unsigned int nr_segs, struct request **same_queue_rq)
-{
-	struct blk_plug *plug;
-	struct request *rq;
-	struct list_head *plug_list;
-
-	plug = blk_mq_plug(q, bio);
-	if (!plug)
-		return false;
-
-	plug_list = &plug->mq_list;
-
-	list_for_each_entry_reverse(rq, plug_list, queuelist) {
-		bool merged = false;
-
-		if (rq->q == q && same_queue_rq) {
-			/*
-			 * Only blk-mq multiple hardware queues case checks the
-			 * rq in the same queue, there should be only one such
-			 * rq in a queue
-			 **/
-			*same_queue_rq = rq;
-		}
-
-		if (rq->q != q || !blk_rq_merge_ok(rq, bio))
-			continue;
-
-		switch (blk_try_merge(rq, bio)) {
-		case ELEVATOR_BACK_MERGE:
-			merged = bio_attempt_back_merge(rq, bio, nr_segs);
-			break;
-		case ELEVATOR_FRONT_MERGE:
-			merged = bio_attempt_front_merge(rq, bio, nr_segs);
-			break;
-		case ELEVATOR_DISCARD_MERGE:
-			merged = bio_attempt_discard_merge(q, rq, bio);
-			break;
-		default:
-			break;
-		}
-
-		if (merged)
-			return true;
-	}
-
-	return false;
-}
-
 static void handle_bad_sector(struct bio *bio, sector_t maxsector)
 {
 	char b[BDEVNAME_SIZE];
block/blk-merge.c +157 -0

166 lines not on this page · raw message

Signature · 1 line
1.8.3.1
Reviewed-by on v1 1/5
#2 lore raw

Re: [PATCH RESEND 1/5] block: Move bio merge related functions into blk-merge.c

Quoted 8 lines · Baolin Wang

It's better to move bio merge related functions into blk-merge.c, which contains all merge related functions.

Signed-off-by Baolin Wang <[email protected]>
---
 block/blk-core.c  | 156 -----------------------------------------------------
 block/blk-merge.c | 157 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 157 insertions(+), 156 deletions(-)

Looks fine:

Reviewed-by Christoph Hellwig <[email protected]>
author
#3 lore raw

PATCH RESEND 2/5 block: Remove redundant blk_mq_sched_allow_merge() validation

In reply to Baolin Wang

Only software queue and kyber IO scheduler will call blk_mq_bio_list_merge() to merge a bio, and kyber IO scheduler did not implement the ops->allow_merge(). Thus we can remove the redundant blk_mq_sched_allow_merge() in blk_mq_bio_list_merge() function.

Signed-off-by Baolin Wang <[email protected]>
---
 block/blk-mq-sched.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)
block/blk-mq-sched.c +2 -6
index a19cdf1..bf62b34 100644
@@ -403,14 +403,10 @@ bool blk_mq_bio_list_merge(struct request_queue *q, struct list_head *list,
 
 		switch (blk_try_merge(rq, bio)) {
 		case ELEVATOR_BACK_MERGE:
-			if (blk_mq_sched_allow_merge(q, rq, bio))
-				merged = bio_attempt_back_merge(rq, bio,
-						nr_segs);
+			merged = bio_attempt_back_merge(rq, bio, nr_segs);
 			break;
 		case ELEVATOR_FRONT_MERGE:
-			if (blk_mq_sched_allow_merge(q, rq, bio))
-				merged = bio_attempt_front_merge(rq, bio,
-						nr_segs);
+			merged = bio_attempt_front_merge(rq, bio, nr_segs);
 			break;
 		case ELEVATOR_DISCARD_MERGE:
 			merged = bio_attempt_discard_merge(q, rq, bio);
Signature · 1 line
1.8.3.1
#4 lore raw

Re: [PATCH RESEND 2/5] block: Remove redundant blk_mq_sched_allow_merge() validation

Quoted 2 lines · Baolin Wang

Only software queue and kyber IO scheduler will call blk_mq_bio_list_merge() to merge a bio, and kyber IO scheduler did not implement the ops->allow_merge().

Please line wrap your changelogs after 73 characters.

Quoted 2 lines

Thus we can remove the redundant blk_mq_sched_allow_merge() in blk_mq_bio_list_merge() function.

This seems potentially dangerous, and I don't think it really buys us much.

author
#5 lore raw

Re: [PATCH RESEND 2/5] block: Remove redundant blk_mq_sched_allow_merge() validation

Quoted 5 lines · Christoph Hellwig
Quoted 2 lines · Baolin Wang

Only software queue and kyber IO scheduler will call blk_mq_bio_list_merge() to merge a bio, and kyber IO scheduler did not implement the ops->allow_merge().

Please line wrap your changelogs after 73 characters.

Sure.

Quoted 6 lines
Quoted 3 lines

Thus we can remove the redundant blk_mq_sched_allow_merge() in blk_mq_bio_list_merge() function.

This seems potentially dangerous, and I don't think it really buys us much.

OK. I can remove this in next version.

author
#6 lore raw

PATCH RESEND 3/5 block: Add a new helper to attempt to merge a bio

In reply to Baolin Wang

There are lots of duplicated code when trying to merge a bio from plug list and sw queue, we can introduce a new helper to attempt to merge a bio, which can simplify the blk_mq_bio_list_merge() and blk_attempt_plug_merge().

Signed-off-by Baolin Wang <[email protected]>
---
 block/blk-merge.c    | 47 +++++++++++++++++++++++++++++------------------
 block/blk-mq-sched.c | 22 ++++------------------
 block/blk.h          |  9 +++++++++
 3 files changed, 42 insertions(+), 36 deletions(-)
block/blk-merge.c +29 -18
index 3619f2f..a8d1649 100644
@@ -975,6 +975,33 @@ bool bio_attempt_discard_merge(struct request_queue *q, struct request *req,
 	return false;
 }
 
+enum bio_merge_status blk_attempt_bio_merge(struct request_queue *q,
+					    struct request *rq,
+					    struct bio *bio,
+					    unsigned int nr_segs)
+{
+	bool merged = false;
+
+	if (!blk_rq_merge_ok(rq, bio))
+		return BIO_MERGE_NONE;
+
+	switch (blk_try_merge(rq, bio)) {
+	case ELEVATOR_BACK_MERGE:
+		merged = bio_attempt_back_merge(rq, bio, nr_segs);
+		break;
+	case ELEVATOR_FRONT_MERGE:
+		merged = bio_attempt_front_merge(rq, bio, nr_segs);
+		break;
+	case ELEVATOR_DISCARD_MERGE:
+		merged = bio_attempt_discard_merge(q, rq, bio);
+		break;
+	default:
+		return BIO_MERGE_NONE;
+	}
+
+	return merged ? BIO_MERGE_OK : BIO_MERGE_FAILED;
+}
+
 /**
  * blk_attempt_plug_merge - try to merge with %current's plugged list
  * @q: request_queue new bio is being queued at

@@ -1011,8 +1038,6 @@ bool blk_attempt_plug_merge(struct request_queue *q, struct bio *bio,
 	plug_list = &plug->mq_list;
 
 	list_for_each_entry_reverse(rq, plug_list, queuelist) {
-		bool merged = false;
-
 		if (rq->q == q && same_queue_rq) {
 			/*
 			 * Only blk-mq multiple hardware queues case checks the

@@ -1022,24 +1047,10 @@ bool blk_attempt_plug_merge(struct request_queue *q, struct bio *bio,
 			*same_queue_rq = rq;
 		}
 
-		if (rq->q != q || !blk_rq_merge_ok(rq, bio))
+		if (rq->q != q)
 			continue;
 
-		switch (blk_try_merge(rq, bio)) {
-		case ELEVATOR_BACK_MERGE:
-			merged = bio_attempt_back_merge(rq, bio, nr_segs);
-			break;
-		case ELEVATOR_FRONT_MERGE:
-			merged = bio_attempt_front_merge(rq, bio, nr_segs);
-			break;
-		case ELEVATOR_DISCARD_MERGE:
-			merged = bio_attempt_discard_merge(q, rq, bio);
-			break;
-		default:
-			break;
-		}
-
-		if (merged)
+		if (blk_attempt_bio_merge(q, rq, bio, nr_segs) == BIO_MERGE_OK)
 			return true;
 	}
block/blk-mq-sched.c +4 -18
index bf62b34..8e9bafe 100644
@@ -391,31 +391,17 @@ bool blk_mq_bio_list_merge(struct request_queue *q, struct list_head *list,
 {
 	struct request *rq;
 	int checked = 8;
+	enum bio_merge_status merge;
 
 	list_for_each_entry_reverse(rq, list, queuelist) {
-		bool merged = false;
-
 		if (!checked--)
 			break;
 
-		if (!blk_rq_merge_ok(rq, bio))
+		merge = blk_attempt_bio_merge(q, rq, bio, nr_segs);
+		if (merge == BIO_MERGE_NONE)
 			continue;
 
-		switch (blk_try_merge(rq, bio)) {
-		case ELEVATOR_BACK_MERGE:
-			merged = bio_attempt_back_merge(rq, bio, nr_segs);
-			break;
-		case ELEVATOR_FRONT_MERGE:
-			merged = bio_attempt_front_merge(rq, bio, nr_segs);
-			break;
-		case ELEVATOR_DISCARD_MERGE:
-			merged = bio_attempt_discard_merge(q, rq, bio);
-			break;
-		default:
-			continue;
-		}
-
-		return merged;
+		return merge == BIO_MERGE_OK ? true: false;
 	}
 
 	return false;
block/blk.h +9 -0
index 49e2928..a6c54e1 100644
@@ -234,6 +234,15 @@ int blk_attempt_req_merge(struct request_queue *q, struct request *rq,
 bool blk_rq_merge_ok(struct request *rq, struct bio *bio);
 enum elv_merge blk_try_merge(struct request *rq, struct bio *bio);
 
+enum bio_merge_status {
+	BIO_MERGE_OK,
+	BIO_MERGE_NONE,
+	BIO_MERGE_FAILED,
+};
+
+enum bio_merge_status blk_attempt_bio_merge(struct request_queue *q,
+		struct request *rq, struct bio *bio, unsigned int nr_segs);
+
 int blk_dev_init(void);
 
 /*
Signature · 1 line
1.8.3.1
#7 lore raw

Re: [PATCH RESEND 3/5] block: Add a new helper to attempt to merge a bio

Quoted 4 lines · Baolin Wang

There are lots of duplicated code when trying to merge a bio from plug list and sw queue, we can introduce a new helper to attempt to merge a bio, which can simplify the blk_mq_bio_list_merge() and blk_attempt_plug_merge().

Looks sensible, but two comments:

Quoted 10 lines
block/blk-merge.c +10 -0
+enum bio_merge_status blk_attempt_bio_merge(struct request_queue *q,
+					    struct request *rq,
+					    struct bio *bio,
+					    unsigned int nr_segs)
+{
+	bool merged = false;
+
+	if (!blk_rq_merge_ok(rq, bio))
+		return BIO_MERGE_NONE;
+

block/blk-merge.c bool bio_attempt_discard_merge(struct request_queue *q, struct request *req, · line 997

Quoted 10 lines
block/blk-merge.c +10 -0
+	switch (blk_try_merge(rq, bio)) {
+	case ELEVATOR_BACK_MERGE:
+		merged = bio_attempt_back_merge(rq, bio, nr_segs);
+		break;
+	case ELEVATOR_FRONT_MERGE:
+		merged = bio_attempt_front_merge(rq, bio, nr_segs);
+		break;
+	case ELEVATOR_DISCARD_MERGE:
+		merged = bio_attempt_discard_merge(q, rq, bio);
+		break;

Can't we also switch the bio_attempt_*merge helpers to return enum bio_merge_status to simplify this a bit?

Also I think these helpers can be marked static now, although I didn't actually apply your series, so I might have missed something.

Quoted 8 lines

+++ b/block/blk-mq-sched.c

block/blk-mq-sched.c +1 -0
@@ -391,31 +391,17 @@ bool blk_mq_bio_list_merge(struct request_queue *q, struct list_head *list,
 {
 	struct request *rq;
 	int checked = 8;
+	enum bio_merge_status merge;
 
 	list_for_each_entry_reverse(rq, list, queuelist) {

block/blk-mq-sched.c bool blk_mq_bio_list_merge(struct request_queue *q, struct list_head *list, · line 404

Quoted 10 lines
block/blk-mq-sched.c +3 -2
-		bool merged = false;
-
 		if (!checked--)
 			break;
 
+		merge = blk_attempt_bio_merge(q, rq, bio, nr_segs);
+		if (merge == BIO_MERGE_NONE)
 			continue;
 
+		return merge == BIO_MERGE_OK ? true: false;

Maybe write this a little more explicit:

		switch (blk_attempt_bio_merge(q, rq, bio, nr_segs)) {
		case BIO_MERGE_NONE:
			continue:
		case BIO_MERGE_OK:
			return true;
		case BIO_MERGE_FAILED:
			return false;
		}
Quoted 16 lines
block/blk.h +9 -0
 enum elv_merge blk_try_merge(struct request *rq, struct bio *bio);
 
+enum bio_merge_status {
+	BIO_MERGE_OK,
+	BIO_MERGE_NONE,
+	BIO_MERGE_FAILED,
+};
+
+enum bio_merge_status blk_attempt_bio_merge(struct request_queue *q,
+		struct request *rq, struct bio *bio, unsigned int nr_segs);
+
 int blk_dev_init(void);
 
 /*
Signature · 1 line
1.8.3.1

---end quoted text---

author
#8 lore raw

Re: [PATCH RESEND 3/5] block: Add a new helper to attempt to merge a bio

Quoted 23 lines · Christoph Hellwig
Quoted 4 lines · Baolin Wang

There are lots of duplicated code when trying to merge a bio from plug list and sw queue, we can introduce a new helper to attempt to merge a bio, which can simplify the blk_mq_bio_list_merge() and blk_attempt_plug_merge().

Looks sensible, but two comments:

Quoted 15 lines
block/blk-merge.c +15 -0
+enum bio_merge_status blk_attempt_bio_merge(struct request_queue *q,
+					    struct request *rq,
+					    struct bio *bio,
+					    unsigned int nr_segs)
+{
+	bool merged = false;
+
+	if (!blk_rq_merge_ok(rq, bio))
+		return BIO_MERGE_NONE;
+
+	switch (blk_try_merge(rq, bio)) {
+	case ELEVATOR_BACK_MERGE:
+		merged = bio_attempt_back_merge(rq, bio, nr_segs);
+		break;
+	case ELEVATOR_FRONT_MERGE:
Quoted 8 lines

block/blk-merge.c bool bio_attempt_discard_merge(struct request_queue *q, struct request *req, · line 997

Quoted 5 lines
block/blk-merge.c +5 -0
+		merged = bio_attempt_front_merge(rq, bio, nr_segs);
+		break;
+	case ELEVATOR_DISCARD_MERGE:
+		merged = bio_attempt_discard_merge(q, rq, bio);
+		break;

Can't we also switch the bio_attempt_*merge helpers to return enum bio_merge_status to simplify this a bit?

Yes, will do.

Quoted 3 lines

Also I think these helpers can be marked static now, although I didn't actually apply your series, so I might have missed something.

Cause this function will be used by blk_mq_bio_list_merge() in blk-mq-sched.c, it should be exported.

Quoted 22 lines
Quoted 9 lines

+++ b/block/blk-mq-sched.c

block/blk-mq-sched.c +1 -0
@@ -391,31 +391,17 @@ bool blk_mq_bio_list_merge(struct request_queue *q, struct list_head *list,
 {
 	struct request *rq;
 	int checked = 8;
+	enum bio_merge_status merge;
 
 	list_for_each_entry_reverse(rq, list, queuelist) {

block/blk-mq-sched.c bool blk_mq_bio_list_merge(struct request_queue *q, struct list_head *list, · line 404

Quoted 10 lines
block/blk-mq-sched.c +3 -2
-		bool merged = false;
-
 		if (!checked--)
 			break;
 
+		merge = blk_attempt_bio_merge(q, rq, bio, nr_segs);
+		if (merge == BIO_MERGE_NONE)
 			continue;
 
+		return merge == BIO_MERGE_OK ? true: false;

Maybe write this a little more explicit:

Quoted 8 lines
		switch (blk_attempt_bio_merge(q, rq, bio, nr_segs)) {
		case BIO_MERGE_NONE:
			continue:
		case BIO_MERGE_OK:
			return true;
		case BIO_MERGE_FAILED:
			return false;
		}

Sure.

Quoted 18 lines
Quoted 17 lines
block/blk.h +9 -0
 enum elv_merge blk_try_merge(struct request *rq, struct bio *bio);
 
+enum bio_merge_status {
+	BIO_MERGE_OK,
+	BIO_MERGE_NONE,
+	BIO_MERGE_FAILED,
+};
+
+enum bio_merge_status blk_attempt_bio_merge(struct request_queue *q,
+		struct request *rq, struct bio *bio, unsigned int nr_segs);
+
 int blk_dev_init(void);
 
 /*
Signature · 1 line
1.8.3.1

---end quoted text---

#9 lore raw

Re: [PATCH RESEND 3/5] block: Add a new helper to attempt to merge a bio

Quoted 33 lines · Baolin Wang
Quoted 23 lines · Christoph Hellwig
Quoted 4 lines · Baolin Wang

There are lots of duplicated code when trying to merge a bio from plug list and sw queue, we can introduce a new helper to attempt to merge a bio, which can simplify the blk_mq_bio_list_merge() and blk_attempt_plug_merge().

Looks sensible, but two comments:

Quoted 15 lines
block/blk-merge.c +15 -0
+enum bio_merge_status blk_attempt_bio_merge(struct request_queue *q,
+					    struct request *rq,
+					    struct bio *bio,
+					    unsigned int nr_segs)
+{
+	bool merged = false;
+
+	if (!blk_rq_merge_ok(rq, bio))
+		return BIO_MERGE_NONE;
+
+	switch (blk_try_merge(rq, bio)) {
+	case ELEVATOR_BACK_MERGE:
+		merged = bio_attempt_back_merge(rq, bio, nr_segs);
+		break;
+	case ELEVATOR_FRONT_MERGE:
Quoted 8 lines

block/blk-merge.c bool bio_attempt_discard_merge(struct request_queue *q, struct request *req, · line 997

Quoted 5 lines
block/blk-merge.c +5 -0
+		merged = bio_attempt_front_merge(rq, bio, nr_segs);
+		break;
+	case ELEVATOR_DISCARD_MERGE:
+		merged = bio_attempt_discard_merge(q, rq, bio);
+		break;

Can't we also switch the bio_attempt_*merge helpers to return enum bio_merge_status to simplify this a bit?

Quoted 8 lines

Yes, will do.

Quoted 3 lines

Also I think these helpers can be marked static now, although I didn't actually apply your series, so I might have missed something.

Cause this function will be used by blk_mq_bio_list_merge() in blk-mq-sched.c, it should be exported.

Shouldn't blk_mq_bio_list_merge move to blk-merge.c as well?

author
#10 lore raw

Re: [PATCH RESEND 3/5] block: Add a new helper to attempt to merge a bio

Quoted 37 lines · Christoph Hellwig
Quoted 36 lines · Baolin Wang
Quoted 23 lines · Christoph Hellwig
Quoted 4 lines · Baolin Wang

There are lots of duplicated code when trying to merge a bio from plug list and sw queue, we can introduce a new helper to attempt to merge a bio, which can simplify the blk_mq_bio_list_merge() and blk_attempt_plug_merge().

Looks sensible, but two comments:

Quoted 15 lines
block/blk-merge.c +15 -0
+enum bio_merge_status blk_attempt_bio_merge(struct request_queue *q,
+					    struct request *rq,
+					    struct bio *bio,
+					    unsigned int nr_segs)
+{
+	bool merged = false;
+
+	if (!blk_rq_merge_ok(rq, bio))
+		return BIO_MERGE_NONE;
+
+	switch (blk_try_merge(rq, bio)) {
+	case ELEVATOR_BACK_MERGE:
+		merged = bio_attempt_back_merge(rq, bio, nr_segs);
+		break;
+	case ELEVATOR_FRONT_MERGE:
Quoted 8 lines

block/blk-merge.c bool bio_attempt_discard_merge(struct request_queue *q, struct request *req, · line 997

Quoted 5 lines
block/blk-merge.c +5 -0
+		merged = bio_attempt_front_merge(rq, bio, nr_segs);
+		break;
+	case ELEVATOR_DISCARD_MERGE:
+		merged = bio_attempt_discard_merge(q, rq, bio);
+		break;

Can't we also switch the bio_attempt_*merge helpers to return enum bio_merge_status to simplify this a bit?

Yes, will do.

Quoted 1 line
Quoted 7 lines
Quoted 5 lines
Quoted 2 lines

Also I think these helpers can be marked static now, although I didn't actually apply your series, so I might have missed something.

Cause this function will be used by blk_mq_bio_list_merge() in blk-mq-sched.c, it should be exported.

Shouldn't blk_mq_bio_list_merge move to blk-merge.c as well?

Yes, I can move it to blk-merge.c and rename to a generic name.

build bot
#11 lore raw

[PATCH] block: fix boolconv.cocci warnings

In reply to Baolin Wang
block/blk-mq-sched.c:404:39-44: WARNING: conversion to bool not needed here

From: kernel test robot <[email protected]>

block/blk-mq-sched.c:404:39-44: WARNING: conversion to bool not needed here

Remove unneeded conversion to bool

Semantic patch information:
Relational and logical operators evaluate to bool, explicit conversion is overly verbose and unneeded.

Generated by: scripts/coccinelle/misc/boolconv.cocci

CC Baolin Wang <[email protected]>
Signed-off-by kernel test robot <[email protected]>

---

url:    https://github.com/0day-ci/linux/commits/Baolin-Wang/Some-clean-ups-for-bio-merge/20200817-121114
base:   https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git for-next

Please take the patch only if it's a positive warning. Thanks!

 blk-mq-sched.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
block/blk-mq-sched.c +1 -1
@@ -401,7 +401,7 @@ bool blk_mq_bio_list_merge(struct reques
 		if (merge == BIO_MERGE_NONE)
 			continue;
 
-		return merge == BIO_MERGE_OK ? true: false;
+		return merge == BIO_MERGE_OK;
 	}
 
 	return false;
build bot
#12 lore raw

Re: [PATCH RESEND 3/5] block: Add a new helper to attempt to merge a bio

In reply to Baolin Wang
I love your patch! Perhaps something to improve:

Hi Baolin,

I love your patch! Perhaps something to improve:

[auto build test WARNING on block/for-next]
[also build test WARNING on v5.9-rc1 next-20200817]
[If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Baolin-Wang/Some-clean-ups-for-bio-merge/20200817-121114
base:   https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git for-next
config: arc-randconfig-c004-20200817 (attached as .config)
compiler: arc-elf-gcc (GCC) 9.3.0

If you fix the issue, kindly add following tag as appropriate

Reported-by kernel test robot <[email protected]>

coccinelle warnings: (new ones prefixed by >>)

>> block/blk-mq-sched.c:404:39-44: WARNING: conversion to bool not needed here

Please review and possibly fold the followup patch.

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

author
#13 lore raw

PATCH RESEND 4/5 block: Remove blk_mq_attempt_merge() function

In reply to Baolin Wang

The small blk_mq_attempt_merge() function is only called by __blk_mq_sched_bio_merge(), just open code it.

Signed-off-by Baolin Wang <[email protected]>
---
 block/blk-mq-sched.c | 33 ++++++++++-----------------------
 1 file changed, 10 insertions(+), 23 deletions(-)
block/blk-mq-sched.c +10 -23
index 8e9bafe..1cc7919 100644
@@ -408,28 +408,6 @@ bool blk_mq_bio_list_merge(struct request_queue *q, struct list_head *list,
 }
 EXPORT_SYMBOL_GPL(blk_mq_bio_list_merge);
 
-/*
- * Reverse check our software queue for entries that we could potentially
- * merge with. Currently includes a hand-wavy stop count of 8, to not spend
- * too much time checking for merges.
- */
-static bool blk_mq_attempt_merge(struct request_queue *q,
-				 struct blk_mq_hw_ctx *hctx,
-				 struct blk_mq_ctx *ctx, struct bio *bio,
-				 unsigned int nr_segs)
-{
-	enum hctx_type type = hctx->type;
-
-	lockdep_assert_held(&ctx->lock);
-
-	if (blk_mq_bio_list_merge(q, &ctx->rq_lists[type], bio, nr_segs)) {
-		ctx->rq_merged++;
-		return true;
-	}
-
-	return false;
-}
-
 bool __blk_mq_sched_bio_merge(struct request_queue *q, struct bio *bio,
 		unsigned int nr_segs)
 {

@@ -447,7 +425,16 @@ bool __blk_mq_sched_bio_merge(struct request_queue *q, struct bio *bio,
 			!list_empty_careful(&ctx->rq_lists[type])) {
 		/* default per sw-queue merge */
 		spin_lock(&ctx->lock);
-		ret = blk_mq_attempt_merge(q, hctx, ctx, bio, nr_segs);
+		/*
+		 * Reverse check our software queue for entries that we could
+		 * potentially merge with. Currently includes a hand-wavy stop
+		 * count of 8, to not spend too much time checking for merges.
+		 */
+		if (blk_mq_bio_list_merge(q, &ctx->rq_lists[type], bio, nr_segs)) {
+			ctx->rq_merged++;
+			ret = true;
+		}
+
 		spin_unlock(&ctx->lock);
 	}
Signature · 1 line
1.8.3.1
#14 lore raw

Re: [PATCH RESEND 4/5] block: Remove blk_mq_attempt_merge() function

Quoted 8 lines · Baolin Wang
block/blk-mq-sched.c +1 -1
 		unsigned int nr_segs)
 {

@@ -447,7 +425,16 @@ bool __blk_mq_sched_bio_merge(struct request_queue *q, struct bio *bio,
 			!list_empty_careful(&ctx->rq_lists[type])) {
 		/* default per sw-queue merge */
 		spin_lock(&ctx->lock);
-		ret = blk_mq_attempt_merge(q, hctx, ctx, bio, nr_segs);
+		/*

block/blk-mq-sched.c bool __blk_mq_sched_bio_merge(struct request_queue *q, struct bio *bio, · line 438

Quoted 10 lines
block/blk-mq-sched.c +9 -0
+		 * Reverse check our software queue for entries that we could
+		 * potentially merge with. Currently includes a hand-wavy stop
+		 * count of 8, to not spend too much time checking for merges.
+		 */
+		if (blk_mq_bio_list_merge(q, &ctx->rq_lists[type], bio, nr_segs)) {
+			ctx->rq_merged++;
+			ret = true;
+		}
+
 		spin_unlock(&ctx->lock);

This adds an overly long line. That being said the whole thing could be nicely simplified to:

	...

	if (e && e->type->ops.bio_merge)
		return e->type->ops.bio_merge(hctx, bio, nr_segs);

	if (!(hctx->flags & BLK_MQ_F_SHOULD_MERGE) ||
	    list_empty_careful(&ctx->rq_lists[hctx->type]))
		return false;

	/*
	 * Reverse check our software queue for entries that we could
	 * potentially merge with. Currently includes a hand-wavy stop count of
	 * 8, to not spend too much time checking for merges.
	 */
	spin_lock(&ctx->lock);
	ret = blk_mq_bio_list_merge(q, &ctx->rq_lists[type], bio, nr_segs);
	if (ret)
		ctx->rq_merged++;
	spin_unlock(&ctx->lock);

Also I think it would make sense to move the locking into blk_mq_bio_list_merge.

author
#15 lore raw

Re: [PATCH RESEND 4/5] block: Remove blk_mq_attempt_merge() function

Quoted 43 lines · Christoph Hellwig
Quoted 8 lines · Baolin Wang
block/blk-mq-sched.c +1 -1
 		unsigned int nr_segs)
 {

@@ -447,7 +425,16 @@ bool __blk_mq_sched_bio_merge(struct request_queue *q, struct bio *bio,
 			!list_empty_careful(&ctx->rq_lists[type])) {
 		/* default per sw-queue merge */
 		spin_lock(&ctx->lock);
-		ret = blk_mq_attempt_merge(q, hctx, ctx, bio, nr_segs);
+		/*

block/blk-mq-sched.c bool __blk_mq_sched_bio_merge(struct request_queue *q, struct bio *bio, · line 438

Quoted 10 lines
block/blk-mq-sched.c +9 -0
+		 * Reverse check our software queue for entries that we could
+		 * potentially merge with. Currently includes a hand-wavy stop
+		 * count of 8, to not spend too much time checking for merges.
+		 */
+		if (blk_mq_bio_list_merge(q, &ctx->rq_lists[type], bio, nr_segs)) {
+			ctx->rq_merged++;
+			ret = true;
+		}
+
 		spin_unlock(&ctx->lock);

This adds an overly long line. That being said the whole thing could be nicely simplified to:

	...

	if (e && e->type->ops.bio_merge)
		return e->type->ops.bio_merge(hctx, bio, nr_segs);

	if (!(hctx->flags & BLK_MQ_F_SHOULD_MERGE) ||
	    list_empty_careful(&ctx->rq_lists[hctx->type]))
		return false;

	/*
	 * Reverse check our software queue for entries that we could
	 * potentially merge with. Currently includes a hand-wavy stop count of
	 * 8, to not spend too much time checking for merges.
	 */
	spin_lock(&ctx->lock);
	ret = blk_mq_bio_list_merge(q, &ctx->rq_lists[type], bio, nr_segs);
	if (ret)
		ctx->rq_merged++;
	spin_unlock(&ctx->lock);
Quoted 2 lines

Also I think it would make sense to move the locking into blk_mq_bio_list_merge.

Sure, will do in next version.

author
#16 lore raw

PATCH RESEND 5/5 block: Remove __blk_mq_sched_bio_merge() helper

In reply to Baolin Wang

The blk_mq_sched_bio_merge() just wrap the __blk_mq_sched_bio_merge(), and no other places will use __blk_mq_sched_bio_merge(). Thus we can combine these 2 similar functions into one function.

Signed-off-by Baolin Wang <[email protected]>
---
 block/blk-mq-sched.c |  5 ++++-
 block/blk-mq-sched.h | 13 ++-----------
 2 files changed, 6 insertions(+), 12 deletions(-)
block/blk-mq-sched.c +4 -1
index 1cc7919..ba34460 100644
@@ -408,7 +408,7 @@ bool blk_mq_bio_list_merge(struct request_queue *q, struct list_head *list,
 }
 EXPORT_SYMBOL_GPL(blk_mq_bio_list_merge);
 
-bool __blk_mq_sched_bio_merge(struct request_queue *q, struct bio *bio,
+bool blk_mq_sched_bio_merge(struct request_queue *q, struct bio *bio,
 		unsigned int nr_segs)
 {
 	struct elevator_queue *e = q->elevator;

@@ -417,6 +417,9 @@ bool __blk_mq_sched_bio_merge(struct request_queue *q, struct bio *bio,
 	bool ret = false;
 	enum hctx_type type;
 
+	if (blk_queue_nomerges(q) || !bio_mergeable(bio))
+		return false;
+
 	if (e && e->type->ops.bio_merge)
 		return e->type->ops.bio_merge(hctx, bio, nr_segs);
block/blk-mq-sched.h +2 -11
index 126021f..65151de 100644
@@ -13,8 +13,6 @@ void blk_mq_sched_free_hctx_data(struct request_queue *q,
 void blk_mq_sched_request_inserted(struct request *rq);
 bool blk_mq_sched_try_merge(struct request_queue *q, struct bio *bio,
 		unsigned int nr_segs, struct request **merged_request);
-bool __blk_mq_sched_bio_merge(struct request_queue *q, struct bio *bio,
-		unsigned int nr_segs);
 bool blk_mq_sched_try_insert_merge(struct request_queue *q, struct request *rq);
 void blk_mq_sched_mark_restart_hctx(struct blk_mq_hw_ctx *hctx);
 void blk_mq_sched_restart(struct blk_mq_hw_ctx *hctx);

@@ -31,15 +29,8 @@ void blk_mq_sched_insert_requests(struct blk_mq_hw_ctx *hctx,
 void blk_mq_exit_sched(struct request_queue *q, struct elevator_queue *e);
 void blk_mq_sched_free_requests(struct request_queue *q);
 
-static inline bool
-blk_mq_sched_bio_merge(struct request_queue *q, struct bio *bio,
-		unsigned int nr_segs)
-{
-	if (blk_queue_nomerges(q) || !bio_mergeable(bio))
-		return false;
-
-	return __blk_mq_sched_bio_merge(q, bio, nr_segs);
-}
+bool blk_mq_sched_bio_merge(struct request_queue *q, struct bio *bio,
+			    unsigned int nr_segs);
 
 static inline bool
 blk_mq_sched_allow_merge(struct request_queue *q, struct request *rq,
Signature · 1 line
1.8.3.1
#17 lore raw

Re: [PATCH RESEND 5/5] block: Remove __blk_mq_sched_bio_merge() helper

Quoted 3 lines · Baolin Wang

The blk_mq_sched_bio_merge() just wrap the __blk_mq_sched_bio_merge(), and no other places will use __blk_mq_sched_bio_merge(). Thus we can combine these 2 similar functions into one function.

I think the idea was to avoid the function call for the nomerges fast path. Not sure if that is really worth it.

author
#18 lore raw

Re: [PATCH RESEND 5/5] block: Remove __blk_mq_sched_bio_merge() helper

Quoted 7 lines · Christoph Hellwig
Quoted 3 lines · Baolin Wang

The blk_mq_sched_bio_merge() just wrap the __blk_mq_sched_bio_merge(), and no other places will use __blk_mq_sched_bio_merge(). Thus we can combine these 2 similar functions into one function.

I think the idea was to avoid the function call for the nomerges fast path. Not sure if that is really worth it.

Um, no places will use __blk_mq_sched_bio_merge(), not sure if it is a good choice we still keep an unused and similar function?

Thanks for all your good suggestion.

#19 lore raw

Re: [PATCH RESEND 5/5] block: Remove __blk_mq_sched_bio_merge() helper

Quoted 11 lines · Baolin Wang
Quoted 7 lines · Christoph Hellwig
Quoted 3 lines · Baolin Wang

The blk_mq_sched_bio_merge() just wrap the __blk_mq_sched_bio_merge(), and no other places will use __blk_mq_sched_bio_merge(). Thus we can combine these 2 similar functions into one function.

I think the idea was to avoid the function call for the nomerges fast path. Not sure if that is really worth it.

Um, no places will use __blk_mq_sched_bio_merge(), not sure if it is a good choice we still keep an unused and similar function?

Well, blk_mq_sched_bio_merge calls __blk_mq_sched_bio_merge, after performing two fast path checks.

author
#20 lore raw

Re: [PATCH RESEND 5/5] block: Remove __blk_mq_sched_bio_merge() helper

Quoted 7 lines · Christoph Hellwig
Quoted 6 lines · Baolin Wang
Quoted 5 lines · Christoph Hellwig
Quoted 3 lines · Baolin Wang

The blk_mq_sched_bio_merge() just wrap the __blk_mq_sched_bio_merge(), and no other places will use __blk_mq_sched_bio_merge(). Thus we can combine these 2 similar functions into one function.

Quoted 8 lines
Quoted 5 lines
Quoted 2 lines

I think the idea was to avoid the function call for the nomerges fast path. Not sure if that is really worth it.

Um, no places will use __blk_mq_sched_bio_merge(), not sure if it is a good choice we still keep an unused and similar function?

Well, blk_mq_sched_bio_merge calls __blk_mq_sched_bio_merge, after performing two fast path checks.

What I mean is blk_mq_sched_bio_merge() just wrap the __blk_mq_sched_bio_merge(), and no other users will call __blk_mq_sched_bio_merge(). Anyway, I will drop this patch as you suggested.