| Message ID | 20211103183222.180268-4-axboe@kernel.dk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Alloc batch fixes | expand |
On Wed, Nov 03, 2021 at 12:32:21PM -0600, Jens Axboe wrote: > Retain the old logic for the fops based submit, but for our internal > blk_mq_submit_bio(), move the queue entering logic into the core > function itself. Can you explain the why? I guess you want to skip the extra reference for the cached requests now that they already have one. But please state that, and explain why it is a fix, as to me it just seems like another little optimization. > We need to be a bit careful if going into the scheduler, as a scheduler > or queue mappings can arbitrarily change before we have entered the queue. > Have the bio scheduler mapping do that separately, it's a very cheap > operation compared to actually doing merging locking and lookups. So just don't do the merges for cache requets and side step this extra bio_queue_enter for that case? > - if (unlikely(bio_queue_enter(bio) != 0)) > - return; > - > if (!submit_bio_checks(bio) || !blk_crypto_bio_prep(&bio)) > - goto queue_exit; > + return; This is broken, we really ant the submit checks under freeze protection to make sure the parameters can't be changed underneath us. > +static inline bool blk_mq_queue_enter(struct request_queue *q, struct bio *bio) > +{ > + if (!blk_try_enter_queue(q, false) && bio_queue_enter(bio)) > + return false; > + return true; > +} This looks weird, as blk_try_enter_queue is already called by bio_queue_enter. > } else { > struct blk_mq_alloc_data data = { > .q = q, > @@ -2528,6 +2534,11 @@ void blk_mq_submit_bio(struct bio *bio) > .cmd_flags = bio->bi_opf, > }; > > + if (unlikely(!blk_mq_queue_enter(q, bio))) > + return; > + > + rq_qos_throttle(q, bio); > + At some point the code in this !cached branch really needs to move into a helper..
On 11/4/21 3:10 AM, Christoph Hellwig wrote: > On Wed, Nov 03, 2021 at 12:32:21PM -0600, Jens Axboe wrote: >> Retain the old logic for the fops based submit, but for our internal >> blk_mq_submit_bio(), move the queue entering logic into the core >> function itself. > > Can you explain the why? I guess you want to skip the extra reference > for the cached requests now that they already have one. But please > state that, and explain why it is a fix, as to me it just seems like > another little optimization. It's just pointless to grab double references, and counter productive too. >> We need to be a bit careful if going into the scheduler, as a scheduler >> or queue mappings can arbitrarily change before we have entered the queue. >> Have the bio scheduler mapping do that separately, it's a very cheap >> operation compared to actually doing merging locking and lookups. > > So just don't do the merges for cache requets and side step this > extra bio_queue_enter for that case? I'd be fine with that, but it's a bit of a chicken and egg situation as we don't know. I guess we could move the plugged request check earlier, and just bypass merging there. Though that makes it a special case thing, and it's generally useful now. Not sure that would be a good idea. >> - if (unlikely(bio_queue_enter(bio) != 0)) >> - return; >> - >> if (!submit_bio_checks(bio) || !blk_crypto_bio_prep(&bio)) >> - goto queue_exit; >> + return; > > This is broken, we really ant the submit checks under freeze > protection to make sure the parameters can't be changed underneath > us. Which parameters are you worried about in submit_bio_checks()? I don't immediately see anything that would make me worry about it. >> +static inline bool blk_mq_queue_enter(struct request_queue *q, struct bio *bio) >> +{ >> + if (!blk_try_enter_queue(q, false) && bio_queue_enter(bio)) >> + return false; >> + return true; >> +} > > This looks weird, as blk_try_enter_queue is already called by > bio_queue_enter. It's just for avoiding a pointless call into bio_queue_enter(), which isn't needed it blk_try_enter_queue() is successful. The latter is short and small and can be inlined, while bio_queue_enter() is a lot bigger. >> } else { >> struct blk_mq_alloc_data data = { >> .q = q, >> @@ -2528,6 +2534,11 @@ void blk_mq_submit_bio(struct bio *bio) >> .cmd_flags = bio->bi_opf, >> }; >> >> + if (unlikely(!blk_mq_queue_enter(q, bio))) >> + return; >> + >> + rq_qos_throttle(q, bio); >> + > > At some point the code in this !cached branch really needs to move > into a helper.. Like in the next patch?
On 11/4/21 5:41 AM, Jens Axboe wrote: >> This is broken, we really ant the submit checks under freeze >> protection to make sure the parameters can't be changed underneath >> us. > > Which parameters are you worried about in submit_bio_checks()? I don't > immediately see anything that would make me worry about it. To player it safer, I would suggest we fold in something like the below. That keeps the submit_checks() under the queue enter. diff --git a/block/blk-core.c b/block/blk-core.c index 2b12a427ffa6..18aab7f8469a 100644 --- a/block/blk-core.c +++ b/block/blk-core.c @@ -746,7 +746,7 @@ static inline blk_status_t blk_check_zone_append(struct request_queue *q, return BLK_STS_OK; } -static noinline_for_stack bool submit_bio_checks(struct bio *bio) +noinline_for_stack bool submit_bio_checks(struct bio *bio) { struct block_device *bdev = bio->bi_bdev; struct request_queue *q = bdev_get_queue(bdev); @@ -868,14 +868,13 @@ static void __submit_bio(struct bio *bio) { struct gendisk *disk = bio->bi_bdev->bd_disk; - if (!submit_bio_checks(bio) || !blk_crypto_bio_prep(&bio)) - return; if (!disk->fops->submit_bio) { blk_mq_submit_bio(bio); } else { if (unlikely(bio_queue_enter(bio) != 0)) return; - disk->fops->submit_bio(bio); + if (submit_bio_checks(bio) && blk_crypto_bio_prep(&bio)) + disk->fops->submit_bio(bio); blk_queue_exit(disk->queue); } } diff --git a/block/blk-mq.c b/block/blk-mq.c index e92c36f2326a..2dab9bdcc51a 100644 --- a/block/blk-mq.c +++ b/block/blk-mq.c @@ -2526,6 +2526,9 @@ void blk_mq_submit_bio(struct bio *bio) unsigned int nr_segs = 1; blk_status_t ret; + if (unlikely(!blk_crypto_bio_prep(&bio))) + return; + blk_queue_bounce(q, &bio); if (blk_may_split(q, bio)) __blk_queue_split(q, &bio, &nr_segs); @@ -2551,6 +2554,8 @@ void blk_mq_submit_bio(struct bio *bio) if (unlikely(!blk_mq_queue_enter(q, bio))) return; + if (unlikely(!submit_bio_checks(bio))) + goto put_exit; rq_qos_throttle(q, bio); diff --git a/block/blk.h b/block/blk.h index f7371d3b1522..79c98ced59c8 100644 --- a/block/blk.h +++ b/block/blk.h @@ -56,6 +56,7 @@ void blk_freeze_queue(struct request_queue *q); void __blk_mq_unfreeze_queue(struct request_queue *q, bool force_atomic); void blk_queue_start_drain(struct request_queue *q); int bio_queue_enter(struct bio *bio); +bool submit_bio_checks(struct bio *bio); static inline bool blk_try_enter_queue(struct request_queue *q, bool pm) {
On Thu, Nov 04, 2021 at 05:41:35AM -0600, Jens Axboe wrote: > >> if (!submit_bio_checks(bio) || !blk_crypto_bio_prep(&bio)) > >> - goto queue_exit; > >> + return; > > > > This is broken, we really ant the submit checks under freeze > > protection to make sure the parameters can't be changed underneath > > us. > > Which parameters are you worried about in submit_bio_checks()? I don't > immediately see anything that would make me worry about it. Mostly checks if certain operations are supported or not, as revalidation could clear those. > > This looks weird, as blk_try_enter_queue is already called by > > bio_queue_enter. > > It's just for avoiding a pointless call into bio_queue_enter(), which > isn't needed it blk_try_enter_queue() is successful. The latter is short > and small and can be inlined, while bio_queue_enter() is a lot bigger. If this is so impotant let's operated with an inlined bio_queue_enter that calls out of line into slow path instead of open coding it like this. > >> } else { > >> struct blk_mq_alloc_data data = { > >> .q = q, > >> @@ -2528,6 +2534,11 @@ void blk_mq_submit_bio(struct bio *bio) > >> .cmd_flags = bio->bi_opf, > >> }; > >> > >> + if (unlikely(!blk_mq_queue_enter(q, bio))) > >> + return; > >> + > >> + rq_qos_throttle(q, bio); > >> + > > > > At some point the code in this !cached branch really needs to move > > into a helper.. > > Like in the next patch? No, I mean the !cached case which is a lot more convoluted.
On Thu, Nov 04, 2021 at 06:14:30AM -0600, Jens Axboe wrote: > To player it safer, I would suggest we fold in something like the > below. That keeps the submit_checks() under the queue enter. Yes, this looks much better. Nit below: > struct block_device *bdev = bio->bi_bdev; > struct request_queue *q = bdev_get_queue(bdev); > @@ -868,14 +868,13 @@ static void __submit_bio(struct bio *bio) > { > struct gendisk *disk = bio->bi_bdev->bd_disk; > > - if (!submit_bio_checks(bio) || !blk_crypto_bio_prep(&bio)) > - return; > if (!disk->fops->submit_bio) { > blk_mq_submit_bio(bio); > } else { > if (unlikely(bio_queue_enter(bio) != 0)) > return; > - disk->fops->submit_bio(bio); > + if (submit_bio_checks(bio) && blk_crypto_bio_prep(&bio)) > + disk->fops->submit_bio(bio); > blk_queue_exit(disk->queue); > } A this point moving the whole ->submit_bio based branch into a helper probably makes sense as well. > + if (unlikely(!blk_crypto_bio_prep(&bio))) > + return; > + > blk_queue_bounce(q, &bio); > if (blk_may_split(q, bio)) > __blk_queue_split(q, &bio, &nr_segs); > @@ -2551,6 +2554,8 @@ void blk_mq_submit_bio(struct bio *bio) > > if (unlikely(!blk_mq_queue_enter(q, bio))) > return; > + if (unlikely(!submit_bio_checks(bio))) > + goto put_exit; This now skips the checks for the cached request case, doesn't it?
On 11/4/21 11:32 AM, Christoph Hellwig wrote: > On Thu, Nov 04, 2021 at 06:14:30AM -0600, Jens Axboe wrote: >> To player it safer, I would suggest we fold in something like the >> below. That keeps the submit_checks() under the queue enter. > > Yes, this looks much better. Nit below: > >> struct block_device *bdev = bio->bi_bdev; >> struct request_queue *q = bdev_get_queue(bdev); >> @@ -868,14 +868,13 @@ static void __submit_bio(struct bio *bio) >> { >> struct gendisk *disk = bio->bi_bdev->bd_disk; >> >> - if (!submit_bio_checks(bio) || !blk_crypto_bio_prep(&bio)) >> - return; >> if (!disk->fops->submit_bio) { >> blk_mq_submit_bio(bio); >> } else { >> if (unlikely(bio_queue_enter(bio) != 0)) >> return; >> - disk->fops->submit_bio(bio); >> + if (submit_bio_checks(bio) && blk_crypto_bio_prep(&bio)) >> + disk->fops->submit_bio(bio); >> blk_queue_exit(disk->queue); >> } > > A this point moving the whole ->submit_bio based branch into a > helper probably makes sense as well. Sure, I can do that. >> + if (unlikely(!blk_crypto_bio_prep(&bio))) >> + return; >> + >> blk_queue_bounce(q, &bio); >> if (blk_may_split(q, bio)) >> __blk_queue_split(q, &bio, &nr_segs); >> @@ -2551,6 +2554,8 @@ void blk_mq_submit_bio(struct bio *bio) >> >> if (unlikely(!blk_mq_queue_enter(q, bio))) >> return; >> + if (unlikely(!submit_bio_checks(bio))) >> + goto put_exit; > > This now skips the checks for the cached request case, doesn't it? It did, I did add that when folding it in though.
On 11/4/21 11:30 AM, Christoph Hellwig wrote: > On Thu, Nov 04, 2021 at 05:41:35AM -0600, Jens Axboe wrote: >>>> if (!submit_bio_checks(bio) || !blk_crypto_bio_prep(&bio)) >>>> - goto queue_exit; >>>> + return; >>> >>> This is broken, we really ant the submit checks under freeze >>> protection to make sure the parameters can't be changed underneath >>> us. >> >> Which parameters are you worried about in submit_bio_checks()? I don't >> immediately see anything that would make me worry about it. > > Mostly checks if certain operations are supported or not, as > revalidation could clear those. > >>> This looks weird, as blk_try_enter_queue is already called by >>> bio_queue_enter. >> >> It's just for avoiding a pointless call into bio_queue_enter(), which >> isn't needed it blk_try_enter_queue() is successful. The latter is short >> and small and can be inlined, while bio_queue_enter() is a lot bigger. > > If this is so impotant let's operated with an inlined bio_queue_enter > that calls out of line into slow path instead of open coding it > like this. Sure, I can do that instead. >>>> } else { >>>> struct blk_mq_alloc_data data = { >>>> .q = q, >>>> @@ -2528,6 +2534,11 @@ void blk_mq_submit_bio(struct bio *bio) >>>> .cmd_flags = bio->bi_opf, >>>> }; >>>> >>>> + if (unlikely(!blk_mq_queue_enter(q, bio))) >>>> + return; >>>> + >>>> + rq_qos_throttle(q, bio); >>>> + >>> >>> At some point the code in this !cached branch really needs to move >>> into a helper.. >> >> Like in the next patch? > > No, I mean the !cached case which is a lot more convoluted. Yeah, a helper there might be appropriate. I'll write it up.
diff --git a/block/blk-core.c b/block/blk-core.c index e00f5a2287cc..2b12a427ffa6 100644 --- a/block/blk-core.c +++ b/block/blk-core.c @@ -868,18 +868,16 @@ static void __submit_bio(struct bio *bio) { struct gendisk *disk = bio->bi_bdev->bd_disk; - if (unlikely(bio_queue_enter(bio) != 0)) - return; - if (!submit_bio_checks(bio) || !blk_crypto_bio_prep(&bio)) - goto queue_exit; + return; if (!disk->fops->submit_bio) { blk_mq_submit_bio(bio); - return; + } else { + if (unlikely(bio_queue_enter(bio) != 0)) + return; + disk->fops->submit_bio(bio); + blk_queue_exit(disk->queue); } - disk->fops->submit_bio(bio); -queue_exit: - blk_queue_exit(disk->queue); } /* diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c index 4a6789e4398b..4be652fa38e7 100644 --- a/block/blk-mq-sched.c +++ b/block/blk-mq-sched.c @@ -370,15 +370,20 @@ bool blk_mq_sched_bio_merge(struct request_queue *q, struct bio *bio, bool ret = false; enum hctx_type type; - if (e && e->type->ops.bio_merge) - return e->type->ops.bio_merge(q, bio, nr_segs); + if (bio_queue_enter(bio)) + return false; + + if (e && e->type->ops.bio_merge) { + ret = e->type->ops.bio_merge(q, bio, nr_segs); + goto out_put; + } ctx = blk_mq_get_ctx(q); hctx = blk_mq_map_queue(q, bio->bi_opf, ctx); type = hctx->type; if (!(hctx->flags & BLK_MQ_F_SHOULD_MERGE) || list_empty_careful(&ctx->rq_lists[type])) - return false; + goto out_put; /* default per sw-queue merge */ spin_lock(&ctx->lock); @@ -391,6 +396,8 @@ bool blk_mq_sched_bio_merge(struct request_queue *q, struct bio *bio, ret = true; spin_unlock(&ctx->lock); +out_put: + blk_queue_exit(q); return ret; } diff --git a/block/blk-mq.c b/block/blk-mq.c index 5498454c2164..4bc98c7264fa 100644 --- a/block/blk-mq.c +++ b/block/blk-mq.c @@ -2478,6 +2478,13 @@ static inline unsigned short blk_plug_max_rq_count(struct blk_plug *plug) return BLK_MAX_REQUEST_COUNT; } +static inline bool blk_mq_queue_enter(struct request_queue *q, struct bio *bio) +{ + if (!blk_try_enter_queue(q, false) && bio_queue_enter(bio)) + return false; + return true; +} + /** * blk_mq_submit_bio - Create and send a request to block device. * @bio: Bio pointer. @@ -2506,21 +2513,20 @@ void blk_mq_submit_bio(struct bio *bio) __blk_queue_split(q, &bio, &nr_segs); if (!bio_integrity_prep(bio)) - goto queue_exit; + return; if (!blk_queue_nomerges(q) && bio_mergeable(bio)) { if (blk_attempt_plug_merge(q, bio, nr_segs, &same_queue_rq)) - goto queue_exit; + return; if (blk_mq_sched_bio_merge(q, bio, nr_segs)) - goto queue_exit; + return; } - rq_qos_throttle(q, bio); - plug = blk_mq_plug(q, bio); if (plug && plug->cached_rq) { rq = rq_list_pop(&plug->cached_rq); INIT_LIST_HEAD(&rq->queuelist); + rq_qos_throttle(q, bio); } else { struct blk_mq_alloc_data data = { .q = q, @@ -2528,6 +2534,11 @@ void blk_mq_submit_bio(struct bio *bio) .cmd_flags = bio->bi_opf, }; + if (unlikely(!blk_mq_queue_enter(q, bio))) + return; + + rq_qos_throttle(q, bio); + if (plug) { data.nr_tags = plug->nr_ios; plug->nr_ios = 1; @@ -2538,7 +2549,8 @@ void blk_mq_submit_bio(struct bio *bio) rq_qos_cleanup(q, bio); if (bio->bi_opf & REQ_NOWAIT) bio_wouldblock_error(bio); - goto queue_exit; + blk_queue_exit(q); + return; } } @@ -2621,10 +2633,6 @@ void blk_mq_submit_bio(struct bio *bio) /* Default case. */ blk_mq_sched_insert_request(rq, false, true, true); } - - return; -queue_exit: - blk_queue_exit(q); } static size_t order_to_size(unsigned int order)
Retain the old logic for the fops based submit, but for our internal blk_mq_submit_bio(), move the queue entering logic into the core function itself. We need to be a bit careful if going into the scheduler, as a scheduler or queue mappings can arbitrarily change before we have entered the queue. Have the bio scheduler mapping do that separately, it's a very cheap operation compared to actually doing merging locking and lookups. Signed-off-by: Jens Axboe <axboe@kernel.dk> --- block/blk-core.c | 14 ++++++-------- block/blk-mq-sched.c | 13 ++++++++++--- block/blk-mq.c | 28 ++++++++++++++++++---------- 3 files changed, 34 insertions(+), 21 deletions(-)