[PATCH 0/2] block: remove unnecessary helpers

[PATCH 0/2] block: remove unnecessary helpers

[Chaitanya Kulkarni]

Hi,

There is only one caller for __blk_account_io_start() and
__blk_account_io_done(), both function are small enough to fit in their
respective callers blk_account_io_start() and blk_account_io_done().

Remove both the functions and opencode in the their respective callers
blk_account_io_start() and blk_account_io_done().

Below is a testlog with simple dd write command on null_blk

-ck

Chaitanya Kulkarni (2):
  block: open code __blk_account_io_start()
  block: open code __blk_account_io_done()

 block/blk-mq.c | 56 ++++++++++++++++++++++----------------------------
 1 file changed, 24 insertions(+), 32 deletions(-)

Debug diff :-

linux-block (for-next) # git diff 
diff --git a/block/blk-mq.c b/block/blk-mq.c
index 1b304f66f4e8..c04fbe7cebc1 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -960,6 +960,7 @@ static inline void blk_account_io_done(struct request *req, u64 now)
         * normal IO on queueing nor completion.  Accounting the
         * containing request is enough.
         */
+       printk(KERN_INFO" <<<<< -%s %d\n", __func__, __LINE__);
        if (blk_do_io_stat(req) && req->part &&
            !(req->rq_flags & RQF_FLUSH_SEQ)) {
                const int sgrp = op_stat_group(req_op(req));
@@ -969,11 +970,13 @@ static inline void blk_account_io_done(struct request *req, u64 now)
                part_stat_inc(req->part, ios[sgrp]);
                part_stat_add(req->part, nsecs[sgrp], now - req->start_time_ns);
                part_stat_unlock();
+               printk(KERN_INFO" <<<<< -%s %d\n", __func__, __LINE__);
        }
 }
 
 static inline void blk_account_io_start(struct request *req)
 {
+       printk(KERN_INFO" <>>>>>> -%s %d\n", __func__, __LINE__);
        if (blk_do_io_stat(req)) {
                /*
                 * All non-passthrough requests are created from a bio with one
@@ -989,6 +992,7 @@ static inline void blk_account_io_start(struct request *req)
                part_stat_lock();
                update_io_ticks(req->part, jiffies, false);
                part_stat_unlock();
+               printk(KERN_INFO" <>>>>>> -%s %d\n", __func__, __LINE__);
        }
 }

[   79.477154]  <<<<< -blk_account_io_done 963
[   79.477155]  <<<<< -blk_account_io_done 973
[   79.477157]  <>>>>>> -blk_account_io_start 979
[   79.477158]  <>>>>>> -blk_account_io_start 995
[   79.477343]  <<<<< -blk_account_io_done 963
[   79.477347]  <<<<< -blk_account_io_done 973
[   79.477354]  <>>>>>> -blk_account_io_start 979
[   79.477355]  <>>>>>> -blk_account_io_start 995
[   79.477538]  <<<<< -blk_account_io_done 963
[   79.477540]  <<<<< -blk_account_io_done 973
[   79.477542]  <>>>>>> -blk_account_io_start 979
[   79.477543]  <>>>>>> -blk_account_io_start 995
[   79.477721]  <<<<< -blk_account_io_done 963
[   79.477722]  <<<<< -blk_account_io_done 973
[   79.477724]  <>>>>>> -blk_account_io_start 979
[   79.477725]  <>>>>>> -blk_account_io_start 995
[   79.477752]  <>>>>>> -blk_account_io_start 979
[   79.477755]  <>>>>>> -blk_account_io_start 995
[   79.477910]  <<<<< -blk_account_io_done 963
[   79.477912]  <<<<< -blk_account_io_done 973
[   79.478005]  <>>>>>> -blk_account_io_start 979
[   79.478006]  <>>>>>> -blk_account_io_start 995
[   79.478091]  <<<<< -blk_account_io_done 963
[   79.478093]  <<<<< -blk_account_io_done 973
[   79.478348]  <<<<< -blk_account_io_done 963
[   79.478353]  <<<<< -blk_account_io_done 973
[   79.478360]  <>>>>>> -blk_account_io_start 979
[   79.478361]  <>>>>>> -blk_account_io_start 995
[   79.478551]  <<<<< -blk_account_io_done 963
[   79.478553]  <<<<< -blk_account_io_done 973
[   79.478556]  <>>>>>> -blk_account_io_start 979
[   79.478557]  <>>>>>> -blk_account_io_start 995
[   79.478731]  <<<<< -blk_account_io_done 963
[   79.478733]  <<<<< -blk_account_io_done 973
[   79.478735]  <>>>>>> -blk_account_io_start 979
[   79.478736]  <>>>>>> -blk_account_io_start 995
[   79.478906]  <<<<< -blk_account_io_done 963
[   79.478907]  <<<<< -blk_account_io_done 973
[   79.478909]  <>>>>>> -blk_account_io_start 979
[   79.478910]  <>>>>>> -blk_account_io_start 995
[   79.479086]  <<<<< -blk_account_io_done 963
[   79.479087]  <<<<< -blk_account_io_done 973
[   79.479089]  <>>>>>> -blk_account_io_start 979
[   79.479090]  <>>>>>> -blk_account_io_start 995
[   79.479262]  <<<<< -blk_account_io_done 963
[   79.479266]  <<<<< -blk_account_io_done 973
[   79.479273]  <>>>>>> -blk_account_io_start 979
[   79.479274]  <>>>>>> -blk_account_io_start 995
[   79.479448]  <<<<< -blk_account_io_done 963
[   79.479449]  <<<<< -blk_account_io_done 973
[   79.479451]  <>>>>>> -blk_account_io_start 979
[   79.479452]  <>>>>>> -blk_account_io_start 995
[   79.479631]  <<<<< -blk_account_io_done 963
[   79.479633]  <<<<< -blk_account_io_done 973
[   79.479635]  <>>>>>> -blk_account_io_start 979
[   79.479636]  <>>>>>> -blk_account_io_start 995
[   79.479797]  <<<<< -blk_account_io_done 963
[   79.479798]  <<<<< -blk_account_io_done 973

-- 
2.29.0

[PATCH 1/2] block: open code __blk_account_io_start()

[Chaitanya Kulkarni]

There is only one caller for __blk_account_io_start(), the function
is small enough to fit in its caller blk_account_io_start().

Remove the function and opencode in the its caller
blk_account_io_start().

Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com>
---
 block/blk-mq.c | 36 ++++++++++++++++--------------------
 1 file changed, 16 insertions(+), 20 deletions(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 4d8d88d55217..f514128f7dad 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -976,28 +976,24 @@ static inline void blk_account_io_done(struct request *req, u64 now)
 		__blk_account_io_done(req, now);
 }
 
-static void __blk_account_io_start(struct request *rq)
-{
-	/*
-	 * All non-passthrough requests are created from a bio with one
-	 * exception: when a flush command that is part of a flush sequence
-	 * generated by the state machine in blk-flush.c is cloned onto the
-	 * lower device by dm-multipath we can get here without a bio.
-	 */
-	if (rq->bio)
-		rq->part = rq->bio->bi_bdev;
-	else
-		rq->part = rq->q->disk->part0;
-
-	part_stat_lock();
-	update_io_ticks(rq->part, jiffies, false);
-	part_stat_unlock();
-}
-
 static inline void blk_account_io_start(struct request *req)
 {
-	if (blk_do_io_stat(req))
-		__blk_account_io_start(req);
+	if (blk_do_io_stat(req)) {
+		/*
+		 * All non-passthrough requests are created from a bio with one
+		 * exception: when a flush command that is part of a flush sequence
+		 * generated by the state machine in blk-flush.c is cloned onto the
+		 * lower device by dm-multipath we can get here without a bio.
+		 */
+		if (req->bio)
+			req->part = req->bio->bi_bdev;
+		else
+			req->part = req->q->disk->part0;
+
+		part_stat_lock();
+		update_io_ticks(req->part, jiffies, false);
+		part_stat_unlock();
+	}
 }
 
 static inline void __blk_mq_end_request_acct(struct request *rq, u64 now)
-- 
2.29.0

Re: [PATCH 1/2] block: open code __blk_account_io_start()

[Christoph Hellwig]

On Mon, Mar 27, 2023 at 12:34:26AM -0700, Chaitanya Kulkarni wrote:
> There is only one caller for __blk_account_io_start(), the function
> is small enough to fit in its caller blk_account_io_start().
> 
> Remove the function and opencode in the its caller
> blk_account_io_start().

Having the account slow path in a separate function actually is nice,
same for the next patch.

> +		/*
> +		 * All non-passthrough requests are created from a bio with one
> +		 * exception: when a flush command that is part of a flush sequence
> +		 * generated by the state machine in blk-flush.c is cloned onto the
> +		 * lower device by dm-multipath we can get here without a bio.
> +		 */

... and now you've created a totally messed up block comment expanding
over 80 characters.

Re: [PATCH 1/2] block: open code __blk_account_io_start()

[Chaitanya Kulkarni]

On 3/27/23 15:12, Christoph Hellwig wrote:
> On Mon, Mar 27, 2023 at 12:34:26AM -0700, Chaitanya Kulkarni wrote:
>> There is only one caller for __blk_account_io_start(), the function
>> is small enough to fit in its caller blk_account_io_start().
>>
>> Remove the function and opencode in the its caller
>> blk_account_io_start().
> Having the account slow path in a separate function actually is nice,
> same for the next patch.
>
>> +		/*
>> +		 * All non-passthrough requests are created from a bio with one
>> +		 * exception: when a flush command that is part of a flush sequence
>> +		 * generated by the state machine in blk-flush.c is cloned onto the
>> +		 * lower device by dm-multipath we can get here without a bio.
>> +		 */
> ... and now you've created a totally messed up block comment expanding
> over 80 characters.

ohh, checkpatch didn't spit out any warning [1].

-ck

[1]
-----------------------------------------------------------------------
p/blk-account-cleanup/0001-block-open-code-__blk_account_io_start.patch
-----------------------------------------------------------------------
total: 0 errors, 0 warnings, 44 lines checked

p/blk-account-cleanup/0001-block-open-code-__blk_account_io_start.patch 
has no obvious style problems and is ready for submission.
----------------------------------------------------------------------
p/blk-account-cleanup/0002-block-open-code-__blk_account_io_done.patch
----------------------------------------------------------------------
total: 0 errors, 0 warnings, 34 lines checked

p/blk-account-cleanup/0002-block-open-code-__blk_account_io_done.patch 
has no obvious style problems and is ready for submission.

NOTE: If any of the errors are false positives, please report
       them to the maintainer, see CHECKPATCH in MAINTAINERS.

Re: [PATCH 1/2] block: open code __blk_account_io_start()

[Jens Axboe]

On 3/27/23 4:12 PM, Christoph Hellwig wrote:
> On Mon, Mar 27, 2023 at 12:34:26AM -0700, Chaitanya Kulkarni wrote:
>> There is only one caller for __blk_account_io_start(), the function
>> is small enough to fit in its caller blk_account_io_start().
>>
>> Remove the function and opencode in the its caller
>> blk_account_io_start().
> 
> Having the account slow path in a separate function actually is nice,
> same for the next patch.

Then it should be made explicitly out-of-line, the compiler would've
inlined that anyway.

-- 
Jens Axboe

[PATCH 2/2] block: open code __blk_account_io_done()

[Chaitanya Kulkarni]

There is only one caller for __blk_account_io_done(), the function
is small enough to fit in its caller blk_account_io_done().

Remove the function and opencode in the its caller
blk_account_io_done().

Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com>
---
 block/blk-mq.c | 22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index f514128f7dad..c782d556488c 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -953,17 +953,6 @@ bool blk_update_request(struct request *req, blk_status_t error,
 }
 EXPORT_SYMBOL_GPL(blk_update_request);
 
-static void __blk_account_io_done(struct request *req, u64 now)
-{
-	const int sgrp = op_stat_group(req_op(req));
-
-	part_stat_lock();
-	update_io_ticks(req->part, jiffies, true);
-	part_stat_inc(req->part, ios[sgrp]);
-	part_stat_add(req->part, nsecs[sgrp], now - req->start_time_ns);
-	part_stat_unlock();
-}
-
 static inline void blk_account_io_done(struct request *req, u64 now)
 {
 	/*
@@ -972,8 +961,15 @@ static inline void blk_account_io_done(struct request *req, u64 now)
 	 * containing request is enough.
 	 */
 	if (blk_do_io_stat(req) && req->part &&
-	    !(req->rq_flags & RQF_FLUSH_SEQ))
-		__blk_account_io_done(req, now);
+	    !(req->rq_flags & RQF_FLUSH_SEQ)) {
+		const int sgrp = op_stat_group(req_op(req));
+
+		part_stat_lock();
+		update_io_ticks(req->part, jiffies, true);
+		part_stat_inc(req->part, ios[sgrp]);
+		part_stat_add(req->part, nsecs[sgrp], now - req->start_time_ns);
+		part_stat_unlock();
+	}
 }
 
 static inline void blk_account_io_start(struct request *req)
-- 
2.29.0

Re: [PATCH 0/2] block: remove unnecessary helpers

[Jens Axboe]

On Mon, 27 Mar 2023 00:34:25 -0700, Chaitanya Kulkarni wrote:
> There is only one caller for __blk_account_io_start() and
> __blk_account_io_done(), both function are small enough to fit in their
> respective callers blk_account_io_start() and blk_account_io_done().
> 
> Remove both the functions and opencode in the their respective callers
> blk_account_io_start() and blk_account_io_done().
> 
> [...]

Applied, thanks!

[0/2] block: remove unnecessary helpers
      (no commit info)
[1/2] block: open code __blk_account_io_start()
      commit: 06965037ce942500c1ce3aa29ca217093a9c5720
[2/2] block: open code __blk_account_io_done()
      commit: 06965037ce942500c1ce3aa29ca217093a9c5720

Best regards,
-- 
Jens Axboe