Re: [PATCH] block: tidy up blk_mq_plug

[Jens Axboe <axboe@kernel.dk>]

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@lst.de>
>>> ---
>>>    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