On Wed, Jul 17 2019 at 11:25pm -0400, Ming Lei <[email protected]> wrote:
> dm-rq needs to free request which has been dispatched and not completed > by underlying queue. However, the underlying queue may have allocated > private stuff for this request in .queue_rq(), so dm-rq will leak the > request private part. No, SCSI (and blk-mq) will leak. DM doesn't know anything about the internal memory SCSI uses. That memory is a SCSI implementation detail. Please fix header to properly reflect which layer is doing the leaking. > Add one new callback of .cleanup_rq() to fix the memory leak issue. > > Another use case is to free request when the hctx is dead during > cpu hotplug context. > > Cc: Ewan D. Milne <[email protected]> > Cc: Bart Van Assche <[email protected]> > Cc: Hannes Reinecke <[email protected]> > Cc: Christoph Hellwig <[email protected]> > Cc: Mike Snitzer <[email protected]> > Cc: [email protected] > Cc: <[email protected]> > Fixes: 396eaf21ee17 ("blk-mq: improve DM's blk-mq IO merging via > blk_insert_cloned_request feedback") > Signed-off-by: Ming Lei <[email protected]> > --- > drivers/md/dm-rq.c | 1 + > include/linux/blk-mq.h | 13 +++++++++++++ > 2 files changed, 14 insertions(+) > > diff --git a/drivers/md/dm-rq.c b/drivers/md/dm-rq.c > index c9e44ac1f9a6..21d5c1784d0c 100644 > --- a/drivers/md/dm-rq.c > +++ b/drivers/md/dm-rq.c > @@ -408,6 +408,7 @@ static int map_request(struct dm_rq_target_io *tio) > ret = dm_dispatch_clone_request(clone, rq); > if (ret == BLK_STS_RESOURCE || ret == BLK_STS_DEV_RESOURCE) { > blk_rq_unprep_clone(clone); > + blk_mq_cleanup_rq(clone); > tio->ti->type->release_clone_rq(clone, &tio->info); > tio->clone = NULL; > return DM_MAPIO_REQUEUE; Requiring upper layer driver (dm-rq) to explicitly call blk_mq_cleanup_rq() seems wrong. In this instance tio->ti->type->release_clone_rq() (dm-mpath's multipath_release_clone) calls blk_put_request(). Why can't blk_put_request(), or blk_mq_free_request(), call blk_mq_cleanup_rq()? Not looked at the cpu hotplug case you mention, but my naive thought is it'd be pretty weird to also sprinkle a call to blk_mq_cleanup_rq() from that specific "dead hctx" code path. Mike
