Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1323247 > unrolled thread

[PATCH] NVMe: do not touch sq door bell if nvmeq has been suspended

Back to article view | Back to linux.kernel

Contents

  [PATCH] NVMe: do not touch sq door bell if nvmeq has been suspended Wenbo Wang <mail_weber_wang@163.com> - 2016-02-01 16:50 +0100
    RE: [PATCH] NVMe: do not touch sq door bell if nvmeq has been  suspended "Busch, Keith" <keith.busch@intel.com> - 2016-02-01 17:00 +0100
      RE: [PATCH] NVMe: do not touch sq door bell if nvmeq has been  suspended Wenbo Wang <wenbo.wang@memblaze.com> - 2016-02-01 17:20 +0100
    Re: [PATCH] NVMe: do not touch sq door bell if nvmeq has been  suspended Jens Axboe <axboe@fb.com> - 2016-02-01 18:00 +0100
      Re: [PATCH] NVMe: do not touch sq door bell if nvmeq has been  suspended Sagi Grimberg <sagig@dev.mellanox.co.il> - 2016-02-02 13:50 +0100
        Re: [PATCH] NVMe: do not touch sq door bell if nvmeq has been  suspended Keith Busch <keith.busch@intel.com> - 2016-02-02 15:30 +0100
          Re: [PATCH] NVMe: do not touch sq door bell if nvmeq has been  suspended Sagi Grimberg <sagig@dev.mellanox.co.il> - 2016-02-02 15:40 +0100
            Re: [PATCH] NVMe: do not touch sq door bell if nvmeq has been  suspended Keith Busch <keith.busch@intel.com> - 2016-02-02 15:50 +0100
              Re: [PATCH] NVMe: do not touch sq door bell if nvmeq has been  suspended Sagi Grimberg <sagig@dev.mellanox.co.il> - 2016-02-02 18:30 +0100
      Re: [PATCH] NVMe: do not touch sq door bell if nvmeq has been  suspended Keith Busch <keith.busch@intel.com> - 2016-02-02 18:30 +0100
      Re: [PATCH] NVMe: do not touch sq door bell if nvmeq has been  suspended Keith Busch <keith.busch@intel.com> - 2016-02-03 15:50 +0100
        Re: [PATCH] NVMe: do not touch sq door bell if nvmeq has been  suspended Keith Busch <keith.busch@intel.com> - 2016-02-03 17:40 +0100
          Re: [PATCH] NVMe: do not touch sq door bell if nvmeq has been  suspended Sagi Grimberg <sagig@dev.mellanox.co.il> - 2016-02-07 14:50 +0100
          Re: [PATCH] NVMe: do not touch sq door bell if nvmeq has been  suspended Keith Busch <keith.busch@intel.com> - 2016-02-08 16:10 +0100

#1323247 — [PATCH] NVMe: do not touch sq door bell if nvmeq has been suspended

If __nvme_submit_cmd races with nvme_dev_disable, nvmeq
could have been suspended and dev->bar could have been
unmapped. Do not touch sq door bell in this case.

Signed-off-by: Wenbo Wang <wenbo.wang@memblaze.com>
Reviewed-by: Wenwei Tao <wenwei.tao@memblaze.com>
CC: linux-nvme@lists.infradead.org
---
 drivers/nvme/host/pci.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 8b1a725..2288712 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -325,7 +325,8 @@ static void __nvme_submit_cmd(struct nvme_queue *nvmeq,
 
 	if (++tail == nvmeq->q_depth)
 		tail = 0;
-	writel(tail, nvmeq->q_db);
+	if (likely(nvmeq->cq_vector >= 0))
+		writel(tail, nvmeq->q_db);
 	nvmeq->sq_tail = tail;
 }
 
-- 
1.8.3.1

[toc] | [next] | [standalone]

#1323270 — RE: [PATCH] NVMe: do not touch sq door bell if nvmeq has been suspended

Does this ever happen? The queue should be stopped before the bar is unmapped. If that's insufficient to guard against this, we've another problem this patch does not cover. That command will just timeout since it was accepted by the driver, but not actually submitted to anything. The request needs to be requeued or return BLK_MQ_RQ_QUEUE_BUSY.

