On 7/11/19 8:37 PM, Damien Le Moal wrote: > On 2019/07/12 3:09, Jens Axboe wrote: >> On 7/11/19 5:17 AM, Christoph Hellwig wrote: >>> Make the zoned device write path the special case and just fall >>> though to the defaul case to make the code easier to read. Also >>> update the top of function comment to use the proper kdoc format >>> and remove the extra in-function comments that just duplicate it. >>> >>> Signed-off-by: Christoph Hellwig <[email protected]> >>> --- >>> block/blk-mq.h | 14 ++++---------- >>> 1 file changed, 4 insertions(+), 10 deletions(-) >>> >>> diff --git a/block/blk-mq.h b/block/blk-mq.h >>> index 32c62c64e6c2..ab80fd2b3803 100644 >>> --- a/block/blk-mq.h >>> +++ b/block/blk-mq.h >>> @@ -233,7 +233,7 @@ static inline void blk_mq_clear_mq_map(struct >>> blk_mq_queue_map *qmap) >>> qmap->mq_map[cpu] = 0; >>> } >>> >>> -/* >>> +/** >>> * blk_mq_plug() - Get caller context plug >>> * @q: request queue >>> * @bio : the bio being submitted by the caller context >>> @@ -254,15 +254,9 @@ static inline void blk_mq_clear_mq_map(struct >>> blk_mq_queue_map *qmap) >>> static inline struct blk_plug *blk_mq_plug(struct request_queue *q, >>> struct bio *bio) >>> { >>> - /* >>> - * For regular block devices or read operations, use the context plug >>> - * which may be NULL if blk_start_plug() was not executed. >>> - */ >>> - if (!blk_queue_is_zoned(q) || !op_is_write(bio_op(bio))) >>> - return current->plug; >>> - >>> - /* Zoned block device write operation case: do not plug the BIO */ >>> - return NULL; >>> + if (blk_queue_is_zoned(q) && op_is_write(bio_op(bio))) >>> + return NULL; >>> + return current->plug; >>> } >>> >>> #endif >> >> I agree it's more readable, but probably also means that the path that we >> care the least about (zoned+write) is now the inline one. > > What about an additional inline function ? > So something like this is very readable I think and blk_mq_plug() can also be > optimized with #ifdef for the !CONFIG_BLK_DEV_ZONED case. > > #ifndef CONFIG_BLK_DEV_ZONED > static inline struct blk_plug *blk_mq_plug(struct request_queue *q, > struct bio *bio) > { > return current->plug; > } > #else > static inline struct blk_plug *blk_zoned_get_plug(struct request_queue *q, > struct bio *bio) > { > if (op_is_write(bio_op(bio))) > return NULL; > > return current->plug; > } > > static inline struct blk_plug *blk_mq_plug(struct request_queue *q, > struct bio *bio) > { > if (!blk_queue_is_zoned(q)) > return current->plug; > > return blk_zoned_get_plug(q, bio); > } > #endif
Let's not go that far into ifdef'ery, please... Besides, that usually solves nothing, as most/all kernels will have zoned support enabled. I'm actually fine with the existing setup. Yes, the other variant is easier to read, but I bet the existing one inlines better. -- Jens Axboe
