block: call elv_bio_merged() when merged

PATCH In review Namhyung Kim 4 participants 8 messages May 20, 2011 – May 23, 2011 Block layer

Share & tools

Namhyung Kim

author

2011-05-20 05:29 UTC #0 lore raw

PATCH 1/1 block: call elv_bio_merged() when merged

Commit 73c101011926 ("block: initial patch for on-stack per-task plugging") removed calls to elv_bio_merged() when @bio merged with @req. Re-add them.

Signed-off-by Namhyung Kim <[email protected]>

Cc Divyesh Shah <[email protected]>

---
 block/blk-core.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

block/blk-core.c +2 -0

index 3fe00a14822a..4dc02ef5fc82 100644
@@ -1132,6 +1132,7 @@ static bool bio_attempt_back_merge(struct request_queue *q, struct request *req,
 	req->ioprio = ioprio_best(req->ioprio, bio_prio(bio));
 
 	drive_stat_acct(req, 0);
+	elv_bio_merged(q, req, bio);
 	return true;
 }

@@ -1173,6 +1174,7 @@ static bool bio_attempt_front_merge(struct request_queue *q,
 	req->ioprio = ioprio_best(req->ioprio, bio_prio(bio));
 
 	drive_stat_acct(req, 0);
+	elv_bio_merged(q, req, bio);
 	return true;
 }

Signature · 1 line

1.7.5

Shaohua Li

2011-05-20 08:31 UTC #1 lore raw

2011/5/20 Namhyung Kim <[email protected]>:

Quoted 22 lines

Commit 73c101011926 ("block: initial patch for on-stack per-task plugging") removed calls to elv_bio_merged() when @bio merged with @req. Re-add them.
Signed-off-by Namhyung Kim <[email protected]>
Cc Divyesh Shah <[email protected]>
---
block/blk-core.c | 2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)
block/blk-core.c +1 -0
index 3fe00a14822a..4dc02ef5fc82 100644
@@ -1132,6 +1132,7 @@ static bool bio_attempt_back_merge(struct request_queue *q, struct request *req,
        req->ioprio = ioprio_best(req->ioprio, bio_prio(bio));
 
        drive_stat_acct(req, 0);
+       elv_bio_merged(q, req, bio);
        return true;
  }
 

block/blk-core.c static bool bio_attempt_front_merge(struct request_queue *q, · line 1179

Quoted 6 lines

block/blk-core.c +1 -0

@@ -1173,6 +1174,7 @@ static bool bio_attempt_front_merge(struct request_queue *q,
        req->ioprio = ioprio_best(req->ioprio, bio_prio(bio));
 
        drive_stat_acct(req, 0);
+       elv_bio_merged(q, req, bio);
        return true;
  }

Looks you should do this in __make_request. when the routine is called in attempt_plug_merge, the request isn't added to elevator yet.

Thanks,
Shaohua

Namhyung Kim

author

2011-05-20 09:51 UTC #2 lore raw

Hello,

2011-05-20 (금), 16:31 +0800, Shaohua Li:

Quoted 25 lines

2011/5/20 Namhyung Kim <[email protected]>:
Quoted 24 lines
Commit 73c101011926 ("block: initial patch for on-stack per-task plugging") removed calls to elv_bio_merged() when @bio merged with @req. Re-add them.
Signed-off-by Namhyung Kim <[email protected]>
Cc Divyesh Shah <[email protected]>
---
 block/blk-core.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)
block/blk-core.c +1 -0
index 3fe00a14822a..4dc02ef5fc82 100644
@@ -1132,6 +1132,7 @@ static bool bio_attempt_back_merge(struct request_queue *q, struct request *req,
       req->ioprio = ioprio_best(req->ioprio, bio_prio(bio));
 
       drive_stat_acct(req, 0);
+       elv_bio_merged(q, req, bio);
       return true;
 }
 
