* [PATCH RESEND 0/5] Some clean-ups for bio merge
@ 2020-08-17 4:09 Baolin Wang
2020-08-17 4:09 ` [PATCH RESEND 1/5] block: Move bio merge related functions into blk-merge.c Baolin Wang
` (4 more replies)
0 siblings, 5 replies; 21+ messages in thread
From: Baolin Wang @ 2020-08-17 4:09 UTC (permalink / raw)
To: axboe; +Cc: ming.lei, hch, baolin.wang, baolin.wang7, linux-block, linux-kernel
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(-)
--
1.8.3.1
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH RESEND 1/5] block: Move bio merge related functions into blk-merge.c
2020-08-17 4:09 [PATCH RESEND 0/5] Some clean-ups for bio merge Baolin Wang
@ 2020-08-17 4:09 ` Baolin Wang
2020-08-17 6:15 ` Christoph Hellwig
2020-08-17 4:09 ` [PATCH RESEND 2/5] block: Remove redundant blk_mq_sched_allow_merge() validation Baolin Wang
` (3 subsequent siblings)
4 siblings, 1 reply; 21+ messages in thread
From: Baolin Wang @ 2020-08-17 4:09 UTC (permalink / raw)
To: axboe; +Cc: ming.lei, hch, baolin.wang, baolin.wang7, linux-block, linux-kernel
It's better to move bio merge related functions into blk-merge.c,
which contains all merge related functions.
Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>
---
block/blk-core.c | 156 -----------------------------------------------------
block/blk-merge.c | 157 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 157 insertions(+), 156 deletions(-)
diff --git a/block/blk-core.c b/block/blk-core.c
index d9d6326..ed79109 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -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];
diff --git a/block/blk-merge.c b/block/blk-merge.c
index 6529e3a..3619f2f 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -11,6 +11,7 @@
#include <trace/events/block.h>
#include "blk.h"
+#include "blk-rq-qos.h"
static inline bool bio_will_gap(struct request_queue *q,
struct request *prev_rq, struct bio *prev, struct bio *next)
@@ -888,3 +889,159 @@ enum elv_merge blk_try_merge(struct request *rq, struct bio *bio)
return ELEVATOR_FRONT_MERGE;
return ELEVATOR_NO_MERGE;
}
+
+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;
+}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH RESEND 2/5] block: Remove redundant blk_mq_sched_allow_merge() validation
2020-08-17 4:09 [PATCH RESEND 0/5] Some clean-ups for bio merge Baolin Wang
2020-08-17 4:09 ` [PATCH RESEND 1/5] block: Move bio merge related functions into blk-merge.c Baolin Wang
@ 2020-08-17 4:09 ` Baolin Wang
2020-08-17 6:22 ` Christoph Hellwig
2020-08-17 4:09 ` [PATCH RESEND 3/5] block: Add a new helper to attempt to merge a bio Baolin Wang
` (2 subsequent siblings)
4 siblings, 1 reply; 21+ messages in thread
From: Baolin Wang @ 2020-08-17 4:09 UTC (permalink / raw)
To: axboe; +Cc: ming.lei, hch, baolin.wang, baolin.wang7, linux-block, linux-kernel
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 <baolin.wang@linux.alibaba.com>
---
block/blk-mq-sched.c | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c
index a19cdf1..bf62b34 100644
--- a/block/blk-mq-sched.c
+++ b/block/blk-mq-sched.c
@@ -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);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH RESEND 3/5] block: Add a new helper to attempt to merge a bio
2020-08-17 4:09 [PATCH RESEND 0/5] Some clean-ups for bio merge Baolin Wang
2020-08-17 4:09 ` [PATCH RESEND 1/5] block: Move bio merge related functions into blk-merge.c Baolin Wang
2020-08-17 4:09 ` [PATCH RESEND 2/5] block: Remove redundant blk_mq_sched_allow_merge() validation Baolin Wang
@ 2020-08-17 4:09 ` Baolin Wang
2020-08-17 6:26 ` Christoph Hellwig
` (2 more replies)
2020-08-17 4:09 ` [PATCH RESEND 4/5] block: Remove blk_mq_attempt_merge() function Baolin Wang
2020-08-17 4:09 ` [PATCH RESEND 5/5] block: Remove __blk_mq_sched_bio_merge() helper Baolin Wang
4 siblings, 3 replies; 21+ messages in thread
From: Baolin Wang @ 2020-08-17 4:09 UTC (permalink / raw)
To: axboe; +Cc: ming.lei, hch, baolin.wang, baolin.wang7, linux-block, linux-kernel
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 <baolin.wang@linux.alibaba.com>
---
block/blk-merge.c | 47 +++++++++++++++++++++++++++++------------------
block/blk-mq-sched.c | 22 ++++------------------
block/blk.h | 9 +++++++++
3 files changed, 42 insertions(+), 36 deletions(-)
diff --git a/block/blk-merge.c b/block/blk-merge.c
index 3619f2f..a8d1649 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -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;
}
diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c
index bf62b34..8e9bafe 100644
--- a/block/blk-mq-sched.c
+++ b/block/blk-mq-sched.c
@@ -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;
diff --git a/block/blk.h b/block/blk.h
index 49e2928..a6c54e1 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -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);
/*
--
1.8.3.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH RESEND 4/5] block: Remove blk_mq_attempt_merge() function
2020-08-17 4:09 [PATCH RESEND 0/5] Some clean-ups for bio merge Baolin Wang
` (2 preceding siblings ...)
2020-08-17 4:09 ` [PATCH RESEND 3/5] block: Add a new helper to attempt to merge a bio Baolin Wang
@ 2020-08-17 4:09 ` Baolin Wang
2020-08-17 6:31 ` Christoph Hellwig
2020-08-17 4:09 ` [PATCH RESEND 5/5] block: Remove __blk_mq_sched_bio_merge() helper Baolin Wang
4 siblings, 1 reply; 21+ messages in thread
From: Baolin Wang @ 2020-08-17 4:09 UTC (permalink / raw)
To: axboe; +Cc: ming.lei, hch, baolin.wang, baolin.wang7, linux-block, linux-kernel
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 <baolin.wang@linux.alibaba.com>
---
block/blk-mq-sched.c | 33 ++++++++++-----------------------
1 file changed, 10 insertions(+), 23 deletions(-)
diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c
index 8e9bafe..1cc7919 100644
--- a/block/blk-mq-sched.c
+++ b/block/blk-mq-sched.c
@@ -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);
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH RESEND 5/5] block: Remove __blk_mq_sched_bio_merge() helper
2020-08-17 4:09 [PATCH RESEND 0/5] Some clean-ups for bio merge Baolin Wang
` (3 preceding siblings ...)
2020-08-17 4:09 ` [PATCH RESEND 4/5] block: Remove blk_mq_attempt_merge() function Baolin Wang
@ 2020-08-17 4:09 ` Baolin Wang
2020-08-17 6:32 ` Christoph Hellwig
4 siblings, 1 reply; 21+ messages in thread
From: Baolin Wang @ 2020-08-17 4:09 UTC (permalink / raw)
To: axboe; +Cc: ming.lei, hch, baolin.wang, baolin.wang7, linux-block, linux-kernel
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 <baolin.wang@linux.alibaba.com>
---
block/blk-mq-sched.c | 5 ++++-
block/blk-mq-sched.h | 13 ++-----------
2 files changed, 6 insertions(+), 12 deletions(-)
diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c
index 1cc7919..ba34460 100644
--- a/block/blk-mq-sched.c
+++ b/block/blk-mq-sched.c
@@ -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);
diff --git a/block/blk-mq-sched.h b/block/blk-mq-sched.h
index 126021f..65151de 100644
--- a/block/blk-mq-sched.h
+++ b/block/blk-mq-sched.h
@@ -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,
--
1.8.3.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH RESEND 1/5] block: Move bio merge related functions into blk-merge.c
2020-08-17 4:09 ` [PATCH RESEND 1/5] block: Move bio merge related functions into blk-merge.c Baolin Wang
@ 2020-08-17 6:15 ` Christoph Hellwig
0 siblings, 0 replies; 21+ messages in thread
From: Christoph Hellwig @ 2020-08-17 6:15 UTC (permalink / raw)
To: Baolin Wang; +Cc: axboe, ming.lei, hch, baolin.wang7, linux-block, linux-kernel
On Mon, Aug 17, 2020 at 12:09:15PM +0800, Baolin Wang wrote:
> It's better to move bio merge related functions into blk-merge.c,
> which contains all merge related functions.
>
> Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>
> ---
> block/blk-core.c | 156 -----------------------------------------------------
> block/blk-merge.c | 157 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 157 insertions(+), 156 deletions(-)
Looks fine:
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH RESEND 2/5] block: Remove redundant blk_mq_sched_allow_merge() validation
2020-08-17 4:09 ` [PATCH RESEND 2/5] block: Remove redundant blk_mq_sched_allow_merge() validation Baolin Wang
@ 2020-08-17 6:22 ` Christoph Hellwig
2020-08-17 12:06 ` Baolin Wang
0 siblings, 1 reply; 21+ messages in thread
From: Christoph Hellwig @ 2020-08-17 6:22 UTC (permalink / raw)
To: Baolin Wang; +Cc: axboe, ming.lei, hch, baolin.wang7, linux-block, linux-kernel
On Mon, Aug 17, 2020 at 12:09:16PM +0800, Baolin Wang wrote:
> 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.
> 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.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH RESEND 3/5] block: Add a new helper to attempt to merge a bio
2020-08-17 4:09 ` [PATCH RESEND 3/5] block: Add a new helper to attempt to merge a bio Baolin Wang
@ 2020-08-17 6:26 ` Christoph Hellwig
2020-08-17 12:10 ` Baolin Wang
2020-08-17 7:55 ` kernel test robot
2020-08-17 7:55 ` [PATCH] block: fix boolconv.cocci warnings kernel test robot
2 siblings, 1 reply; 21+ messages in thread
From: Christoph Hellwig @ 2020-08-17 6:26 UTC (permalink / raw)
To: Baolin Wang; +Cc: axboe, ming.lei, hch, baolin.wang7, linux-block, linux-kernel
On Mon, Aug 17, 2020 at 12:09:17PM +0800, Baolin Wang wrote:
> 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:
> +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;
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.
> +++ b/block/blk-mq-sched.c
> @@ -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;
>
> + 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;
}
> 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);
>
> /*
> --
> 1.8.3.1
---end quoted text---
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH RESEND 4/5] block: Remove blk_mq_attempt_merge() function
2020-08-17 4:09 ` [PATCH RESEND 4/5] block: Remove blk_mq_attempt_merge() function Baolin Wang
@ 2020-08-17 6:31 ` Christoph Hellwig
2020-08-17 12:10 ` Baolin Wang
0 siblings, 1 reply; 21+ messages in thread
From: Christoph Hellwig @ 2020-08-17 6:31 UTC (permalink / raw)
To: Baolin Wang; +Cc: axboe, ming.lei, hch, baolin.wang7, linux-block, linux-kernel
On Mon, Aug 17, 2020 at 12:09:18PM +0800, Baolin Wang wrote:
> 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);
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.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH RESEND 5/5] block: Remove __blk_mq_sched_bio_merge() helper
2020-08-17 4:09 ` [PATCH RESEND 5/5] block: Remove __blk_mq_sched_bio_merge() helper Baolin Wang
@ 2020-08-17 6:32 ` Christoph Hellwig
2020-08-17 12:14 ` Baolin Wang
0 siblings, 1 reply; 21+ messages in thread
From: Christoph Hellwig @ 2020-08-17 6:32 UTC (permalink / raw)
To: Baolin Wang; +Cc: axboe, ming.lei, hch, baolin.wang7, linux-block, linux-kernel
On Mon, Aug 17, 2020 at 12:09:19PM +0800, Baolin Wang wrote:
> 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.
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH] block: fix boolconv.cocci warnings
2020-08-17 4:09 ` [PATCH RESEND 3/5] block: Add a new helper to attempt to merge a bio Baolin Wang
2020-08-17 6:26 ` Christoph Hellwig
2020-08-17 7:55 ` kernel test robot
@ 2020-08-17 7:55 ` kernel test robot
2 siblings, 0 replies; 21+ messages in thread
From: kernel test robot @ 2020-08-17 7:55 UTC (permalink / raw)
To: Baolin Wang, axboe
Cc: kbuild-all, ming.lei, hch, baolin.wang, baolin.wang7,
linux-block, linux-kernel
From: kernel test robot <lkp@intel.com>
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 <baolin.wang@linux.alibaba.com>
Signed-off-by: kernel test robot <lkp@intel.com>
---
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(-)
--- a/block/blk-mq-sched.c
+++ b/block/blk-mq-sched.c
@@ -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;
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH RESEND 3/5] block: Add a new helper to attempt to merge a bio
2020-08-17 4:09 ` [PATCH RESEND 3/5] block: Add a new helper to attempt to merge a bio Baolin Wang
2020-08-17 6:26 ` Christoph Hellwig
@ 2020-08-17 7:55 ` kernel test robot
2020-08-17 7:55 ` [PATCH] block: fix boolconv.cocci warnings kernel test robot
2 siblings, 0 replies; 21+ messages in thread
From: kernel test robot @ 2020-08-17 7:55 UTC (permalink / raw)
To: Baolin Wang, axboe
Cc: kbuild-all, ming.lei, hch, baolin.wang, baolin.wang7,
linux-block, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1037 bytes --]
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 <lkp@intel.com>
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@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 33989 bytes --]
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH RESEND 2/5] block: Remove redundant blk_mq_sched_allow_merge() validation
2020-08-17 6:22 ` Christoph Hellwig
@ 2020-08-17 12:06 ` Baolin Wang
0 siblings, 0 replies; 21+ messages in thread
From: Baolin Wang @ 2020-08-17 12:06 UTC (permalink / raw)
To: Christoph Hellwig
Cc: axboe, ming.lei, baolin.wang7, linux-block, linux-kernel
On Mon, Aug 17, 2020 at 08:22:24AM +0200, Christoph Hellwig wrote:
> On Mon, Aug 17, 2020 at 12:09:16PM +0800, Baolin Wang wrote:
> > 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.
>
> > 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.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH RESEND 3/5] block: Add a new helper to attempt to merge a bio
2020-08-17 6:26 ` Christoph Hellwig
@ 2020-08-17 12:10 ` Baolin Wang
2020-08-17 12:24 ` Christoph Hellwig
0 siblings, 1 reply; 21+ messages in thread
From: Baolin Wang @ 2020-08-17 12:10 UTC (permalink / raw)
To: Christoph Hellwig
Cc: axboe, ming.lei, baolin.wang7, linux-block, linux-kernel
On Mon, Aug 17, 2020 at 08:26:34AM +0200, Christoph Hellwig wrote:
> On Mon, Aug 17, 2020 at 12:09:17PM +0800, Baolin Wang wrote:
> > 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:
>
> > +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;
>
> Can't we also switch the bio_attempt_*merge helpers to return
> enum bio_merge_status to simplify this a bit?
Yes, will do.
>
> 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.
>
> > +++ b/block/blk-mq-sched.c
> > @@ -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;
> >
> > + 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;
> }
Sure.
>
> > 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);
> >
> > /*
> > --
> > 1.8.3.1
> ---end quoted text---
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH RESEND 4/5] block: Remove blk_mq_attempt_merge() function
2020-08-17 6:31 ` Christoph Hellwig
@ 2020-08-17 12:10 ` Baolin Wang
0 siblings, 0 replies; 21+ messages in thread
From: Baolin Wang @ 2020-08-17 12:10 UTC (permalink / raw)
To: Christoph Hellwig
Cc: axboe, ming.lei, baolin.wang7, linux-block, linux-kernel
On Mon, Aug 17, 2020 at 08:31:53AM +0200, Christoph Hellwig wrote:
> On Mon, Aug 17, 2020 at 12:09:18PM +0800, Baolin Wang wrote:
> > 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);
>
> 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.
Sure, will do in next version.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH RESEND 5/5] block: Remove __blk_mq_sched_bio_merge() helper
2020-08-17 6:32 ` Christoph Hellwig
@ 2020-08-17 12:14 ` Baolin Wang
2020-08-17 12:26 ` Christoph Hellwig
0 siblings, 1 reply; 21+ messages in thread
From: Baolin Wang @ 2020-08-17 12:14 UTC (permalink / raw)
To: Christoph Hellwig
Cc: axboe, ming.lei, baolin.wang7, linux-block, linux-kernel
On Mon, Aug 17, 2020 at 08:32:41AM +0200, Christoph Hellwig wrote:
> On Mon, Aug 17, 2020 at 12:09:19PM +0800, Baolin Wang wrote:
> > 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.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH RESEND 3/5] block: Add a new helper to attempt to merge a bio
2020-08-17 12:10 ` Baolin Wang
@ 2020-08-17 12:24 ` Christoph Hellwig
2020-08-18 3:30 ` Baolin Wang
0 siblings, 1 reply; 21+ messages in thread
From: Christoph Hellwig @ 2020-08-17 12:24 UTC (permalink / raw)
To: Baolin Wang
Cc: Christoph Hellwig, axboe, ming.lei, baolin.wang7, linux-block,
linux-kernel
On Mon, Aug 17, 2020 at 08:10:02PM +0800, Baolin Wang wrote:
> On Mon, Aug 17, 2020 at 08:26:34AM +0200, Christoph Hellwig wrote:
> > On Mon, Aug 17, 2020 at 12:09:17PM +0800, Baolin Wang wrote:
> > > 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:
> >
> > > +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;
> >
> > Can't we also switch the bio_attempt_*merge helpers to return
> > enum bio_merge_status to simplify this a bit?
>
> Yes, will do.
>
> >
> > 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?
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH RESEND 5/5] block: Remove __blk_mq_sched_bio_merge() helper
2020-08-17 12:14 ` Baolin Wang
@ 2020-08-17 12:26 ` Christoph Hellwig
2020-08-18 3:29 ` Baolin Wang
0 siblings, 1 reply; 21+ messages in thread
From: Christoph Hellwig @ 2020-08-17 12:26 UTC (permalink / raw)
To: Baolin Wang
Cc: Christoph Hellwig, axboe, ming.lei, baolin.wang7, linux-block,
linux-kernel
On Mon, Aug 17, 2020 at 08:14:08PM +0800, Baolin Wang wrote:
> On Mon, Aug 17, 2020 at 08:32:41AM +0200, Christoph Hellwig wrote:
> > On Mon, Aug 17, 2020 at 12:09:19PM +0800, Baolin Wang wrote:
> > > 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.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH RESEND 5/5] block: Remove __blk_mq_sched_bio_merge() helper
2020-08-17 12:26 ` Christoph Hellwig
@ 2020-08-18 3:29 ` Baolin Wang
0 siblings, 0 replies; 21+ messages in thread
From: Baolin Wang @ 2020-08-18 3:29 UTC (permalink / raw)
To: Christoph Hellwig
Cc: axboe, ming.lei, baolin.wang7, linux-block, linux-kernel
On Mon, Aug 17, 2020 at 02:26:08PM +0200, Christoph Hellwig wrote:
> On Mon, Aug 17, 2020 at 08:14:08PM +0800, Baolin Wang wrote:
> > On Mon, Aug 17, 2020 at 08:32:41AM +0200, Christoph Hellwig wrote:
> > > On Mon, Aug 17, 2020 at 12:09:19PM +0800, Baolin Wang wrote:
> > > > 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.
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.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH RESEND 3/5] block: Add a new helper to attempt to merge a bio
2020-08-17 12:24 ` Christoph Hellwig
@ 2020-08-18 3:30 ` Baolin Wang
0 siblings, 0 replies; 21+ messages in thread
From: Baolin Wang @ 2020-08-18 3:30 UTC (permalink / raw)
To: Christoph Hellwig
Cc: axboe, ming.lei, baolin.wang7, linux-block, linux-kernel
On Mon, Aug 17, 2020 at 02:24:40PM +0200, Christoph Hellwig wrote:
> On Mon, Aug 17, 2020 at 08:10:02PM +0800, Baolin Wang wrote:
> > On Mon, Aug 17, 2020 at 08:26:34AM +0200, Christoph Hellwig wrote:
> > > On Mon, Aug 17, 2020 at 12:09:17PM +0800, Baolin Wang wrote:
> > > > 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:
> > >
> > > > +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;
> > >
> > > Can't we also switch the bio_attempt_*merge helpers to return
> > > enum bio_merge_status to simplify this a bit?
> >
> > Yes, will do.
> >
> > >
> > > 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.
^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2020-08-18 3:31 UTC | newest]
Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-17 4:09 [PATCH RESEND 0/5] Some clean-ups for bio merge Baolin Wang
2020-08-17 4:09 ` [PATCH RESEND 1/5] block: Move bio merge related functions into blk-merge.c Baolin Wang
2020-08-17 6:15 ` Christoph Hellwig
2020-08-17 4:09 ` [PATCH RESEND 2/5] block: Remove redundant blk_mq_sched_allow_merge() validation Baolin Wang
2020-08-17 6:22 ` Christoph Hellwig
2020-08-17 12:06 ` Baolin Wang
2020-08-17 4:09 ` [PATCH RESEND 3/5] block: Add a new helper to attempt to merge a bio Baolin Wang
2020-08-17 6:26 ` Christoph Hellwig
2020-08-17 12:10 ` Baolin Wang
2020-08-17 12:24 ` Christoph Hellwig
2020-08-18 3:30 ` Baolin Wang
2020-08-17 7:55 ` kernel test robot
2020-08-17 7:55 ` [PATCH] block: fix boolconv.cocci warnings kernel test robot
2020-08-17 4:09 ` [PATCH RESEND 4/5] block: Remove blk_mq_attempt_merge() function Baolin Wang
2020-08-17 6:31 ` Christoph Hellwig
2020-08-17 12:10 ` Baolin Wang
2020-08-17 4:09 ` [PATCH RESEND 5/5] block: Remove __blk_mq_sched_bio_merge() helper Baolin Wang
2020-08-17 6:32 ` Christoph Hellwig
2020-08-17 12:14 ` Baolin Wang
2020-08-17 12:26 ` Christoph Hellwig
2020-08-18 3:29 ` Baolin Wang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).