| Message ID | 20200702134838.2822844-1-ming.lei@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | blk-mq: release driver tag before freeing request via .end_io | expand |
On 7/2/20 3:48 PM, Ming Lei wrote: > The built-in flush request shares tag with the request inserted to flush > machinery, turns out its .end_io callback has to touch the built-in > flush request's internal tag or tag. > > On the other hand, we have to make sure that driver tag is released > from __blk_mq_end_request(), since this request may not be completed > via blk_mq_complete_request(). > > Given we have moved blk_mq_put_driver_tag() out of header file, fix this > issue by releasing driver tag before calling .end_io(). > > Cc: Christoph Hellwig <hch@lst.de> > Reported-by: Marek Szyprowski <m.szyprowski@samsung.com> > Tested-by: Marek Szyprowski <m.szyprowski@samsung.com> > Fixes: 36a3df5a4574("blk-mq: put driver tag when this request is completed") > Signed-off-by: Ming Lei <ming.lei@redhat.com> > --- > block/blk-mq.c | 46 ++++++++++++++++++++++++---------------------- > 1 file changed, 24 insertions(+), 22 deletions(-) > > diff --git a/block/blk-mq.c b/block/blk-mq.c > index 948987e9b6ab..6b36969220c1 100644 > --- a/block/blk-mq.c > +++ b/block/blk-mq.c > @@ -532,6 +532,26 @@ void blk_mq_free_request(struct request *rq) > } > EXPORT_SYMBOL_GPL(blk_mq_free_request); > > +static void __blk_mq_put_driver_tag(struct blk_mq_hw_ctx *hctx, > + struct request *rq) > +{ > + blk_mq_put_tag(hctx->tags, rq->mq_ctx, rq->tag); > + rq->tag = BLK_MQ_NO_TAG; > + > + if (rq->rq_flags & RQF_MQ_INFLIGHT) { > + rq->rq_flags &= ~RQF_MQ_INFLIGHT; > + atomic_dec(&hctx->nr_active); > + } > +} > + > +static inline void blk_mq_put_driver_tag(struct request *rq) > +{ > + if (rq->tag == BLK_MQ_NO_TAG || rq->internal_tag == BLK_MQ_NO_TAG) > + return; > + > + __blk_mq_put_driver_tag(rq->mq_hctx, rq); > +} > + > inline void __blk_mq_end_request(struct request *rq, blk_status_t error) > { > u64 now = 0; > @@ -551,6 +571,7 @@ inline void __blk_mq_end_request(struct request *rq, blk_status_t error) > > if (rq->end_io) { > rq_qos_done(rq->q, rq); > + blk_mq_put_driver_tag(rq); > rq->end_io(rq, error); > } else { > blk_mq_free_request(rq); > @@ -660,26 +681,6 @@ static inline bool blk_mq_complete_need_ipi(struct request *rq) > return cpu_online(rq->mq_ctx->cpu); > } > > -static void __blk_mq_put_driver_tag(struct blk_mq_hw_ctx *hctx, > - struct request *rq) > -{ > - blk_mq_put_tag(hctx->tags, rq->mq_ctx, rq->tag); > - rq->tag = BLK_MQ_NO_TAG; > - > - if (rq->rq_flags & RQF_MQ_INFLIGHT) { > - rq->rq_flags &= ~RQF_MQ_INFLIGHT; > - atomic_dec(&hctx->nr_active); > - } > -} > - > -static inline void blk_mq_put_driver_tag(struct request *rq) > -{ > - if (rq->tag == BLK_MQ_NO_TAG || rq->internal_tag == BLK_MQ_NO_TAG) > - return; > - > - __blk_mq_put_driver_tag(rq->mq_hctx, rq); > -} > - > bool blk_mq_complete_request_remote(struct request *rq) > { > WRITE_ONCE(rq->state, MQ_RQ_COMPLETE); > @@ -983,9 +984,10 @@ static bool blk_mq_check_expired(struct blk_mq_hw_ctx *hctx, > if (blk_mq_req_expired(rq, next)) > blk_mq_rq_timed_out(rq, reserved); > > - if (is_flush_rq(rq, hctx)) > + if (is_flush_rq(rq, hctx)) { > + blk_mq_put_driver_tag(rq); > rq->end_io(rq, 0); > - else if (refcount_dec_and_test(&rq->ref)) > + } else if (refcount_dec_and_test(&rq->ref)) > __blk_mq_free_request(rq); > > return true; > Hmm. Let's hope that no-one in the ->end_io() handler will need to look at the tag, because that's gone now. But if that fixes the flush machinery: Reviewed-by: Hannes Reinecke <hare@suse.de> Cheers, Hannes
On 7/2/20 7:48 AM, Ming Lei wrote: > The built-in flush request shares tag with the request inserted to flush > machinery, turns out its .end_io callback has to touch the built-in > flush request's internal tag or tag. > > On the other hand, we have to make sure that driver tag is released > from __blk_mq_end_request(), since this request may not be completed > via blk_mq_complete_request(). > > Given we have moved blk_mq_put_driver_tag() out of header file, fix this > issue by releasing driver tag before calling .end_io(). As I wrote yesterday, I reverted the three patches: https://git.kernel.dk/cgit/linux-block/log/?h=for-5.9/block so you'll need to send a new and updated series. Please fold in the 'unused variable' and related fixes as well.
diff --git a/block/blk-mq.c b/block/blk-mq.c index 948987e9b6ab..6b36969220c1 100644 --- a/block/blk-mq.c +++ b/block/blk-mq.c @@ -532,6 +532,26 @@ void blk_mq_free_request(struct request *rq) } EXPORT_SYMBOL_GPL(blk_mq_free_request); +static void __blk_mq_put_driver_tag(struct blk_mq_hw_ctx *hctx, + struct request *rq) +{ + blk_mq_put_tag(hctx->tags, rq->mq_ctx, rq->tag); + rq->tag = BLK_MQ_NO_TAG; + + if (rq->rq_flags & RQF_MQ_INFLIGHT) { + rq->rq_flags &= ~RQF_MQ_INFLIGHT; + atomic_dec(&hctx->nr_active); + } +} + +static inline void blk_mq_put_driver_tag(struct request *rq) +{ + if (rq->tag == BLK_MQ_NO_TAG || rq->internal_tag == BLK_MQ_NO_TAG) + return; + + __blk_mq_put_driver_tag(rq->mq_hctx, rq); +} + inline void __blk_mq_end_request(struct request *rq, blk_status_t error) { u64 now = 0; @@ -551,6 +571,7 @@ inline void __blk_mq_end_request(struct request *rq, blk_status_t error) if (rq->end_io) { rq_qos_done(rq->q, rq); + blk_mq_put_driver_tag(rq); rq->end_io(rq, error); } else { blk_mq_free_request(rq); @@ -660,26 +681,6 @@ static inline bool blk_mq_complete_need_ipi(struct request *rq) return cpu_online(rq->mq_ctx->cpu); } -static void __blk_mq_put_driver_tag(struct blk_mq_hw_ctx *hctx, - struct request *rq) -{ - blk_mq_put_tag(hctx->tags, rq->mq_ctx, rq->tag); - rq->tag = BLK_MQ_NO_TAG; - - if (rq->rq_flags & RQF_MQ_INFLIGHT) { - rq->rq_flags &= ~RQF_MQ_INFLIGHT; - atomic_dec(&hctx->nr_active); - } -} - -static inline void blk_mq_put_driver_tag(struct request *rq) -{ - if (rq->tag == BLK_MQ_NO_TAG || rq->internal_tag == BLK_MQ_NO_TAG) - return; - - __blk_mq_put_driver_tag(rq->mq_hctx, rq); -} - bool blk_mq_complete_request_remote(struct request *rq) { WRITE_ONCE(rq->state, MQ_RQ_COMPLETE); @@ -983,9 +984,10 @@ static bool blk_mq_check_expired(struct blk_mq_hw_ctx *hctx, if (blk_mq_req_expired(rq, next)) blk_mq_rq_timed_out(rq, reserved); - if (is_flush_rq(rq, hctx)) + if (is_flush_rq(rq, hctx)) { + blk_mq_put_driver_tag(rq); rq->end_io(rq, 0); - else if (refcount_dec_and_test(&rq->ref)) + } else if (refcount_dec_and_test(&rq->ref)) __blk_mq_free_request(rq); return true;