diff mbox series

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

Message ID 5b932aa51fc2b46c381d7b83d591a6ddbf05b199.1597637287.git.baolin.wang@linux.alibaba.com (mailing list archive)
State New, archived
Headers show
Series Some clean-ups for bio merge | expand

Commit Message

Baolin Wang Aug. 17, 2020, 4:09 a.m. UTC 
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(-)

Comments

Christoph Hellwig Aug. 17, 2020, 6:26 a.m. UTC | #1 
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---
kernel test robot Aug. 17, 2020, 7:55 a.m. UTC | #2 
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
Baolin Wang Aug. 17, 2020, 12:10 p.m. UTC | #3 
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---
Christoph Hellwig Aug. 17, 2020, 12:24 p.m. UTC | #4 
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?
Baolin Wang Aug. 18, 2020, 3:30 a.m. UTC | #5 
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.
diff mbox series

Patch

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);
 
 /*