* [PATCH] NVMe: Fix memory leak on retried commands
@ 2015-10-13 22:13 ` Keith Busch
0 siblings, 0 replies; 9+ messages in thread
From: Keith Busch @ 2015-10-13 22:13 UTC (permalink / raw)
To: linux-nvme; +Cc: Keith Busch, Jens Axboe, stable
Resources are reallocated for requeued commands, so unmap and release
the iod for the failed command.
Cc: Jens Axboe <axboe@fb.com>
Cc: stable@vger.kernel.org
Cc: <stable@vger.kernel.org> # 4.0.x-
Signed-off-by: Keith Busch <keith.busch@intel.com>
---
Hi Jens,
If you're okay with this fix, it needs to get to 4.0, 4.1 and 4.2 stable,
and might cause some issues merging with your 'for-next' branch. This
is based on 'for-linus' since this urgently needs to get fixed in 4.3.
The 3.19 kernel is okay because we reused the iod in req->special.
Thanks!
Keith
drivers/block/nvme-core.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/block/nvme-core.c b/drivers/block/nvme-core.c
index 3a81df9..d5399fb 100644
--- a/drivers/block/nvme-core.c
+++ b/drivers/block/nvme-core.c
@@ -603,6 +603,7 @@ static void req_completion(struct nvme_queue *nvmeq, void *ctx,
struct nvme_iod *iod = ctx;
struct request *req = iod_get_private(iod);
struct nvme_cmd_info *cmd_rq = blk_mq_rq_to_pdu(req);
+ bool requeue = false;
u16 status = le16_to_cpup(&cqe->status) >> 1;
@@ -611,12 +612,13 @@ static void req_completion(struct nvme_queue *nvmeq, void *ctx,
&& (jiffies - req->start_time) < req->timeout) {
unsigned long flags;
+ requeue = true;
blk_mq_requeue_request(req);
spin_lock_irqsave(req->q->queue_lock, flags);
if (!blk_queue_stopped(req->q))
blk_mq_kick_requeue_list(req->q);
spin_unlock_irqrestore(req->q->queue_lock, flags);
- return;
+ goto release_iod;
}
if (req->cmd_type == REQ_TYPE_DRV_PRIV) {
@@ -637,6 +639,7 @@ static void req_completion(struct nvme_queue *nvmeq, void *ctx,
"completing aborted command with status:%04x\n",
status);
+ release_iod:
if (iod->nents) {
dma_unmap_sg(nvmeq->dev->dev, iod->sg, iod->nents,
rq_data_dir(req) ? DMA_TO_DEVICE : DMA_FROM_DEVICE);
@@ -649,7 +652,8 @@ static void req_completion(struct nvme_queue *nvmeq, void *ctx,
}
nvme_free_iod(nvmeq->dev, iod);
- blk_mq_complete_request(req, status);
+ if (likely(!requeue))
+ blk_mq_complete_request(req, status);
}
/* length is in bytes. gfp flags indicates whether we may sleep. */
--
1.7.10.4
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH] NVMe: Fix memory leak on retried commands
@ 2015-10-13 22:13 ` Keith Busch
0 siblings, 0 replies; 9+ messages in thread
From: Keith Busch @ 2015-10-13 22:13 UTC (permalink / raw)
Resources are reallocated for requeued commands, so unmap and release
the iod for the failed command.
Cc: Jens Axboe <axboe at fb.com>
Cc: stable at vger.kernel.org
Cc: <stable at vger.kernel.org> # 4.0.x-
Signed-off-by: Keith Busch <keith.busch at intel.com>
---
Hi Jens,
If you're okay with this fix, it needs to get to 4.0, 4.1 and 4.2 stable,
and might cause some issues merging with your 'for-next' branch. This
is based on 'for-linus' since this urgently needs to get fixed in 4.3.
The 3.19 kernel is okay because we reused the iod in req->special.
Thanks!
Keith
drivers/block/nvme-core.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/block/nvme-core.c b/drivers/block/nvme-core.c
index 3a81df9..d5399fb 100644
--- a/drivers/block/nvme-core.c
+++ b/drivers/block/nvme-core.c
@@ -603,6 +603,7 @@ static void req_completion(struct nvme_queue *nvmeq, void *ctx,
struct nvme_iod *iod = ctx;
struct request *req = iod_get_private(iod);
struct nvme_cmd_info *cmd_rq = blk_mq_rq_to_pdu(req);
+ bool requeue = false;
u16 status = le16_to_cpup(&cqe->status) >> 1;
@@ -611,12 +612,13 @@ static void req_completion(struct nvme_queue *nvmeq, void *ctx,
&& (jiffies - req->start_time) < req->timeout) {
unsigned long flags;
+ requeue = true;
blk_mq_requeue_request(req);
spin_lock_irqsave(req->q->queue_lock, flags);
if (!blk_queue_stopped(req->q))
blk_mq_kick_requeue_list(req->q);
spin_unlock_irqrestore(req->q->queue_lock, flags);
- return;
+ goto release_iod;
}
if (req->cmd_type == REQ_TYPE_DRV_PRIV) {
@@ -637,6 +639,7 @@ static void req_completion(struct nvme_queue *nvmeq, void *ctx,
"completing aborted command with status:%04x\n",
status);
+ release_iod:
if (iod->nents) {
dma_unmap_sg(nvmeq->dev->dev, iod->sg, iod->nents,
rq_data_dir(req) ? DMA_TO_DEVICE : DMA_FROM_DEVICE);
@@ -649,7 +652,8 @@ static void req_completion(struct nvme_queue *nvmeq, void *ctx,
}
nvme_free_iod(nvmeq->dev, iod);
- blk_mq_complete_request(req, status);
+ if (likely(!requeue))
+ blk_mq_complete_request(req, status);
}
/* length is in bytes. gfp flags indicates whether we may sleep. */
--
1.7.10.4
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] NVMe: Fix memory leak on retried commands
2015-10-13 22:13 ` Keith Busch
@ 2015-10-14 14:12 ` Christoph Hellwig
-1 siblings, 0 replies; 9+ messages in thread
From: Christoph Hellwig @ 2015-10-14 14:12 UTC (permalink / raw)
To: Keith Busch; +Cc: linux-nvme, Jens Axboe, stable
Looks good,
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] NVMe: Fix memory leak on retried commands
@ 2015-10-14 14:12 ` Christoph Hellwig
0 siblings, 0 replies; 9+ messages in thread
From: Christoph Hellwig @ 2015-10-14 14:12 UTC (permalink / raw)
Looks good,
Reviewed-by: Christoph Hellwig <hch at lst.de>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] NVMe: Fix memory leak on retried commands
2015-10-13 22:13 ` Keith Busch
@ 2015-10-15 19:34 ` Keith Busch
-1 siblings, 0 replies; 9+ messages in thread
From: Keith Busch @ 2015-10-15 19:34 UTC (permalink / raw)
To: Keith Busch; +Cc: linux-nvme, Jens Axboe, stable, Christoph Hellwig
On Tue, 13 Oct 2015, Keith Busch wrote:
> Resources are reallocated for requeued commands, so unmap and release
> the iod for the failed command.
Hi Jens,
Since you're applying changes for 4.3, can you add this one? It's a
pretty bad memory leak and causes a kernel hang if you remove a drive
because of a busy dma pool. You'll get messages spewing like this:
nvme 0000:xx:xx.x: dma_pool_destroy prp list 256, ffff880420dec000 busy
and lock up pci and the driver since removal never completes while
holding a lock.
I think this should go into stable back to 4.0. I'm not entirely sure how
to do that. Will the Cc on the original achieve that when this is merged?
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] NVMe: Fix memory leak on retried commands
@ 2015-10-15 19:34 ` Keith Busch
0 siblings, 0 replies; 9+ messages in thread
From: Keith Busch @ 2015-10-15 19:34 UTC (permalink / raw)
On Tue, 13 Oct 2015, Keith Busch wrote:
> Resources are reallocated for requeued commands, so unmap and release
> the iod for the failed command.
Hi Jens,
Since you're applying changes for 4.3, can you add this one? It's a
pretty bad memory leak and causes a kernel hang if you remove a drive
because of a busy dma pool. You'll get messages spewing like this:
nvme 0000:xx:xx.x: dma_pool_destroy prp list 256, ffff880420dec000 busy
and lock up pci and the driver since removal never completes while
holding a lock.
I think this should go into stable back to 4.0. I'm not entirely sure how
to do that. Will the Cc on the original achieve that when this is merged?
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] NVMe: Fix memory leak on retried commands
2015-10-15 19:34 ` Keith Busch
@ 2015-10-15 19:40 ` Jens Axboe
-1 siblings, 0 replies; 9+ messages in thread
From: Jens Axboe @ 2015-10-15 19:40 UTC (permalink / raw)
To: Keith Busch; +Cc: linux-nvme, stable, Christoph Hellwig
On 10/15/2015 01:34 PM, Keith Busch wrote:
> On Tue, 13 Oct 2015, Keith Busch wrote:
>> Resources are reallocated for requeued commands, so unmap and release
>> the iod for the failed command.
>
> Hi Jens,
>
> Since you're applying changes for 4.3, can you add this one? It's a
> pretty bad memory leak and causes a kernel hang if you remove a drive
> because of a busy dma pool. You'll get messages spewing like this:
>
> nvme 0000:xx:xx.x: dma_pool_destroy prp list 256, ffff880420dec000 busy
>
> and lock up pci and the driver since removal never completes while
> holding a lock.
Yeah, it's applied. It's hand applied since it no longer applied after
the error fixup from Christoph:
http://git.kernel.dk/cgit/linux-block/commit/?h=for-linus&id=0dfc70c33409afc232ef0b9ec210535dfbf9bc61
> I think this should go into stable back to 4.0. I'm not entirely sure how
> to do that. Will the Cc on the original achieve that when this is merged?
I think your patch is fine, it's marked stable and with 4.0 and up. So
should be no need to do anything else.
--
Jens Axboe
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] NVMe: Fix memory leak on retried commands
@ 2015-10-15 19:40 ` Jens Axboe
0 siblings, 0 replies; 9+ messages in thread
From: Jens Axboe @ 2015-10-15 19:40 UTC (permalink / raw)
On 10/15/2015 01:34 PM, Keith Busch wrote:
> On Tue, 13 Oct 2015, Keith Busch wrote:
>> Resources are reallocated for requeued commands, so unmap and release
>> the iod for the failed command.
>
> Hi Jens,
>
> Since you're applying changes for 4.3, can you add this one? It's a
> pretty bad memory leak and causes a kernel hang if you remove a drive
> because of a busy dma pool. You'll get messages spewing like this:
>
> nvme 0000:xx:xx.x: dma_pool_destroy prp list 256, ffff880420dec000 busy
>
> and lock up pci and the driver since removal never completes while
> holding a lock.
Yeah, it's applied. It's hand applied since it no longer applied after
the error fixup from Christoph:
http://git.kernel.dk/cgit/linux-block/commit/?h=for-linus&id=0dfc70c33409afc232ef0b9ec210535dfbf9bc61
> I think this should go into stable back to 4.0. I'm not entirely sure how
> to do that. Will the Cc on the original achieve that when this is merged?
I think your patch is fine, it's marked stable and with 4.0 and up. So
should be no need to do anything else.
--
Jens Axboe
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] NVMe: Fix memory leak on retried commands
@ 2015-11-06 16:23 Keith Busch
0 siblings, 0 replies; 9+ messages in thread
From: Keith Busch @ 2015-11-06 16:23 UTC (permalink / raw)
To: stable; +Cc: gregkh, hch, Keith Busch, Jens Axboe
commit 0dfc70c33409afc232ef0b9ec210535dfbf9bc61 upstream
Resources are reallocated for requeued commands, so unmap and release
the iod for the failed command.
It's a pretty bad memory leak and causes a kernel hang if you remove a
drive because of a busy dma pool. You'll get messages spewing like this:
nvme 0000:xx:xx.x: dma_pool_destroy prp list 256, ffff880420dec000 busy
and lock up pci and the driver since removal never completes while
holding a lock.
Cc: stable@vger.kernel.org
Cc: <stable@vger.kernel.org> # 4.0.x-
Signed-off-by: Keith Busch <keith.busch@intel.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Jens Axboe <axboe@fb.com>
---
drivers/block/nvme-core.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/block/nvme-core.c b/drivers/block/nvme-core.c
index e23be20..28da73c 100644
--- a/drivers/block/nvme-core.c
+++ b/drivers/block/nvme-core.c
@@ -597,6 +597,7 @@ static void req_completion(struct nvme_queue *nvmeq, void *ctx,
struct nvme_iod *iod = ctx;
struct request *req = iod_get_private(iod);
struct nvme_cmd_info *cmd_rq = blk_mq_rq_to_pdu(req);
+ bool requeue = false;
u16 status = le16_to_cpup(&cqe->status) >> 1;
@@ -605,12 +606,13 @@ static void req_completion(struct nvme_queue *nvmeq, void *ctx,
&& (jiffies - req->start_time) < req->timeout) {
unsigned long flags;
+ requeue = true;
blk_mq_requeue_request(req);
spin_lock_irqsave(req->q->queue_lock, flags);
if (!blk_queue_stopped(req->q))
blk_mq_kick_requeue_list(req->q);
spin_unlock_irqrestore(req->q->queue_lock, flags);
- return;
+ goto release_iod;
}
req->errors = nvme_error_status(status);
} else
@@ -620,7 +622,7 @@ static void req_completion(struct nvme_queue *nvmeq, void *ctx,
dev_warn(&nvmeq->dev->pci_dev->dev,
"completing aborted command with status:%04x\n",
status);
-
+ release_iod:
if (iod->nents) {
dma_unmap_sg(&nvmeq->dev->pci_dev->dev, iod->sg, iod->nents,
rq_data_dir(req) ? DMA_TO_DEVICE : DMA_FROM_DEVICE);
@@ -633,7 +635,8 @@ static void req_completion(struct nvme_queue *nvmeq, void *ctx,
}
nvme_free_iod(nvmeq->dev, iod);
- blk_mq_complete_request(req);
+ if (likely(!requeue))
+ blk_mq_complete_request(req);
}
/* length is in bytes. gfp flags indicates whether we may sleep. */
--
2.6.2.307.g37023ba
^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2015-11-06 16:23 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-10-13 22:13 [PATCH] NVMe: Fix memory leak on retried commands Keith Busch
2015-10-13 22:13 ` Keith Busch
2015-10-14 14:12 ` Christoph Hellwig
2015-10-14 14:12 ` Christoph Hellwig
2015-10-15 19:34 ` Keith Busch
2015-10-15 19:34 ` Keith Busch
2015-10-15 19:40 ` Jens Axboe
2015-10-15 19:40 ` Jens Axboe
2015-11-06 16:23 Keith Busch
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.