[PATCH] bcache: fix unmatched generic_end_io_acct() & generic_start_io_acct()

Zhai Zhaoxuan

author

2018-01-03 04:39 UTC #0 lore raw

PATCH 1/1 bcache: fix unmatched generic_end_io_acct() & generic_start_io_acct()

The function cached_dev_make_request() and flash_dev_make_request() call generic_start_io_acct() with (struct bcache_device)->disk when they start a closure. Then the function bio_complete() calls generic_end_io_acct() with (struct search)->orig_bio->bi_disk when the closure has done. Since the bi_disk is not the bcache device, the generic_end_io_acct() is called with a wrong device queue.

It causes the "inflight" (in struct hd_struct) counter keep increasing without decreasing.

This patch fix the problem by calling generic_end_io_acct() with (struct bcache_device)->disk.

Signed-off-by Zhai Zhaoxuan <[email protected]>

---
 drivers/md/bcache/request.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/md/bcache/request.c +2 -2

index 643c3021624f..83de85fe4542 100644
@@ -611,8 +611,8 @@ static void request_endio(struct bio *bio)
 static void bio_complete(struct search *s)
 {
 	if (s->orig_bio) {
-		struct request_queue *q = s->orig_bio->bi_disk->queue;
-		generic_end_io_acct(q, bio_data_dir(s->orig_bio),
+		generic_end_io_acct(s->d->disk->queue,
+				    bio_data_dir(s->orig_bio),
 				    &s->d->disk->part0, s->start_time);
 
 		trace_bcache_request_end(s->d, s->orig_bio);

Signature · 1 line

2.15.1

Coly Li

Reviewed-by on v1 1/1

2018-01-03 05:45 UTC #1 lore raw

Quoted 7 lines · Zhai Zhaoxuan

The function cached_dev_make_request() and flash_dev_make_request() call generic_start_io_acct() with (struct bcache_device)->disk when they start a closure. Then the function bio_complete() calls generic_end_io_acct() with (struct search)->orig_bio->bi_disk when the closure has done. Since the bi_disk is not the bcache device, the generic_end_io_acct() is called with a wrong device queue.

Quoted 7 lines

It causes the "inflight" (in struct hd_struct) counter keep increasing without decreasing.
This patch fix the problem by calling generic_end_io_acct() with (struct bcache_device)->disk.
Signed-off-by Zhai Zhaoxuan <[email protected]>

Hi Zhaoxuan,

Nice catch, it makes sense. Thanks.

Reviewed-by Coly Li <[email protected]>

One more question, do you experience any problem when inflight counter increases only ? I just wondering.

Coly

Quoted 20 lines

---
 drivers/md/bcache/request.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/md/bcache/request.c +2 -2

index 643c3021624f..83de85fe4542 100644
@@ -611,8 +611,8 @@ static void request_endio(struct bio *bio)
 static void bio_complete(struct search *s)
 {
 	if (s->orig_bio) {
-		struct request_queue *q = s->orig_bio->bi_disk->queue;
-		generic_end_io_acct(q, bio_data_dir(s->orig_bio),
+		generic_end_io_acct(s->d->disk->queue,
+				    bio_data_dir(s->orig_bio),
 				    &s->d->disk->part0, s->start_time);
 
 		trace_bcache_request_end(s->d, s->orig_bio);

Zhaoxuan

author

2018-01-03 07:05 UTC #2 lore raw

Quoted 15 lines · Coly Li

Quoted 7 lines · Zhai Zhaoxuan
The function cached_dev_make_request() and flash_dev_make_request() call generic_start_io_acct() with (struct bcache_device)->disk when they start a closure. Then the function bio_complete() calls generic_end_io_acct() with (struct search)->orig_bio->bi_disk when the closure has done. Since the bi_disk is not the bcache device, the generic_end_io_acct() is called with a wrong device queue.
Quoted 7 lines
It causes the "inflight" (in struct hd_struct) counter keep increasing without decreasing.
This patch fix the problem by calling generic_end_io_acct() with (struct bcache_device)->disk.
Signed-off-by Zhai Zhaoxuan <[email protected]>

Quoted 8 lines

Hi Zhaoxuan,
Nice catch, it makes sense. Thanks.
Reviewed-by Coly Li <[email protected]>
One more question, do you experience any problem when inflight counter increases only ? I just wondering.

Hi Coly,

Thanks for your review.

There is a problem in iostat -x. iostat shows the bcache device keep 100.00% busy (100.00 in the "%util" column), even if the bcache is idle.

[root@base ~]# iostat -x -d 1
Linux 4.15.0-rc6-ARCH (base) 01/03/18 _x86_64_ (2 CPU)

Device: rrqm/s wrqm/s r/s w/s rkB/s wkB/s avgrq-sz avgqu-sz await r_await w_await svctm %util vda 7.85 1.07 69.98 13.20 1818.60 158.20 47.54 0.04 1.08 0.67 3.28 0.30 2.47 vdb 0.00 0.00 2.61 0.00 75.80 0.00 58.09 0.00 0.75 0.75 0.00 0.00 0.00 bcache0 0.00 0.00 1.98 0.00 52.00 0.00 52.64 77.73 1.06 1.06 0.00 485.93 96.00

Device: rrqm/s wrqm/s r/s w/s rkB/s wkB/s avgrq-sz avgqu-sz await r_await w_await svctm %util vda 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 vdb 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 bcache0 0.00 0.00 0.00 0.00 0.00 0.00 0.00 81.00 0.00 0.00 0.00 0.00 100.00

Device: rrqm/s wrqm/s r/s w/s rkB/s wkB/s avgrq-sz avgqu-sz await r_await w_await svctm %util vda 0.00 9.00 0.00 2.00 0.00 44.00 44.00 0.00 10.00 0.00 10.00 0.00 0.00 vdb 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 bcache0 0.00 0.00 0.00 0.00 0.00 0.00 0.00 81.00 0.00 0.00 0.00 0.00 100.00

Device: rrqm/s wrqm/s r/s w/s rkB/s wkB/s avgrq-sz avgqu-sz await r_await w_await svctm %util vda 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 vdb 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 bcache0 0.00 0.00 0.00 0.00 0.00 0.00 0.00 81.00 0.00 0.00 0.00 0.00 100.00

^C
[root@base ~]# bcache-super-show /dev/vdb
sb.magic                ok
sb.first_sector         8 [match]
sb.csum                 54824DA53D2DC101 [match]
sb.version              1 [backing device]

dev.label               (empty)
dev.uuid                c5e43e66-4b28-48d7-bd46-73fb6e1efdbb dev.sectors_per_block   1
dev.sectors_per_bucket 1024
dev.data.first_sector   16
dev.data.cache_mode     0 [writethrough] dev.data.cache_state    1 [clean]

cset.uuid               a79f8422-ed64-4b92-8688-692f48cd6d97 [root@base ~]# bcache-super-show /dev/vda2 sb.magic                ok
sb.first_sector         8 [match]
sb.csum                 EF7AB1A8928862E6 [match] sb.version              3 [cache device]

dev.label               (empty)
dev.uuid                5762b745-77f8-4831-81ce-9ee7866b8d84 dev.sectors_per_block   1
dev.sectors_per_bucket 1024 dev.cache.first_sector 1024 dev.cache.cache_sectors 2096128 dev.cache.total_sectors 2097152
dev.cache.ordered       yes
dev.cache.discard       no
dev.cache.pos           0
dev.cache.replacement   0 [lru]

cset.uuid a79f8422-ed64-4b92-8688-692f48cd6d97

Quoted 22 lines

Coly

Quoted 20 lines

---
  drivers/md/bcache/request.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

drivers/md/bcache/request.c +2 -2

index 643c3021624f..83de85fe4542 100644
@@ -611,8 +611,8 @@ static void request_endio(struct bio *bio)
  static void bio_complete(struct search *s)
  {
  	if (s->orig_bio) {
-		struct request_queue *q = s->orig_bio->bi_disk->queue;
-		generic_end_io_acct(q, bio_data_dir(s->orig_bio),
+		generic_end_io_acct(s->d->disk->queue,
+				    bio_data_dir(s->orig_bio),
  				    &s->d->disk->part0, s->start_time);
 
  		trace_bcache_request_end(s->d, s->orig_bio);

Coly Li

2018-01-03 07:21 UTC #3 lore raw

Quoted 110 lines · Zhaoxuan

Quoted 18 lines · Coly Li
Quoted 10 lines · Zhai Zhaoxuan
The function cached_dev_make_request() and flash_dev_make_request() call generic_start_io_acct() with (struct bcache_device)->disk when they start a
closure. Then the function bio_complete() calls generic_end_io_acct() with
(struct search)->orig_bio->bi_disk when the closure has done. Since the bi_disk is not the bcache device, the generic_end_io_acct() is
called with a wrong device queue.
Quoted 7 lines
It causes the "inflight" (in struct hd_struct) counter keep increasing without decreasing.
This patch fix the problem by calling generic_end_io_acct() with (struct bcache_device)->disk.
Signed-off-by Zhai Zhaoxuan <[email protected]>
Quoted 8 lines
Hi Zhaoxuan,
Nice catch, it makes sense. Thanks.
Reviewed-by Coly Li <[email protected]>
One more question, do you experience any problem when inflight counter increases only ? I just wondering.
Hi Coly,
Thanks for your review.
There is a problem in iostat -x. iostat shows the bcache device keep 100.00%
busy (100.00 in the "%util" column), even if the bcache is idle.
[root@base ~]# iostat -x -d 1
Linux 4.15.0-rc6-ARCH (base)    01/03/18        _x86_64_        (2 CPU)
Device:         rrqm/s   wrqm/s     r/s     w/s    rkB/s    wkB/s avgrq-sz avgqu-sz   await r_await w_await svctm %util vda               7.85     1.07   69.98   13.20 1818.60   158.20 47.54     0.04    1.08    0.67    3.28   0.30 2.47
vdb               0.00     0.00    2.61    0.00    75.80     0.00 58.09     0.00    0.75    0.75    0.00   0.00 0.00 bcache0           0.00     0.00    1.98    0.00    52.00     0.00 52.64    77.73    1.06    1.06    0.00 485.93 96.00
Device:         rrqm/s   wrqm/s     r/s     w/s    rkB/s    wkB/s avgrq-sz avgqu-sz   await r_await w_await svctm %util vda               0.00     0.00    0.00    0.00     0.00 0.00 0.00     0.00    0.00    0.00    0.00   0.00 0.00
vdb               0.00     0.00    0.00    0.00     0.00 0.00 0.00     0.00    0.00    0.00    0.00   0.00 0.00 bcache0           0.00     0.00    0.00    0.00     0.00 0.00 0.00    81.00    0.00    0.00    0.00   0.00 100.00
Device:         rrqm/s   wrqm/s     r/s     w/s    rkB/s    wkB/s avgrq-sz avgqu-sz   await r_await w_await svctm %util vda               0.00     9.00    0.00    2.00     0.00    44.00 44.00     0.00   10.00    0.00   10.00   0.00   0.00 vdb               0.00     0.00    0.00    0.00     0.00 0.00 0.00     0.00    0.00    0.00    0.00   0.00   0.00 bcache0           0.00     0.00    0.00    0.00     0.00 0.00 0.00    81.00    0.00    0.00    0.00   0.00 100.00
Device:         rrqm/s   wrqm/s     r/s     w/s    rkB/s    wkB/s avgrq-sz avgqu-sz   await r_await w_await svctm %util vda               0.00     0.00    0.00    0.00     0.00 0.00 0.00     0.00    0.00    0.00    0.00   0.00   0.00
vdb               0.00     0.00    0.00    0.00     0.00 0.00 0.00     0.00    0.00    0.00    0.00   0.00   0.00 bcache0           0.00     0.00    0.00    0.00     0.00 0.00 0.00    81.00    0.00    0.00    0.00   0.00 100.00
^C
[root@base ~]# bcache-super-show /dev/vdb
sb.magic                ok
sb.first_sector         8 [match]
sb.csum                 54824DA53D2DC101 [match]
sb.version              1 [backing device]
dev.label               (empty)
dev.uuid                c5e43e66-4b28-48d7-bd46-73fb6e1efdbb dev.sectors_per_block   1
dev.sectors_per_bucket 1024
dev.data.first_sector   16
dev.data.cache_mode     0 [writethrough] dev.data.cache_state    1 [clean]
cset.uuid               a79f8422-ed64-4b92-8688-692f48cd6d97 [root@base ~]# bcache-super-show /dev/vda2 sb.magic                ok
sb.first_sector         8 [match]
sb.csum                 EF7AB1A8928862E6 [match] sb.version              3 [cache device]
dev.label               (empty)
dev.uuid                5762b745-77f8-4831-81ce-9ee7866b8d84 dev.sectors_per_block   1
dev.sectors_per_bucket 1024 dev.cache.first_sector 1024 dev.cache.cache_sectors 2096128 dev.cache.total_sectors 2097152
dev.cache.ordered       yes
dev.cache.discard       no
dev.cache.pos           0
dev.cache.replacement   0 [lru]

Quoted 2 lines

cset.uuid a79f8422-ed64-4b92-8688-692f48cd6d97

Hi Zhaoxuan,

Cool, this is an already reported issue but I still have no idea how to fix it yet... Thank you for the fix and the above detailed information!

Coly Li

Michael Lyle

Reviewed-by on v1 1/1

2018-01-03 16:57 UTC #4 lore raw

In reply to Zhai Zhaoxuan

Quoted 7 lines · Zhai Zhaoxuan

The function cached_dev_make_request() and flash_dev_make_request() call generic_start_io_acct() with (struct bcache_device)->disk when they start a closure. Then the function bio_complete() calls generic_end_io_acct() with (struct search)->orig_bio->bi_disk when the closure has done. Since the bi_disk is not the bcache device, the generic_end_io_acct() is called with a wrong device queue.

Quoted 7 lines

It causes the "inflight" (in struct hd_struct) counter keep increasing without decreasing.
This patch fix the problem by calling generic_end_io_acct() with (struct bcache_device)->disk.
Signed-off-by Zhai Zhaoxuan <[email protected]>

Verified, looks good to me, added to my for-next staging branch.

Reviewed-by Michael Lyle <[email protected]>