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 <hch@xxxxxx>
>>>> ---
>>>> 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