@@ -1173,6 +1174,7 @@ static bool bio_attempt_front_merge(struct request_queue *q,
       req->ioprio = ioprio_best(req->ioprio, bio_prio(bio));
 

Quoted 7 lines

block/blk-core.c static bool bio_attempt_back_merge(struct request_queue *q, struct request *req, · line 1137
Quoted 4 lines
block/blk-core.c +1 -0
       drive_stat_acct(req, 0);
+       elv_bio_merged(q, req, bio);
       return true;
 }
Looks you should do this in __make_request. when the routine is called in attempt_plug_merge, the request isn't added to elevator yet.

Hmm.. anyway it is merged. Is there any reason why we shouldn't collect the stat - or invoke the callback routine - if the @req is not in the elevator? Or we need to add a separate stat item for this case?

Thanks.

Signature · 2 lines

Regards,
Namhyung Kim

Jens Axboe

maintainer

2011-05-20 18:50 UTC #3 lore raw

Quoted 36 lines · Namhyung Kim

Hello,
2011-05-20 (금), 16:31 +0800, Shaohua Li:
Quoted 25 lines
2011/5/20 Namhyung Kim <[email protected]>:
Quoted 24 lines
Commit 73c101011926 ("block: initial patch for on-stack per-task plugging") removed calls to elv_bio_merged() when @bio merged with @req. Re-add them.
Signed-off-by Namhyung Kim <[email protected]>
Cc Divyesh Shah <[email protected]>
---
 block/blk-core.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)
block/blk-core.c +1 -0
index 3fe00a14822a..4dc02ef5fc82 100644
@@ -1132,6 +1132,7 @@ static bool bio_attempt_back_merge(struct request_queue *q, struct request *req,
       req->ioprio = ioprio_best(req->ioprio, bio_prio(bio));
 
       drive_stat_acct(req, 0);
+       elv_bio_merged(q, req, bio);
       return true;
 }
 
@@ -1173,6 +1174,7 @@ static bool bio_attempt_front_merge(struct request_queue *q,
       req->ioprio = ioprio_best(req->ioprio, bio_prio(bio));
 
Quoted 7 lines
block/blk-core.c static bool bio_attempt_back_merge(struct request_queue *q, struct request *req, · line 1137
Quoted 4 lines
block/blk-core.c +1 -0
       drive_stat_acct(req, 0);
+       elv_bio_merged(q, req, bio);
       return true;
 }
Looks you should do this in __make_request. when the routine is called in attempt_plug_merge, the request isn't added to elevator yet.

Quoted 3 lines

Hmm.. anyway it is merged. Is there any reason why we shouldn't collect the stat - or invoke the callback routine - if the @req is not in the elevator? Or we need to add a separate stat item for this case?

Your patch should be safe, it's essentially only for the cgroup stuff that does its own accounting and has appropriate protection for it. We'd want to do this for both the plug and not-plugged merge case.

It's a bit of a shame to add this though, since now we are hitting the cgroup lock for each merge.

Signature · 1 line

Jens Axboe

Vivek Goyal

2011-05-20 19:19 UTC #4 lore raw

Quoted 41 lines · Jens Axboe

Quoted 36 lines · Namhyung Kim
Hello,
2011-05-20 (금), 16:31 +0800, Shaohua Li:
Quoted 25 lines
2011/5/20 Namhyung Kim <[email protected]>:
Quoted 24 lines
Commit 73c101011926 ("block: initial patch for on-stack per-task plugging") removed calls to elv_bio_merged() when @bio merged with @req. Re-add them.
Signed-off-by Namhyung Kim <[email protected]>
Cc Divyesh Shah <[email protected]>
---
 block/blk-core.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)
block/blk-core.c +1 -0
index 3fe00a14822a..4dc02ef5fc82 100644
@@ -1132,6 +1132,7 @@ static bool bio_attempt_back_merge(struct request_queue *q, struct request *req,
       req->ioprio = ioprio_best(req->ioprio, bio_prio(bio));
 
       drive_stat_acct(req, 0);
+       elv_bio_merged(q, req, bio);
       return true;
 }
 
