On 2019/07/12 11:44, Jens Axboe wrote: > 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. >
OK. Thanks. -- Damien Le Moal Western Digital Research
