* [PATCH] block: tidy up blk_mq_plug
@ 2019-07-11 11:17 Christoph Hellwig
2019-07-11 18:08 ` Chaitanya Kulkarni
2019-07-11 18:09 ` Jens Axboe
0 siblings, 2 replies; 6+ messages in thread
From: Christoph Hellwig @ 2019-07-11 11:17 UTC (permalink / raw)
To: axboe; +Cc: damien.lemoal, linux-block
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
--
2.20.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] block: tidy up blk_mq_plug
2019-07-11 11:17 [PATCH] block: tidy up blk_mq_plug Christoph Hellwig
@ 2019-07-11 18:08 ` Chaitanya Kulkarni
2019-07-11 18:09 ` Jens Axboe
1 sibling, 0 replies; 6+ messages in thread
From: Chaitanya Kulkarni @ 2019-07-11 18:08 UTC (permalink / raw)
To: Christoph Hellwig, axboe; +Cc: Damien Le Moal, linux-block
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] block: tidy up blk_mq_plug
2019-07-11 11:17 [PATCH] block: tidy up blk_mq_plug Christoph Hellwig
2019-07-11 18:08 ` Chaitanya Kulkarni
@ 2019-07-11 18:09 ` Jens Axboe
2019-07-12 2:37 ` Damien Le Moal
1 sibling, 1 reply; 6+ messages in thread
From: Jens Axboe @ 2019-07-11 18:09 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: damien.lemoal, linux-block
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.
--
Jens Axboe
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] block: tidy up blk_mq_plug
2019-07-11 18:09 ` Jens Axboe
@ 2019-07-12 2:37 ` Damien Le Moal
2019-07-12 2:44 ` Jens Axboe
0 siblings, 1 reply; 6+ messages in thread
From: Damien Le Moal @ 2019-07-12 2:37 UTC (permalink / raw)
To: Jens Axboe, Christoph Hellwig; +Cc: linux-block
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
--
Damien Le Moal
Western Digital Research
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] block: tidy up blk_mq_plug
2019-07-12 2:37 ` Damien Le Moal
@ 2019-07-12 2:44 ` Jens Axboe
2019-07-12 2:50 ` Damien Le Moal
0 siblings, 1 reply; 6+ messages in thread
From: Jens Axboe @ 2019-07-12 2:44 UTC (permalink / raw)
To: Damien Le Moal, Christoph Hellwig; +Cc: linux-block
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] block: tidy up blk_mq_plug
2019-07-12 2:44 ` Jens Axboe
@ 2019-07-12 2:50 ` Damien Le Moal
0 siblings, 0 replies; 6+ messages in thread
From: Damien Le Moal @ 2019-07-12 2:50 UTC (permalink / raw)
To: Jens Axboe, Christoph Hellwig; +Cc: linux-block
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.
--
Damien Le Moal
Western Digital Research
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-07-12 2:50 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-11 11:17 [PATCH] block: tidy up blk_mq_plug Christoph Hellwig
2019-07-11 18:08 ` Chaitanya Kulkarni
2019-07-11 18:09 ` Jens Axboe
2019-07-12 2:37 ` Damien Le Moal
2019-07-12 2:44 ` Jens Axboe
2019-07-12 2:50 ` Damien Le Moal
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.