diff mbox series

[V2,1/2] blk-mq: add callback of .cleanup_rq

Message ID 20190720030637.14447-2-ming.lei@redhat.com (mailing list archive)
State Superseded, archived
Delegated to: Mike Snitzer
Headers show
Series block/scsi/dm-rq: fix leak of request private data in dm-mpath | expand

Commit Message

Ming Lei July 20, 2019, 3:06 a.m. UTC
dm-rq needs to free request which has been dispatched and not completed
by underlying queue. However, the underlying queue may have allocated
private data for this request in .queue_rq(), so the request private
part is leaked.

Add one new callback of .cleanup_rq() to free the request private data.

Another use case is to free request when the hctx is dead during
cpu hotplug context.

Cc: Ewan D. Milne <emilne@redhat.com>
Cc: Bart Van Assche <bvanassche@acm.org>
Cc: Hannes Reinecke <hare@suse.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Mike Snitzer <snitzer@redhat.com>
Cc: dm-devel@redhat.com
Cc: <stable@vger.kernel.org>
Fixes: 396eaf21ee17 ("blk-mq: improve DM's blk-mq IO merging via blk_insert_cloned_request feedback")
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/blk-mq.c         | 3 +++
 include/linux/blk-mq.h | 7 +++++++
 2 files changed, 10 insertions(+)

Comments

Sasha Levin July 20, 2019, 12:23 p.m. UTC | #1
Hi,

[This is an automated email]

This commit has been processed because it contains a "Fixes:" tag,
fixing commit: 396eaf21ee17 blk-mq: improve DM's blk-mq IO merging via blk_insert_cloned_request feedback.

The bot has tested the following trees: v5.2.1, v5.1.18, v4.19.59.

v5.2.1: Build OK!
v5.1.18: Build OK!
v4.19.59: Failed to apply! Possible dependencies:
    36e765392e48 ("blk-mq: complete req in softirq context in case of single queue")
    9ba20527f4d1 ("blk-mq: provide mq_ops->busy() hook")
    c7bb9ad1744e ("block: get rid of q->softirq_done_fn()")


NOTE: The patch will not be queued to stable trees until it is upstream.

How should we proceed with this patch?

--
Thanks,
Sasha

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
Bart Van Assche July 22, 2019, 4:51 p.m. UTC | #2
On 7/19/19 8:06 PM, Ming Lei wrote:
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index b038ec680e84..fc38d95c557f 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -502,6 +502,9 @@ void blk_mq_free_request(struct request *rq)
>   	struct blk_mq_ctx *ctx = rq->mq_ctx;
>   	struct blk_mq_hw_ctx *hctx = rq->mq_hctx;
>   
> +	if (q->mq_ops->cleanup_rq)
> +		q->mq_ops->cleanup_rq(rq);
> +
>   	if (rq->rq_flags & RQF_ELVPRIV) {
>   		if (e && e->type->ops.finish_request)
>   			e->type->ops.finish_request(rq);

I'm concerned about the performance impact of this change. How about not 
introducing .cleanup_rq() and adding a call to
scsi_mq_uninit_cmd() in scsi_queue_rq() just before that function 
returns BLK_STS_RESOURCE or BLK_STS_DEV_RESOURCE?

Thanks,

Bart.

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
Ming Lei July 23, 2019, 1 a.m. UTC | #3
On Mon, Jul 22, 2019 at 09:51:27AM -0700, Bart Van Assche wrote:
> On 7/19/19 8:06 PM, Ming Lei wrote:
> > diff --git a/block/blk-mq.c b/block/blk-mq.c
> > index b038ec680e84..fc38d95c557f 100644
> > --- a/block/blk-mq.c
> > +++ b/block/blk-mq.c
> > @@ -502,6 +502,9 @@ void blk_mq_free_request(struct request *rq)
> >   	struct blk_mq_ctx *ctx = rq->mq_ctx;
> >   	struct blk_mq_hw_ctx *hctx = rq->mq_hctx;
> > +	if (q->mq_ops->cleanup_rq)
> > +		q->mq_ops->cleanup_rq(rq);
> > +
> >   	if (rq->rq_flags & RQF_ELVPRIV) {
> >   		if (e && e->type->ops.finish_request)
> >   			e->type->ops.finish_request(rq);
> 
> I'm concerned about the performance impact of this change. How about not

Not see any performance impact in my test, and q->mq_ops should be in
data cache at that time.

> introducing .cleanup_rq() and adding a call to
> scsi_mq_uninit_cmd() in scsi_queue_rq() just before that function returns
> BLK_STS_RESOURCE or BLK_STS_DEV_RESOURCE?

The problem is that only dm-rq needs to free the request private data
when BLK_STS_RESOURCE or BLK_STS_DEV_RESOURCE is returned. If we do that
unconditionally, performance impact might be visible.


Thanks,
Ming

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
diff mbox series

Patch

diff --git a/block/blk-mq.c b/block/blk-mq.c
index b038ec680e84..fc38d95c557f 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -502,6 +502,9 @@  void blk_mq_free_request(struct request *rq)
 	struct blk_mq_ctx *ctx = rq->mq_ctx;
 	struct blk_mq_hw_ctx *hctx = rq->mq_hctx;
 
+	if (q->mq_ops->cleanup_rq)
+		q->mq_ops->cleanup_rq(rq);
+
 	if (rq->rq_flags & RQF_ELVPRIV) {
 		if (e && e->type->ops.finish_request)
 			e->type->ops.finish_request(rq);
diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
index 3fa1fa59f9b2..5882675c1989 100644
--- a/include/linux/blk-mq.h
+++ b/include/linux/blk-mq.h
@@ -140,6 +140,7 @@  typedef int (poll_fn)(struct blk_mq_hw_ctx *);
 typedef int (map_queues_fn)(struct blk_mq_tag_set *set);
 typedef bool (busy_fn)(struct request_queue *);
 typedef void (complete_fn)(struct request *);
+typedef void (cleanup_rq_fn)(struct request *);
 
 
 struct blk_mq_ops {
@@ -200,6 +201,12 @@  struct blk_mq_ops {
 	/* Called from inside blk_get_request() */
 	void (*initialize_rq_fn)(struct request *rq);
 
+	/*
+	 * Called before freeing one request which isn't completed yet,
+	 * and usually for freeing the driver private part
+	 */
+	cleanup_rq_fn		*cleanup_rq;
+
 	/*
 	 * If set, returns whether or not this queue currently is busy
 	 */