block: tidy up blk_mq_plug
diff mbox series

Message ID 20190711111714.4802-1-hch@lst.de
State New
Headers show
Series
  • block: tidy up blk_mq_plug
Related show

Commit Message

Christoph Hellwig July 11, 2019, 11:17 a.m. UTC
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(-)

Comments

Chaitanya Kulkarni July 11, 2019, 6:08 p.m. UTC | #1
Looks good, we should avoid "!" conditions when possible it
makes code much easier to read.

Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>

On 07/11/2019 04: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
>
Jens Axboe July 11, 2019, 6:09 p.m. UTC | #2
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.
Damien Le Moal July 12, 2019, 2:37 a.m. UTC | #3
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
Jens Axboe July 12, 2019, 2:44 a.m. UTC | #4
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.
Damien Le Moal July 12, 2019, 2:50 a.m. UTC | #5
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@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.
> 

OK. Thanks.

Patch
diff mbox series

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