[PATCH 0/4] random improvements and cleanups for elevator.c

[PATCH 0/4] random improvements and cleanups for elevator.c

[Jinlong Chen]

The series slightly improves the readability of elevator.c.

Jinlong Chen (4):
  elevator: print none at first in elv_iosched_show even if the queue
    has a scheduler
  elevator: replace continue with else-if in elv_iosched_show
  elevator: repalce "len+name" with "name+len" in elv_iosched_show
  elevator: use bool instead of int as the return type of
    elv_iosched_allow_bio_merge

 block/elevator.c | 21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

-- 
2.34.1

[PATCH 1/4] elevator: print none at first in elv_iosched_show even if the queue has a scheduler

[Jinlong Chen]

This makes the printing order of the io schedulers consistent, and removes
a redundant q->elevator check.

Signed-off-by: Jinlong Chen <nickyc975@zju.edu.cn>
---
 block/elevator.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/block/elevator.c b/block/elevator.c
index 599413620558..308bee253564 100644
--- a/block/elevator.c
+++ b/block/elevator.c
@@ -767,10 +767,12 @@ ssize_t elv_iosched_show(struct request_queue *q, char *name)
 	if (!elv_support_iosched(q))
 		return sprintf(name, "none\n");
 
-	if (!q->elevator)
+	if (!q->elevator) {
 		len += sprintf(name+len, "[none] ");
-	else
+	} else {
+		len += sprintf(name+len, "none ");
 		cur = eq->type;
+	}
 
 	spin_lock(&elv_list_lock);
 	list_for_each_entry(e, &elv_list, list) {
@@ -783,9 +785,6 @@ ssize_t elv_iosched_show(struct request_queue *q, char *name)
 	}
 	spin_unlock(&elv_list_lock);
 
-	if (q->elevator)
-		len += sprintf(name+len, "none");
-
 	len += sprintf(len+name, "\n");
 	return len;
 }
-- 
2.34.1

[PATCH 2/4] elevator: replace continue with else-if in elv_iosched_show

[Jinlong Chen]

else-if is more readable than continue here.

Signed-off-by: Jinlong Chen <nickyc975@zju.edu.cn>
---
 block/elevator.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/block/elevator.c b/block/elevator.c
index 308bee253564..ffa750976d25 100644
--- a/block/elevator.c
+++ b/block/elevator.c
@@ -776,11 +776,9 @@ ssize_t elv_iosched_show(struct request_queue *q, char *name)
 
 	spin_lock(&elv_list_lock);
 	list_for_each_entry(e, &elv_list, list) {
-		if (e == cur) {
+		if (e == cur)
 			len += sprintf(name+len, "[%s] ", cur->elevator_name);
-			continue;
-		}
-		if (elv_support_features(q, e))
+		else if (elv_support_features(q, e))
 			len += sprintf(name+len, "%s ", e->elevator_name);
 	}
 	spin_unlock(&elv_list_lock);
-- 
2.34.1

[PATCH 3/4] elevator: repalce "len+name" with "name+len" in elv_iosched_show

[Jinlong Chen]

The "pointer + offset" pattern is more resonable.

Signed-off-by: Jinlong Chen <nickyc975@zju.edu.cn>
---
 block/elevator.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/elevator.c b/block/elevator.c
index ffa750976d25..93dbaa560b67 100644
--- a/block/elevator.c
+++ b/block/elevator.c
@@ -783,7 +783,7 @@ ssize_t elv_iosched_show(struct request_queue *q, char *name)
 	}
 	spin_unlock(&elv_list_lock);
 
-	len += sprintf(len+name, "\n");
+	len += sprintf(name+len, "\n");
 	return len;
 }
 
-- 
2.34.1

[PATCH 4/4] elevator: use bool instead of int as the return type of elv_iosched_allow_bio_merge

[Jinlong Chen]

We have bool type now, update the old signature.

Signed-off-by: Jinlong Chen <nickyc975@zju.edu.cn>
---
 block/elevator.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/elevator.c b/block/elevator.c
index 93dbaa560b67..cb1c9a69026c 100644
--- a/block/elevator.c
+++ b/block/elevator.c
@@ -57,7 +57,7 @@ static LIST_HEAD(elv_list);
  * Query io scheduler to see if the current process issuing bio may be
  * merged with rq.
  */
-static int elv_iosched_allow_bio_merge(struct request *rq, struct bio *bio)
+static bool elv_iosched_allow_bio_merge(struct request *rq, struct bio *bio)
 {
 	struct request_queue *q = rq->q;
 	struct elevator_queue *e = q->elevator;
@@ -65,7 +65,7 @@ static int elv_iosched_allow_bio_merge(struct request *rq, struct bio *bio)
 	if (e->type->ops.allow_merge)
 		return e->type->ops.allow_merge(q, rq, bio);
 
-	return 1;
+	return true;
 }
 
 /*
-- 
2.34.1

Re: [PATCH 1/4] elevator: print none at first in elv_iosched_show even if the queue has a scheduler

[Christoph Hellwig]

On Fri, Nov 25, 2022 at 11:53:11PM +0800, Jinlong Chen wrote:
> This makes the printing order of the io schedulers consistent, and removes
> a redundant q->elevator check.

Looks good:

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

Re: [PATCH 2/4] elevator: replace continue with else-if in elv_iosched_show

[Christoph Hellwig]

On Fri, Nov 25, 2022 at 11:53:12PM +0800, Jinlong Chen wrote:
>  	list_for_each_entry(e, &elv_list, list) {
> -		if (e == cur) {
> +		if (e == cur)
>  			len += sprintf(name+len, "[%s] ", cur->elevator_name);
> -			continue;
> -		}
> -		if (elv_support_features(q, e))
> +		else if (elv_support_features(q, e))
>  			len += sprintf(name+len, "%s ", e->elevator_name);

Looks good.  But to make this even more obvious I'd also switch to
pinting e->elevator_name for the cur case instead of cur.

Re: [PATCH 3/4] elevator: repalce "len+name" with "name+len" in elv_iosched_show

[Christoph Hellwig]

On Fri, Nov 25, 2022 at 11:53:13PM +0800, Jinlong Chen wrote:
> The "pointer + offset" pattern is more resonable.

Looks good:

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

Re: [PATCH 4/4] elevator: use bool instead of int as the return type of elv_iosched_allow_bio_merge

[Christoph Hellwig]

On Fri, Nov 25, 2022 at 11:53:14PM +0800, Jinlong Chen wrote:
> We have bool type now, update the old signature.

Looks good, especially as the allow_merge method already returns a bool:

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

Re: Re: [PATCH 2/4] elevator: replace continue with else-if in elv_iosched_show

[Jinlong Chen]

> >  	list_for_each_entry(e, &elv_list, list) {
> > -		if (e == cur) {
> > +		if (e == cur)
> >  			len += sprintf(name+len, "[%s] ", cur->elevator_name);
> > -			continue;
> > -		}
> > -		if (elv_support_features(q, e))
> > +		else if (elv_support_features(q, e))
> >  			len += sprintf(name+len, "%s ", e->elevator_name);
> 
> Looks good.  But to make this even more obvious I'd also switch to
> pinting e->elevator_name for the cur case instead of cur.

That's truely better. I'll send a v2 soon.

Thanks!
Jinlong Chen