| Message ID | 20190711111714.4802-1-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block: tidy up blk_mq_plug | expand |
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 >
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.
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
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.
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.
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
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(-)