> -----Original Message-----
> From: Wenbo Wang [mailto:mail_weber_wang@163.com]
> Sent: Monday, February 01, 2016 8:42 AM
> To: axboe@fb.com; Busch, Keith
> Cc: linux-kernel@vger.kernel.org; Wenbo Wang; Wenbo Wang; linux-nvme@lists.infradead.org
> Subject: [PATCH] NVMe: do not touch sq door bell if nvmeq has been suspended
> 
> If __nvme_submit_cmd races with nvme_dev_disable, nvmeq
> could have been suspended and dev->bar could have been
> unmapped. Do not touch sq door bell in this case.
> 
> Signed-off-by: Wenbo Wang <wenbo.wang@memblaze.com>
> Reviewed-by: Wenwei Tao <wenwei.tao@memblaze.com>
> CC: linux-nvme@lists.infradead.org
> ---
>  drivers/nvme/host/pci.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index 8b1a725..2288712 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -325,7 +325,8 @@ static void __nvme_submit_cmd(struct nvme_queue *nvmeq,
> 
>  	if (++tail == nvmeq->q_depth)
>  		tail = 0;
> -	writel(tail, nvmeq->q_db);
> +	if (likely(nvmeq->cq_vector >= 0))
> +		writel(tail, nvmeq->q_db);
>  	nvmeq->sq_tail = tail;
>  }
> 
> --
> 1.8.3.1

[toc] | [prev] | [next] | [standalone]

#1323292 — RE: [PATCH] NVMe: do not touch sq door bell if nvmeq has been suspended

No, this issue has not been seen yet. 

Since blk_mq_run_hw_queue tests queue stopped in the very beginning. It should be possible to race with nvme_dev_disable.

I agree that the request shall be re-queued.

-----Original Message-----
From: Busch, Keith [mailto:keith.busch@intel.com] 
Sent: Tuesday, February 2, 2016 12:00 AM
To: Wenbo Wang; axboe@fb.com
Cc: linux-kernel@vger.kernel.org; Wenbo Wang; linux-nvme@lists.infradead.org
Subject: RE: [PATCH] NVMe: do not touch sq door bell if nvmeq has been suspended

Does this ever happen? The queue should be stopped before the bar is unmapped. If that's insufficient to guard against this, we've another problem this patch does not cover. That command will just timeout since it was accepted by the driver, but not actually submitted to anything. The request needs to be requeued or return BLK_MQ_RQ_QUEUE_BUSY.

> -----Original Message-----
> From: Wenbo Wang [mailto:mail_weber_wang@163.com]
> Sent: Monday, February 01, 2016 8:42 AM
> To: axboe@fb.com; Busch, Keith
> Cc: linux-kernel@vger.kernel.org; Wenbo Wang; Wenbo Wang; 
> linux-nvme@lists.infradead.org
> Subject: [PATCH] NVMe: do not touch sq door bell if nvmeq has been 
> suspended
> 
> If __nvme_submit_cmd races with nvme_dev_disable, nvmeq could have 
> been suspended and dev->bar could have been unmapped. Do not touch sq 
> door bell in this case.
> 
> Signed-off-by: Wenbo Wang <wenbo.wang@memblaze.com>
> Reviewed-by: Wenwei Tao <wenwei.tao@memblaze.com>
> CC: linux-nvme@lists.infradead.org
> ---
>  drivers/nvme/host/pci.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c index 
> 8b1a725..2288712 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -325,7 +325,8 @@ static void __nvme_submit_cmd(struct nvme_queue 
> *nvmeq,
> 
>  	if (++tail == nvmeq->q_depth)
>  		tail = 0;
> -	writel(tail, nvmeq->q_db);
> +	if (likely(nvmeq->cq_vector >= 0))
> +		writel(tail, nvmeq->q_db);
>  	nvmeq->sq_tail = tail;
>  }
> 
> --
> 1.8.3.1

[toc] | [prev] | [next] | [standalone]

#1323315 — Re: [PATCH] NVMe: do not touch sq door bell if nvmeq has been suspended

