* [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.