| 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 |
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
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
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 --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 */
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(+)