| Message ID | 20230424203329.2369688-3-bvanassche@acm.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mq-deadline: Improve support for zoned block devices | expand |
On Mon, Apr 24, 2023 at 01:33:22PM -0700, Bart Van Assche wrote: > @@ -367,8 +367,9 @@ static inline void blk_mq_clear_mq_map(struct blk_mq_queue_map *qmap) > static inline struct blk_plug *blk_mq_plug( struct bio *bio) > { > /* Zoned block device write operation case: do not plug the BIO */ > - if (IS_ENABLED(CONFIG_BLK_DEV_ZONED) && > - bdev_op_is_zoned_write(bio->bi_bdev, bio_op(bio))) > + if ((bio_op(bio) == REQ_OP_WRITE || > + bio_op(bio) == REQ_OP_WRITE_ZEROES) && > + disk_zone_is_seq(bio->bi_bdev->bd_disk, bio->bi_iter.bi_sector)) > return NULL; I find this a bit hard to hard to read. Why not: if (disk_zone_is_seq(bio->bi_bdev->bd_disk, bio->bi_iter.bi_sector)) { /* * Do not plug for writes that require zone locking. */ if (bio_op(bio) == REQ_OP_WRITE || bio_op(bio) == REQ_OP_WRITE_ZEROES) return NULL; } > bool blk_req_needs_zone_write_lock(struct request *rq) > { > - if (!rq->q->disk->seq_zones_wlock) > - return false; > - > - if (bdev_op_is_zoned_write(rq->q->disk->part0, req_op(rq))) > - return blk_rq_zone_is_seq(rq); > - > - return false; > + return rq->q->disk->seq_zones_wlock && > + (req_op(rq) == REQ_OP_WRITE || > + req_op(rq) == REQ_OP_WRITE_ZEROES) && > + blk_rq_zone_is_seq(rq); Similaly here. The old version did flow much better, so I'd prefer something like: if (!rq->q->disk->seq_zones_wlock || !blk_rq_zone_is_seq(rq)) return false; return req_op(rq) == REQ_OP_WRITE || req_op(rq) == REQ_OP_WRITE_ZEROES); I also wonder if the check that and op is write or write zeroes, that is needs zone locking would be useful instead of dupliating it all over. That is instead of removing bdev_op_is_zoned_write keep a op_is_zoned_write without the bdev_is_zoned check.
On 4/27/23 22:44, Christoph Hellwig wrote: > On Mon, Apr 24, 2023 at 01:33:22PM -0700, Bart Van Assche wrote: >> @@ -367,8 +367,9 @@ static inline void blk_mq_clear_mq_map(struct blk_mq_queue_map *qmap) >> static inline struct blk_plug *blk_mq_plug( struct bio *bio) >> { >> /* Zoned block device write operation case: do not plug the BIO */ >> - if (IS_ENABLED(CONFIG_BLK_DEV_ZONED) && >> - bdev_op_is_zoned_write(bio->bi_bdev, bio_op(bio))) >> + if ((bio_op(bio) == REQ_OP_WRITE || >> + bio_op(bio) == REQ_OP_WRITE_ZEROES) && >> + disk_zone_is_seq(bio->bi_bdev->bd_disk, bio->bi_iter.bi_sector)) >> return NULL; > > I find this a bit hard to hard to read. Why not: > > if (disk_zone_is_seq(bio->bi_bdev->bd_disk, bio->bi_iter.bi_sector)) { > /* > * Do not plug for writes that require zone locking. > */ > if (bio_op(bio) == REQ_OP_WRITE || > bio_op(bio) == REQ_OP_WRITE_ZEROES) > return NULL; > } In the above alternative the expensive check happens before a check that is not expensive at all. Do you really want me to call disk_zone_is_seq() before checking the operation type? >> bool blk_req_needs_zone_write_lock(struct request *rq) >> { >> - if (!rq->q->disk->seq_zones_wlock) >> - return false; >> - >> - if (bdev_op_is_zoned_write(rq->q->disk->part0, req_op(rq))) >> - return blk_rq_zone_is_seq(rq); >> - >> - return false; >> + return rq->q->disk->seq_zones_wlock && >> + (req_op(rq) == REQ_OP_WRITE || >> + req_op(rq) == REQ_OP_WRITE_ZEROES) && >> + blk_rq_zone_is_seq(rq); > > Similaly here. The old version did flow much better, so I'd prefer > something like: > > > if (!rq->q->disk->seq_zones_wlock || !blk_rq_zone_is_seq(rq)) > return false; > return req_op(rq) == REQ_OP_WRITE || req_op(rq) == REQ_OP_WRITE_ZEROES); > > I also wonder if the check that and op is write or write zeroes, that > is needs zone locking would be useful instead of dupliating it all > over. That is instead of removing bdev_op_is_zoned_write > keep a op_is_zoned_write without the bdev_is_zoned check. I will introduce an op_is_zoned_write() helper function. Thanks, Bart.
On Fri, Apr 28, 2023 at 12:46:06PM -0700, Bart Van Assche wrote: >>> + if ((bio_op(bio) == REQ_OP_WRITE || >>> + bio_op(bio) == REQ_OP_WRITE_ZEROES) && >>> + disk_zone_is_seq(bio->bi_bdev->bd_disk, bio->bi_iter.bi_sector)) >>> return NULL; >> >> I find this a bit hard to hard to read. Why not: >> >> if (disk_zone_is_seq(bio->bi_bdev->bd_disk, bio->bi_iter.bi_sector)) { >> /* >> * Do not plug for writes that require zone locking. >> */ >> if (bio_op(bio) == REQ_OP_WRITE || >> bio_op(bio) == REQ_OP_WRITE_ZEROES) >> return NULL; >> } > > In the above alternative the expensive check happens before a check that is not > expensive at all. Do you really want me to call disk_zone_is_seq() before checking > the operation type? What expensive check? The first check in disk_zone_is_seq is for a zoned device, avoiding any further check if it is not.
diff --git a/block/blk-mq.h b/block/blk-mq.h index e876584d3516..6bb1281a61f2 100644 --- a/block/blk-mq.h +++ b/block/blk-mq.h @@ -367,8 +367,9 @@ static inline void blk_mq_clear_mq_map(struct blk_mq_queue_map *qmap) static inline struct blk_plug *blk_mq_plug( struct bio *bio) { /* Zoned block device write operation case: do not plug the BIO */ - if (IS_ENABLED(CONFIG_BLK_DEV_ZONED) && - bdev_op_is_zoned_write(bio->bi_bdev, bio_op(bio))) + if ((bio_op(bio) == REQ_OP_WRITE || + bio_op(bio) == REQ_OP_WRITE_ZEROES) && + disk_zone_is_seq(bio->bi_bdev->bd_disk, bio->bi_iter.bi_sector)) return NULL; /* diff --git a/block/blk-zoned.c b/block/blk-zoned.c index 835d9e937d4d..4640b75ae66f 100644 --- a/block/blk-zoned.c +++ b/block/blk-zoned.c @@ -57,13 +57,10 @@ EXPORT_SYMBOL_GPL(blk_zone_cond_str); */ bool blk_req_needs_zone_write_lock(struct request *rq) { - if (!rq->q->disk->seq_zones_wlock) - return false; - - if (bdev_op_is_zoned_write(rq->q->disk->part0, req_op(rq))) - return blk_rq_zone_is_seq(rq); - - return false; + return rq->q->disk->seq_zones_wlock && + (req_op(rq) == REQ_OP_WRITE || + req_op(rq) == REQ_OP_WRITE_ZEROES) && + blk_rq_zone_is_seq(rq); } EXPORT_SYMBOL_GPL(blk_req_needs_zone_write_lock); diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h index e3242e67a8e3..b700c935e230 100644 --- a/include/linux/blkdev.h +++ b/include/linux/blkdev.h @@ -1284,15 +1284,6 @@ static inline unsigned int bdev_zone_no(struct block_device *bdev, sector_t sec) return disk_zone_no(bdev->bd_disk, sec); } -static inline bool bdev_op_is_zoned_write(struct block_device *bdev, - blk_opf_t op) -{ - if (!bdev_is_zoned(bdev)) - return false; - - return op == REQ_OP_WRITE || op == REQ_OP_WRITE_ZEROES; -} - static inline sector_t bdev_zone_sectors(struct block_device *bdev) { struct request_queue *q = bdev_get_queue(bdev);
Micro-optimize blk_req_needs_zone_write_lock() by inlining bdev_op_is_zoned_write(). This is a micro-optimization because the number of pointers that is dereferenced while testing whether the request queue is associated with a zoned device is reduced. Cc: Damien Le Moal <damien.lemoal@opensource.wdc.com> Cc: Christoph Hellwig <hch@lst.de> Cc: Ming Lei <ming.lei@redhat.com> Signed-off-by: Bart Van Assche <bvanassche@acm.org> --- block/blk-mq.h | 5 +++-- block/blk-zoned.c | 11 ++++------- include/linux/blkdev.h | 9 --------- 3 files changed, 7 insertions(+), 18 deletions(-)