bcache: dont reset bio opf in bch_data_insert_start
PATCH v3 1/1 bcache: dont reset bio opf in bch_data_insert_start
commit ad0d9e76(bcache: use bio op accessors) makes the bi_opf modified by bio_set_op_attrs(). But there is a logical problem in this commit:
trace_bcache_cache_insert(k);
bch_keylist_push(&op->insert_keys);- n->bi_rw |= REQ_WRITE;
+ bio_set_op_attrs(n, REQ_OP_WRITE, 0);
bch_submit_bbio(n, op->c, k, 0);
} while (n != bio);The old code add REQ_WRITE into bio n and keep other flags; the new code set REQ_OP_WRITE to bi_opf, but reset all other flags.
This problem is discoverd in our performance testing:
(1) start a fio with 1M x 128depth for read in /dev/nvme0n1p1
(2) start a fio with 1M x 128depth for write in /dev/escache0 (cache device is /dev/nvme0n1p2)
We found the BW of reading is 2000+M/s, but the BW of writing is 0-100M/s. After some debugging, we found the problem is io submit in writting is very slow.
bch_data_insert_start() insert a bio to /dev/nvme0n1p1, but as cached_dev submit stack bio will be added into current->bio_list, and return.Then __submit_bio_noacct() will submit the new bio in bio_list into /dev/nvme0n1p1. This operation would be slow in blk_mq_submit_bio() -> rq_qos_throttle(q, bio);
The rq_qos_throttle() will call wbt_should_throttle(),
static inline bool wbt_should_throttle(struct rq_wb *rwb, struct bio *bio)
{
switch (bio_op(bio)) {
case REQ_OP_WRITE:
/*
* Don't throttle WRITE_ODIRECT
*/
if ((bio->bi_opf & (REQ_SYNC | REQ_IDLE)) ==
(REQ_SYNC | REQ_IDLE))
return false;... ...
}
As the bio_set_op_attrs() reset the (REQ_SYNC | REQ_IDLE), so this write bio will be considered as non-direct write.
After this fix, bio to nvme will flaged as (REQ_SYNC | REQ_IDLE), then fio for writing will get about 1000M/s bandwidth.
---
V3-V2:
remove an unused close-line in commit message.
drivers/md/bcache/request.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/md/bcache/request.c +1 -1
@@ -244,7 +244,7 @@ static void bch_data_insert_start(struct closure *cl)trace_bcache_cache_insert (k);bch_keylist_push (&op->insert_keys); -bio_set_op_attrs (n,REQ_OP_WRITE ,0 ); + n->bi_opf |=REQ_OP_WRITE ;bch_submit_bbio (n, op->c, k,0 ); }while (n != bio);
Signature · 1 line
2.25.1
Quoted 47 lines · Dongsheng Yang
commit ad0d9e76(bcache: use bio op accessors) makes the
bi_opfmodified bybio_set_op_attrs(). But there is a logical problem in this commit:trace_bcache_cache_insert(k); bch_keylist_push(&op->insert_keys);drivers/md/bcache/request.c +1
-1- n->bi_rw |=REQ_WRITE ; +bio_set_op_attrs (n,REQ_OP_WRITE ,0 );bch_submit_bbio (n, op->c, k,0 ); }while (n != bio);The old code add
REQ_WRITEinto bio n and keep other flags; the new code setREQ_OP_WRITEtobi_opf, but reset all other flags.This problem is discoverd in our performance testing:
(1) start a fio with 1M x 128depth for read in/dev/nvme0n1p1
(2) start a fio with 1M x 128depth for write in/dev/escache0(cache device is/dev/nvme0n1p2)We found the BW of reading is 2000+M/s, but the BW of writing is 0-100M/s. After some debugging, we found the problem is io submit in writting is very slow.
bch_data_insert_start()insert a bio to/dev/nvme0n1p1, but ascached_devsubmit stack bio will be added intocurrent->bio_list, and return.Then__submit_bio_noacct()will submit the new bio inbio_listinto/dev/nvme0n1p1. This operation would be slow inblk_mq_submit_bio()->rq_qos_throttle(q, bio);The
rq_qos_throttle()will callwbt_should_throttle(),
static inline boolwbt_should_throttle(struct rq_wb *rwb, struct bio *bio){ switch (bio_op(bio)) { case REQ_OP_WRITE: /* * Don't throttle WRITE_ODIRECT */ if ((bio->bi_opf & (REQ_SYNC | REQ_IDLE)) == (REQ_SYNC | REQ_IDLE)) return false;... ...
}As the
bio_set_op_attrs()reset the (REQ_SYNC|REQ_IDLE), so this write bio will be considered as non-direct write.
Quoted 6 lines
After this fix, bio to nvme will flaged as (
REQ_SYNC|REQ_IDLE), then fio for writing will get about 1000M/s bandwidth.Fixes ad0d9e76a412 ("bcache: use bio op accessors")CC Mike Christie <[email protected]>Signed-off-by Dongsheng Yang <[email protected]>
The V3 patch is added to my for-next queue. Thanks for the fix up.
Coly Li
Quoted 21 lines
---
V3-V2:
remove an unused close-line in commit message.drivers/md/bcache/request.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/md/bcache/request.c +1
-1@@ -244,7 +244,7 @@ static void bch_data_insert_start(struct closure *cl)trace_bcache_cache_insert (k);bch_keylist_push (&op->insert_keys); -bio_set_op_attrs (n,REQ_OP_WRITE ,0 ); + n->bi_opf |=REQ_OP_WRITE ;bch_submit_bbio (n, op->c, k,0 ); }while (n != bio);