diff mbox series

[1/1] blk-mq: Inline status checkers

Message ID 1cd320dad54bd78cb6721f7fe8dd2e197b9fbfa2.1569830796.git.asml.silence@gmail.com (mailing list archive)
State New, archived
Headers show
Series [1/1] blk-mq: Inline status checkers | expand

Commit Message

Pavel Begunkov Sept. 30, 2019, 8:25 a.m. UTC 
From: Pavel Begunkov <asml.silence@gmail.com>

blk_mq_request_completed() and blk_mq_request_started() are
short, inline it.

Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 block/blk-mq.c         | 12 ------------
 block/blk-mq.h         |  9 ---------
 include/linux/blk-mq.h | 20 ++++++++++++++++++--
 3 files changed, 18 insertions(+), 23 deletions(-)

Comments

Christoph Hellwig Sept. 30, 2019, 8:35 a.m. UTC | #1 
On Mon, Sep 30, 2019 at 11:25:49AM +0300, Pavel Begunkov (Silence) wrote:
> From: Pavel Begunkov <asml.silence@gmail.com>
> 
> blk_mq_request_completed() and blk_mq_request_started() are
> short, inline it.
> 
> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
> ---
>  block/blk-mq.c         | 12 ------------
>  block/blk-mq.h         |  9 ---------
>  include/linux/blk-mq.h | 20 ++++++++++++++++++--
>  3 files changed, 18 insertions(+), 23 deletions(-)
> 
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index 279b138a9e50..d97181d9a3ec 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -647,18 +647,6 @@ bool blk_mq_complete_request(struct request *rq)
>  }
>  EXPORT_SYMBOL(blk_mq_complete_request);
>  
> -int blk_mq_request_started(struct request *rq)
> -{
> -	return blk_mq_rq_state(rq) != MQ_RQ_IDLE;
> -}
> -EXPORT_SYMBOL_GPL(blk_mq_request_started);
> -
> -int blk_mq_request_completed(struct request *rq)
> -{
> -	return blk_mq_rq_state(rq) == MQ_RQ_COMPLETE;
> -}
> -EXPORT_SYMBOL_GPL(blk_mq_request_completed);

How about just killing these helpers instead?
Pavel Begunkov Sept. 30, 2019, 7:45 p.m. UTC | #2 
On 30/09/2019 11:35, Christoph Hellwig wrote:
> On Mon, Sep 30, 2019 at 11:25:49AM +0300, Pavel Begunkov (Silence) wrote:
>> From: Pavel Begunkov <asml.silence@gmail.com>
>>
>> blk_mq_request_completed() and blk_mq_request_started() are
>> short, inline it.
>>
>> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
>> ---
>>  block/blk-mq.c         | 12 ------------
>>  block/blk-mq.h         |  9 ---------
>>  include/linux/blk-mq.h | 20 ++++++++++++++++++--
>>  3 files changed, 18 insertions(+), 23 deletions(-)
>>
>> diff --git a/block/blk-mq.c b/block/blk-mq.c
>> index 279b138a9e50..d97181d9a3ec 100644
>> --- a/block/blk-mq.c
>> +++ b/block/blk-mq.c
>> @@ -647,18 +647,6 @@ bool blk_mq_complete_request(struct request *rq)
>>  }
>>  EXPORT_SYMBOL(blk_mq_complete_request);
>>  
>> -int blk_mq_request_started(struct request *rq)
>> -{
>> -	return blk_mq_rq_state(rq) != MQ_RQ_IDLE;
>> -}
>> -EXPORT_SYMBOL_GPL(blk_mq_request_started);
>> -
>> -int blk_mq_request_completed(struct request *rq)
>> -{
>> -	return blk_mq_rq_state(rq) == MQ_RQ_COMPLETE;
>> -}
>> -EXPORT_SYMBOL_GPL(blk_mq_request_completed);
> 
> How about just killing these helpers instead?
> 
I'm not sure that this is better. That's more intrusive and
blk_mq_request_started() looks clearer than
(blk_mq_rq_state(rq) != MQ_RQ_IDLE).

