Some clean-ups for bio merge
v2 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.
Changes from v1:
- Drop patch 2 and patch 5 in v1 patch set.
- Add reviewed-by tag from Christoph.
- Move blk_mq_bio_list_merge() into blk-merge.c and rename it.
- Some coding style improvements.
Baolin Wang (3):
block: Move bio merge related functions into blk-merge.c
block: Add a new helper to attempt to merge a bio
block: Remove blk_mq_attempt_merge() function
block/blk-core.c | 156 -------------------------------------- block/blk-merge.c | 202 +++++++++++++++++++++++++++++++++++++++++++++++++ block/blk-mq-sched.c | 94 +++++------------------ block/blk.h | 23 ++++-- block/kyber-iosched.c | 2 +- include/linux/blk-mq.h | 2 - 6 files changed, 239 insertions(+), 240 deletions(-)
Signature · 1 line
1.8.3.1
PATCH v2 1/3 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.
--- block/blk-core.c | 156 ----------------------------------------------------- block/blk-merge.c | 157 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 157 insertions(+), 156 deletions(-)
block/blk-core.c +0 -156
@@ -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
PATCH v2 2/3 block: Add a new helper to attempt to merge a bio
In reply to Baolin WangThere 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().
Meanwhile move the blk_mq_bio_list_merge() into blk-merge.c and rename it as a generic name.
--- block/blk-merge.c | 101 +++++++++++++++++++++++++++++++++++-------------- block/blk-mq-sched.c | 52 ++----------------------- block/blk.h | 23 ++++++++--- block/kyber-iosched.c | 2 +- include/linux/blk-mq.h | 2 - 5 files changed, 95 insertions(+), 85 deletions(-)
block/blk-merge.c +73 -28
@@ -900,13 +900,14 @@ static void blk_account_io_merge_bio(struct request *req)part_stat_unlock (); } -bool bio_attempt_back_merge (struct request *req,struct bio *bio, -unsigned int nr_segs) +enum bio_merge_status 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; +return BIO_MERGE_FAILED ;trace_block_bio_backmerge (req->q, req, bio);rq_qos_merge (req->q, req, bio);
@@ -921,16 +922,17 @@ bool bio_attempt_back_merge(struct request *req, struct bio *bio,bio_crypt_free_ctx (bio);blk_account_io_merge_bio (req); -return true; +return BIO_MERGE_OK ; } -bool bio_attempt_front_merge (struct request *req,struct bio *bio, -unsigned int nr_segs) +enum bio_merge_status 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; +return BIO_MERGE_FAILED ;trace_block_bio_frontmerge (req->q, req, bio);rq_qos_merge (req->q, req, bio);
@@ -947,11 +949,12 @@ bool bio_attempt_front_merge(struct request *req, struct bio *bio,bio_crypt_do_front_merge (req, bio);blk_account_io_merge_bio (req); -return true; +return BIO_MERGE_OK ; } -bool bio_attempt_discard_merge (struct request_queue *q,struct request *req, -struct bio *bio) +enum bio_merge_status bio_attempt_discard_merge (struct request_queue *q, +struct request *req, +struct bio *bio) {unsigned short segments =blk_rq_nr_discard_segments (req);
@@ -969,10 +972,39 @@ bool bio_attempt_discard_merge(struct request_queue *q, struct request *req, req->nr_phys_segments = segments +1 ;blk_account_io_merge_bio (req); -return true; +return BIO_MERGE_OK ; no_merge:req_set_nomerge (q, req); -return false; +return BIO_MERGE_FAILED ; +} + +static enum bio_merge_status blk_attempt_bio_merge (struct request_queue *q, +struct request *rq, +struct bio *bio, +unsigned int nr_segs, +bool sched_allow_merge) +{ +if (!blk_rq_merge_ok (rq, bio)) +return BIO_MERGE_NONE ; + +switch (blk_try_merge (rq, bio)) { +case ELEVATOR_BACK_MERGE : +if (!sched_allow_merge || + (sched_allow_merge &&blk_mq_sched_allow_merge (q, rq, bio))) +return bio_attempt_back_merge (rq, bio, nr_segs); +break ; +case ELEVATOR_FRONT_MERGE : +if (!sched_allow_merge || + (sched_allow_merge &&blk_mq_sched_allow_merge (q, rq, bio))) +return bio_attempt_front_merge (rq, bio, nr_segs); +break ; +case ELEVATOR_DISCARD_MERGE : +return bio_attempt_discard_merge (q, rq, bio); +default : +return BIO_MERGE_NONE ; + } + +return BIO_MERGE_FAILED ; }/**
@@ -1011,8 +1043,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,26 +1052,41 @@ 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 : +if (blk_attempt_bio_merge (q, rq, bio, nr_segs, false) ==BIO_MERGE_OK ) +return true; + } + +return false; +} + +/* +* Iterate list of requests and see if we can merge this bio with any +* of them. +*/ +bool blk_bio_list_merge (struct request_queue *q,struct list_head *list, +struct bio *bio,unsigned int nr_segs) +{ +struct request *rq; +int checked =8 ; + +list_for_each_entry_reverse (rq, list, queuelist) { +if (!checked--)break ; - } -if (merged) +switch (blk_attempt_bio_merge (q, rq, bio, nr_segs, true)) { +default : +case BIO_MERGE_NONE : +continue ; +case BIO_MERGE_OK :return true; +case BIO_MERGE_FAILED : +return false; + } }return false; } +EXPORT_SYMBOL_GPL (blk_bio_list_merge);
block/blk-mq-sched.c +4 -48
82 lines not on this page · raw message
block/blk.h +17 -6
@@ -29,6 +29,12 @@ struct blk_flush_queue {spinlock_t mq_flush_lock; }; +enum bio_merge_status { +BIO_MERGE_OK , +BIO_MERGE_NONE , +BIO_MERGE_FAILED , +}; +extern struct kmem_cache *blk_requestq_cachep;extern struct kobj_type blk_queue_ktype;extern struct ida blk_queue_ida;
@@ -169,14 +175,19 @@ static inline void blk_integrity_del(struct gendisk *disk)unsigned long blk_rq_timeout (unsigned long timeout);void blk_add_timer (struct request *req); -bool bio_attempt_front_merge (struct request *req,struct bio *bio, -unsigned int nr_segs); -bool bio_attempt_back_merge (struct request *req,struct bio *bio, -unsigned int nr_segs); -bool bio_attempt_discard_merge (struct request_queue *q,struct request *req, -struct bio *bio); +enum bio_merge_status bio_attempt_front_merge (struct request *req, +struct bio *bio, +unsigned int nr_segs); +enum bio_merge_status bio_attempt_back_merge (struct request *req, +struct bio *bio, +unsigned int nr_segs); +enum bio_merge_status bio_attempt_discard_merge (struct request_queue *q, +struct request *req, +struct bio *bio);bool blk_attempt_plug_merge (struct request_queue *q,struct bio *bio,unsigned int nr_segs,struct request **same_queue_rq); +bool blk_bio_list_merge (struct request_queue *q,struct list_head *list, +struct bio *bio,unsigned int nr_segs);void blk_account_io_start (struct request *req);void blk_account_io_done (struct request *req,u64 now);
block/kyber-iosched.c +1 -1
@@ -573,7 +573,7 @@ static bool kyber_bio_merge(struct blk_mq_hw_ctx *hctx, struct bio *bio,bool merged;spin_lock (&kcq->lock); - merged =blk_mq_bio_list_merge (hctx->queue, rq_list, bio, nr_segs); + merged =blk_bio_list_merge (hctx->queue, rq_list, bio, nr_segs);spin_unlock (&kcq->lock);return merged;
include/linux/blk-mq.h +0 -2
@@ -489,8 +489,6 @@ static inline int blk_mq_request_completed(struct request *rq)void blk_mq_delay_kick_requeue_list (struct request_queue *q,unsigned long msecs);void blk_mq_complete_request (struct request *rq);bool blk_mq_complete_request_remote (struct request *rq); -bool blk_mq_bio_list_merge (struct request_queue *q,struct list_head *list, -struct bio *bio,unsigned int nr_segs);bool blk_mq_queue_stopped (struct request_queue *q);void blk_mq_stop_hw_queue (struct blk_mq_hw_ctx *hctx);void blk_mq_start_hw_queue (struct blk_mq_hw_ctx *hctx);
Signature · 1 line
1.8.3.1
Re: [PATCH v2 2/3] block: Add a new helper to attempt to merge a bio
Quoted 2 lines · Baolin Wang
Meanwhile move the
blk_mq_bio_list_merge()intoblk-merge.cand rename it as a generic name.
That should probably a separate patch.
block/blk-merge.c bool blk_attempt_plug_merge(struct request_queue *q, struct bio *bio, · line 1059
Quoted 2 lines
block/blk-merge.c +2
-0+if (blk_attempt_bio_merge (q, rq, bio, nr_segs, false) ==BIO_MERGE_OK ) +return true;
This adds an overly long line.
block/blk-merge.c bool blk_attempt_plug_merge(struct request_queue *q, struct bio *bio, · line 1087
Quoted 10 lines
block/blk-merge.c +8
-1-if (merged) +switch (blk_attempt_bio_merge (q, rq, bio, nr_segs, true)) { +default : +case BIO_MERGE_NONE : +continue ; +case BIO_MERGE_OK :return true; +case BIO_MERGE_FAILED : +return false; + }
I don't think we need a default statement here as we handle all possible values of the enum.
Re: [PATCH v2 2/3] block: Add a new helper to attempt to merge a bio
Quoted 5 lines · Christoph Hellwig
Quoted 2 lines · Baolin Wang
Meanwhile move the
blk_mq_bio_list_merge()intoblk-merge.cand rename it as a generic name.That should probably a separate patch.
Sure.
Quoted 5 lines
block/blk-merge.c bool blk_attempt_plug_merge(struct request_queue *q, struct bio *bio, · line 1059
Quoted 3 lines
block/blk-merge.c +2
-0+if (blk_attempt_bio_merge (q, rq, bio, nr_segs, false) ==BIO_MERGE_OK ) +return true;This adds an overly long line.
The checkpatch.pl has increased the default limit to 100 characters, so I did not get a long line warning. Anyway I will change a new line if you concern about this.
Quoted 5 lines
Quoted 5 lines
block/blk-merge.c +4
-1-if (merged) +switch (blk_attempt_bio_merge (q, rq, bio, nr_segs, true)) { +default : +case BIO_MERGE_NONE : +continue ;
Quoted 8 lines
block/blk-merge.c bool blk_attempt_plug_merge(struct request_queue *q, struct bio *bio, · line 1087
Quoted 5 lines
block/blk-merge.c +4
-0+case BIO_MERGE_OK :return true; +case BIO_MERGE_FAILED : +return false; + }I don't think we need a default statement here as we handle all possible values of the enum.
OK. Will remove it in next version. Thanks.
PATCH v2 3/3 block: Remove blk_mq_attempt_merge() function
In reply to Baolin WangThe small blk_mq_attempt_merge() function is only called by __blk_mq_sched_bio_merge(), just open code it.
--- block/blk-mq-sched.c | 44 ++++++++++++++++---------------------------- 1 file changed, 16 insertions(+), 28 deletions(-)
block/blk-mq-sched.c +16 -28
@@ -382,28 +382,6 @@ bool blk_mq_sched_try_merge(struct request_queue *q, struct bio *bio, }EXPORT_SYMBOL_GPL (blk_mq_sched_try_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_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) {
@@ -417,14 +395,24 @@ bool __blk_mq_sched_bio_merge(struct request_queue *q, struct bio *bio,return e->type->ops.bio_merge (hctx, bio, nr_segs); type = hctx->type; -if ((hctx->flags &BLK_MQ_F_SHOULD_MERGE ) && - !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); -spin_unlock (&ctx->lock); +if (!(hctx->flags &BLK_MQ_F_SHOULD_MERGE ) || +list_empty_careful (&ctx->rq_lists[type])) +return false; + +/* default per sw-queue merge */ +spin_lock (&ctx->lock); +/* +* 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_bio_list_merge (q, &ctx->rq_lists[type], bio, nr_segs)) { + ctx->rq_merged++; + ret = true; } +spin_unlock (&ctx->lock); +return ret; }
Signature · 1 line
1.8.3.1
Re: [PATCH v2 3/3] block: Remove blk_mq_attempt_merge() function
Quoted 4 lines · 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]>
Looks good,
Hi,
Quoted 11 lines · Baolin Wang
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.
Changes from v1:
- Drop patch 2 and patch 5 in v1 patch set.
- Add reviewed-by tag from Christoph.
- Moveblk_mq_bio_list_merge()intoblk-merge.cand rename it.
- Some coding style improvements.
Any comments for this patch set? Thanks.
Quoted 16 lines
Baolin Wang (3):
block: Move bio merge related functions intoblk-merge.c
block: Add a new helper to attempt to merge a bio
block: Removeblk_mq_attempt_merge()functionblock/blk-core.c | 156 -------------------------------------- block/blk-merge.c | 202 +++++++++++++++++++++++++++++++++++++++++++++++++ block/blk-mq-sched.c | 94 +++++------------------ block/blk.h | 23 ++++-- block/kyber-iosched.c | 2 +- include/linux/blk-mq.h | 2 - 6 files changed, 239 insertions(+), 240 deletions(-)Signature · 1 line
1.8.3.1