On 02/01/2016 08:42 AM, Wenbo Wang wrote:
> If __nvme_submit_cmd races with nvme_dev_disable, nvmeq
> could have been suspended and dev->bar could have been
> unmapped. Do not touch sq door bell in this case.
>
> Signed-off-by: Wenbo Wang <wenbo.wang@memblaze.com>
> Reviewed-by: Wenwei Tao <wenwei.tao@memblaze.com>
> CC: linux-nvme@lists.infradead.org
> ---
>   drivers/nvme/host/pci.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index 8b1a725..2288712 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -325,7 +325,8 @@ static void __nvme_submit_cmd(struct nvme_queue *nvmeq,
>
>   	if (++tail == nvmeq->q_depth)
>   		tail = 0;
> -	writel(tail, nvmeq->q_db);
> +	if (likely(nvmeq->cq_vector >= 0))
> +		writel(tail, nvmeq->q_db);
>   	nvmeq->sq_tail = tail;

What Keith said (this should not happen), and additionally, this won't 
work for a polled CQ without a vector.

-- 
Jens Axboe

[toc] | [prev] | [next] | [standalone]

#1324049 — Re: [PATCH] NVMe: do not touch sq door bell if nvmeq has been suspended

> Jens,
>
> I did the following test to validate the issue.
>
> 1. Modify code as below to increase the chance of races.
> 	Add 10s delay after nvme_dev_unmap() in nvme_dev_disable()
> 	Add 10s delay before __nvme_submit_cmd()
> 2. Run dd and at the same time, echo 1 to reset_controller to trigger device reset. Finally kernel crashes due to accessing unmapped door bell register.
>
> Following is the execution order of the two code paths:
> __blk_mq_run_hw_queue
>    Test BLK_MQ_S_STOPPED
> 					nvme_dev_disable()
> 					     nvme_stop_queues()  <-- set BLK_MQ_S_STOPPED
> 					     nvme_dev_unmap(dev)  <-- unmap door bell
>    nvme_queue_rq()
>        Touch door bell	<-- panic here

First of all, I think we need to cancel all
inflight requests before nvme_dev_unmap.

With my patches that move I/O termination to the
nvme core ([PATCH v1 0/3] Move active IO termination to the core)
the change needed is:
--
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index e921165..2288bdb 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -1890,10 +1890,11 @@ static void nvme_dev_shutdown(struct nvme_dev *dev)
                 nvme_shutdown_ctrl(&dev->ctrl);
                 nvme_disable_queue(dev, 0);
         }
-       nvme_dev_unmap(dev);

         blk_mq_tagset_busy_iter(&dev->tagset, nvme_cancel_io, dev);
         blk_mq_tagset_busy_iter(&dev->admin_tagset, nvme_cancel_io, dev);
+
+       nvme_dev_unmap(dev);
  }

  static int nvme_setup_prp_pools(struct nvme_dev *dev)
--

But still we need a way to wait out for all active
queue_rq to end. It seems like we need to maintain
the request_fn_active for blk-mq and provide an
API (blk_mq_wait_for_active_requests ?) that waits for
it to drop to zero.

Thoughts?

[toc] | [prev] | [next] | [standalone]

#1324118 — Re: [PATCH] NVMe: do not touch sq door bell if nvmeq has been suspended

On Tue, Feb 02, 2016 at 02:41:37PM +0200, Sagi Grimberg wrote:
> First of all, I think we need to cancel all
> inflight requests before nvme_dev_unmap.

IO cancelling is where it is because it protects against host memory
corruption. If you're going to mess with the ordering, just make sure
the PCI device is disabled from bus mastering first.

[toc] | [prev] | [next] | [standalone]

#1324132 — Re: [PATCH] NVMe: do not touch sq door bell if nvmeq has been suspended

Hey Keith,

>> First of all, I think we need to cancel all
>> inflight requests before nvme_dev_unmap.
>
> IO cancelling is where it is because it protects against host memory
> corruption. If you're going to mess with the ordering, just make sure
> the PCI device is disabled from bus mastering first.

Little help? :)

What corruption is the ordering protecting against?

[toc] | [prev] | [next] | [standalone]

#1324167 — Re: [PATCH] NVMe: do not touch sq door bell if nvmeq has been suspended

On Tue, Feb 02, 2016 at 04:33:10PM +0200, Sagi Grimberg wrote:
> Hey Keith,
> 
> >>First of all, I think we need to cancel all
> >>inflight requests before nvme_dev_unmap.
> >
> >IO cancelling is where it is because it protects against host memory
> >corruption. If you're going to mess with the ordering, just make sure
> >the PCI device is disabled from bus mastering first.
> 
> Little help? :)
> 
> What corruption is the ordering protecting against?

Sure thing. :)

We free the transfer buffers when a command is cancelled. The controller,
however, may still own the command and may try to write to them. We
have to fence the controller off from being able to do that, so we can't
cancel inflight commands while the PCI device is still bus master enabled.

In a perfect world, we could trust in disabling with NVMe registers,
but sometimes we can't rely on that.

This was commit 07836e659c81ec6b0d683dfbf7958339a22a7b69, which might
explain the scenario a little better, and was reported by end user.

[toc] | [prev] | [next] | [standalone]

#1324306 — Re: [PATCH] NVMe: do not touch sq door bell if nvmeq has been suspended

> We free the transfer buffers when a command is cancelled. The controller,
> however, may still own the command and may try to write to them. We
> have to fence the controller off from being able to do that, so we can't
> cancel inflight commands while the PCI device is still bus master enabled.
>
> In a perfect world, we could trust in disabling with NVMe registers,
> but sometimes we can't rely on that.

OK, I wasn't aware that we cannot rely on that.

So it looks like we cannot change the ordering. So this leaves us with
the need to guarantee that no queue_rq is inflight before we unmap.

[toc] | [prev] | [next] | [standalone]