@@ -1173,6 +1174,7 @@ static bool bio_attempt_front_merge(struct request_queue *q,
       req->ioprio = ioprio_best(req->ioprio, bio_prio(bio));
 
Quoted 7 lines
block/blk-core.c static bool bio_attempt_back_merge(struct request_queue *q, struct request *req, · line 1137
Quoted 4 lines
block/blk-core.c +1 -0
       drive_stat_acct(req, 0);
+       elv_bio_merged(q, req, bio);
       return true;
 }
Looks you should do this in __make_request. when the routine is called in attempt_plug_merge, the request isn't added to elevator yet.
Quoted 3 lines
Hmm.. anyway it is merged. Is there any reason why we shouldn't collect the stat - or invoke the callback routine - if the @req is not in the elevator? Or we need to add a separate stat item for this case?

Quoted 6 lines

Your patch should be safe, it's essentially only for the cgroup stuff that does its own accounting and has appropriate protection for it. We'd want to do this for both the plug and not-plugged merge case.
It's a bit of a shame to add this though, since now we are hitting the cgroup lock for each merge.

I think wer can make MERGED per cpu as I have done in my other patch series for following dispatch stats.

BLKIO_STAT_CPU_SECTORS,
/* Total bytes transferred */
BLKIO_STAT_CPU_SERVICE_BYTES,
/* Total IOs serviced, post merge */
BLKIO_STAT_CPU_SERVICED,

Jens are you planning to merge lockless throttling series? Once that gets merged, we can convert this MERGED stat also per cpu and get rid of need of taking blkg->stats_lock.

Thanks
Vivek

Jens Axboe

maintainer

2011-05-20 19:21 UTC #5 lore raw

Quoted 49 lines · Vivek Goyal

Quoted 41 lines · Jens Axboe
Quoted 36 lines · Namhyung Kim
Hello,
2011-05-20 (금), 16:31 +0800, Shaohua Li:
Quoted 25 lines
2011/5/20 Namhyung Kim <[email protected]>:
Quoted 24 lines
Commit 73c101011926 ("block: initial patch for on-stack per-task plugging") removed calls to elv_bio_merged() when @bio merged with @req. Re-add them.
Signed-off-by Namhyung Kim <[email protected]>
Cc Divyesh Shah <[email protected]>
---
 block/blk-core.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)
block/blk-core.c +1 -0
index 3fe00a14822a..4dc02ef5fc82 100644
@@ -1132,6 +1132,7 @@ static bool bio_attempt_back_merge(struct request_queue *q, struct request *req,
       req->ioprio = ioprio_best(req->ioprio, bio_prio(bio));
 
       drive_stat_acct(req, 0);
+       elv_bio_merged(q, req, bio);
       return true;
 }
 
@@ -1173,6 +1174,7 @@ static bool bio_attempt_front_merge(struct request_queue *q,
       req->ioprio = ioprio_best(req->ioprio, bio_prio(bio));
 
Quoted 7 lines
block/blk-core.c static bool bio_attempt_back_merge(struct request_queue *q, struct request *req, · line 1137
Quoted 4 lines
block/blk-core.c +1 -0
       drive_stat_acct(req, 0);
+       elv_bio_merged(q, req, bio);
       return true;
 }
Looks you should do this in __make_request. when the routine is called in attempt_plug_merge, the request isn't added to elevator yet.
Quoted 3 lines
Hmm.. anyway it is merged. Is there any reason why we shouldn't collect the stat - or invoke the callback routine - if the @req is not in the elevator? Or we need to add a separate stat item for this case?
Quoted 6 lines
Your patch should be safe, it's essentially only for the cgroup stuff that does its own accounting and has appropriate protection for it. We'd want to do this for both the plug and not-plugged merge case.
It's a bit of a shame to add this though, since now we are hitting the cgroup lock for each merge.

Quoted 8 lines

I think wer can make MERGED per cpu as I have done in my other patch series for following dispatch stats.
BLKIO_STAT_CPU_SECTORS,
/* Total bytes transferred */
BLKIO_STAT_CPU_SERVICE_BYTES,
/* Total IOs serviced, post merge */
BLKIO_STAT_CPU_SERVICED,

Yep, lets please do that and we can re-instate the merge calls with that, too.

Quoted 3 lines

Jens are you planning to merge lockless throttling series? Once that gets merged, we can convert this MERGED stat also per cpu and get rid of need of taking blkg->stats_lock.

Merged them about an hour ago :-)

Send further updates relative to for-2.6.40/core.

Signature · 1 line

Jens Axboe

Vivek Goyal

2011-05-20 21:02 UTC #6 lore raw

Quoted 62 lines · Jens Axboe

Quoted 49 lines · Vivek Goyal
Quoted 41 lines · Jens Axboe
Quoted 36 lines · Namhyung Kim
Hello,
2011-05-20 (금), 16:31 +0800, Shaohua Li:
Quoted 25 lines
2011/5/20 Namhyung Kim <[email protected]>:
Quoted 24 lines
Commit 73c101011926 ("block: initial patch for on-stack per-task plugging") removed calls to elv_bio_merged() when @bio merged with @req. Re-add them.
Signed-off-by Namhyung Kim <[email protected]>
Cc Divyesh Shah <[email protected]>
---
 block/blk-core.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)
