blk-mq: release driver tag before freeing request via .end_io
diff mbox series

Message ID 20200702134838.2822844-1-ming.lei@redhat.com
State New
Headers show
Series
  • blk-mq: release driver tag before freeing request via .end_io
Related show

Commit Message

Ming Lei July 2, 2020, 1:48 p.m. UTC
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(-)

Comments

Hannes Reinecke July 2, 2020, 4:42 p.m. UTC | #1
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
Jens Axboe July 2, 2020, 7:05 p.m. UTC | #2
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.

Patch
diff mbox series

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;