#1324312 — Re: [PATCH] NVMe: do not touch sq door bell if nvmeq has been suspended

On Tue, Feb 02, 2016 at 07:15:57AM +0000, Wenbo Wang wrote:
> Jens,
> 
> I did the following test to validate the issue.
> 
> 1. Modify code as below to increase the chance of races.
> 	Add 10s delay after nvme_dev_unmap() in nvme_dev_disable()
> 	Add 10s delay before __nvme_submit_cmd()

If running sync IO, preempt is disabled. You can't just put a 10 second
delay there. Wouldn't you hit a "scheduling while atomic" bug instead?

If blk-mq is running the h/w context from its work queue, that might
be a different issue. Maybe we can change the "cancel_delayed_work" to
"cancel_delayed_work_sync" in blk_mq_stop_hw_queues.

If there's still a window where blk-mq can insert a request after the
driver requested to stop queues, I think we should try to close it with
the block layer.

[toc] | [prev] | [next] | [standalone]

#1325505 — Re: [PATCH] NVMe: do not touch sq door bell if nvmeq has been suspended

On Tue, Feb 02, 2016 at 07:15:57AM +0000, Wenbo Wang wrote:
> I did the following test to validate the issue.
> 
> 1. Modify code as below to increase the chance of races.
> 	Add 10s delay after nvme_dev_unmap() in nvme_dev_disable()
> 	Add 10s delay before __nvme_submit_cmd()
> 2. Run dd and at the same time, echo 1 to reset_controller to trigger device reset. Finally kernel crashes due to accessing unmapped door bell register.
> 
> Following is the execution order of the two code paths:
> __blk_mq_run_hw_queue
>   Test BLK_MQ_S_STOPPED
> 					nvme_dev_disable()
> 					     nvme_stop_queues()  <-- set BLK_MQ_S_STOPPED
> 					     nvme_dev_unmap(dev)  <-- unmap door bell
>   nvme_queue_rq()
>       Touch door bell	<-- panic here

Does the following force the first to complete before the unmap?

---
@@ -1415,10 +1421,21 @@ void nvme_stop_queues(struct nvme_ctrl *ctrl)
 
 		blk_mq_cancel_requeue_work(ns->queue);
 		blk_mq_stop_hw_queues(ns->queue);
+		blk_sync_queue(ns->queue);
 	}
 	mutex_unlock(&ctrl->namespaces_mutex);
 }
--

[toc] | [prev] | [next] | [standalone]

#1325625 — Re: [PATCH] NVMe: do not touch sq door bell if nvmeq has been suspended

On Wed, Feb 03, 2016 at 04:35:03PM +0000, Wenbo Wang wrote:
> For async io (executed by run_work worker) it should work. However for sync io in blk_mq_run_hw_queue, this seems not help, there is still a window.

Alright, for lack of a better way to sync a stopped queue, the driver's
indication looks like the best we can do at this point.

[toc] | [prev] | [next] | [standalone]

#1328488 — Re: [PATCH] NVMe: do not touch sq door bell if nvmeq has been suspended

> Keith,
>
> Is the following solution OK?
> synchronize_rcu guarantee that no queue_rq is running concurrently with device disable code.
> Together with your another patch (adding blk_sync_queue), both sync/async path shall be handled correctly.

This can be acceptable I think.

> Do you think synchronize_rcu shall be added to blk_sync_queue?

Or, we'll add it to blk_mq_stop_hw_queues() and then scsi
will enjoy it as well.

> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index 4c0622f..bfe9132 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -865,7 +865,9 @@ void blk_mq_run_hw_queue(struct blk_mq_hw_ctx *hctx, bool async)
>          if (!async) {
>                  int cpu = get_cpu();
>                  if (cpumask_test_cpu(cpu, hctx->cpumask)) {
> +                       rcu_read_lock();
>                          __blk_mq_run_hw_queue(hctx);
> +                       rcu_read_unlock();

I think the rcu is better folded into __blk_mq_run_hw_queue
to cover all the call sites.

[toc] | [prev] | [next] | [standalone]

#1329147 — Re: [PATCH] NVMe: do not touch sq door bell if nvmeq has been suspended

On Sat, Feb 06, 2016 at 02:32:24PM +0000, Wenbo Wang wrote:
> Keith,
> 
> Is the following solution OK?
> synchronize_rcu guarantee that no queue_rq is running concurrently with device disable code. Together with your another patch (adding blk_sync_queue), both sync/async path shall be handled correctly.
> 
> Do you think synchronize_rcu shall be added to blk_sync_queue?

I was nearly going to suggest the same last week, but it feels wrong since
no one takes rcu_read_lock in the path we're trying to sychronoize. Is
this safe if the task is interrupted?

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web