block/blk-core.c +1 -0
index 3fe00a14822a..4dc02ef5fc82 100644
@@ -1132,6 +1132,7 @@ static bool bio_attempt_back_merge(struct request_queue *q, struct request *req,
       req->ioprio = ioprio_best(req->ioprio, bio_prio(bio));
 
       drive_stat_acct(req, 0);
+       elv_bio_merged(q, req, bio);
       return true;
 }
 
@@ -1173,6 +1174,7 @@ static bool bio_attempt_front_merge(struct request_queue *q,
       req->ioprio = ioprio_best(req->ioprio, bio_prio(bio));
 
Quoted 7 lines
block/blk-core.c static bool bio_attempt_back_merge(struct request_queue *q, struct request *req, · line 1137
Quoted 4 lines
block/blk-core.c +1 -0
       drive_stat_acct(req, 0);
+       elv_bio_merged(q, req, bio);
       return true;
 }
Looks you should do this in __make_request. when the routine is called in attempt_plug_merge, the request isn't added to elevator yet.
Quoted 3 lines
Hmm.. anyway it is merged. Is there any reason why we shouldn't collect the stat - or invoke the callback routine - if the @req is not in the elevator? Or we need to add a separate stat item for this case?
Quoted 6 lines
Your patch should be safe, it's essentially only for the cgroup stuff that does its own accounting and has appropriate protection for it. We'd want to do this for both the plug and not-plugged merge case.
It's a bit of a shame to add this though, since now we are hitting the cgroup lock for each merge.
Quoted 8 lines
I think wer can make MERGED per cpu as I have done in my other patch series for following dispatch stats.
BLKIO_STAT_CPU_SECTORS,
/* Total bytes transferred */
BLKIO_STAT_CPU_SERVICE_BYTES,
/* Total IOs serviced, post merge */
BLKIO_STAT_CPU_SERVICED,
Yep, lets please do that and we can re-instate the merge calls with that, too.

Quoted 7 lines

Quoted 3 lines
Jens are you planning to merge lockless throttling series? Once that gets merged, we can convert this MERGED stat also per cpu and get rid of need of taking blkg->stats_lock.
Merged them about an hour ago :-)
Send further updates relative to for-2.6.40/core.

Sure. I am working on a patch now.

Thanks
Vivek

Shaohua Li

2011-05-23 02:10 UTC #7 lore raw

In reply to Jens Axboe

Quoted 37 lines · Jens Axboe

Quoted 36 lines · Namhyung Kim
Hello,
2011-05-20 (금), 16:31 +0800, Shaohua Li:
Quoted 32 lines
2011/5/20 Namhyung Kim <[email protected]>:
Quoted 22 lines
Commit 73c101011926 ("block: initial patch for on-stack per-task plugging") removed calls to elv_bio_merged() when @bio merged with @req. Re-add them.
Signed-off-by Namhyung Kim <[email protected]>
Cc Divyesh Shah <[email protected]>
---
 block/blk-core.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)
block/blk-core.c +1 -0
index 3fe00a14822a..4dc02ef5fc82 100644
@@ -1132,6 +1132,7 @@ static bool bio_attempt_back_merge(struct request_queue *q, struct request *req,
       req->ioprio = ioprio_best(req->ioprio, bio_prio(bio));
 
       drive_stat_acct(req, 0);
+       elv_bio_merged(q, req, bio);
       return true;
 }
 
block/blk-core.c static bool bio_attempt_front_merge(struct request_queue *q, · line 1179
Quoted 6 lines
block/blk-core.c +1 -0
@@ -1173,6 +1174,7 @@ static bool bio_attempt_front_merge(struct request_queue *q,
       req->ioprio = ioprio_best(req->ioprio, bio_prio(bio));
 
       drive_stat_acct(req, 0);
+       elv_bio_merged(q, req, bio);
       return true;
 }
Looks you should do this in __make_request. when the routine is called in attempt_plug_merge, the request isn't added to elevator yet.

Quoted 6 lines

Quoted 3 lines
Hmm.. anyway it is merged. Is there any reason why we shouldn't collect the stat - or invoke the callback routine - if the @req is not in the elevator? Or we need to add a separate stat item for this case?
Your patch should be safe, it's essentially only for the cgroup stuff that does its own accounting and has appropriate protection for it.

I'm just worrying about there is duplicate accounting, but anyway it's safe.