Anyway, I've sent v2 and fine with both.
Bart Van Assche Sept. 30, 2019, 7:53 p.m. UTC | #3 
On 9/30/19 12:43 PM, Pavel Begunkov (Silence) wrote:
> @@ -282,7 +282,7 @@ static bool bt_tags_iter(struct sbitmap *bitmap, unsigned int bitnr, void *data)
>  	 * test and set the bit before assining ->rqs[].
>  	 */
>  	rq = tags->rqs[bitnr];
> -	if (rq && blk_mq_request_started(rq))
> +	if (rq && blk_mq_rq_state(rq) != MQ_RQ_IDLE)
>  		return iter_data->fn(rq, iter_data->data, reserved);
>  
>  	return true>
> @@ -360,7 +360,7 @@ static bool blk_mq_tagset_count_completed_rqs(struct request *rq,
>  {
>  	unsigned *count = data;
>  
> -	if (blk_mq_request_completed(rq))
> +	if (blk_mq_rq_state(rq) == MQ_RQ_COMPLETE)
>  		(*count)++;
>  	return true;
>  }

Changes like the above significantly reduce readability of the code in
the block layer core. I don't like this. I think this patch is a step
backwards instead of a step forwards.

Bart.
Pavel Begunkov Sept. 30, 2019, 8:12 p.m. UTC | #4 
On 30/09/2019 22:53, Bart Van Assche wrote:
> On 9/30/19 12:43 PM, Pavel Begunkov (Silence) wrote:
>> @@ -282,7 +282,7 @@ static bool bt_tags_iter(struct sbitmap *bitmap, unsigned int bitnr, void *data)
>>  	 * test and set the bit before assining ->rqs[].
>>  	 */
>>  	rq = tags->rqs[bitnr];
>> -	if (rq && blk_mq_request_started(rq))
>> +	if (rq && blk_mq_rq_state(rq) != MQ_RQ_IDLE)
>>  		return iter_data->fn(rq, iter_data->data, reserved);
>>  
>>  	return true>
>> @@ -360,7 +360,7 @@ static bool blk_mq_tagset_count_completed_rqs(struct request *rq,
>>  {
>>  	unsigned *count = data;
>>  
>> -	if (blk_mq_request_completed(rq))
>> +	if (blk_mq_rq_state(rq) == MQ_RQ_COMPLETE)
>>  		(*count)++;
>>  	return true;
>>  }
> 
> Changes like the above significantly reduce readability of the code in
> the block layer core. I don't like this. I think this patch is a step
> backwards instead of a step forwards.

Yep, looks too bulky.

Jens, could you consider the first version?
Mike Snitzer Sept. 30, 2019, 8:13 p.m. UTC | #5 
On Mon, Sep 30 2019 at  3:53pm -0400,
Bart Van Assche <bvanassche@acm.org> wrote:

> On 9/30/19 12:43 PM, Pavel Begunkov (Silence) wrote:
> > @@ -282,7 +282,7 @@ static bool bt_tags_iter(struct sbitmap *bitmap, unsigned int bitnr, void *data)
> >  	 * test and set the bit before assining ->rqs[].
> >  	 */
> >  	rq = tags->rqs[bitnr];
> > -	if (rq && blk_mq_request_started(rq))
> > +	if (rq && blk_mq_rq_state(rq) != MQ_RQ_IDLE)
> >  		return iter_data->fn(rq, iter_data->data, reserved);
> >  
> >  	return true>
> > @@ -360,7 +360,7 @@ static bool blk_mq_tagset_count_completed_rqs(struct request *rq,
> >  {
> >  	unsigned *count = data;
> >  
> > -	if (blk_mq_request_completed(rq))
> > +	if (blk_mq_rq_state(rq) == MQ_RQ_COMPLETE)
> >  		(*count)++;
> >  	return true;
> >  }
> 
> Changes like the above significantly reduce readability of the code in
> the block layer core. I don't like this. I think this patch is a step
> backwards instead of a step forwards.

I agree, not helpful.
Jens Axboe Oct. 2, 2019, 1:09 p.m. UTC | #6 
On 9/30/19 2:12 PM, Pavel Begunkov wrote:
> On 30/09/2019 22:53, Bart Van Assche wrote:
>> On 9/30/19 12:43 PM, Pavel Begunkov (Silence) wrote:
>>> @@ -282,7 +282,7 @@ static bool bt_tags_iter(struct sbitmap *bitmap, unsigned int bitnr, void *data)
>>>   	 * test and set the bit before assining ->rqs[].
>>>   	 */
>>>   	rq = tags->rqs[bitnr];
>>> -	if (rq && blk_mq_request_started(rq))
>>> +	if (rq && blk_mq_rq_state(rq) != MQ_RQ_IDLE)
>>>   		return iter_data->fn(rq, iter_data->data, reserved);
>>>   
>>>   	return true>
>>> @@ -360,7 +360,7 @@ static bool blk_mq_tagset_count_completed_rqs(struct request *rq,
>>>   {
>>>   	unsigned *count = data;
>>>   
>>> -	if (blk_mq_request_completed(rq))
>>> +	if (blk_mq_rq_state(rq) == MQ_RQ_COMPLETE)
>>>   		(*count)++;
>>>   	return true;
>>>   }
>>
>> Changes like the above significantly reduce readability of the code in
>> the block layer core. I don't like this. I think this patch is a step
>> backwards instead of a step forwards.
> 
> Yep, looks too bulky.
> 
> Jens, could you consider the first version?

Yes, first one is fine, I have applied it. Thanks.
diff mbox series

Patch

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 279b138a9e50..d97181d9a3ec 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -647,18 +647,6 @@  bool blk_mq_complete_request(struct request *rq)
 }
 EXPORT_SYMBOL(blk_mq_complete_request);
 
-int blk_mq_request_started(struct request *rq)
-{
-	return blk_mq_rq_state(rq) != MQ_RQ_IDLE;
-}
-EXPORT_SYMBOL_GPL(blk_mq_request_started);
-
-int blk_mq_request_completed(struct request *rq)
-{
-	return blk_mq_rq_state(rq) == MQ_RQ_COMPLETE;
-}
-EXPORT_SYMBOL_GPL(blk_mq_request_completed);
-
 void blk_mq_start_request(struct request *rq)
 {
 	struct request_queue *q = rq->q;
diff --git a/block/blk-mq.h b/block/blk-mq.h
index 32c62c64e6c2..eaaca8fc1c28 100644
--- a/block/blk-mq.h
+++ b/block/blk-mq.h
@@ -128,15 +128,6 @@  extern void blk_mq_hctx_kobj_init(struct blk_mq_hw_ctx *hctx);
 
 void blk_mq_release(struct request_queue *q);
 
-/**
- * blk_mq_rq_state() - read the current MQ_RQ_* state of a request
- * @rq: target request.
- */
-static inline enum mq_rq_state blk_mq_rq_state(struct request *rq)
-{
-	return READ_ONCE(rq->state);
-}
-
 static inline struct blk_mq_ctx *__blk_mq_get_ctx(struct request_queue *q,
 					   unsigned int cpu)
 {
diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
index 0bf056de5cc3..090a5601be15 100644
--- a/include/linux/blk-mq.h
+++ b/include/linux/blk-mq.h
@@ -301,9 +301,25 @@  static inline u16 blk_mq_unique_tag_to_tag(u32 unique_tag)
 	return unique_tag & BLK_MQ_UNIQUE_TAG_MASK;
 }
 
+/**
+ * blk_mq_rq_state() - read the current MQ_RQ_* state of a request
+ * @rq: target request.
+ */
+static inline enum mq_rq_state blk_mq_rq_state(struct request *rq)
+{
+	return READ_ONCE(rq->state);
+}
+
+static inline int blk_mq_request_started(struct request *rq)
+{
+	return blk_mq_rq_state(rq) != MQ_RQ_IDLE;
+}
+
+static inline int blk_mq_request_completed(struct request *rq)
+{
+	return blk_mq_rq_state(rq) == MQ_RQ_COMPLETE;
+}
 
-int blk_mq_request_started(struct request *rq);
-int blk_mq_request_completed(struct request *rq);
 void blk_mq_start_request(struct request *rq);
 void blk_mq_end_request(struct request *rq, blk_status_t error);
 void __blk_mq_end_request(struct request *rq, blk_status_t error);