NVMe: Fix possible scheduling while atomic error
PATCH 1/1 NVMe: Fix possible scheduling while atomic error
Stopping a h/w queue could have scheduled the task to wait on cancelling work. This patch removes stopping the the delayed work so we can safely hold rcu_read_lock() while stopping h/w queues.
Since blk-mq requeue work may restart the h/w queues during a reset, the nvme driver's queue_rq will stop them again if the driver receives a command during the reset window.
--- drivers/nvme/host/core.c | 6 ++++-- drivers/nvme/host/pci.c | 6 ++++++ 2 files changed, 10 insertions(+), 2 deletions(-)
drivers/nvme/host/core.c +4 -2
@@ -1823,7 +1823,6 @@ void nvme_stop_queues(struct nvme_ctrl *ctrl)queue_flag_set (QUEUE_FLAG_STOPPED , ns->queue);spin_unlock_irq (ns->queue->queue_lock); -blk_mq_cancel_requeue_work (ns->queue);blk_mq_stop_hw_queues (ns->queue); }rcu_read_unlock ();
@@ -1836,7 +1835,10 @@ void nvme_start_queues(struct nvme_ctrl *ctrl)rcu_read_lock ();list_for_each_entry_rcu (ns, &ctrl->namespaces, list) { -queue_flag_clear_unlocked (QUEUE_FLAG_STOPPED , ns->queue); +spin_lock_irq (ns->queue->queue_lock); +queue_flag_clear (QUEUE_FLAG_STOPPED , ns->queue); +spin_unlock_irq (ns->queue->queue_lock); +blk_mq_start_stopped_hw_queues (ns->queue, true);blk_mq_kick_requeue_list (ns->queue); }
drivers/nvme/host/pci.c +6 -0
@@ -609,6 +609,12 @@ static int nvme_queue_rq(struct blk_mq_hw_ctx *hctx,spin_unlock_irq (&nvmeq->q_lock);return BLK_MQ_RQ_QUEUE_OK ; out: +if (ret ==BLK_MQ_RQ_QUEUE_BUSY ) { +spin_lock_irq (ns->queue->queue_lock); +if (blk_queue_stopped (req->q)) +blk_mq_stop_hw_queues (ns->queue); +spin_unlock_irq (ns->queue->queue_lock); + }nvme_free_iod (dev, req);return ret; }
Signature · 1 line
2.7.2
PATCH 1/1 NVMe: Fix possible scheduling while atomic error
Quoted 7 lines · Keith Busch
drivers/nvme/host/core.c +0
-1spin_unlock_irq (ns->queue->queue_lock); -blk_mq_cancel_requeue_work (ns->queue);blk_mq_stop_hw_queues (ns->queue); }rcu_read_unlock ();
drivers/nvme/host/core.c void nvme_start_queues(struct nvme_ctrl *ctrl) · line 1840
Quoted 7 lines
drivers/nvme/host/core.c +3
-1@@ -1836,7 +1835,10 @@ void nvme_start_queues(struct nvme_ctrl *ctrl)rcu_read_lock ();list_for_each_entry_rcu (ns, &ctrl->namespaces, list) { -queue_flag_clear_unlocked (QUEUE_FLAG_STOPPED , ns->queue); +spin_lock_irq (ns->queue->queue_lock); +queue_flag_clear (QUEUE_FLAG_STOPPED , ns->queue); +spin_unlock_irq (ns->queue->queue_lock);
What's the rationale for this change?
drivers/nvme/host/pci.c static int nvme_queue_rq(struct blk_mq_hw_ctx *hctx, · line 616
Quoted 11 lines
drivers/nvme/host/pci.c +5
-0@@ -609,6 +609,12 @@ static int nvme_queue_rq(struct blk_mq_hw_ctx *hctx,spin_unlock_irq (&nvmeq->q_lock);return BLK_MQ_RQ_QUEUE_OK ; out: +if (ret ==BLK_MQ_RQ_QUEUE_BUSY ) { +spin_lock_irq (ns->queue->queue_lock); +if (blk_queue_stopped (req->q)) +blk_mq_stop_hw_queues (ns->queue); +spin_unlock_irq (ns->queue->queue_lock);
Shouldn't we do this where set the stopped flag on the queue instead?
PATCH 1/1 NVMe: Fix possible scheduling while atomic error
Quoted 9 lines · Christoph Hellwig
drivers/nvme/host/core.c void nvme_start_queues(struct nvme_ctrl *ctrl) · line 1840
Quoted 6 lines · Keith Busch
drivers/nvme/host/core.c +3
-1rcu_read_lock ();list_for_each_entry_rcu (ns, &ctrl->namespaces, list) { -queue_flag_clear_unlocked (QUEUE_FLAG_STOPPED , ns->queue); +spin_lock_irq (ns->queue->queue_lock); +queue_flag_clear (QUEUE_FLAG_STOPPED , ns->queue); +spin_unlock_irq (ns->queue->queue_lock);What's the rationale for this change?
That was to make it so nvme_queue_rq wouldn't see a stopped flag just before it was starting.
Quoted 11 lines
drivers/nvme/host/pci.c static int nvme_queue_rq(struct blk_mq_hw_ctx *hctx, · line 616
Quoted 9 lines
drivers/nvme/host/pci.c +5
-0@@ -609,6 +609,12 @@ static int nvme_queue_rq(struct blk_mq_hw_ctx *hctx,spin_unlock_irq (&nvmeq->q_lock);return BLK_MQ_RQ_QUEUE_OK ; out: +if (ret ==BLK_MQ_RQ_QUEUE_BUSY ) { +spin_lock_irq (ns->queue->queue_lock); +if (blk_queue_stopped (req->q)) +blk_mq_stop_hw_queues (ns->queue); +spin_unlock_irq (ns->queue->queue_lock);Shouldn't we do this where set the stopped flag on the queue instead?
It's moved out of that function holding the rcu read lock since blk_mq_stop_hw_queues potentially sleeps.
But the above is also broken because it's holding a spinlock, so this patch is no good.
PATCH 1/1 NVMe: Fix possible scheduling while atomic error
Quoted 11 lines · Keith Busch
Quoted 9 lines · Christoph Hellwig
drivers/nvme/host/core.c void nvme_start_queues(struct nvme_ctrl *ctrl) · line 1840
Quoted 6 lines · Keith Busch
drivers/nvme/host/core.c +3
-1rcu_read_lock ();list_for_each_entry_rcu (ns, &ctrl->namespaces, list) { -queue_flag_clear_unlocked (QUEUE_FLAG_STOPPED , ns->queue); +spin_lock_irq (ns->queue->queue_lock); +queue_flag_clear (QUEUE_FLAG_STOPPED , ns->queue); +spin_unlock_irq (ns->queue->queue_lock);What's the rationale for this change?
Quoted 2 lines
That was to make it so
nvme_queue_rqwouldn't see a stopped flag just before it was starting.
I don't really see how that helps us with any race..
Quoted 12 lines
Quoted 11 lines
drivers/nvme/host/pci.c static int nvme_queue_rq(struct blk_mq_hw_ctx *hctx, · line 616
Quoted 9 lines
drivers/nvme/host/pci.c +5
-0@@ -609,6 +609,12 @@ static int nvme_queue_rq(struct blk_mq_hw_ctx *hctx,spin_unlock_irq (&nvmeq->q_lock);return BLK_MQ_RQ_QUEUE_OK ; out: +if (ret ==BLK_MQ_RQ_QUEUE_BUSY ) { +spin_lock_irq (ns->queue->queue_lock); +if (blk_queue_stopped (req->q)) +blk_mq_stop_hw_queues (ns->queue); +spin_unlock_irq (ns->queue->queue_lock);Shouldn't we do this where set the stopped flag on the queue instead?
Quoted 2 lines
It's moved out of that function holding the rcu read lock since
blk_mq_stop_hw_queuespotentially sleeps.
blk_mq_stop_hw_queues is a loop around blk_mq_stop_hw_queue, which itself does two calls to cancel_delayed_work and a set_bit. None of them can block.
PATCH 1/1 NVMe: Fix possible scheduling while atomic error
Quoted 12 lines · Christoph Hellwig
Quoted 11 lines · Keith Busch
Quoted 9 lines · Christoph Hellwig
drivers/nvme/host/core.c void nvme_start_queues(struct nvme_ctrl *ctrl) · line 1840
Quoted 6 lines · Keith Busch
drivers/nvme/host/core.c +3
-1rcu_read_lock ();list_for_each_entry_rcu (ns, &ctrl->namespaces, list) { -queue_flag_clear_unlocked (QUEUE_FLAG_STOPPED , ns->queue); +spin_lock_irq (ns->queue->queue_lock); +queue_flag_clear (QUEUE_FLAG_STOPPED , ns->queue); +spin_unlock_irq (ns->queue->queue_lock);What's the rationale for this change?
Quoted 4 lines
Quoted 2 lines
That was to make it so
nvme_queue_rqwouldn't see a stopped flag just before it was starting.I don't really see how that helps us with any race..
Without the additional spin lock protecting QUEUE_FLAG_STOPPED:
CPU A CPU B ----- ----- nvme_start_queues nvme_queue_rq
if (nvmeq->cq_vector < 0) <-- queue is disabled during reset, returning BLK_MQ_RQ_QUEUE_BUSY
goto out;
if (blk_queue_stopped()) <-- returns true
queue_flag_clear
blk_mq_start_stopped_hw_queues
blk_mq_stop_hw_queues() <-- incorrectly stops h/w queues
Locking the queue in nvme_start_queues before clearing the flag fixes that up.
Quoted 6 lines
Quoted 2 lines
It's moved out of that function holding the rcu read lock since
blk_mq_stop_hw_queuespotentially sleeps.
blk_mq_stop_hw_queuesis a loop aroundblk_mq_stop_hw_queue, which itself does two calls tocancel_delayed_workand aset_bit. None of them can block.
Oops, I'm crossing wires and mixing up what API's do what ... need to wake up before starting work.
PATCH 1/1 NVMe: Fix possible scheduling while atomic error
Quoted 7 lines · Keith Busch
Quoted 7 lines
Quoted 3 lines
It's moved out of that function holding the rcu read lock since
blk_mq_stop_hw_queuespotentially sleeps.
blk_mq_stop_hw_queuesis a loop aroundblk_mq_stop_hw_queue, which itself does two calls tocancel_delayed_workand aset_bit. None of them can block.
It's blk_mq_cancel_requeue_work() that potentially sleeps.
PATCH 1/1 NVMe: Fix possible scheduling while atomic error
Quoted 2 lines · Ming Lin
It's
blk_mq_cancel_requeue_work()that potentially sleeps.
Right, I don't know what I was thinking...
AFAICT, the patch is fine as-is if Christoph is okay with it.
PATCH 1/1 NVMe: Fix possible scheduling while atomic error
Quoted 7 lines · Keith Busch
Quoted 2 lines · Ming Lin
It's
blk_mq_cancel_requeue_work()that potentially sleeps.Right, I don't know what I was thinking...
AFAICT, the patch is fine as-is if Christoph is okay with it.
I still don't why we need to delay the call to blk_mq_stop_hw_queues to ->queue_rq.
PATCH 1/1 NVMe: Fix possible scheduling while atomic error
Quoted 11 lines · Christoph Hellwig
Quoted 7 lines · Keith Busch
Quoted 2 lines · Ming Lin
It's
blk_mq_cancel_requeue_work()that potentially sleeps.Right, I don't know what I was thinking...
AFAICT, the patch is fine as-is if Christoph is okay with it.
I still don't why we need to delay the call to
blk_mq_stop_hw_queuesto ->queue_rq.
Eh? It's still stopped in the same place as always, so it's not delayed. We just call it again from queue_rq because requeue_work can start h/w queues on its own. This is very similar to scsi's queue_rq in scsi_lib.c.
PATCH 1/1 NVMe: Fix possible scheduling while atomic error
Quoted 8 lines · Keith Busch
Quoted 8 lines
Quoted 7 lines · Ming Lin
Quoted 2 lines
It's
blk_mq_cancel_requeue_work()that potentially sleeps.Right, I don't know what I was thinking...
AFAICT, the patch is fine as-is if Christoph is okay with it.
Quoted 7 lines
Quoted 2 lines
I still don't why we need to delay the call to
blk_mq_stop_hw_queuesto ->queue_rq.Eh? It's still stopped in the same place as always, so it's not delayed. We just call it again from
queue_rqbecauserequeue_workcan start h/w queues on its own. This is very similar to scsi'squeue_rqinscsi_lib.c.
The rationale in SCSI is that we stop the queue when we are above the hosts or targets queuing limit, and only I/O ocmpletions will wake it up again. Kicking of the sotpped queues in blk_mq_requeue_work is SCSI specific behavior that leaked into the core, and it's my fault..
Well' let's get the fix in as-is for now, but I really need to sort this out properly, so maybe add some comments.
Quoted 5 lines
_______________________________________________
Linux-nvme mailing list
Linux-nvme at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
---end quoted text---
PATCH 1/1 NVMe: Fix possible scheduling while atomic error
Quoted 11 lines
Quoted 10 lines
Quoted 5 lines
Quoted 2 lines
AFAICT, the patch is fine as-is if Christoph is okay with it.
I still don't why we need to delay the call to
blk_mq_stop_hw_queuesto ->queue_rq.Eh? It's still stopped in the same place as always, so it's not delayed. We just call it again from
queue_rqbecauserequeue_workcan start h/w queues on its own. This is very similar to scsi'squeue_rqinscsi_lib.c.
Quoted 7 lines
The rationale in SCSI is that we stop the queue when we are above the hosts or targets queuing limit, and only I/O ocmpletions will wake it up again. Kicking of the sotpped queues in
blk_mq_requeue_workis SCSI specific behavior that leaked into the core, and it's my fault..Well' let's get the fix in as-is for now, but I really need to sort this out properly, so maybe add some comments.
I really don't like this patch (sorry), having queue_rq being aware of what requeue_work *might* do looks backwards to me... now I'm thinking what I need to do in fabrics rdma and loop. This is why I didn't like the NVME_NS_DEAD check in queue_rq as well.
I'd really prefer to not propagate this backwards logic into other transports...
But, if this is mandatory for now, does this patch makes sense for rdma?
drivers/nvme/host/rdma.c +0 -0
@@ -1450,6 +1450,12 @@ static int nvme_rdma_queue_rq(struct
blk_mq_hw_ctx *hctx,
goto err;
}+ /* XXX: This is really not the correct layer to check this */
+ if (ns && !test_bit(NVME_NS_DEAD, &ns->flags)) {
+ ret = -EAGAIN;
+ goto err;
+ }
+
ib_dma_sync_single_for_device(dev, sqe->dma,
sizeof(struct nvme_command), DMA_TO_DEVICE);@@ -1464,8 +1470,14 @@ static int nvme_rdma_queue_rq(struct blk_mq_hw_ctx *hctx,
return BLK_MQ_RQ_QUEUE_OK;
err:
- return (ret == -ENOMEM || ret == -EAGAIN) ?
- BLK_MQ_RQ_QUEUE_BUSY : BLK_MQ_RQ_QUEUE_ERROR;
+ if (ret == -ENOMEM || ret == -EAGAIN) {
+ spin_lock_irq(ns->queue->queue_lock);
+ if (blk_queue_stopped(rq->q))
+ blk_mq_stop_hw_queues(ns->queue);
+ spin_unlock_irq(ns->queue->queue_lock);
+ return BLK_MQ_RQ_QUEUE_BUSY;
+ }
+ return BLK_MQ_RQ_QUEUE_ERROR;
}
static int nvme_rdma_poll(struct blk_mq_hw_ctx *hctx, unsigned int tag)
PATCH 1/1 NVMe: Fix possible scheduling while atomic error
Quoted 4 lines · Sagi Grimberg
I really don't like this patch (sorry), having
queue_rqbeing aware of whatrequeue_work*might* do looks backwards to me... now I'm thinking what I need to do in fabrics rdma and loop. This is why I didn't like theNVME_NS_DEADcheck inqueue_rqas well.
I agree, but I didn't find another card we can play to get the right sequence.
Quoted 5 lines
I'd really prefer to not propagate this backwards logic into other transports...
But, if this is mandatory for now, does this patch makes sense for rdma?
Looks right to me.
Quoted 38 lines
drivers/nvme/host/rdma.c +14
-2@@ -1450,6 +1450,12 @@ static int nvme_rdma_queue_rq(struct blk_mq_hw_ctx *hctx, goto err; } +/* XXX: This is really not the correct layer to check this */ +if (ns && !test_bit (NVME_NS_DEAD , &ns->flags)) { + ret = -EAGAIN ; + goto err; + } +ib_dma_sync_single_for_device (dev, sqe->dma,sizeof (struct nvme_command ),DMA_TO_DEVICE );@@ -1464,8 +1470,14 @@ static int nvme_rdma_queue_rq(struct blk_mq_hw_ctx *hctx, return BLK_MQ_RQ_QUEUE_OK; err: -return (ret == -ENOMEM || ret == -EAGAIN ) ? -BLK_MQ_RQ_QUEUE_BUSY :BLK_MQ_RQ_QUEUE_ERROR ; +if (ret == -ENOMEM || ret == -EAGAIN ) { +spin_lock_irq (ns->queue->queue_lock); +if (blk_queue_stopped (rq->q)) +blk_mq_stop_hw_queues (ns->queue); +spin_unlock_irq (ns->queue->queue_lock); +return BLK_MQ_RQ_QUEUE_BUSY ; + } +return BLK_MQ_RQ_QUEUE_ERROR ; }static int
nvme_rdma_poll(struct blk_mq_hw_ctx *hctx, unsigned int tag)